rdelaage
/
postgresql
mirror of https://git.postgresql.org/git/postgresql.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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 0de8f9bc86
commit 36c9f7d96e
3 changed files with 82 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
contrib/postgres_fdw/expected/postgres_fdw.out
View File

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

118
contrib/postgres_fdw/postgres_fdw.c
View File

@ -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)
/*
* 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 = (ForeignScan *) fsstate->ss.ps.plan;
int varno = 0;
AttrNumber colno = 0;
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))
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;
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;
}
}
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)

1
contrib/postgres_fdw/sql/postgres_fdw.sql
View File

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