On Wed, Mar 17, 2021 at 4:52 PM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
>
> On Tue, Mar 16, 2021 at 2:21 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> > > Thanks for pointing to the changes in the commit message. I corrected
> > > them. Attaching v4 patch set, consider it for further review.
> >
> > I took a quick look at this. I'm quite worried about the potential
> > performance cost of the v4-0001 patch (the one for fixing
> > slot_store_error_callback). Previously, we didn't pay any noticeable
> > cost for having the callback unless there actually was an error.
> > As patched, we perform several catalog lookups per column per row,
> > even in the non-error code path. That seems like it'd be a noticeable
> > performance hit. Just to add insult to injury, it leaks memory.
> >
> > I propose a more radical but simpler solution: let's just not bother
> > with including the type names in the context message. How much are
> > they really buying?
>
> Thanks. In that case, the message can only return the local and remote
> columns names and ignore the types (something like below). And the
> user will have to figure out what are the types of those columns in
> local and remote separately in case of error. Then the function
> logicalrep_typmap_gettypname can also be removed. I'm not sure if this
> is okay. Thoughts?
Hi Tom,
As suggested earlier, I'm attaching a v5 patch that avoids printing
the column type names in the context message thus no cache lookups
have to be done in the error context callback. I think the column name
is enough to know on which column the error occurred and if required
it's type can be known by the user. This patch gets rid of printing
local and remote type names in slot_store_error_callback and also
logicalrep_typmap_gettypname because it's unnecessary. I'm not sure if
this solution is acceptable. Thoughts?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From a65e13d591bb151eb71eb8ec42834ce28a3db304 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
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
