On Wed, Jan 27, 2021 at 7:48 AM Zhihong Yu <z...@yugabyte.com> wrote:
> For 0002, a few comments on the description:
>
> bq. Avoid accessing system catalogues inside conversion_error_callback
>
> catalogues -> catalog
>
> bq. error context callback, because the the entire transaction might
>
> There is redundant 'the'
>
> bq. Since get_attname() and get_rel_name() could search the sys cache by
> store the required information
>
> store -> storing
Thanks for pointing to the changes in the commit message. I corrected
them. Attaching v4 patch set, consider it for further review.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 88767e3176cb6758ac3dd0a0da91f3747f84a4ed Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Tue, 26 Jan 2021 13:49:18 +0530
Subject: [PATCH v4] 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.
Since logicalrep_typmap_gettypname() could search the sys cache by
calling to format_type_be(), store both local and remote type names
to SlotErrCallbackArg so that we can just set the names in the error
callback without sys cache lookup.
Author: Masahiko Sawada
---
src/backend/replication/logical/worker.c | 48 +++++++++++++-----------
1 file changed, 27 insertions(+), 21 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index eb7db89cef..f7c4fe9c4d 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -132,8 +132,9 @@ static dlist_head lsn_mapping = DLIST_STATIC_INIT(lsn_mapping);
typedef struct SlotErrCallbackArg
{
LogicalRepRelMapEntry *rel;
- int local_attnum;
int remote_attnum;
+ char *local_typename;
+ char *remote_typename;
} SlotErrCallbackArg;
/*
@@ -430,30 +431,19 @@ static void
slot_store_error_callback(void *arg)
{
SlotErrCallbackArg *errarg = (SlotErrCallbackArg *) arg;
- LogicalRepRelMapEntry *rel;
- char *remotetypname;
- Oid remotetypoid,
- localtypoid;
+ LogicalRepRelMapEntry *rel = errarg->rel;
/* 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);
+ Assert(errarg->local_typename && errarg->remote_typename);
errcontext("processing remote data for replication target relation \"%s.%s\" column \"%s\", "
"remote type %s, local type %s",
rel->remoterel.nspname, rel->remoterel.relname,
rel->remoterel.attnames[errarg->remote_attnum],
- remotetypname,
- format_type_be(localtypoid));
+ errarg->remote_typename, errarg->local_typename);
}
/*
@@ -474,8 +464,9 @@ 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;
+ errarg.local_typename = NULL;
+ errarg.remote_typename = NULL;
errcallback.callback = slot_store_error_callback;
errcallback.arg = (void *) &errarg;
errcallback.previous = error_context_stack;
@@ -491,11 +482,17 @@ slot_store_data(TupleTableSlot *slot, LogicalRepRelMapEntry *rel,
if (!att->attisdropped && remoteattnum >= 0)
{
StringInfo colvalue = &tupleData->colvalues[remoteattnum];
+ Oid remotetypid = rel->remoterel.atttyps[remoteattnum];
Assert(remoteattnum < tupleData->ncols);
- errarg.local_attnum = i;
+ /*
+ * Set the attribute number and both local and remote type names
+ * from the local sys cache and LogicalRepTypMap respectively.
+ */
errarg.remote_attnum = remoteattnum;
+ errarg.local_typename = format_type_be(att->atttypid);
+ errarg.remote_typename = logicalrep_typmap_gettypname(remotetypid);
if (tupleData->colstatus[remoteattnum] == LOGICALREP_COLUMN_TEXT)
{
@@ -543,8 +540,9 @@ slot_store_data(TupleTableSlot *slot, LogicalRepRelMapEntry *rel,
slot->tts_isnull[i] = true;
}
- errarg.local_attnum = -1;
errarg.remote_attnum = -1;
+ errarg.local_typename = NULL;
+ errarg.remote_typename = NULL;
}
else
{
@@ -600,8 +598,9 @@ 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;
+ errarg.local_typename = NULL;
+ errarg.remote_typename = NULL;
errcallback.callback = slot_store_error_callback;
errcallback.arg = (void *) &errarg;
errcallback.previous = error_context_stack;
@@ -622,9 +621,15 @@ slot_modify_data(TupleTableSlot *slot, TupleTableSlot *srcslot,
if (tupleData->colstatus[remoteattnum] != LOGICALREP_COLUMN_UNCHANGED)
{
StringInfo colvalue = &tupleData->colvalues[remoteattnum];
+ Oid remotetypid = rel->remoterel.atttyps[remoteattnum];
- errarg.local_attnum = i;
+ /*
+ * Set the attribute number and both local and remote type names
+ * from the local sys cache and LogicalRepTypMap respectively.
+ */
errarg.remote_attnum = remoteattnum;
+ errarg.local_typename = format_type_be(att->atttypid);
+ errarg.remote_typename = logicalrep_typmap_gettypname(remotetypid);
if (tupleData->colstatus[remoteattnum] == LOGICALREP_COLUMN_TEXT)
{
@@ -668,8 +673,9 @@ slot_modify_data(TupleTableSlot *slot, TupleTableSlot *srcslot,
slot->tts_isnull[i] = true;
}
- errarg.local_attnum = -1;
errarg.remote_attnum = -1;
+ errarg.local_typename = NULL;
+ errarg.remote_typename = NULL;
}
}
--
2.25.1
From 14ad2079ee307a3e5b0752f79e35d32b5144159e Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Wed, 27 Jan 2021 08:56:53 +0530
Subject: [PATCH v4] Avoid Catalogue Accesses In conversion_error_callback
Avoid accessing system catalogs inside conversion_error_callback
error context callback, because the entire transaction might
have been broken at this point.
Since get_attname() and get_rel_name() could search the sys cache by
storing the required information into ConversionLocation so that we can
just use that info in the error callback without sys cache lookup.
Author: Bharath Rupireddy
---
contrib/postgres_fdw/postgres_fdw.c | 97 ++++++++++++++++++-----------
1 file changed, 62 insertions(+), 35 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 2ce42ce3f1..69d24ca6be 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -293,16 +293,10 @@ typedef struct
*/
typedef struct ConversionLocation
{
- Relation rel; /* foreign table's relcache entry. */
+ const char *relname; /* relation name */
AttrNumber cur_attno; /* attribute number being processed, or 0 */
-
- /*
- * In case of foreign join push down, fdw_scan_tlist is used to identify
- * the Var node corresponding to the error location and
- * fsstate->ss.ps.state gives access to the RTEs of corresponding relation
- * to get the relation name and attribute name.
- */
- ForeignScanState *fsstate;
+ const char *attname; /* attribute name */
+ bool is_wholerow; /* whole-row reference to foreign table? */
} ConversionLocation;
/* Callback argument for ec_member_matches_foreign */
@@ -499,6 +493,9 @@ static HeapTuple make_tuple_from_result_row(PGresult *res,
ForeignScanState *fsstate,
MemoryContext temp_context);
static void conversion_error_callback(void *arg);
+static void set_conversion_error_callback_info(ConversionLocation *errpos,
+ Relation rel, int cur_attno,
+ ForeignScanState *fsstate);
static bool foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel,
JoinType jointype, RelOptInfo *outerrel, RelOptInfo *innerrel,
JoinPathExtraData *extra);
@@ -6522,9 +6519,10 @@ make_tuple_from_result_row(PGresult *res,
/*
* Set up and install callback to report where conversion error occurs.
*/
- errpos.rel = rel;
errpos.cur_attno = 0;
- errpos.fsstate = fsstate;
+ errpos.attname = NULL;
+ errpos.relname = NULL;
+ errpos.is_wholerow = false;
errcallback.callback = conversion_error_callback;
errcallback.arg = (void *) &errpos;
errcallback.previous = error_context_stack;
@@ -6545,12 +6543,20 @@ make_tuple_from_result_row(PGresult *res,
else
valstr = PQgetvalue(res, row, j);
+ /*
+ * Set error context callback info, so that we could avoid accessing
+ * the system catalogs while processing the error in
+ * conversion_error_callback. System catalog accesses are not safe in
+ * error context callbacks because the transaction might have been
+ * broken by then.
+ */
+ set_conversion_error_callback_info(&errpos, rel, i, fsstate);
+
/*
* convert value to internal representation
*
* Note: we ignore system columns other than ctid and oid in result
*/
- errpos.cur_attno = i;
if (i > 0)
{
/* ordinary column */
@@ -6573,7 +6579,12 @@ make_tuple_from_result_row(PGresult *res,
ctid = (ItemPointer) DatumGetPointer(datum);
}
}
+
+ /* Reset error context callback info */
errpos.cur_attno = 0;
+ errpos.attname = NULL;
+ errpos.relname = NULL;
+ errpos.is_wholerow = false;
j++;
}
@@ -6623,40 +6634,39 @@ make_tuple_from_result_row(PGresult *res,
}
/*
- * Callback function which is called when error occurs during column value
- * conversion. Print names of column and relation.
+ * Set the information required by the error context callback function, that is
+ * conversion_error_callback.
*/
static void
-conversion_error_callback(void *arg)
+set_conversion_error_callback_info(ConversionLocation *errpos, Relation rel,
+ int cur_attno, ForeignScanState *fsstate)
{
const char *attname = NULL;
const char *relname = NULL;
- bool is_wholerow = false;
- ConversionLocation *errpos = (ConversionLocation *) arg;
+ bool is_wholerow = false;
- if (errpos->rel)
+ if (rel)
{
- /* error occurred in a scan against a foreign table */
- TupleDesc tupdesc = RelationGetDescr(errpos->rel);
- Form_pg_attribute attr = TupleDescAttr(tupdesc, errpos->cur_attno - 1);
+ /* Scan against a foreign table */
+ TupleDesc tupdesc = RelationGetDescr(rel);
+ Form_pg_attribute attr = TupleDescAttr(tupdesc, cur_attno - 1);
- if (errpos->cur_attno > 0 && errpos->cur_attno <= tupdesc->natts)
+ if (cur_attno > 0 && cur_attno <= tupdesc->natts)
attname = NameStr(attr->attname);
- else if (errpos->cur_attno == SelfItemPointerAttributeNumber)
+ else if (cur_attno == SelfItemPointerAttributeNumber)
attname = "ctid";
- relname = RelationGetRelationName(errpos->rel);
+ relname = RelationGetRelationName(rel);
}
else
{
- /* error occurred in a scan against a foreign join */
- ForeignScanState *fsstate = errpos->fsstate;
+ /* Scan against a foreign join */
ForeignScan *fsplan = castNode(ForeignScan, fsstate->ss.ps.plan);
EState *estate = fsstate->ss.ps.state;
TargetEntry *tle;
tle = list_nth_node(TargetEntry, fsplan->fdw_scan_tlist,
- errpos->cur_attno - 1);
+ cur_attno - 1);
/*
* Target list can have Vars and expressions. For Vars, we can get
@@ -6677,18 +6687,35 @@ conversion_error_callback(void *arg)
relname = get_rel_name(rte->relid);
}
- else
- errcontext("processing expression at position %d in select list",
- errpos->cur_attno);
}
- if (relname)
+ errpos->cur_attno = cur_attno;
+ errpos->attname = attname;
+ errpos->relname = relname;
+ errpos->is_wholerow = is_wholerow;
+}
+
+/*
+ * Callback function which is called when error occurs during column value
+ * conversion. Print names of column and relation.
+ */
+static void
+conversion_error_callback(void *arg)
+{
+ ConversionLocation *errpos = (ConversionLocation *) arg;
+
+ if (errpos->relname)
{
- if (is_wholerow)
- errcontext("whole-row reference to foreign table \"%s\"", relname);
- else if (attname)
- errcontext("column \"%s\" of foreign table \"%s\"", attname, relname);
+ if (errpos->is_wholerow)
+ errcontext("whole-row reference to foreign table \"%s\"",
+ errpos->relname);
+ else if (errpos->attname)
+ errcontext("column \"%s\" of foreign table \"%s\"",
+ errpos->attname, errpos->relname);
}
+ else
+ errcontext("processing expression at position %d in select list",
+ errpos->cur_attno);
}
/*
--
2.25.1
