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

I wrote:
> Didn't look at 0002 yet.

... and now that I have, I don't like it much.  It adds a lot of
complexity, plus some lookup cycles that might be wasted.  I experimented
with looking into the range table as I suggested previously, and
that seems to work; see attached.  (This includes a little bit of
code cleanup along with the bug fix proper.)

An interesting point here is that the range table data will represent
table and column aliases, not necessarily their true names.  I don't
find that wrong, it's just different from what the code presently
does.  If we go with this, likely we should change the plain-relation
code path so that it also prints aliases from the RTE instead of
the actual names.

            regards, tom lane

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index fafbab6b02..b40c331f8f 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -7185,6 +7185,10 @@ 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 info from the
+ * relcache entry (for a simple scan) or the query rangetable (for joins).
  */
 static void
 conversion_error_callback(void *arg)
@@ -7198,10 +7202,14 @@ conversion_error_callback(void *arg)
     {
         /* 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)
+        {
+            Form_pg_attribute attr = TupleDescAttr(tupdesc,
+                                                   errpos->cur_attno - 1);
+
             attname = NameStr(attr->attname);
+        }
         else if (errpos->cur_attno == SelfItemPointerAttributeNumber)
             attname = "ctid";

@@ -7220,35 +7228,32 @@ 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;
+            RangeTblEntry *rte = exec_rt_fetch(var->varno, estate);

-            rte = exec_rt_fetch(var->varno, estate);
+            relname = rte->eref->aliasname;

             if (var->varattno == 0)
                 is_wholerow = true;
-            else
-                attname = get_attname(rte->relid, var->varattno, false);
-
-            relname = get_rel_name(rte->relid);
+            else if (var->varattno > 0 &&
+                     var->varattno <= list_length(rte->eref->colnames))
+                attname = strVal(list_nth(rte->eref->colnames,
+                                          var->varattno - 1));
         }
-        else
-            errcontext("processing expression at position %d in select list",
-                       errpos->cur_attno);
     }

-    if (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 (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);
 }

 /*