Skip to content

Commit 32d50b8

Browse files
tglsfdctglsfdc
committed
Don't try to print data type names in slot_store_error_callback().
The existing code tried to do syscache lookups in an already-failed transaction, which is problematic to say the least. After some consideration of alternatives, the best fix seems to be to just drop type names from the error message altogether. The table and column names seem like sufficient localization. If the user is unsure what types are involved, she can check the local and remote table definitions. Having done that, we can also discard the LogicalRepTypMap hash table, which had no other use. Arguably, LOGICAL_REP_MSG_TYPE replication messages are now obsolete as well; but we should probably keep them in case some other use emerges. (The complexity of removing something from the replication protocol would likely outweigh any savings anyhow.) Masahiko Sawada and Bharath Rupireddy, per complaint from Andres Freund. Back-patch to v10 where this code originated. Discussion: https://postgr.es/m/20210106020229.ne5xnuu6wlondjpe@alap3.anarazel.de
1 parent 4180417 commit 32d50b8

File tree

3 files changed

+6
-134
lines changed

3 files changed

+6
-134
lines changed

‎src/backend/replication/logical/relation.c

Lines changed: 1 addition & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -24,16 +24,12 @@
2424
#include "nodes/makefuncs.h"
2525
#include "replication/logicalrelation.h"
2626
#include "replication/worker_internal.h"
27-
#include "utils/builtins.h"
2827
#include "utils/inval.h"
29-
#include "utils/lsyscache.h"
30-
#include "utils/memutils.h"
31-
#include "utils/syscache.h"
28+
3229

3330
static MemoryContext LogicalRepRelMapContext = NULL;
3431

3532
static HTAB *LogicalRepRelMap = NULL;
36-
static HTAB *LogicalRepTypMap = NULL;
3733

3834

3935
/*
@@ -100,16 +96,6 @@ logicalrep_relmap_init(void)
10096
LogicalRepRelMap = hash_create("logicalrep relation map cache", 128, &ctl,
10197
HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
10298

103-
/* Initialize the type hash table. */
104-
MemSet(&ctl, 0, sizeof(ctl));
105-
ctl.keysize = sizeof(Oid);
106-
ctl.entrysize = sizeof(LogicalRepTyp);
107-
ctl.hcxt = LogicalRepRelMapContext;
108-
109-
/* This will usually be small. */
110-
LogicalRepTypMap = hash_create("logicalrep type map cache", 2, &ctl,
111-
HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
112-
11399
/* Watch for invalidation events. */
114100
CacheRegisterRelcacheCallback(logicalrep_relmap_invalidate_cb,
115101
(Datum) 0);
@@ -398,92 +384,3 @@ logicalrep_rel_close(LogicalRepRelMapEntry *rel, LOCKMODE lockmode)
398384
heap_close(rel->localrel, lockmode);
399385
rel->localrel = NULL;
400386
}
401-
402-
/*
403-
* Free the type map cache entry data.
404-
*/
405-
static void
406-
logicalrep_typmap_free_entry(LogicalRepTyp *entry)
407-
{
408-
pfree(entry->nspname);
409-
pfree(entry->typname);
410-
}
411-
412-
/*
413-
* Add new entry or update existing entry in the type map cache.
414-
*/
415-
void
416-
logicalrep_typmap_update(LogicalRepTyp *remotetyp)
417-
{
418-
MemoryContext oldctx;
419-
LogicalRepTyp *entry;
420-
bool found;
421-
422-
if (LogicalRepTypMap == NULL)
423-
logicalrep_relmap_init();
424-
425-
/*
426-
* HASH_ENTER returns the existing entry if present or creates a new one.
427-
*/
428-
entry = hash_search(LogicalRepTypMap, (void *) &remotetyp->remoteid,
429-
HASH_ENTER, &found);
430-
431-
if (found)
432-
logicalrep_typmap_free_entry(entry);
433-
434-
/* Make cached copy of the data */
435-
entry->remoteid = remotetyp->remoteid;
436-
oldctx = MemoryContextSwitchTo(LogicalRepRelMapContext);
437-
entry->nspname = pstrdup(remotetyp->nspname);
438-
entry->typname = pstrdup(remotetyp->typname);
439-
MemoryContextSwitchTo(oldctx);
440-
}
441-
442-
/*
443-
* Fetch type name from the cache by remote type OID.
444-
*
445-
* Return a substitute value if we cannot find the data type; no message is
446-
* sent to the log in that case, because this is used by error callback
447-
* already.
448-
*/
449-
char *
450-
logicalrep_typmap_gettypname(Oid remoteid)
451-
{
452-
LogicalRepTyp *entry;
453-
bool found;
454-
455-
/* Internal types are mapped directly. */
456-
if (remoteid < FirstBootstrapObjectId)
457-
{
458-
if (!get_typisdefined(remoteid))
459-
{
460-
/*
461-
* This can be caused by having a publisher with a higher
462-
* PostgreSQL major version than the subscriber.
463-
*/
464-
return psprintf("unrecognized %u", remoteid);
465-
}
466-
467-
return format_type_be(remoteid);
468-
}
469-
470-
if (LogicalRepTypMap == NULL)
471-
{
472-
/*
473-
* If the typemap is not initialized yet, we cannot possibly attempt
474-
* to search the hash table; but there's no way we know the type
475-
* locally yet, since we haven't received a message about this type,
476-
* so this is the best we can do.
477-
*/
478-
return psprintf("unrecognized %u", remoteid);
479-
}
480-
481-
/* search the mapping */
482-
entry = hash_search(LogicalRepTypMap, (void *) &remoteid,
483-
HASH_FIND, &found);
484-
if (!found)
485-
return psprintf("unrecognized %u", remoteid);
486-
487-
Assert(OidIsValid(entry->remoteid));
488-
return psprintf("%s.%s", entry->nspname, entry->typname);
489-
}

