Skip to content

Navigation Menu

Sign in

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 a9460db

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 151d30e commit a9460db
Copy full SHA for a9460db

File tree

3 files changed

+51
-53
lines changed
Filter options

3 files changed

+51
-53
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
@@ -4190,15 +4190,17 @@ DROP FUNCTION f_test(int);
41904190
-- conversion error
41914191
-- ===================================================================
41924192
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE int;
4193-
SELECT * FROM ft1 WHERE c1 = 1; -- ERROR
4193+
SELECT * FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8) WHERE x1 = 1; -- ERROR
41944194
ERROR: invalid input syntax for integer: "foo"
4195-
CONTEXT: column "c8" of foreign table "ft1"
4196-
SELECT ft1.c1, ft2.c2, ft1.c8 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
4195+
CONTEXT: column "x8" of foreign table "ftx"
4196+
SELECT ftx.x1, ft2.c2, ftx.x8 FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
4197+
WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
41974198
ERROR: invalid input syntax for integer: "foo"
4198-
CONTEXT: column "c8" of foreign table "ft1"
4199-
SELECT ft1.c1, ft2.c2, ft1 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
4199+
CONTEXT: column "x8" of foreign table "ftx"
4200+
SELECT ftx.x1, ft2.c2, ftx FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
4201+
WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
42004202
ERROR: invalid input syntax for integer: "foo"
4201-
CONTEXT: whole-row reference to foreign table "ft1"
4203+
CONTEXT: whole-row reference to foreign table "ftx"
42024204
SELECT sum(c2), array_agg(c8) FROM ft1 GROUP BY c8; -- ERROR
42034205
ERROR: invalid input syntax for integer: "foo"
42044206
CONTEXT: processing expression at position 2 in select list

‎contrib/postgres_fdw/postgres_fdw.c

Copy file name to clipboardExpand all lines: contrib/postgres_fdw/postgres_fdw.c
+38-44Lines changed: 38 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -253,16 +253,8 @@ typedef struct PgFdwAnalyzeState
253253
*/
254254
typedef struct ConversionLocation
255255
{
256-
Relation rel; /* foreign table's relcache entry. */
257256
AttrNumber cur_attno; /* attribute number being processed, or 0 */
258-
259-
/*
260-
* In case of foreign join push down, fdw_scan_tlist is used to identify
261-
* the Var node corresponding to the error location and
262-
* fsstate->ss.ps.state gives access to the RTEs of corresponding relation
263-
* to get the relation name and attribute name.
264-
*/
265-
ForeignScanState *fsstate;
257+
ForeignScanState *fsstate; /* plan node being processed */
266258
} ConversionLocation;
267259

