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
This commit is contained in:
Tom Lane 2021-10-06 15:50:24 -04:00
parent db692b0c84
commit 3071bbfe44
3 changed files with 78 additions and 36 deletions

View File

@ -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

View File

@ -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)
/*
* 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 table */
varno = fsplan->scan.scanrelid;
colno = errpos->cur_attno;
}
else
{
/* 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);
/*
* 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)
{
Var *var = (Var *) tle->expr;
/* 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;
varno = var->varno;
colno = var->varattno;
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)
{
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 (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";
}

View File

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