8000 Avoid doing catalog lookups in postgres_fdw's conversion_error_callback. · postgres/postgres@b23ac5a · GitHub
[go: up one dir, main page]
Skip to content

Commit b23ac5a

Browse 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 9034b68 commit b23ac5a

File tree

3 files changed

+51
-52
lines changed

3 files changed

+51
-52
lines changed

‎contrib/postgres_fdw/expected/postgres_fdw.out

Lines changed: 8 additions & 6 deletions
Origin 8000 al file line numberDiff line numberDiff line change
@@ -4175,15 +4175,17 @@ DROP FUNCTION f_test(int);
41754175
-- conversion error
41764176
-- ===================================================================
41774177
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE int;
4178-
SELECT * FROM ft1 WHERE c1 = 1; -- ERROR
4178+
SELECT * FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8) WHERE x1 = 1; -- ERROR
41794179
ERROR: invalid input syntax for integer: "foo"
4180-
CONTEXT: column "c8" of foreign table "ft1"
4181-
SELECT ft1.c1, ft2.c2, ft1.c8 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
4180+
CONTEXT: column "x8" of foreign table "ftx"
4181+
SELECT ftx.x1, ft2.c2, ftx.x8 FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
4182+
WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
41824183
ERROR: invalid input syntax for integer: "foo"
4183-
CONTEXT: column "c8" of foreign table "ft1"
4184-
SELECT ft1.c1, ft2.c2, ft1 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
4184+
CONTEXT: column "x8" of foreign table "ftx"
4185+
SELECT ftx.x1, ft2.c2, ftx FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
4186+
WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
41854187
ERROR: invalid input syntax for integer: "foo"
4186-
CONTEXT: whole-row reference to foreign table "ft1"
4188+
CONTEXT: whole-row reference to foreign table "ftx"
41874189
SELECT sum(c2), array_agg(c8) FROM ft1 GROUP BY c8; -- ERROR
41884190
ERROR: invalid input syntax for integer: "foo"
41894191
CONTEXT: processing expression at position 2 in select list

‎contrib/postgres_fdw/postgres_fdw.c

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

