Skip to content

Commit 7fc9775

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 8d2be14 commit 7fc9775

File tree

3 files changed

+6
-135
lines changed

3 files changed

+6
-135
lines changed

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

Lines changed: 1 addition & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -17,24 +17,19 @@
1717
#include "postgres.h"
1818

1919
#include "access/relation.h"
20-
#include "access/sysattr.h"
2120
#include "access/table.h"
2221
#include "catalog/namespace.h"
2322
#include "catalog/pg_subscription_rel.h"
2423
#include "executor/executor.h"
2524
#include "nodes/makefuncs.h"
2625
#include "replication/logicalrelation.h"
2726
#include "replication/worker_internal.h"
28-
#include "utils/builtins.h"
2927
#include "utils/inval.h"
30-
#include "utils/lsyscache.h"
31-
#include "utils/memutils.h"
32-
#include "utils/syscache.h"
28+
3329

3430
static MemoryContext LogicalRepRelMapContext = NULL;
3531

3632
static HTAB *LogicalRepRelMap = NULL;
37-
static HTAB *LogicalRepTypMap = NULL;
3833

3934
/*
4035
* Partition map (LogicalRepPartMap)
@@ -119,16 +114,6 @@ logicalrep_relmap_init(void)
119114
LogicalRepRelMap = hash_create("logicalrep relation map cache", 128, &ctl,
120115
HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
121116

122-
/* Initialize the type hash table. */
123-
MemSet(&ctl, 0, sizeof(ctl));
124-
ctl.keysize = sizeof(Oid);
125-
ctl.entrysize = sizeof(LogicalRepTyp);
126-
ctl.hcxt = LogicalRepRelMapContext;
127-
128-
/* This will usually be small. */
129-
LogicalRepTypMap = hash_create("logicalrep type map cache", 2, &ctl,
130-
HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
131-
132117
/* Watch for invalidation events. */
133118
CacheRegisterRelcacheCallback(logicalrep_relmap_invalidate_cb,
134119
(Datum) 0);
@@ -418,95 +403,6 @@ logicalrep_rel_close(LogicalRepRelMapEntry *rel, LOCKMODE lockmode)
418403
rel->localrel = NULL;
419404
}
420405

421-
/*
422-
* Free the type map cache entry data.
423-
*/
424-
static void
425-
logicalrep_typmap_free_entry(LogicalRepTyp *entry)
426-
{
427-
pfree(entry->nspname);
428-
pfree(entry->typname);
429-
}
430-
431-
/*
432-
* Add new entry or update existing entry in the type map cache.
433-
*/
434-
void
435-
logicalrep_typmap_update(LogicalRepTyp *remotetyp)
436-
{
437-
MemoryContext oldctx;
438-
LogicalRepTyp *entry;
439-
bool found;
440-
441-
if (LogicalRepTypMap == NULL)
442-
logicalrep_relmap_init();
443-
444-
/*
445-
* HASH_ENTER returns the existing entry if present or creates a new one.
446-
*/
447-
entry = hash_search(LogicalRepTypMap, (void *) &remotetyp->remoteid,
448-
HASH_ENTER, &found);
449-
450-
if (found)
451-
logicalrep_typmap_free_entry(entry);
452-
453-
/* Make cached copy of the data */
454-
entry->remoteid = remotetyp->remoteid;
455-
oldctx = MemoryContextSwitchTo(LogicalRepRelMapContext);
456-
entry->nspname = pstrdup(remotetyp->nspname);
457-
entry->typname = pstrdup(remotetyp->typname);
458-
MemoryContextSwitchTo(oldctx);
459-
}
460-
461-
/*
462-
* Fetch type name from the cache by remote type OID.
463-
*
464-
* Return a substitute value if we cannot find the data type; no message is
465-
* sent to the log in that case, because this is used by error callback
466-
* already.
467-
*/
468-
char *
469-
logicalrep_typmap_gettypname(Oid remoteid)
470-
{
471-
LogicalRepTyp *entry;
472-
bool found;
473-
474-
/* Internal types are mapped directly. */
475-
if (remoteid < FirstGenbkiObjectId)
476-
{
477-
if (!get_typisdefined(remoteid))
478-
{
479-
/*
480-
* This can be caused by having a publisher with a higher
481-
* PostgreSQL major version than the subscriber.
482-
*/
483-
return psprintf("unrecognized %u", remoteid);
484-
}
485-
486-
return format_type_be(remoteid);
487-
}
488-
489-
if (LogicalRepTypMap == NULL)
490-
{
491-
/*
492-
* If the typemap is not initialized yet, we cannot possibly attempt
493-
* to search the hash table; but there's no way we know the type
494-
* locally yet, since we haven't received a message about this type,
495-
* so this is the best we can do.
496-
*/
497-
return psprintf("unrecognized %u", remoteid);
498-
}
499-
500-
/* search the mapping */
501-
entry = hash_search(LogicalRepTypMap, (void *) &remoteid,
502-
HASH_FIND, &found);
503-
if (!found)
504-
return psprintf("unrecognized %u", remoteid);
505-
506-
Assert(OidIsValid(entry->remoteid));
507-
return psprintf("%s.%s", entry->nspname, entry->typname);
508-
}
509-
510406
/*
511407
* Partition cache: look up partition LogicalRepRelMapEntry's
512408
*

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

Lines changed: 5 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,6 @@ static dlist_head lsn_mapping = DLIST_STATIC_INIT(lsn_mapping);
9393
typedef struct SlotErrCallbackArg
9494
{
9595
LogicalRepRelMapEntry *rel;
96-
int local_attnum;
9796
int remote_attnum;
9897
} SlotErrCallbackArg;
9998

@@ -344,36 +343,23 @@ slot_fill_defaults(LogicalRepRelMapEntry *rel, EState *estate,
344343
}
345344

346345
/*
347-
* Error callback to give more context info about type conversion failure.
346+
* Error callback to give more context info about data conversion failures
347+
* while reading data from the remote server.
348348
*/
349349
static void
350350
slot_store_error_callback(void *arg)
351351
{
352352
SlotErrCallbackArg *errarg = (SlotErrCallbackArg *) arg;
353353
LogicalRepRelMapEntry *rel;
354-
char *remotetypname;
355-
Oid remotetypoid,
356-
localtypoid;
357354

358355
/* Nothing to do if remote attribute number is not set */
359356
if (errarg->remote_attnum < 0)
360357
return;
361358

362359
rel = errarg->rel;
363-
remotetypoid = rel->remoterel.atttyps[errarg->remote_attnum];
364-
365-
/* Fetch remote type name from the LogicalRepTypMap cache */
366-
remotetypname = logicalrep_typmap_gettypname(remotetypoid);
367-
368-
/* Fetch local type OID from the local sys cache */
369-
localtypoid = get_atttype(rel->localreloid, errarg->local_attnum + 1);
370-
371-
errcontext("processing remote data for replication target relation \"%s.%s\" column \"%s\", "
372-
"remote type %s, local type %s",
360+
errcontext("processing remote data for replication target relation \"%s.%s\" column \"%s\"",
373361
rel->remoterel.nspname, rel->remoterel.relname,
374-
rel->remoterel.attnames[errarg->remote_attnum],
375-
remotetypname,
376-
format_type_be(localtypoid));
362+
rel->remoterel.attnames[errarg->remote_attnum]);
377363
}
378364