‎src/backend/replication/logical/worker.c

Lines changed: 5 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,6 @@ static dlist_head lsn_mapping = DLIST_STATIC_INIT(lsn_mapping);
105105
typedef struct SlotErrCallbackArg
106106
{
107107
LogicalRepRelMapEntry *rel;
108-
int local_attnum;
109108
int remote_attnum;
110109
} SlotErrCallbackArg;
111110

@@ -307,36 +306,23 @@ slot_fill_defaults(LogicalRepRelMapEntry *rel, EState *estate,
307306
}
308307

309308
/*
310-
* Error callback to give more context info about type conversion failure.
309+
* Error callback to give more context info about data conversion failures
310+
* while reading data from the remote server.
311311
*/
312312
static void
313313
slot_store_error_callback(void *arg)
314314
{
315315
SlotErrCallbackArg *errarg = (SlotErrCallbackArg *) arg;
316316
LogicalRepRelMapEntry *rel;
317-
char *remotetypname;
318-
Oid remotetypoid,
319-
localtypoid;
320317

321318
/* Nothing to do if remote attribute number is not set */
322319
if (errarg->remote_attnum < 0)
323320
return;
324321

325322
rel = errarg->rel;
326-
remotetypoid = rel->remoterel.atttyps[errarg->remote_attnum];
327-
328-
/* Fetch remote type name from the LogicalRepTypMap cache */
329-
remotetypname = logicalrep_typmap_gettypname(remotetypoid);
330-
331-
/* Fetch local type OID from the local sys cache */
332-
localtypoid = get_atttype(rel->localreloid, errarg->local_attnum + 1);
333-
334-
errcontext("processing remote data for replication target relation \"%s.%s\" column \"%s\", "
335-
"remote type %s, local type %s",
323+
errcontext("processing remote data for replication target relation \"%s.%s\" column \"%s\"",
336324
rel->remoterel.nspname, rel->remoterel.relname,
337-
rel->remoterel.attnames[errarg->remote_attnum],
338-
remotetypname,
339-
format_type_be(localtypoid));
325+
rel->remoterel.attnames[errarg->remote_attnum]);
340326
}
341327

342328
/*
@@ -357,7 +343,6 @@ slot_store_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel,
357343

358344
/* Push callback + info on the error context stack */
359345
errarg.rel = rel;
360-
errarg.local_attnum = -1;
361346
errarg.remote_attnum = -1;
362347
errcallback.callback = slot_store_error_callback;
363348
errcallback.arg = (void *) &errarg;
@@ -376,7 +361,6 @@ slot_store_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel,
376361
Oid typinput;
377362
Oid typioparam;
378363

379-
errarg.local_attnum = i;
380364
errarg.remote_attnum = remoteattnum;
381365

382366
getTypeInputInfo(att->atttypid, &typinput, &typioparam);
@@ -385,7 +369,6 @@ slot_store_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel,
385369
typioparam, att->atttypmod);
386370
slot->tts_isnull[i] = false;
387371

388-
errarg.local_attnum = -1;
389372
errarg.remote_attnum = -1;
390373
}
391374
else
@@ -441,7 +424,6 @@ slot_modify_cstrings(TupleTableSlot *slot, TupleTableSlot *srcslot,
441424

442425
/* For error reporting, push callback + info on the error context stack */
443426
errarg.rel = rel;
444-
errarg.local_attnum = -1;
445427
errarg.remote_attnum = -1;
446428
errcallback.callback = slot_store_error_callback;
447429
errcallback.arg = (void *) &errarg;
@@ -465,7 +447,6 @@ slot_modify_cstrings(TupleTableSlot *slot, TupleTableSlot *srcslot,
465447
Oid typinput;
466448
Oid typioparam;
467449

468-
errarg.local_attnum = i;
469450
errarg.remote_attnum = remoteattnum;
470451

471452
getTypeInputInfo(att->atttypid, &typinput, &typioparam);
@@ -474,7 +455,6 @@ slot_modify_cstrings(TupleTableSlot *slot, TupleTableSlot *srcslot,
474455
typioparam, att->atttypmod);
475456
slot->tts_isnull[i] = false;
476457

477-
errarg.local_attnum = -1;
478458
errarg.remote_attnum = -1;
479459
}
480460
else
@@ -598,16 +578,14 @@ apply_handle_relation(StringInfo s)
598578
/*
599579
* Handle TYPE message.
600580
*
601-
* Note we don't do local mapping here, that's done when the type is
602-
* actually used.
581+
* This is now vestigial; we read the info and discard it.
603582
*/
604583
static void
605584
apply_handle_type(StringInfo s)
606585
{
607586
LogicalRepTyp typ;
608587

609588
logicalrep_read_typ(s, &typ);
610-
logicalrep_typmap_update(&typ);
611589
}
612590

613591
/*

‎src/include/replication/logicalrelation.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,4 @@ extern LogicalRepRelMapEntry *logicalrep_rel_open(LogicalRepRelId remoteid,
3838
extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
3939
LOCKMODE lockmode);
4040

41-
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
42-
extern char *logicalrep_typmap_gettypname(Oid remoteid);
43-
4441
#endif /* LOGICALRELATION_H */

0 commit comments

Comments
 (0)