268260
/* Callback argument for ec_member_matches_foreign */
@@ -5695,7 +5687,6 @@ make_tuple_from_result_row(PGresult *res,
56955687
/*
56965688
* Set up and install callback to report where conversion error occurs.
56975689
*/
5698-
errpos.rel = rel;
56995690
errpos.cur_attno = 0;
57005691
errpos.fsstate = fsstate;
57015692
errcallback.callback = conversion_error_callback;
@@ -5815,72 +5806,75 @@ make_tuple_from_result_row(PGresult *res,
58155806
/*
58165807
* Callback function which is called when error occurs during column value
58175808
* conversion. Print names of column and relation.
5809+
*
5810+
* Note that this function mustn't do any catalog lookups, since we are in
5811+
* an already-failed transaction. Fortunately, we can get the needed info
5812+
* from the query's rangetable instead.
58185813
*/
58195814
static void
58205815
conversion_error_callback(void *arg)
58215816
{
5817+
ConversionLocation *errpos = (ConversionLocation *) arg;
5818+
ForeignScanState *fsstate = errpos->fsstate;
5819+
ForeignScan *fsplan = castNode(ForeignScan, fsstate->ss.ps.plan);
5820+
int varno = 0;
5821+
AttrNumber colno = 0;
58225822
const char *attname = NULL;
58235823
const char *relname = NULL;
58245824
bool is_wholerow = false;
5825-
ConversionLocation *errpos = (ConversionLocation *) arg;
58265825

5827-
if (errpos->rel)
5826+
if (fsplan->scan.scanrelid > 0)
58285827
{
58295828
/* error occurred in a scan against a foreign table */
5830-
TupleDesc tupdesc = RelationGetDescr(errpos->rel);
5831-
Form_pg_attribute attr = TupleDescAttr(tupdesc, errpos->cur_attno - 1);
5832-
5833-
if (errpos->cur_attno > 0 && errpos->cur_attno <= tupdesc->natts)
5834-
attname = NameStr(attr->attname);
5835-
else if (errpos->cur_attno == SelfItemPointerAttributeNumber)
5836-
attname = "ctid";
5837-
else if (errpos->cur_attno == ObjectIdAttributeNumber)
5838-
attname = "oid";
5839-
5840-
relname = RelationGetRelationName(errpos->rel);
5829+
varno = fsplan->scan.scanrelid;
5830+
colno = errpos->cur_attno;
58415831
}
58425832
else
58435833
{
58445834
/* error occurred in a scan against a foreign join */
5845-
ForeignScanState *fsstate = errpos->fsstate;
5846-
ForeignScan *fsplan = castNode(ForeignScan, fsstate->ss.ps.plan);
5847-
EState *estate = fsstate->ss.ps.state;
58485835
TargetEntry *tle;
58495836

58505837
tle = list_nth_node(TargetEntry, fsplan->fdw_scan_tlist,
58515838
errpos->cur_attno - 1);
58525839

58535840
/*
58545841
* Target list can have Vars and expressions. For Vars, we can get
5855-
* its relation, however for expressions we can't. Thus for
5842+
* some information, however for expressions we can't. Thus for
58565843
* expressions, just show generic context message.
58575844
*/
58585845
if (IsA(tle->expr, Var))
58595846
{
5860-
RangeTblEntry *rte;
58615847
Var *var = (Var *) tle->expr;
58625848

5863-
rte = rt_fetch(var->varno, estate->es_range_table);
5864-
5865-
if (var->varattno == 0)
5866-
is_wholerow = true;
5867-
else
5868-
attname = get_attname(rte->relid, var->varattno, false);
5869-
5870-
relname = get_rel_name(rte->relid);
5849+
varno = var->varno;
5850+
colno = var->varattno;
58715851
}
5872-
else
5873-
errcontext("processing expression at position %d in select list",
5874-
errpos->cur_attno);
58755852
}
58765853

5877-
if (relname)
5854+
if (varno > 0)
58785855
{
5879-
if (is_wholerow)
5880-
errcontext("whole-row reference to foreign table \"%s\"", relname);
5881-
else if (attname)
5882-
errcontext("column \"%s\" of foreign table \"%s\"", attname, relname);
5856+
EState *estate = fsstate->ss.ps.state;
5857+
RangeTblEntry *rte = rt_fetch(varno, estate->es_range_table);
5858+
5859+
relname = rte->eref->aliasname;
5860+
5861+
if (colno == 0)
5862+
is_wholerow = true;
5863+
else if (colno > 0 && colno <= list_length(rte->eref->colnames))
5864+
attname = strVal(list_nth(rte->eref->colnames, colno - 1));
5865+
else if (colno == SelfItemPointerAttributeNumber)
5866+
attname = "ctid";
5867+
else if (colno == ObjectIdAttributeNumber)
5868+
attname = "oid";
58835869
}
5870+
5871+
if (relname && is_wholerow)
5872+
errcontext("whole-row reference to foreign table \"%s\"", relname);
5873+
else if (relname && attname)
5874+
errcontext("column \"%s\" of foreign table \"%s\"", attname, relname);
5875+
else
5876+
errcontext("processing expression at position %d in select list",
5877+
errpos->cur_attno);
58845878
}
58855879

58865880
/*

‎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
@@ -1061,9 +1061,11 @@ DROP FUNCTION f_test(int);
10611061
-- conversion error
10621062
-- ===================================================================
10631063
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE int;
1064-
SELECT * FROM ft1 WHERE c1 = 1; -- ERROR
1065-
SELECT ft1.c1, ft2.c2, ft1.c8 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
1066-
SELECT ft1.c1, ft2.c2, ft1 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
1064+
SELECT * FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8) WHERE x1 = 1; -- ERROR
1065+
SELECT ftx.x1, ft2.c2, ftx.x8 FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
1066+
WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
1067+
SELECT ftx.x1, ft2.c2, ftx FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
1068+
WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
10671069
SELECT sum(c2), array_agg(c8) FROM ft1 GROUP BY c8; -- ERROR
10681070
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
10691071

0 commit comments

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