Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 67f925b

Browse filesBrowse files
tglsfdctglsfdc
committed
Avoid doing catalog lookups in postgres_fdw's conversion_error_callback.
As in 50371df, this is a bad idea since the callback can't really know what error is being thrown and thus whether or not it is safe to attempt catalog accesses. Rather than pushing said accesses into the mainline code where they'd usually be a waste of cycles, we can look at the query's rangetable instead. This change does mean that we'll be printing query aliases (if any were used) rather than the table or column's true name. But that doesn't seem like a bad thing: it's certainly a more useful definition in self-join cases, for instance. In any case, it seems unlikely that any applications would be depending on this detail, so it seems safe to change. Patch by me. Original complaint by Andres Freund; Bharath Rupireddy noted the connection to conversion_error_callback. Discussion: https://postgr.es/m/20210106020229.ne5xnuu6wlondjpe@alap3.anarazel.de
1 parent c1123be commit 67f925b
Copy full SHA for 67f925b

File tree

3 files changed

+59
-48
lines changed
Filter options

3 files changed

+59
-48
lines changed

‎contrib/postgres_fdw/expected/postgres_fdw.out

Copy file name to clipboardExpand all lines: contrib/postgres_fdw/expected/postgres_fdw.out
+8-6Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2869,15 +2869,17 @@ DROP FUNCTION f_test(int);
28692869
-- conversion error
28702870
-- ===================================================================
28712871
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE int;
2872-
SELECT * FROM ft1 WHERE c1 = 1; -- ERROR
2872+
SELECT * FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8) WHERE x1 = 1; -- ERROR
28732873
ERROR: invalid input syntax for integer: "foo"
2874-
CONTEXT: column "c8" of foreign table "ft1"
2875-
SELECT ft1.c1, ft2.c2, ft1.c8 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
2874+
CONTEXT: column "x8" of foreign table "ftx"
2875+
SELECT ftx.x1, ft2.c2, ftx.x8 FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
2876+
WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
28762877
ERROR: invalid input syntax for integer: "foo"
2877-
CONTEXT: column "c8" of foreign table "ft1"
2878-
SELECT ft1.c1, ft2.c2, ft1 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
2878+
CONTEXT: column "x8" of foreign table "ftx"
2879+
SELECT ftx.x1, ft2.c2, ftx FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
2880+
WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
28792881
ERROR: invalid input syntax for integer: "foo"
2880-
CONTEXT: whole-row reference to foreign table "ft1"
2882+
CONTEXT: whole-row reference to foreign table "ftx"
28812883
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
28822884
-- ===================================================================
28832885
-- subtransaction

‎contrib/postgres_fdw/postgres_fdw.c

Copy file name to clipboardExpand all lines: contrib/postgres_fdw/postgres_fdw.c
+46-39Lines changed: 46 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -243,16 +243,8 @@ typedef struct PgFdwAnalyzeState
243243
*/
244244
typedef struct ConversionLocation
245245
{
246-
Relation rel; /* foreign table's relcache entry. */
247246
AttrNumber cur_attno; /* attribute number being processed, or 0 */
248-
249-
/*
250-
* In case of foreign join push down, fdw_scan_tlist is used to identify
251-
* the Var node corresponding to the error location and
252-
* fsstate->ss.ps.state gives access to the RTEs of corresponding relation
253-
* to get the relation name and attribute name.
254-
*/
255-
ForeignScanState *fsstate;
247+
ForeignScanState *fsstate; /* plan node being processed */
256248
} ConversionLocation;
257249

