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;
 
 -- ===================================================================