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 6a19c5cd08a0b1063701d8d7c6d8effe05da0b12..22e4ce1241e7d86101c4a8945a980ebded5aa96e 100644 (file)
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2880,6 +2880,9 @@ 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 integer: "foo"
 CONTEXT:  whole-row reference to foreign table "ftx"
+ANALYZE ft1; -- ERROR
+ERROR:  invalid input syntax for 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 07e0c9b6d6dc009b2edaab35499b0a4f88149d3d..d6bf258b794466c91366a91d3bbb956908f212f3 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -244,7 +244,8 @@ typedef struct PgFdwAnalyzeState
 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 */
@@ -4396,7 +4397,12 @@ postgresGetForeignJoinPaths(PlannerInfo *root,
  * 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,
@@ -4427,6 +4433,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
@@ -4447,6 +4457,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;
@@ -4547,65 +4558,92 @@ 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 = (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;
-
-               Assert(IsA(fsplan, ForeignScan));
-               tle = (TargetEntry *) list_nth(fsplan->fdw_scan_tlist,
-                                                                          errpos->cur_attno - 1);
-               Assert(IsA(tle, TargetEntry));
+               /* ForeignScan case */
+               ForeignScan *fsplan = (ForeignScan *) fsstate->ss.ps.plan;
+               int                     varno = 0;
+               AttrNumber      colno = 0;
 
-               /*
-                * 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))
+               if (fsplan->scan.scanrelid > 0)
+               {
+                       /* error occurred in a scan against a foreign table */
+                       varno = fsplan->scan.scanrelid;
+                       colno = errpos->cur_attno;
+               }
+               else
                {
-                       Var                *var = (Var *) tle->expr;
+                       /* error occurred in a scan against a foreign join */
+                       TargetEntry *tle;
+
+                       Assert(IsA(fsplan, ForeignScan));
+                       tle = (TargetEntry *) list_nth(fsplan->fdw_scan_tlist,
+                                                                                  errpos->cur_attno - 1);
+                       Assert(IsA(tle, TargetEntry));
+
+                       /*
+                        * 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;
+                       }
+               }
 
-                       varno = var->varno;
-                       colno = var->varattno;
+               if (varno > 0)
+               {
+                       EState     *estate = fsstate->ss.ps.state;
+                       RangeTblEntry *rte = rt_fetch(varno, estate->es_range_table);
+
+                       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";
+                       else if (colno == ObjectIdAttributeNumber)
+                               attname = "oid";
                }
        }
-
-       if (varno > 0)
+       else if (rel)
        {
-               EState     *estate = fsstate->ss.ps.state;
-               RangeTblEntry *rte = rt_fetch(varno, estate->es_range_table);
+               /* 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";
-               else if (colno == ObjectIdAttributeNumber)
-                       attname = "oid";
        }
 
        if (relname && is_wholerow)

diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index fbefd06d4bcc616d404bc7a52069d1fd8e035daa..54fe596bd64e2640395fa6b03f0c05115ee5ac2b 100644 (file)
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -707,6 +707,7 @@ 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
+ANALYZE ft1; -- ERROR
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
 
 -- ===================================================================