Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 37a3058

Browse files
committed
Fix interaction of foreign tuple routing with remote triggers.
Without these fixes, changes to the inserted tuple made by remote triggers are ignored when building local RETURNING tuples. In the core code, call ExecInitRoutingInfo at a later point from within ExecInitPartitionInfo so that the FDW callback gets invoked after the returning list has been built. But move CheckValidResultRel out of ExecInitRoutingInfo so that it can happen at an earlier stage. In postgres_fdw, refactor assorted deparsing functions to work with the RTE rather than the PlannerInfo, which saves us having to construct a fake PlannerInfo in cases where we don't have a real one. Then, we can pass down a constructed RTE that yields the correct deparse result when no real one exists. Unfortunately, this necessitates a hack that understands how the core code manages RT indexes for update tuple routing, which is ugly, but we don't have a better idea right now. Original report, analysis, and patch by Etsuro Fujita. Heavily refactored by me. Then worked over some more by Amit Langote. Discussion: http://postgr.es/m/5AD4882B.10002@lab.ntt.co.jp
1 parent 6594ee2 commit 37a3058

File tree

7 files changed

+234
-70
lines changed

7 files changed

+234
-70
lines changed

contrib/postgres_fdw/deparse.c

+25-33
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ static char *deparse_type_name(Oid type_oid, int32 typemod);
125125
* Functions to construct string representation of a node tree.
126126
*/
127127
static void deparseTargetList(StringInfo buf,
128-
PlannerInfo *root,
128+
RangeTblEntry *rte,
129129
Index rtindex,
130130
Relation rel,
131131
bool is_returning,
@@ -137,13 +137,13 @@ static void deparseExplicitTargetList(List *tlist,
137137
List **retrieved_attrs,
138138
deparse_expr_cxt *context);
139139
static void deparseSubqueryTargetList(deparse_expr_cxt *context);
140-
static void deparseReturningList(StringInfo buf, PlannerInfo *root,
140+
static void deparseReturningList(StringInfo buf, RangeTblEntry *rte,
141141
Index rtindex, Relation rel,
142142
bool trig_after_row,
143143
List *returningList,
144144
List **retrieved_attrs);
145145
static void deparseColumnRef(StringInfo buf, int varno, int varattno,
146-
PlannerInfo *root, bool qualify_col);
146+
RangeTblEntry *rte, bool qualify_col);
147147
static void deparseRelation(StringInfo buf, Relation rel);
148148
static void deparseExpr(Expr *expr, deparse_expr_cxt *context);
149149
static void deparseVar(Var *node, deparse_expr_cxt *context);
@@ -1050,7 +1050,7 @@ deparseSelectSql(List *tlist, bool is_subquery, List **retrieved_attrs,
10501050
*/
10511051
Relation rel = heap_open(rte->relid, NoLock);
10521052

1053-
deparseTargetList(buf, root, foreignrel->relid, rel, false,
1053+
deparseTargetList(buf, rte, foreignrel->relid, rel, false,
10541054
fpinfo->attrs_used, false, retrieved_attrs);
10551055
heap_close(rel, NoLock);
10561056
}
@@ -1099,7 +1099,7 @@ deparseFromExpr(List *quals, deparse_expr_cxt *context)
10991099
*/
11001100
static void
11011101
deparseTargetList(StringInfo buf,
1102-
PlannerInfo *root,
1102+
RangeTblEntry *rte,
11031103
Index rtindex,
11041104
Relation rel,
11051105
bool is_returning,
@@ -1137,7 +1137,7 @@ deparseTargetList(StringInfo buf,
11371137
appendStringInfoString(buf, " RETURNING ");
11381138
first = false;
11391139

1140-
deparseColumnRef(buf, rtindex, i, root, qualify_col);
1140+
deparseColumnRef(buf, rtindex, i, rte, qualify_col);
11411141

11421142
*retrieved_attrs = lappend_int(*retrieved_attrs, i);
11431143
}
@@ -1649,7 +1649,7 @@ deparseRangeTblRef(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel,
16491649
* to *retrieved_attrs.
16501650
*/
16511651
void
1652-
deparseInsertSql(StringInfo buf, PlannerInfo *root,
1652+
deparseInsertSql(StringInfo buf, RangeTblEntry *rte,
16531653
Index rtindex, Relation rel,
16541654
List *targetAttrs, bool doNothing,
16551655
List *returningList, List **retrieved_attrs)
@@ -1674,7 +1674,7 @@ deparseInsertSql(StringInfo buf, PlannerInfo *root,
16741674
appendStringInfoString(buf, ", ");
16751675
first = false;
16761676

1677-
deparseColumnRef(buf, rtindex, attnum, root, false);
1677+
deparseColumnRef(buf, rtindex, attnum, rte, false);
16781678
}
16791679

16801680
appendStringInfoString(buf, ") VALUES (");
@@ -1699,7 +1699,7 @@ deparseInsertSql(StringInfo buf, PlannerInfo *root,
16991699
if (doNothing)
17001700
appendStringInfoString(buf, " ON CONFLICT DO NOTHING");
17011701

1702-
deparseReturningList(buf, root, rtindex, rel,
1702+
deparseReturningList(buf, rte, rtindex, rel,
17031703
rel->trigdesc && rel->trigdesc->trig_insert_after_row,
17041704
returningList, retrieved_attrs);
17051705
}
@@ -1712,7 +1712,7 @@ deparseInsertSql(StringInfo buf, PlannerInfo *root,
17121712
* to *retrieved_attrs.
17131713
*/
17141714
void
1715-
deparseUpdateSql(StringInfo buf, PlannerInfo *root,
1715+
deparseUpdateSql(StringInfo buf, RangeTblEntry *rte,
17161716
Index rtindex, Relation rel,
17171717
List *targetAttrs, List *returningList,
17181718
List **retrieved_attrs)
@@ -1735,13 +1735,13 @@ deparseUpdateSql(StringInfo buf, PlannerInfo *root,
17351735
appendStringInfoString(buf, ", ");
17361736
first = false;
17371737

1738-
deparseColumnRef(buf, rtindex, attnum, root, false);
1738+
deparseColumnRef(buf, rtindex, attnum, rte, false);
17391739
appendStringInfo(buf, " = $%d", pindex);
17401740
pindex++;
17411741
}
17421742
appendStringInfoString(buf, " WHERE ctid = $1");
17431743

1744-
deparseReturningList(buf, root, rtindex, rel,
1744+
deparseReturningList(buf, rte, rtindex, rel,
17451745
rel->trigdesc && rel->trigdesc->trig_update_after_row,
17461746
returningList, retrieved_attrs);
17471747
}
@@ -1777,6 +1777,7 @@ deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
17771777
int nestlevel;
17781778
bool first;
17791779
ListCell *lc;
1780+
RangeTblEntry *rte = planner_rt_fetch(rtindex, root);
17801781

17811782
/* Set up context struct for recursion */
17821783
context.root = root;
@@ -1808,7 +1809,7 @@ deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
18081809
appendStringInfoString(buf, ", ");
18091810
first = false;
18101811

1811-
deparseColumnRef(buf, rtindex, attnum, root, false);
1812+
deparseColumnRef(buf, rtindex, attnum, rte, false);
18121813
appendStringInfoString(buf, " = ");
18131814
deparseExpr((Expr *) tle->expr, &context);
18141815
}
@@ -1835,7 +1836,7 @@ deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
18351836
deparseExplicitTargetList(returningList, true, retrieved_attrs,
18361837
&context);
18371838
else
1838-
deparseReturningList(buf, root, rtindex, rel, false,
1839+
deparseReturningList(buf, rte, rtindex, rel, false,
18391840
returningList, retrieved_attrs);
18401841
}
18411842

@@ -1847,7 +1848,7 @@ deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
18471848
* to *retrieved_attrs.
18481849
*/
18491850
void
1850-
deparseDeleteSql(StringInfo buf, PlannerInfo *root,
1851+
deparseDeleteSql(StringInfo buf, RangeTblEntry *rte,
18511852
Index rtindex, Relation rel,
18521853
List *returningList,
18531854
List **retrieved_attrs)
@@ -1856,7 +1857,7 @@ deparseDeleteSql(StringInfo buf, PlannerInfo *root,
18561857
deparseRelation(buf, rel);
18571858
appendStringInfoString(buf, " WHERE ctid = $1");
18581859

1859-
deparseReturningList(buf, root, rtindex, rel,
1860+
deparseReturningList(buf, rte, rtindex, rel,
18601861
rel->trigdesc && rel->trigdesc->trig_delete_after_row,
18611862
returningList, retrieved_attrs);
18621863
}
@@ -1918,15 +1919,16 @@ deparseDirectDeleteSql(StringInfo buf, PlannerInfo *root,
19181919
deparseExplicitTargetList(returningList, true, retrieved_attrs,
19191920
&context);
19201921
else
1921-
deparseReturningList(buf, root, rtindex, rel, false,
1922+
deparseReturningList(buf, planner_rt_fetch(rtindex, root),
1923+
rtindex, rel, false,
19221924
returningList, retrieved_attrs);
19231925
}
19241926

19251927
/*
19261928
* Add a RETURNING clause, if needed, to an INSERT/UPDATE/DELETE.
19271929
*/
19281930
static void
1929-
deparseReturningList(StringInfo buf, PlannerInfo *root,
1931+
deparseReturningList(StringInfo buf, RangeTblEntry *rte,
19301932
Index rtindex, Relation rel,
19311933
bool trig_after_row,
19321934
List *returningList,
@@ -1952,7 +1954,7 @@ deparseReturningList(StringInfo buf, PlannerInfo *root,
19521954
}
19531955

19541956
if (attrs_used != NULL)
1955-
deparseTargetList(buf, root, rtindex, rel, true, attrs_used, false,
1957+
deparseTargetList(buf, rte, rtindex, rel, true, attrs_used, false,
19561958
retrieved_attrs);
19571959
else
19581960
*retrieved_attrs = NIL;
@@ -2048,11 +2050,9 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List **retrieved_attrs)
20482050
* If qualify_col is true, qualify column name with the alias of relation.
20492051
*/
20502052
static void
2051-
deparseColumnRef(StringInfo buf, int varno, int varattno, PlannerInfo *root,
2053+
deparseColumnRef(StringInfo buf, int varno, int varattno, RangeTblEntry *rte,
20522054
bool qualify_col)
20532055
{
2054-
RangeTblEntry *rte;
2055-
20562056
/* We support fetching the remote side's CTID and OID. */
20572057
if (varattno == SelfItemPointerAttributeNumber)
20582058
{
@@ -2077,10 +2077,7 @@ deparseColumnRef(StringInfo buf, int varno, int varattno, PlannerInfo *root,
20772077
Oid fetchval = 0;
20782078

20792079
if (varattno == TableOidAttributeNumber)
2080-
{
2081-
rte = planner_rt_fetch(varno, root);
20822080
fetchval = rte->relid;
2083-
}
20842081

20852082
if (qualify_col)
20862083
{
@@ -2100,9 +2097,6 @@ deparseColumnRef(StringInfo buf, int varno, int varattno, PlannerInfo *root,
21002097
/* Required only to be passed down to deparseTargetList(). */
21012098
List *retrieved_attrs;
21022099

2103-
/* Get RangeTblEntry from array in PlannerInfo. */
2104-
rte = planner_rt_fetch(varno, root);
2105-
21062100
/*
21072101
* The lock on the relation will be held by upper callers, so it's
21082102
* fine to open it with no lock here.
@@ -2134,7 +2128,7 @@ deparseColumnRef(StringInfo buf, int varno, int varattno, PlannerInfo *root,
21342128
}
21352129

21362130
appendStringInfoString(buf, "ROW(");
2137-
deparseTargetList(buf, root, varno, rel, false, attrs_used, qualify_col,
2131+
deparseTargetList(buf, rte, varno, rel, false, attrs_used, qualify_col,
21382132
&retrieved_attrs);
21392133
appendStringInfoChar(buf, ')');
21402134

@@ -2154,9 +2148,6 @@ deparseColumnRef(StringInfo buf, int varno, int varattno, PlannerInfo *root,
21542148
/* varno must not be any of OUTER_VAR, INNER_VAR and INDEX_VAR. */
21552149
Assert(!IS_SPECIAL_VARNO(varno));
21562150

2157-
/* Get RangeTblEntry from array in PlannerInfo. */
2158-
rte = planner_rt_fetch(varno, root);
2159-
21602151
/*
21612152
* If it's a column of a foreign table, and it has the column_name FDW
21622153
* option, use that value.
@@ -2354,7 +2345,8 @@ deparseVar(Var *node, deparse_expr_cxt *context)
23542345

23552346
if (bms_is_member(node->varno, relids) && node->varlevelsup == 0)
23562347
deparseColumnRef(context->buf, node->varno, node->varattno,
2357-
context->root, qualify_col);
2348+
planner_rt_fetch(node->varno, context->root),
2349+
qualify_col);
23582350
else
23592351
{
23602352
/* Treat like a Param */

contrib/postgres_fdw/expected/postgres_fdw.out

+93
Original file line numberDiff line numberDiff line change
@@ -7454,6 +7454,48 @@ select tableoid::regclass, * FROM itrtest;
74547454
remp1 | 1 | foo
74557455
(1 row)
74567456

7457+
delete from itrtest;
7458+
drop index loct1_idx;
7459+
-- Test that remote triggers work with insert tuple routing
7460+
create function br_insert_trigfunc() returns trigger as $$
7461+
begin
7462+
new.b := new.b || ' triggered !';
7463+
return new;
7464+
end
7465+
$$ language plpgsql;
7466+
create trigger loct1_br_insert_trigger before insert on loct1
7467+
for each row execute procedure br_insert_trigfunc();
7468+
create trigger loct2_br_insert_trigger before insert on loct2
7469+
for each row execute procedure br_insert_trigfunc();
7470+
-- The new values are concatenated with ' triggered !'
7471+
insert into itrtest values (1, 'foo') returning *;
7472+
a | b
7473+
---+-----------------
7474+
1 | foo triggered !
7475+
(1 row)
7476+
7477+
insert into itrtest values (2, 'qux') returning *;
7478+
a | b
7479+
---+-----------------
7480+
2 | qux triggered !
7481+
(1 row)
7482+
7483+
insert into itrtest values (1, 'test1'), (2, 'test2') returning *;
7484+
a | b
7485+
---+-------------------
7486+
1 | test1 triggered !
7487+
2 | test2 triggered !
7488+
(2 rows)
7489+
7490+
with result as (insert into itrtest values (1, 'test1'), (2, 'test2') returning *) select * from result;
7491+
a | b
7492+
---+-------------------
7493+
1 | test1 triggered !
7494+
2 | test2 triggered !
7495+
(2 rows)
7496+
7497+
drop trigger loct1_br_insert_trigger on loct1;
7498+
drop trigger loct2_br_insert_trigger on loct2;
74577499
drop table itrtest;
74587500
drop table loct1;
74597501
drop table loct2;
@@ -7518,6 +7560,57 @@ select tableoid::regclass, * FROM locp;
75187560

75197561
-- The executor should not let unexercised FDWs shut down
75207562
update utrtest set a = 1 where b = 'foo';
7563+
-- Test that remote triggers work with update tuple routing
7564+
create trigger loct_br_insert_trigger before insert on loct
7565+
for each row execute procedure br_insert_trigfunc();
7566+
delete from utrtest;
7567+
insert into utrtest values (2, 'qux');
7568+
-- Check case where the foreign partition is a subplan target rel
7569+
explain (verbose, costs off)
7570+
update utrtest set a = 1 where a = 1 or a = 2 returning *;
7571+
QUERY PLAN
7572+
----------------------------------------------------------------------------------------------
7573+
Update on public.utrtest
7574+
Output: remp.a, remp.b
7575+
Foreign Update on public.remp
7576+
Update on public.locp
7577+
-> Foreign Update on public.remp
7578+
Remote SQL: UPDATE public.loct SET a = 1 WHERE (((a = 1) OR (a = 2))) RETURNING a, b
7579+
-> Seq Scan on public.locp
7580+
Output: 1, locp.b, locp.ctid
7581+
Filter: ((locp.a = 1) OR (locp.a = 2))
7582+
(9 rows)
7583+
7584+
-- The new values are concatenated with ' triggered !'
7585+
update utrtest set a = 1 where a = 1 or a = 2 returning *;
7586+
a | b
7587+
---+-----------------
7588+
1 | qux triggered !
7589+
(1 row)
7590+
7591+
delete from utrtest;
7592+
insert into utrtest values (2, 'qux');
7593+
-- Check case where the foreign partition isn't a subplan target rel
7594+
explain (verbose, costs off)
7595+
update utrtest set a = 1 where a = 2 returning *;
7596+
QUERY PLAN
7597+
--------------------------------------
7598+
Update on public.utrtest
7599+
Output: locp.a, locp.b
7600+
Update on public.locp
7601+
-> Seq Scan on public.locp
7602+
Output: 1, locp.b, locp.ctid
7603+
Filter: (locp.a = 2)
7604+
(6 rows)
7605+
7606+
-- The new values are concatenated with ' triggered !'
7607+
update utrtest set a = 1 where a = 2 returning *;
7608+
a | b
7609+
---+-----------------
7610+
1 | qux triggered !
7611+
(1 row)
7612+
7613+
drop trigger loct_br_insert_trigger on loct;
75217614
drop table utrtest;
75227615
drop table loct;
75237616
-- Test copy tuple routing

0 commit comments

Comments
 (0)