Fix null-pointer crash in postgres_fdw's conversion_error_callback.

Commit c7b7311f6 adjusted conversion_error_callback to always use
information from the query's rangetable, to avoid doing catalog lookups
in an already-failed transaction.  However, as a result of the utterly
inadequate documentation for make_tuple_from_result_row, I failed to
realize that fsstate could be NULL in some contexts.  That led to a
crash if we got a conversion error in such a context.  Fix by falling
back to the previous coding when fsstate is NULL.  Improve the
commentary, too.

Per report from Andrey Borodin.  Back-patch to 9.6, like the previous
patch.

Discussion: https://postgr.es/m/08916396-55E4-4D68-AB3A-BD6066F9E5C0@yandex-team.ru

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index c7b7db80650193b45f075831ee3a5668e7f545c5..44c4367b8f9bc399ed7575938bb3e0ab95ba0a63 100644 (file)
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -4200,6 +4200,9 @@ 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
+ANALYZE ft1; -- ERROR
+ERROR:  invalid input syntax for type integer: "foo"
+CONTEXT:  column "c8" of foreign table "ft1"
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
 -- ===================================================================
 -- subtransaction

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 76d4fea21c4df1d18ea9877ae3568990d5579d5f..45a09337d08e10cd98fa147507f95c6cb7251de5 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -303,7 +303,8 @@ typedef struct
 typedef struct ConversionLocation
 {
    AttrNumber  cur_attno;      /* attribute number being processed, or 0 */
-   ForeignScanState *fsstate;  /* plan node being processed */
+   Relation    rel;            /* foreign table being processed, or NULL */
+   ForeignScanState *fsstate;  /* plan node being processed, or NULL */
 } ConversionLocation;
 
 /* Callback argument for ec_member_matches_foreign */
@@ -7113,7 +7114,12 @@ complete_pending_request(AsyncRequest *areq)
  * rel is the local representation of the foreign table, attinmeta is
  * conversion data for the rel's tupdesc, and retrieved_attrs is an
  * integer list of the table column numbers present in the PGresult.
+ * fsstate is the ForeignScan plan node's execution state.
  * temp_context is a working context that can be reset after each tuple.
+ *
+ * Note: either rel or fsstate, but not both, can be NULL.  rel is NULL
+ * if we're processing a remote join, while fsstate is NULL in a non-query
+ * context such as ANALYZE, or if we're processing a non-scan query node.
  */
 static HeapTuple
 make_tuple_from_result_row(PGresult *res,
@@ -7144,6 +7150,10 @@ make_tuple_from_result_row(PGresult *res,
     */
    oldcontext = MemoryContextSwitchTo(temp_context);
 
+   /*
+    * Get the tuple descriptor for the row.  Use the rel's tupdesc if rel is
+    * provided, otherwise look to the scan node's ScanTupleSlot.
+    */
    if (rel)
        tupdesc = RelationGetDescr(rel);
    else
@@ -7161,6 +7171,7 @@ make_tuple_from_result_row(PGresult *res,
     * Set up and install callback to report where conversion error occurs.
     */
    errpos.cur_attno = 0;
+   errpos.rel = rel;
    errpos.fsstate = fsstate;
    errcallback.callback = conversion_error_callback;
    errcallback.arg = (void *) &errpos;
@@ -7265,60 +7276,87 @@ make_tuple_from_result_row(PGresult *res,
  *
  * 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.
+ * from the relation or the query's rangetable instead.
  */
 static void
 conversion_error_callback(void *arg)
 {
    ConversionLocation *errpos = (ConversionLocation *) arg;
+   Relation    rel = errpos->rel;
    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;
 
-   if (fsplan->scan.scanrelid > 0)
-   {
-       /* error occurred in a scan against a foreign table */
-       varno = fsplan->scan.scanrelid;
-       colno = errpos->cur_attno;
-   }
-   else
+   /*
+    * If we're in a scan node, always use aliases from the rangetable, for
+    * consistency between the simple-relation and remote-join cases.  Look at
+    * the relation's tupdesc only if we're not in a scan node.
+    */
+   if (fsstate)
    {
-       /* error occurred in a scan against a foreign join */
-       TargetEntry *tle;
+       /* ForeignScan case */
+       ForeignScan *fsplan = castNode(ForeignScan, fsstate->ss.ps.plan);
+       int         varno = 0;
+       AttrNumber  colno = 0;
 
-       tle = list_nth_node(TargetEntry, fsplan->fdw_scan_tlist,
-                           errpos->cur_attno - 1);
+       if (fsplan->scan.scanrelid > 0)
+       {
+           /* error occurred in a scan against a foreign table */
+           varno = fsplan->scan.scanrelid;
+           colno = errpos->cur_attno;
+       }
+       else
+       {
+           /* error occurred in a scan against a foreign join */
+           TargetEntry *tle;
 
-       /*
-        * Target list can have Vars and expressions.  For Vars, we can get
-        * some information, however for expressions we can't.  Thus for
-        * expressions, just show generic context message.
-        */
-       if (IsA(tle->expr, Var))
+           tle = list_nth_node(TargetEntry, fsplan->fdw_scan_tlist,
+                               errpos->cur_attno - 1);
+
+           /*
+            * Target list can have Vars and expressions.  For Vars, we can
+            * get some information, however for expressions we can't.  Thus
+            * for expressions, just show generic context message.
+            */
+           if (IsA(tle->expr, Var))
+           {
+               Var        *var = (Var *) tle->expr;
+
+               varno = var->varno;
+               colno = var->varattno;
+           }
+       }
+
+       if (varno > 0)
        {
-           Var        *var = (Var *) tle->expr;
+           EState     *estate = fsstate->ss.ps.state;
+           RangeTblEntry *rte = exec_rt_fetch(varno, estate);
 
-           varno = var->varno;
-           colno = var->varattno;
+           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 (varno > 0)
+   else if (rel)
    {
-       EState     *estate = fsstate->ss.ps.state;
-       RangeTblEntry *rte = exec_rt_fetch(varno, estate);
+       /* Non-ForeignScan case (we should always have a rel here) */
+       TupleDesc   tupdesc = RelationGetDescr(rel);
 
-       relname = rte->eref->aliasname;
+       relname = RelationGetRelationName(rel);
+       if (errpos->cur_attno > 0 && errpos->cur_attno <= tupdesc->natts)
+       {
+           Form_pg_attribute attr = TupleDescAttr(tupdesc,
+                                                  errpos->cur_attno - 1);
 
-       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 = NameStr(attr->attname);
+       }
+       else if (errpos->cur_attno == SelfItemPointerAttributeNumber)
            attname = "ctid";
    }
 

diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 38f4a7837fe7f1303ab3e641a3bd9d31983d0638..e7b869f8ceac91e51c1cce6c952d491f5cfc0e7b 100644 (file)
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1164,6 +1164,7 @@ SELECT ftx.x1, ft2.c2, ftx.x8 FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
 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
+ANALYZE ft1; -- ERROR
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
 
 -- ===================================================================