258250
/* Callback argument for ec_member_matches_foreign */
@@ -4454,7 +4446,6 @@ make_tuple_from_result_row(PGresult *res,
44544446
/*
44554447
* Set up and install callback to report where conversion error occurs.
44564448
*/
4457-
errpos.rel = rel;
44584449
errpos.cur_attno = 0;
44594450
errpos.fsstate = fsstate;
44604451
errcallback.callback = conversion_error_callback;
@@ -4553,61 +4544,77 @@ make_tuple_from_result_row(PGresult *res,
45534544
/*
45544545
* Callback function which is called when error occurs during column value
45554546
* conversion. Print names of column and relation.
4547+
*
4548+
* Note that this function mustn't do any catalog lookups, since we are in
4549+
* an already-failed transaction. Fortunately, we can get the needed info
4550+
* from the query's rangetable instead.
45564551
*/
45574552
static void
45584553
conversion_error_callback(void *arg)
45594554
{
4555+
ConversionLocation *errpos = (ConversionLocation *) arg;
4556+
ForeignScanState *fsstate = errpos->fsstate;
4557+
ForeignScan *fsplan = (ForeignScan *) fsstate->ss.ps.plan;
4558+
int varno = 0;
4559+
AttrNumber colno = 0;
45604560
const char *attname = NULL;
45614561
const char *relname = NULL;
45624562
bool is_wholerow = false;
4563-
ConversionLocation *errpos = (ConversionLocation *) arg;
45644563

4565-
if (errpos->rel)
4564+
if (fsplan->scan.scanrelid > 0)
45664565
{
45674566
/* error occurred in a scan against a foreign table */
4568-
TupleDesc tupdesc = RelationGetDescr(errpos->rel);
4569-
4570-
if (errpos->cur_attno > 0 && errpos->cur_attno <= tupdesc->natts)
4571-
attname = NameStr(tupdesc->attrs[errpos->cur_attno - 1]->attname);
4572-
else if (errpos->cur_attno == SelfItemPointerAttributeNumber)
4573-
attname = "ctid";
4574-
4575-
relname = RelationGetRelationName(errpos->rel);
4567+
varno = fsplan->scan.scanrelid;
4568+
colno = errpos->cur_attno;
45764569
}
45774570
else
45784571
{
45794572
/* error occurred in a scan against a foreign join */
4580-
ForeignScanState *fsstate = errpos->fsstate;
4581-
ForeignScan *fsplan = (ForeignScan *) fsstate->ss.ps.plan;
4582-
EState *estate = fsstate->ss.ps.state;
45834573
TargetEntry *tle;
4584-
Var *var;
4585-
RangeTblEntry *rte;
45864574

45874575
Assert(IsA(fsplan, ForeignScan));
45884576
tle = (TargetEntry *) list_nth(fsplan->fdw_scan_tlist,
45894577
errpos->cur_attno - 1);
45904578
Assert(IsA(tle, TargetEntry));
4591-
var = (Var *) tle->expr;
4592-
Assert(IsA(var, Var));
4593-
4594-
rte = rt_fetch(var->varno, estate->es_range_table);
45954579

4596-
if (var->varattno == 0)
4597-
is_wholerow = true;
4598-
else
4599-
attname = get_relid_attribute_name(rte->relid, var->varattno);
4580+
/*
4581+
* Target list can have Vars and expressions. For Vars, we can get
4582+
* some information, however for expressions we can't. Thus for
4583+
* expressions, just show generic context message.
4584+
*/
4585+
if (IsA(tle->expr, Var))
4586+
{
4587+
Var *var = (Var *) tle->expr;
46004588

4601-
relname = get_rel_name(rte->relid);
4589+
varno = var->varno;
4590+
colno = var->varattno;
4591+
}
46024592
}
46034593

4604-
if (relname)
4594+
if (varno > 0)
46054595
{
4606-
if (is_wholerow)
4607-
errcontext("whole-row reference to foreign table \"%s\"", relname);
4608-
else if (attname)
4609-
errcontext("column \"%s\" of foreign table \"%s\"", attname, relname);
4596+
EState *estate = fsstate->ss.ps.state;
4597+
RangeTblEntry *rte = rt_fetch(varno, estate->es_range_table);
4598+
4599+
relname = rte->eref->aliasname;
4600+
4601+
if (colno == 0)
4602+
is_wholerow = true;
4603+
else if (colno > 0 && colno <= list_length(rte->eref->colnames))
4604+
attname = strVal(list_nth(rte->eref->colnames, colno - 1));
4605+
else if (colno == SelfItemPointerAttributeNumber)
4606+
attname = "ctid";
4607+
else if (colno == ObjectIdAttributeNumber)
4608+
attname = "oid";
46104609
}
4610+
4611+
if (relname && is_wholerow)
4612+
errcontext("whole-row reference to foreign table \"%s\"", relname);
4613+
else if (relname && attname)
4614+
errcontext("column \"%s\" of foreign table \"%s\"", attname, relname);
4615+
else
4616+
errcontext("processing expression at position %d in select list",
4617+
errpos->cur_attno);
46114618
}
46124619

46134620
/*

‎contrib/postgres_fdw/sql/postgres_fdw.sql

Copy file name to clipboardExpand all lines: contrib/postgres_fdw/sql/postgres_fdw.sql
+5-3Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -702,9 +702,11 @@ DROP FUNCTION f_test(int);
702702
-- conversion error
703703
-- ===================================================================
704704
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE int;
705-
SELECT * FROM ft1 WHERE c1 = 1; -- ERROR
706-
SELECT ft1.c1, ft2.c2, ft1.c8 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
707-
SELECT ft1.c1, ft2.c2, ft1 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
705+
SELECT * FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8) WHERE x1 = 1; -- ERROR
706+
SELECT ftx.x1, ft2.c2, ftx.x8 FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
707+
WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
708+
SELECT ftx.x1, ft2.c2, ftx FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
709+
WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
708710
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
709711

710712
-- ===================================================================

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.