From feff6155734e75d9471bb84319afb6f9ca114686 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 2 Jul 2021 16:04:54 -0400 Subject: [PATCH] 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 --- src/backend/replication/logical/relation.c | 106 +-------------------- src/backend/replication/logical/worker.c | 32 +------ src/include/replication/logicalrelation.h | 3 - 3 files changed, 6 insertions(+), 135 deletions(-) diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c index 2c1313ba52..16f4c56bfd 100644 --- a/src/backend/replication/logical/relation.c +++ b/src/backend/replication/logical/relation.c @@ -17,7 +17,6 @@ #include "postgres.h" #include "access/relation.h" -#include "access/sysattr.h" #include "access/table.h" #include "catalog/namespace.h" #include "catalog/pg_subscription_rel.h" @@ -25,16 +24,12 @@ #include "nodes/makefuncs.h" #include "replication/logicalrelation.h" #include "replication/worker_internal.h" -#include "utils/builtins.h" #include "utils/inval.h" -#include "utils/lsyscache.h" -#include "utils/memutils.h" -#include "utils/syscache.h" + static MemoryContext LogicalRepRelMapContext = NULL; static HTAB *LogicalRepRelMap = NULL; -static HTAB *LogicalRepTypMap = NULL; /* @@ -101,16 +96,6 @@ logicalrep_relmap_init(void) LogicalRepRelMap = hash_create("logicalrep relation map cache", 128, &ctl, HASH_ELEM | HASH_BLOBS | HASH_CONTEXT); - /* Initialize the type hash table. */ - MemSet(&ctl, 0, sizeof(ctl)); - ctl.keysize = sizeof(Oid); - ctl.entrysize = sizeof(LogicalRepTyp); - ctl.hcxt = LogicalRepRelMapContext; - - /* This will usually be small. */ - LogicalRepTypMap = hash_create("logicalrep type map cache", 2, &ctl, - HASH_ELEM | HASH_BLOBS | HASH_CONTEXT); - /* Watch for invalidation events. */ CacheRegisterRelcacheCallback(logicalrep_relmap_invalidate_cb, (Datum) 0); @@ -399,92 +384,3 @@ logicalrep_rel_close(LogicalRepRelMapEntry *rel, LOCKMODE lockmode) table_close(rel->localrel, lockmode); rel->localrel = NULL; } - -/* - * Free the type map cache entry data. - */ -static void -logicalrep_typmap_free_entry(LogicalRepTyp *entry) -{ - pfree(entry->nspname); - pfree(entry->typname); -} - -/* - * Add new entry or update existing entry in the type map cache. - */ -void -logicalrep_typmap_update(LogicalRepTyp *remotetyp) -{ - MemoryContext oldctx; - LogicalRepTyp *entry; - bool found; - - if (LogicalRepTypMap == NULL) - logicalrep_relmap_init(); - - /* - * HASH_ENTER returns the existing entry if present or creates a new one. - */ - entry = hash_search(LogicalRepTypMap, (void *) &remotetyp->remoteid, - HASH_ENTER, &found); - - if (found) - logicalrep_typmap_free_entry(entry); - - /* Make cached copy of the data */ - entry->remoteid = remotetyp->remoteid; - oldctx = MemoryContextSwitchTo(LogicalRepRelMapContext); - entry->nspname = pstrdup(remotetyp->nspname); - entry->typname = pstrdup(remotetyp->typname); - MemoryContextSwitchTo(oldctx); -} - -/* - * Fetch type name from the cache by remote type OID. - * - * Return a substitute value if we cannot find the data type; no message is - * sent to the log in that case, because this is used by error callback - * already. - */ -char * -logicalrep_typmap_gettypname(Oid remoteid) -{ - LogicalRepTyp *entry; - bool found; - - /* Internal types are mapped directly. */ - if (remoteid < FirstGenbkiObjectId) - { - if (!get_typisdefined(remoteid)) - { - /* - * This can be caused by having a publisher with a higher - * PostgreSQL major version than the subscriber. - */ - return psprintf("unrecognized %u", remoteid); - } - - return format_type_be(remoteid); - } - - if (LogicalRepTypMap == NULL) - { - /* - * If the typemap is not initialized yet, we cannot possibly attempt - * to search the hash table; but there's no way we know the type - * locally yet, since we haven't received a message about this type, - * so this is the best we can do. - */ - return psprintf("unrecognized %u", remoteid); - } - - /* search the mapping */ - entry = hash_search(LogicalRepTypMap, (void *) &remoteid, - HASH_FIND, &found); - if (!found) - return psprintf("unrecognized %u", remoteid); - - Assert(OidIsValid(entry->remoteid)); - return psprintf("%s.%s", entry->nspname, entry->typname); -} diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c index 571b0e717e..2f448036ea 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -89,7 +89,6 @@ static dlist_head lsn_mapping = DLIST_STATIC_INIT(lsn_mapping); typedef struct SlotErrCallbackArg { LogicalRepRelMapEntry *rel; - int local_attnum; int remote_attnum; } SlotErrCallbackArg; @@ -288,36 +287,23 @@ slot_fill_defaults(LogicalRepRelMapEntry *rel, EState *estate, } /* - * Error callback to give more context info about type conversion failure. + * Error callback to give more context info about data conversion failures + * while reading data from the remote server. */ static void slot_store_error_callback(void *arg) { SlotErrCallbackArg *errarg = (SlotErrCallbackArg *) arg; LogicalRepRelMapEntry *rel; - char *remotetypname; - Oid remotetypoid, - localtypoid; /* Nothing to do if remote attribute number is not set */ if (errarg->remote_attnum < 0) return; rel = errarg->rel; - remotetypoid = rel->remoterel.atttyps[errarg->remote_attnum]; - - /* Fetch remote type name from the LogicalRepTypMap cache */ - remotetypname = logicalrep_typmap_gettypname(remotetypoid); - - /* Fetch local type OID from the local sys cache */ - localtypoid = get_atttype(rel->localreloid, errarg->local_attnum + 1); - - errcontext("processing remote data for replication target relation \"%s.%s\" column \"%s\", " - "remote type %s, local type %s", + errcontext("processing remote data for replication target relation \"%s.%s\" column \"%s\"", rel->remoterel.nspname, rel->remoterel.relname, - rel->remoterel.attnames[errarg->remote_attnum], - remotetypname, - format_type_be(localtypoid)); + rel->remoterel.attnames[errarg->remote_attnum]); } /* @@ -338,7 +324,6 @@ slot_store_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel, /* Push callback + info on the error context stack */ errarg.rel = rel; - errarg.local_attnum = -1; errarg.remote_attnum = -1; errcallback.callback = slot_store_error_callback; errcallback.arg = (void *) &errarg; @@ -357,7 +342,6 @@ slot_store_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel, Oid typinput; Oid typioparam; - errarg.local_attnum = i; errarg.remote_attnum = remoteattnum; getTypeInputInfo(att->atttypid, &typinput, &typioparam); @@ -366,7 +350,6 @@ slot_store_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel, typioparam, att->atttypmod); slot->tts_isnull[i] = false; - errarg.local_attnum = -1; errarg.remote_attnum = -1; } else @@ -422,7 +405,6 @@ slot_modify_cstrings(TupleTableSlot *slot, TupleTableSlot *srcslot, /* For error reporting, push callback + info on the error context stack */ errarg.rel = rel; - errarg.local_attnum = -1; errarg.remote_attnum = -1; errcallback.callback = slot_store_error_callback; errcallback.arg = (void *) &errarg; @@ -446,7 +428,6 @@ slot_modify_cstrings(TupleTableSlot *slot, TupleTableSlot *srcslot, Oid typinput; Oid typioparam; - errarg.local_attnum = i; errarg.remote_attnum = remoteattnum; getTypeInputInfo(att->atttypid, &typinput, &typioparam); @@ -455,7 +436,6 @@ slot_modify_cstrings(TupleTableSlot *slot, TupleTableSlot *srcslot, typioparam, att->atttypmod); slot->tts_isnull[i] = false; - errarg.local_attnum = -1; errarg.remote_attnum = -1; } else @@ -579,8 +559,7 @@ apply_handle_relation(StringInfo s) /* * Handle TYPE message. * - * Note we don't do local mapping here, that's done when the type is - * actually used. + * This is now vestigial; we read the info and discard it. */ static void apply_handle_type(StringInfo s) @@ -588,7 +567,6 @@ apply_handle_type(StringInfo s) LogicalRepTyp typ; logicalrep_read_typ(s, &typ); - logicalrep_typmap_update(&typ); } /* diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h index dfb1fea48b..e4ec28b947 100644 --- a/src/include/replication/logicalrelation.h +++ b/src/include/replication/logicalrelation.h @@ -38,7 +38,4 @@ extern LogicalRepRelMapEntry *logicalrep_rel_open(LogicalRepRelId remoteid, extern void logicalrep_rel_close(LogicalRepRelMapEntry *rel, LOCKMODE lockmode); -extern void logicalrep_typmap_update(LogicalRepTyp *remotetyp); -extern char *logicalrep_typmap_gettypname(Oid remoteid); - #endif /* LOGICALRELATION_H */