259251
/* Callback argument for ec_member_matches_foreign */
@@ -5026,7 +5018,6 @@ make_tuple_from_result_row(PGresult *res,
50265018
/*
50275019
* Set up and install callback to report where conversion error occurs.
50285020
*/
5029-
errpos.rel = rel;
50305021
errpos.cur_attno = 0;
50315022
errpos.fsstate = fsstate;
50325023
errcallback.callback = conversion_error_callback;
@@ -5146,71 +5137,75 @@ make_tuple_from_result_row(PGresult *res,
51465137
/*
51475138
* Callback function which is called when error occurs during column value
51485139
* conversion. Print names of column and relation.
5140+
*
5141+
* Note that this function mustn't do any catalog lookups, since we are in
5142+
* an already-failed transaction. Fortunately, we can get the needed info
5143+
* from the query's rangetable instead.
51495144
*/
51505145
static void
51515146
conversion_error_callback(void *arg)
51525147
{
5148+
ConversionLocation *errpos = (ConversionLocation *) arg;
5149+
ForeignScanState *fsstate = errpos->fsstate;
5150+
ForeignScan *fsplan = castNode(ForeignScan, fsstate->ss.ps.plan);
5151+
int varno = 0;
5152+
AttrNumber colno = 0;
51535153
const char *attname = NULL;
51545154
const char *relname = NULL;
51555155
bool is_wholerow = false;
5156-
ConversionLocation *errpos = (ConversionLocation *) arg;
51575156

5158-
if (errpos->rel)
5157+
if (fsplan->scan.scanrelid > 0)
51595158
{
51605159
/* error occurred in a scan against a foreign table */
5161-
TupleDesc tupdesc = RelationGetDescr(errpos->rel);
5162-
5163-
if (errpos->cur_attno > 0 && errpos->cur_attno <= tupdesc->natts)
5164-
attname = NameStr(tupdesc->attrs[errpos->cur_attno - 1]->attname);
5165-
else if (errpos->cur_attno == SelfItemPointerAttributeNumber)
5166-
attname = "ctid";
5167-
else if (errpos->cur_attno == ObjectIdAttributeNumber)
5168-
attname = "oid";
5169-
5170-
relname = RelationGetRelationName(errpos->rel);
5160+
varno = fsplan->scan.scanrelid;
5161+
colno = errpos->cur_attno;
51715162
}
51725163
else
51735164
{
51745165
/* error occurred in a scan against a foreign join */
5175-
ForeignScanState *fsstate = errpos->fsstate;
5176-
ForeignScan *fsplan = castNode(ForeignScan, fsstate->ss.ps.plan);
5177-
EState *estate = fsstate->ss.ps.state;
51785166
TargetEntry *tle;
51795167

51805168
tle = list_nth_node(TargetEntry, fsplan->fdw_scan_tlist,
51815169
errpos->cur_attno - 1);
51825170

51835171
/*
51845172
* Target list can have Vars and expressions. For Vars, we can get
5185-
* it's relation, however for expressions we can't. Thus for
5173+
* some information, however for expressions we can't. Thus for
51865174
* expressions, just show generic context message.
51875175
*/
51885176
if (IsA(tle->expr, Var))
51895177
{
5190-
RangeTblEntry *rte;
51915178
Var *var = (Var *) tle->expr;
51925179

5193-
rte = rt_fetch(var->varno, estate->es_range_table);
5194-
5195-
if (var->varattno == 0)
5196-
is_wholerow = true;
5197-
else
5198-
attname = get_relid_attribute_name(rte->relid, var->varattno);
5199-
5200-
relname = get_rel_name(rte->relid);
5180+
varno = var->varno;
5181+
colno = var->varattno;
52015182
}
5202-
else
5203-
errcontext("processing expression at position %d in select list",
5204-
errpos->cur_attno);
52055183
}
52065184

5207-
if (relname)
5185+
if (varno > 0)
52085186
{
5209-
if (is_wholerow)
5210-
errcontext("whole-row reference to foreign table \"%s\"", relname);
5211-
else if (attname)
5212-
errcontext("column \"%s\" of foreign table \"%s\"", attname, relname);
5187+
EState *estate = fsstate->ss.ps.state;
5188+
RangeTblEntry *rte = rt_fetch(varno, estate->es_range_table);
5189+
5190+
relname = rte->eref->aliasname;
5191+
5192+
if (colno == 0)
5193+
is_wholerow = true;
5194+
else if (colno > 0 && colno <= list_length(rte->eref->colnames))
5195+
attname = strVal(list_nth(rte->eref->colnames, colno - 1));
5196+
else if (colno == SelfItemPointerAttributeNumber)
5197+
attname = "ctid";
5198+
else if (colno == ObjectIdAttributeNumber)
5199+
attname = "oid";
52135200
}
5201+
5202+
if (relname && is_wholerow)
5203+
errcontext("whole-row reference to foreign table \"%s\"", relname);
5204+
else if (relname && attname)
5205+
errcontext("column \"%s\" of foreign table \"%s\"", attname, relname);
5206+
else
5207+
errcontext("processing expression at position %d in select list",
5208+
errpos->cur_attno);
52145209
}
52155210

52165211
/*

‎contrib/postgres_fdw/sql/postgres_fdw.sql

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1055,9 +1055,11 @@ DROP FUNCTION f_test(int);
10551055
-- conversion error
10561056
-- ===================================================================
10571057
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE int;
1058-
SELECT * FROM ft1 WHERE c1 = 1; -- ERROR
1059-
SELECT ft1.c1, ft2.c2, ft1.c8 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
1060-
SELECT ft1.c1, ft2.c2, ft1 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = 1; -- ERROR
1058+
SELECT * FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8) WHERE x1 = 1; -- ERROR
1059+
SELECT ftx.x1, ft2.c2, ftx.x8 FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
1060+
WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
1061+
SELECT ftx.x1, ft2.c2, ftx FROM ft1 ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
1062+
WHERE ftx.x1 = ft2.c1 AND ftx.x1 = 1; -- ERROR
10611063
SELECT sum(c2), array_agg(c8) FROM ft1 GROUP BY c8; -- ERROR
10621064
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
10631065

0 commit comments

Comments
 (0)
0