379365
/*
@@ -394,7 +380,6 @@ slot_store_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel,
394380

395381
/* Push callback + info on the error context stack */
396382
errarg.rel = rel;
397-
errarg.local_attnum = -1;
398383
errarg.remote_attnum = -1;
399384
errcallback.callback = slot_store_error_callback;
400385
errcallback.arg = (void *) &errarg;
@@ -414,7 +399,6 @@ slot_store_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel,
414399
Oid typinput;
415400
Oid typioparam;
416401

417-
errarg.local_attnum = i;
418402
errarg.remote_attnum = remoteattnum;
419403

420404
getTypeInputInfo(att->atttypid, &typinput, &typioparam);
@@ -423,7 +407,6 @@ slot_store_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel,
423407
typioparam, att->atttypmod);
424408
slot->tts_isnull[i] = false;
425409

426-
errarg.local_attnum = -1;
427410
errarg.remote_attnum = -1;
428411
}
429412
else
@@ -479,7 +462,6 @@ slot_modify_cstrings(TupleTableSlot *slot, TupleTableSlot *srcslot,
479462

480463
/* For error reporting, push callback + info on the error context stack */
481464
errarg.rel = rel;
482-
errarg.local_attnum = -1;
483465
errarg.remote_attnum = -1;
484466
errcallback.callback = slot_store_error_callback;
485467
errcallback.arg = (void *) &errarg;
@@ -504,7 +486,6 @@ slot_modify_cstrings(TupleTableSlot *slot, TupleTableSlot *srcslot,
504486
Oid typinput;
505487
Oid typioparam;
506488

507-
errarg.local_attnum = i;
508489
errarg.remote_attnum = remoteattnum;
509490

510491
getTypeInputInfo(att->atttypid, &typinput, &typioparam);
@@ -513,7 +494,6 @@ slot_modify_cstrings(TupleTableSlot *slot, TupleTableSlot *srcslot,
513494
typioparam, att->atttypmod);
514495
slot->tts_isnull[i] = false;
515496

516-
errarg.local_attnum = -1;
517497
errarg.remote_attnum = -1;
518498
}
519499
else
@@ -637,16 +617,14 @@ apply_handle_relation(StringInfo s)
637617
/*
638618
* Handle TYPE message.
639619
*
640-
* Note we don't do local mapping here, that's done when the type is
641-
* actually used.
620+
* This is now vestigial; we read the info and discard it.
642621
*/
643622
static void
644623
apply_handle_type(StringInfo s)
645624
{
646625
LogicalRepTyp typ;
647626

648627
logicalrep_read_typ(s, &typ);
649-
logicalrep_typmap_update(&typ);
650628
}
651629

652630
/*

‎src/include/replication/logicalrelation.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,4 @@ extern LogicalRepRelMapEntry *logicalrep_partition_open(LogicalRepRelMapEntry *r
4141
extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel,
4242
LOCKMODE lockmode);
4343

44-
extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp);
45-
extern char *logicalrep_typmap_gettypname(Oid remoteid);
46-
4744
#endif /* LOGICALRELATION_H */

0 commit comments

Comments
 (0)