On Tue, Jan 12, 2021 at 9:40 AM Masahiko Sawada <sawada.m...@gmail.com> wrote:
>
> On Mon, Jan 11, 2021 at 4:46 PM Bharath Rupireddy
> <bharath.rupireddyforpostg...@gmail.com> wrote:
> >
> > On Mon, Jan 11, 2021 at 10:56 AM Masahiko Sawada <sawada.m...@gmail.com>
> > wrote:
> > > Agreed. Attached the updated patch.
> >
> > Thanks for the updated patch. Looks like the comment crosses the 80
> > char limit, I have corrected it. And also changed the variable name
> > from remotetypeid to remotetypid, so that logicalrep_typmap_gettypname
> > will not cross the 80 char limit. And also added a commit message.
> > Attaching v3 patch, please have a look. Both make check and make
> > check-world passes.
>
> Thanks! The change looks good to me.
Thanks!
> > > > I quickly searched in places where error callbacks are being used, I
> > > > think we need a similar kind of fix for conversion_error_callback in
> > > > postgres_fdw.c, because get_attname and get_rel_name are being used
> > > > which do catalogue lookups. ISTM that all the other error callbacks
> > > > are good in the sense that they are not doing sys catalogue lookups.
> > >
> > > Indeed. If we need to disallow the catalog lookup during executing
> > > error callbacks we might want to have an assertion checking that in
> > > SearchCatCacheInternal(), in addition to Assert(IsTransactionState()).
> >
> > I tried to add(as attached in
> > v1-0001-Avoid-Sys-Cache-Lookups-in-Error-Callbacks.patch) the
> > Assert(!error_context_stack); in SearchCatCacheInternal, initdb itself
> > fails [1]. That means, we are doing a bunch of catalogue access from
> > error context callbacks. Given this, I'm not quite sure whether we can
> > have such an assertion in SearchCatCacheInternal.
>
> I think checking !error_context_stack is not a correct check if we're
> executing an error context callback since it's a stack to store
> callbacks. It can be non-NULL by setting an error callback, see
> setup_parser_errposition_callback() for instance.
Thanks! Yes, you are right, even though we are not processing the
actual error callback, the error_context_stack can be non-null, hence
the Assert(!error_context_stack); doesn't make any sense.
> Probably we need to check if (recursion_depth > 0) and elevel.
> Attached a patch for that as an example.
IIUC, we must be knowing in SearchCatCacheInternal, whether errstart
is called with elevel >= ERROR and we have recursion_depth > 0. If
both are true, then the assertion in SearchCatCacheInternal should
fail. I see that in your patch, elevel is being fetched from
errordata, that's fine. What I'm not quite clear is the following
assumption:
+ /* If we doesn't set any error data yet, assume it's an error */
+ if (errordata_stack_depth == -1)
+ return true;
Is it always true that we are in error processing when
errordata_stack_depth is -1, what happens if errordata_stack_depth <
-1? Maybe I'm missing something.
IMHO, adding an assertion in SearchCatCacheInternal(which is a most
generic code part within the server) with a few error context global
variables may not be always safe. Because we might miss using the
error context variables properly. Instead, we could add a comment in
ErrorContextCallback structure saying something like, "it's not
recommended to access any system catalogues within an error context
callback when the callback is expected to be called while processing
an error, because the transaction might have been broken in that
case." And let the future callback developers take care of it.
Thoughts?
As I said earlier [1], currently only two(there could be more) error
context callbacks access the sys catalogues, one is in
slot_store_error_callback which will be fixed with your patch. Another
is in conversion_error_callback, we can also fix this by storing the
relname, attname and other required info in ConversionLocation,
something like the attached. Please have a look.
Thoughts?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 8212d2d635096096211d4330ef08d6a21d4ad568 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Tue, 12 Jan 2021 14:42:45 +0530
Subject: [PATCH v1] Avoid Catalogue Accesses In conversion_error_callback
Avoid accessing system catalogues inside conversion_error_callback
error context callback, because the the entire transaction might
have been broken at this point.
Since get_attname() and get_rel_name() could search the sys cache by
store 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 | 102 ++++++++++++++++++----------
1 file changed, 68 insertions(+), 34 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 2f2d4d171c..56735821fe 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -283,16 +283,11 @@ 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? */
+ bool show_generic_message; /* show generic context message? */
} ConversionLocation;
/* Callback argument for ec_member_matches_foreign */
@@ -479,6 +474,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_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);
@@ -6347,9 +6345,11 @@ 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;
+ errpos.show_generic_message = false;
errcallback.callback = conversion_error_callback;
errcallback.arg = (void *) &errpos;
errcallback.previous = error_context_stack;
@@ -6370,12 +6370,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 catalogues while processing the error in
+ * conversion_error_callback. System catalogue accesses are not safe in
+ * error context callbacks because the transaction might have been
+ * broken by then.
+ */
+ set_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 */
@@ -6398,7 +6406,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;
+ errpos.show_generic_message = false;
j++;
}
@@ -6448,40 +6461,40 @@ 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_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;
+ bool show_generic_message = 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
@@ -6503,16 +6516,37 @@ conversion_error_callback(void *arg)
relname = get_rel_name(rte->relid);
}
else
- errcontext("processing expression at position %d in select list",
- errpos->cur_attno);
+ show_generic_message = true;
}
- if (relname)
+ errpos->cur_attno = cur_attno;
+ errpos->attname = attname;
+ errpos->relname = relname;
+ errpos->is_wholerow = is_wholerow;
+ errpos->show_generic_message = show_generic_message;
+}
+
+/*
+ * 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->show_generic_message)
+ errcontext("processing expression at position %d in select list",
+ errpos->cur_attno);
+
+ 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);
}
}
--
2.25.1
