From a65e13d591bb151eb71eb8ec42834ce28a3db304 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Wed, 14 Apr 2021 13:25:16 +0530 Subject: [PATCH v5] Avoid Catalogue Accesses In slot_store_error_callback Avoid accessing system catalogues inside slot_store_error_callback error context callback, because the the entire transaction might have been broken at this point. logicalrep_typmap_gettypname() and format_type_be could search the sys cache. As per suggestion from Tom, a simple solution is to just avoid printing the column type names in the message, just the column name is enough to know on which column the error occurred. The above solution makes logicalrep_typmap_gettypname redundant hence removed it. --- src/backend/replication/logical/relation.c | 49 ---------------------- src/backend/replication/logical/worker.c | 25 +---------- src/include/replication/logicalrelation.h | 2 - 3 files changed, 2 insertions(+), 74 deletions(-) diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c index e861c0ff80..cbc5612dbe 100644 --- a/src/backend/replication/logical/relation.c +++ b/src/backend/replication/logical/relation.c @@ -490,55 +490,6 @@ logicalrep_typmap_update(LogicalRepTyp *remotetyp) 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); -} - /* * Partition cache: look up partition LogicalRepRelMapEntry's * diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c index fb3ba5c415..6494bef1d9 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -132,7 +132,6 @@ static dlist_head lsn_mapping = DLIST_STATIC_INIT(lsn_mapping); typedef struct SlotErrCallbackArg { LogicalRepRelMapEntry *rel; - int local_attnum; int remote_attnum; } SlotErrCallbackArg; @@ -431,29 +430,15 @@ 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]); } /* @@ -474,7 +459,6 @@ slot_store_data(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; @@ -494,7 +478,6 @@ slot_store_data(TupleTableSlot *slot, LogicalRepRelMapEntry *rel, Assert(remoteattnum < tupleData->ncols); - errarg.local_attnum = i; errarg.remote_attnum = remoteattnum; if (tupleData->colstatus[remoteattnum] == LOGICALREP_COLUMN_TEXT) @@ -543,7 +526,6 @@ slot_store_data(TupleTableSlot *slot, LogicalRepRelMapEntry *rel, slot->tts_isnull[i] = true; } - errarg.local_attnum = -1; errarg.remote_attnum = -1; } else @@ -600,7 +582,6 @@ slot_modify_data(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; @@ -623,7 +604,6 @@ slot_modify_data(TupleTableSlot *slot, TupleTableSlot *srcslot, { StringInfo colvalue = &tupleData->colvalues[remoteattnum]; - errarg.local_attnum = i; errarg.remote_attnum = remoteattnum; if (tupleData->colstatus[remoteattnum] == LOGICALREP_COLUMN_TEXT) @@ -668,7 +648,6 @@ slot_modify_data(TupleTableSlot *slot, TupleTableSlot *srcslot, slot->tts_isnull[i] = true; } - errarg.local_attnum = -1; errarg.remote_attnum = -1; } } diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h index 3f0b3deefb..0f67a64bb3 100644 --- a/src/include/replication/logicalrelation.h +++ b/src/include/replication/logicalrelation.h @@ -47,6 +47,4 @@ 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 */ -- 2.25.1