Re: logical replication worker accesses catalogs in error context callback - Mailing list pgsql-hackers

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> The patch basically looks good to me except a minor comment to have a
> local variable for var->varattno which makes the code shorter.

Here's a restructured version that uses rangetable data for the
simple-relation case too.  I also modified the relevant test cases
so that it's visible that we're reporting aliases not true names.

            regards, tom lane

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 31b5de91ad..25112df916 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -4096,15 +4096,17 @@ DROP TABLE reind_fdw_parent;
 -- conversion error
 -- ===================================================================
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE int;
-SELECT * FROM ft1 WHERE c1 = 1;  -- ERROR
+SELECT * FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8) WHERE x1 = 1;  -- ERROR
 ERROR:  invalid input syntax for type integer: "foo"
-CONTEXT:  column "c8" of foreign table "ft1"
-SELECT  ft1.c1,  ft2.c2, ft1.c8 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
+CONTEXT:  column "x8" of foreign table "ftx"
+SELECT ftx.x1, ft2.c2, ftx.x8 FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
+  WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
 ERROR:  invalid input syntax for type integer: "foo"
-CONTEXT:  column "c8" of foreign table "ft1"
-SELECT  ft1.c1,  ft2.c2, ft1 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
+CONTEXT:  column "x8" of foreign table "ftx"
+SELECT ftx.x1, ft2.c2, ftx FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
+  WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
 ERROR:  invalid input syntax for type integer: "foo"
-CONTEXT:  whole-row reference to foreign table "ft1"
+CONTEXT:  whole-row reference to foreign table "ftx"
 SELECT sum(c2), array_agg(c8) FROM ft1 GROUP BY c8; -- ERROR
 ERROR:  invalid input syntax for type integer: "foo"
 CONTEXT:  processing expression at position 2 in select list
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index fafbab6b02..696277ba10 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -302,16 +302,8 @@ typedef struct
  */
 typedef struct ConversionLocation
 {
-    Relation    rel;            /* foreign table's relcache entry. */
     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;
+    ForeignScanState *fsstate;    /* plan node being processed */
 } ConversionLocation;

 /* Callback argument for ec_member_matches_foreign */
@@ -7082,7 +7074,6 @@ 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;
     errcallback.callback = conversion_error_callback;
@@ -7185,34 +7176,32 @@ 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.
+ *
+ * Note that this function mustn't do any catalog lookups, since we are in
+ * an already-failed transaction.  Fortunately, we can get the needed info
+ * from the query's rangetable instead.
  */
 static void
 conversion_error_callback(void *arg)
 {
+    ConversionLocation *errpos = (ConversionLocation *) arg;
+    ForeignScanState *fsstate = errpos->fsstate;
+    ForeignScan *fsplan = castNode(ForeignScan, fsstate->ss.ps.plan);
+    int            varno = 0;
+    AttrNumber    colno = 0;
     const char *attname = NULL;
     const char *relname = NULL;
     bool        is_wholerow = false;
-    ConversionLocation *errpos = (ConversionLocation *) arg;

-    if (errpos->rel)
+    if (fsplan->scan.scanrelid > 0)
     {
         /* error occurred in a scan against a foreign table */
-        TupleDesc    tupdesc = RelationGetDescr(errpos->rel);
-        Form_pg_attribute attr = TupleDescAttr(tupdesc, errpos->cur_attno - 1);
-
-        if (errpos->cur_attno > 0 && errpos->cur_attno <= tupdesc->natts)
-            attname = NameStr(attr->attname);
-        else if (errpos->cur_attno == SelfItemPointerAttributeNumber)
-            attname = "ctid";
-
-        relname = RelationGetRelationName(errpos->rel);
+        varno = fsplan->scan.scanrelid;
+        colno = errpos->cur_attno;
     }
     else
     {
         /* error occurred in a scan against a foreign join */
-        ForeignScanState *fsstate = errpos->fsstate;
-        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,
@@ -7220,35 +7209,42 @@ conversion_error_callback(void *arg)

         /*
          * Target list can have Vars and expressions.  For Vars, we can get
-         * its relation, however for expressions we can't.  Thus for
+         * some information, however for expressions we can't.  Thus for
          * expressions, just show generic context message.
          */
         if (IsA(tle->expr, Var))
         {
-            RangeTblEntry *rte;
             Var           *var = (Var *) tle->expr;

-            rte = exec_rt_fetch(var->varno, estate);
-
-            if (var->varattno == 0)
-                is_wholerow = true;
-            else
-                attname = get_attname(rte->relid, var->varattno, false);
-
-            relname = get_rel_name(rte->relid);
+            varno = var->varno;
+            colno = var->varattno;
         }
-        else
-            errcontext("processing expression at position %d in select list",
-                       errpos->cur_attno);
     }

-    if (relname)
+    if (varno > 0)
     {
-        if (is_wholerow)
-            errcontext("whole-row reference to foreign table \"%s\"", relname);
-        else if (attname)
-            errcontext("column \"%s\" of foreign table \"%s\"", attname, relname);
+        EState       *estate = fsstate->ss.ps.state;
+        RangeTblEntry *rte = exec_rt_fetch(varno, estate);
+
+        relname = rte->eref->aliasname;
+
+        if (colno == 0)
+            is_wholerow = true;
+        else if (colno > 0 &&
+                 colno <= list_length(rte->eref->colnames))
+            attname = strVal(list_nth(rte->eref->colnames,
+                                      colno - 1));
+        else if (colno == SelfItemPointerAttributeNumber)
+            attname = "ctid";
     }
+
+    if (relname && is_wholerow)
+        errcontext("whole-row reference to foreign table \"%s\"", relname);
+    else if (relname && attname)
+        errcontext("column \"%s\" of foreign table \"%s\"", attname, relname);
+    else
+        errcontext("processing expression at position %d in select list",
+                   errpos->cur_attno);
 }

 /*
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 286dd99573..95862e38ed 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1129,9 +1129,11 @@ DROP TABLE reind_fdw_parent;
 -- conversion error
 -- ===================================================================
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE int;
-SELECT * FROM ft1 WHERE c1 = 1;  -- ERROR
-SELECT  ft1.c1,  ft2.c2, ft1.c8 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
-SELECT  ft1.c1,  ft2.c2, ft1 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
+SELECT * FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8) WHERE x1 = 1;  -- ERROR
+SELECT ftx.x1, ft2.c2, ftx.x8 FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
+  WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
+SELECT ftx.x1, ft2.c2, ftx FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
+  WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
 SELECT sum(c2), array_agg(c8) FROM ft1 GROUP BY c8; -- ERROR
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;