Horiguchi-san,
Thanks for taking a look at it.
On 2018/04/25 16:42, Kyotaro HORIGUCHI wrote:
> At Wed, 25 Apr 2018 11:19:23 +0900, Amit Langote wrote:
>> After the refactoring, it appears to me that we only need this much:
>>
>> + rte = makeNode(RangeTblEntry);
>> + rte->rtekind = RTE_RELATION;
>> + rte->relid = RelationGetRelid(rel);
>> + rte->relkind = RELKIND_FOREIGN_TABLE;
>
> Mmm.. That is, only relid is required to deparse (I don't mean
> that it should be refactored so.). On the other hand
> create_foreign_modify requires rte->checkAsUser as well. The
> following query (probably) unexpectedly fails with the latest
> patch. It succeeds with -3 patch.
>
> =# create user usr1 login;
> =# create view v1 as select * from itrtest;
> =# revoke all ON itrtest FROM PUBLIC;
> =# grant SELECT, INSERT ON v1 to usr1;
> => select * from v1; -- OK
> => insert into v1 (select n, 'foo' from (values (1), (2)) as t(n)) returning
> *;
> ERROR: password is required
> DETAIL: Non-superusers must provide a password in the user mapping.
>
> We need to read it of the parent?
Hmm, I missed that we do need information from a proper RTE as well. So,
I suppose we should be doing this instead of creating the RTE for foreign
partition from scratch.
+ rte = list_nth(estate->es_range_table, resultRelation - 1);
+ rte = copyObject(rte);
+ rte->relid = RelationGetRelid(rel);
+ rte->relkind = RELKIND_FOREIGN_TABLE;
If we apply the other patch I proposed, resultRelation always points to
the correct RTE.
>> I tried to do that. So, attached are two patches.
>>
>> 1. Fix for ExecInitPartitionInfo to use the right RT index to pass to
>> InitResultRelInfo
>>
>> 2. v5 of the patch to fix the bug of foreign partitions
>>
>> Thoughts?
>
> Maybe, one reason that I feel uneasy is how the patch accesses
> desired resultRelInfo.
>
> + Index firstVarno = mtstate->resultRelInfo[0].ri_RangeTableIndex;
>
> Looking ExecInitModifyTable, just one resultRelInfo has been
> passed to BeginForeignModify so it should not access as an
> array. I will feel at easy if the line were in the following
> shape. Does it make sense?
>
> + Index firstVarno = mtstate->resultRelInfo->ri_RangeTableIndex;
This is the comment on
teach-ExecInitPartitionInfo-to-use-correct-RT-index.patch, right? I
haven't seen either ExecInitModifyTable or BeginForeignModify being
involved in this discussion, but I see your point. I see no problem with
doing it that way, I have updated that patch to do it that way. Also,
changed the line above it that is unrelated to this patch just to be
consistent.
Attached updated patches:
1. teach-ExecInitPartitionInfo-to-use-correct-RT-index-2.patch
2. fix-tuple-routing-for-foreign-partitions-6.patch
Thanks,
Amit
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 6e2fa1420c..d272719ff4 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -125,7 +125,7 @@ static char *deparse_type_name(Oid type_oid, int32 typemod);
* Functions to construct string representation of a node tree.
*/
static void deparseTargetList(StringInfo buf,
- PlannerInfo *root,
+ RangeTblEntry *rte,
Index rtindex,
Relation rel,
bool is_returning,
@@ -137,13 +137,13 @@ static void deparseExplicitTargetList(List *tlist,
List **retrieved_attrs,
deparse_expr_cxt *context);
static void deparseSubqueryTargetList(deparse_expr_cxt *context);
-static void deparseReturningList(StringInfo buf, PlannerInfo *root,
+static void deparseReturningList(StringInfo buf, RangeTblEntry *rte,
Index rtindex, Relation rel,
bool trig_after_row,
List *returningList,
List **retrieved_attrs);
static void deparseColumnRef(StringInfo buf, int varno, int varattno,
- PlannerInfo *root, bool qualify_col);
+ RangeTblEntry *rte, bool qualify_col);
static void deparseRelation(StringInfo buf, Relation rel);
static void deparseExpr(Expr *expr, deparse_expr_cxt *context);
static void deparseVar(Var *node, deparse_expr_cxt *context);
@@ -1050,7 +1050,7 @@ deparseSelectSql(List *tlist, bool is_subquery, List
**retrieved_attrs,
*/
Relation rel = heap_open(rte->relid, NoLock);
- deparseTargetList(buf, root, foreignrel->relid, rel, false,
+ deparseTargetList(buf, rte, foreignrel->relid, rel, false,
fpinfo->attrs_used, false,
retrieved_attrs);
heap_close(rel, NoLock);
}
@@ -1099,7 +1099,7 @@ deparseFromExpr(List *quals, deparse_expr_cxt *context)
*/
static void
deparseTargetList(StringInfo buf,
- PlannerInfo *root,
+ RangeTblEntry *rte,
Index rtindex,
Relation rel,
bool is_returning,
@@ -1137,7 +1137,7 @@ deparseTargetList(StringInfo buf,
appendStringInfoString(buf, " RETURNING ");
first = false;
- deparseColumnRef(buf, rtindex, i, root, qualify_col);
+ deparseColumnRef(buf, rtindex, i, rte, qualify_col);
*retrieved_attrs = lappend_int(*retrieved_attrs, i);
}
@@ -1649,7 +1649,7 @@ deparseRangeTblRef(StringInfo buf, PlannerInfo *root,
RelOptInfo *foreignrel,
* to *retrieved_attrs.
*/
void
-deparseInsertSql(StringInfo buf, PlannerInfo *root,
+deparseInsertSql(StringInfo buf, RangeTblEntry *rte,
Index rtindex, Relation rel,
List *targetAttrs, bool doNothing,
List *returningList, List **retrieved_attrs)
@@ -1674,7 +1674,7 @@ deparseInsertSql(StringInfo buf, PlannerInfo *root,
appendStringInfoString(buf, ", ");
first = false;
- deparseColumnRef(buf, rtindex, attnum, root, false);
+ deparseColumnRef(buf, rtindex, attnum, rte, false);
}
appendStringInfoString(buf, ") VALUES (");
@@ -1699,7 +1699,7 @@ deparseInsertSql(StringInfo buf, PlannerInfo *root,
if (doNothing)
appendStringInfoString(buf, " ON CONFLICT DO NOTHING");
- deparseReturningList(buf, root, rtindex, rel,
+ deparseReturningList(buf, rte, rtindex, rel,
rel->trigdesc &&
rel->trigdesc->trig_insert_after_row,
returningList,
retrieved_attrs);
}
@@ -1712,7 +1712,7 @@ deparseInsertSql(StringInfo buf, PlannerInfo *root,
* to *retrieved_attrs.
*/
void
-deparseUpdateSql(StringInfo buf, PlannerInfo *root,
+deparseUpdateSql(StringInfo buf, RangeTblEntry *rte,
Index rtindex, Relation rel,
List *targetAttrs, List *returningList,
List **retrieved_attrs)
@@ -1735,13 +1735,13 @@ deparseUpdateSql(StringInfo buf, PlannerInfo *root,
appendStringInfoString(buf, ", ");
first = false;
- deparseColumnRef(buf, rtindex, attnum, root, false);
+ deparseColumnRef(buf, rtindex, attnum, rte, false);
appendStringInfo(buf, " = $%d", pindex);
pindex++;
}
appendStringInfoString(buf, " WHERE ctid = $1");
- deparseReturningList(buf, root, rtindex, rel,
+ deparseReturningList(buf, rte, rtindex, rel,
rel->trigdesc &&
rel->trigdesc->trig_update_after_row,
returningList,
retrieved_attrs);
}
@@ -1777,6 +1777,7 @@ deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
int nestlevel;
bool first;
ListCell *lc;
+ RangeTblEntry *rte = planner_rt_fetch(rtindex, root);
/* Set up context struct for recursion */
context.root = root;
@@ -1808,7 +1809,7 @@ deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
appendStringInfoString(buf, ", ");
first = false;
- deparseColumnRef(buf, rtindex, attnum, root, false);
+ deparseColumnRef(buf, rtindex, attnum, rte, false);
appendStringInfoString(buf, " = ");
deparseExpr((Expr *) tle->expr, &context);
}
@@ -1835,7 +1836,7 @@ deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
deparseExplicitTargetList(returningList, true, retrieved_attrs,
&context);
else
- deparseReturningList(buf, root, rtindex, rel, false,
+ deparseReturningList(buf, rte, rtindex, rel, false,
returningList,
retrieved_attrs);
}
@@ -1847,7 +1848,7 @@ deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
* to *retrieved_attrs.
*/
void
-deparseDeleteSql(StringInfo buf, PlannerInfo *root,
+deparseDeleteSql(StringInfo buf, RangeTblEntry *rte,
Index rtindex, Relation rel,
List *returningList,
List **retrieved_attrs)
@@ -1856,7 +1857,7 @@ deparseDeleteSql(StringInfo buf, PlannerInfo *root,
deparseRelation(buf, rel);
appendStringInfoString(buf, " WHERE ctid = $1");
- deparseReturningList(buf, root, rtindex, rel,
+ deparseReturningList(buf, rte, rtindex, rel,
rel->trigdesc &&
rel->trigdesc->trig_delete_after_row,
returningList,
retrieved_attrs);
}
@@ -1918,7 +1919,8 @@ deparseDirectDeleteSql(StringInfo buf, PlannerInfo *root,
deparseExplicitTargetList(returningList, true, retrieved_attrs,
&context);
else
- deparseReturningList(buf, root, rtindex, rel, false,
+ deparseReturningList(buf, planner_rt_fetch(rtindex, root),
+ rtindex, rel, false,
returningList,
retrieved_attrs);
}
@@ -1926,7 +1928,7 @@ deparseDirectDeleteSql(StringInfo buf, PlannerInfo *root,
* Add a RETURNING clause, if needed, to an INSERT/UPDATE/DELETE.
*/
static void
-deparseReturningList(StringInfo buf, PlannerInfo *root,
+deparseReturningList(StringInfo buf, RangeTblEntry *rte,
Index rtindex, Relation rel,
bool trig_after_row,
List *returningList,
@@ -1952,7 +1954,7 @@ deparseReturningList(StringInfo buf, PlannerInfo *root,
}
if (attrs_used != NULL)
- deparseTargetList(buf, root, rtindex, rel, true, attrs_used,
false,
+ deparseTargetList(buf, rte, rtindex, rel, true, attrs_used,
false,
retrieved_attrs);
else
*retrieved_attrs = NIL;
@@ -2048,11 +2050,9 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List
**retrieved_attrs)
* If qualify_col is true, qualify column name with the alias of relation.
*/
static void
-deparseColumnRef(StringInfo buf, int varno, int varattno, PlannerInfo *root,
+deparseColumnRef(StringInfo buf, int varno, int varattno, RangeTblEntry *rte,
bool qualify_col)
{
- RangeTblEntry *rte;
-
/* We support fetching the remote side's CTID and OID. */
if (varattno == SelfItemPointerAttributeNumber)
{
@@ -2077,10 +2077,7 @@ deparseColumnRef(StringInfo buf, int varno, int
varattno, PlannerInfo *root,
Oid fetchval = 0;
if (varattno == TableOidAttributeNumber)
- {
- rte = planner_rt_fetch(varno, root);
fetchval = rte->relid;
- }
if (qualify_col)
{
@@ -2100,9 +2097,6 @@ deparseColumnRef(StringInfo buf, int varno, int varattno,
PlannerInfo *root,
/* Required only to be passed down to deparseTargetList(). */
List *retrieved_attrs;
- /* Get RangeTblEntry from array in PlannerInfo. */
- rte = planner_rt_fetch(varno, root);
-
/*
* The lock on the relation will be held by upper callers, so
it's
* fine to open it with no lock here.
@@ -2134,7 +2128,7 @@ deparseColumnRef(StringInfo buf, int varno, int varattno,
PlannerInfo *root,
}
appendStringInfoString(buf, "ROW(");
- deparseTargetList(buf, root, varno, rel, false, attrs_used,
qualify_col,
+ deparseTargetList(buf, rte, varno, rel, false, attrs_used,
qualify_col,
&retrieved_attrs);
appendStringInfoChar(buf, ')');
@@ -2154,9 +2148,6 @@ deparseColumnRef(StringInfo buf, int varno, int varattno,
PlannerInfo *root,
/* varno must not be any of OUTER_VAR, INNER_VAR and INDEX_VAR.
*/
Assert(!IS_SPECIAL_VARNO(varno));
- /* Get RangeTblEntry from array in PlannerInfo. */
- rte = planner_rt_fetch(varno, root);
-
/*
* If it's a column of a foreign table, and it has the
column_name FDW
* option, use that value.
@@ -2354,7 +2345,8 @@ deparseVar(Var *node, deparse_expr_cxt *context)
if (bms_is_member(node->varno, relids) && node->varlevelsup == 0)
deparseColumnRef(context->buf, node->varno, node->varattno,
- context->root, qualify_col);
+ planner_rt_fetch(node->varno,
context->root),
+ qualify_col);
else
{
/* Treat like a Param */
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out
b/contrib/postgres_fdw/expected/postgres_fdw.out
index 53ed285875..bb6b1a8fdf 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -7454,6 +7454,48 @@ select tableoid::regclass, * FROM itrtest;
remp1 | 1 | foo
(1 row)
+delete from itrtest;
+drop index loct1_idx;
+-- Test that remote triggers work with insert tuple routing
+create function br_insert_trigfunc() returns trigger as $$
+begin
+ new.b := new.b || ' triggered !';
+ return new;
+end
+$$ language plpgsql;
+create trigger loct1_br_insert_trigger before insert on loct1
+ for each row execute procedure br_insert_trigfunc();
+create trigger loct2_br_insert_trigger before insert on loct2
+ for each row execute procedure br_insert_trigfunc();
+-- The new values are concatenated with ' triggered !'
+insert into itrtest values (1, 'foo') returning *;
+ a | b
+---+-----------------
+ 1 | foo triggered !
+(1 row)
+
+insert into itrtest values (2, 'qux') returning *;
+ a | b
+---+-----------------
+ 2 | qux triggered !
+(1 row)
+
+insert into itrtest values (1, 'test1'), (2, 'test2') returning *;
+ a | b
+---+-------------------
+ 1 | test1 triggered !
+ 2 | test2 triggered !
+(2 rows)
+
+with result as (insert into itrtest values (1, 'test1'), (2, 'test2')
returning *) select * from result;
+ a | b
+---+-------------------
+ 1 | test1 triggered !
+ 2 | test2 triggered !
+(2 rows)
+
+drop trigger loct1_br_insert_trigger on loct1;
+drop trigger loct2_br_insert_trigger on loct2;
drop table itrtest;
drop table loct1;
drop table loct2;
@@ -7518,6 +7560,57 @@ select tableoid::regclass, * FROM locp;
-- The executor should not let unexercised FDWs shut down
update utrtest set a = 1 where b = 'foo';
+-- Test that remote triggers work with update tuple routing
+create trigger loct_br_insert_trigger before insert on loct
+ for each row execute procedure br_insert_trigfunc();
+delete from utrtest;
+insert into utrtest values (2, 'qux');
+-- Check case where the foreign partition is a subplan target rel
+explain (verbose, costs off)
+update utrtest set a = 1 where a = 1 or a = 2 returning *;
+ QUERY PLAN
+----------------------------------------------------------------------------------------------
+ Update on public.utrtest
+ Output: remp.a, remp.b
+ Foreign Update on public.remp
+ Update on public.locp
+ -> Foreign Update on public.remp
+ Remote SQL: UPDATE public.loct SET a = 1 WHERE (((a = 1) OR (a = 2)))
RETURNING a, b
+ -> Seq Scan on public.locp
+ Output: 1, locp.b, locp.ctid
+ Filter: ((locp.a = 1) OR (locp.a = 2))
+(9 rows)
+
+-- The new values are concatenated with ' triggered !'
+update utrtest set a = 1 where a = 1 or a = 2 returning *;
+ a | b
+---+-----------------
+ 1 | qux triggered !
+(1 row)
+
+delete from utrtest;
+insert into utrtest values (2, 'qux');
+-- Check case where the foreign partition isn't a subplan target rel
+explain (verbose, costs off)
+update utrtest set a = 1 where a = 2 returning *;
+ QUERY PLAN
+--------------------------------------
+ Update on public.utrtest
+ Output: locp.a, locp.b
+ Update on public.locp
+ -> Seq Scan on public.locp
+ Output: 1, locp.b, locp.ctid
+ Filter: (locp.a = 2)
+(6 rows)
+
+-- The new values are concatenated with ' triggered !'
+update utrtest set a = 1 where a = 2 returning *;
+ a | b
+---+-----------------
+ 1 | qux triggered !
+(1 row)
+
+drop trigger loct_br_insert_trigger on loct;
drop table utrtest;
drop table loct;
-- Test copy tuple routing
diff --git a/contrib/postgres_fdw/postgres_fdw.c
b/contrib/postgres_fdw/postgres_fdw.c
index a46160df7c..ac5521e971 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -381,6 +381,7 @@ static void create_cursor(ForeignScanState *node);
static void fetch_more_data(ForeignScanState *node);
static void close_cursor(PGconn *conn, unsigned int cursor_number);
static PgFdwModifyState *create_foreign_modify(EState *estate,
+ RangeTblEntry *rte,
ResultRelInfo *resultRelInfo,
CmdType operation,
Plan *subplan,
@@ -1653,17 +1654,17 @@ postgresPlanForeignModify(PlannerInfo *root,
switch (operation)
{
case CMD_INSERT:
- deparseInsertSql(&sql, root, resultRelation, rel,
+ deparseInsertSql(&sql, rte, resultRelation, rel,
targetAttrs,
doNothing, returningList,
&retrieved_attrs);
break;
case CMD_UPDATE:
- deparseUpdateSql(&sql, root, resultRelation, rel,
+ deparseUpdateSql(&sql, rte, resultRelation, rel,
targetAttrs,
returningList,
&retrieved_attrs);
break;
case CMD_DELETE:
- deparseDeleteSql(&sql, root, resultRelation, rel,
+ deparseDeleteSql(&sql, rte, resultRelation, rel,
returningList,
&retrieved_attrs);
break;
@@ -1700,6 +1701,7 @@ postgresBeginForeignModify(ModifyTableState *mtstate,
List *target_attrs;
bool has_returning;
List *retrieved_attrs;
+ RangeTblEntry *rte;
/*
* Do nothing in EXPLAIN (no ANALYZE) case. resultRelInfo->ri_FdwState
@@ -1718,8 +1720,13 @@ postgresBeginForeignModify(ModifyTableState *mtstate,
retrieved_attrs = (List *) list_nth(fdw_private,
FdwModifyPrivateRetrievedAttrs);
+ /* Find RTE. */
+ rte = rt_fetch(resultRelInfo->ri_RangeTableIndex,
+ mtstate->ps.state->es_range_table);
+
/* Construct an execution state. */
fmstate = create_foreign_modify(mtstate->ps.state,
+ rte,
resultRelInfo,
mtstate->operation,
mtstate->mt_plans[subplan_index]->plan,
@@ -1974,11 +1981,11 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
ResultRelInfo *resultRelInfo)
{
PgFdwModifyState *fmstate;
- Plan *plan = mtstate->ps.plan;
+ ModifyTable *plan = castNode(ModifyTable, mtstate->ps.plan);
+ EState *estate = mtstate->ps.state;
+ Index resultRelation = resultRelInfo->ri_RangeTableIndex;
Relation rel = resultRelInfo->ri_RelationDesc;
RangeTblEntry *rte;
- Query *query;
- PlannerInfo *root;
TupleDesc tupdesc = RelationGetDescr(rel);
int attnum;
StringInfoData sql;
@@ -1988,18 +1995,6 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
initStringInfo(&sql);
- /* Set up largely-dummy planner state. */
- rte = makeNode(RangeTblEntry);
- rte->rtekind = RTE_RELATION;
- rte->relid = RelationGetRelid(rel);
- rte->relkind = RELKIND_FOREIGN_TABLE;
- query = makeNode(Query);
- query->commandType = CMD_INSERT;
- query->resultRelation = 1;
- query->rtable = list_make1(rte);
- root = makeNode(PlannerInfo);
- root->parse = query;
-
/* We transmit all columns that are defined in the foreign table. */
for (attnum = 1; attnum <= tupdesc->natts; attnum++)
{
@@ -2022,12 +2017,18 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
(int) onConflictAction);
}
+ rte = list_nth(estate->es_range_table, resultRelation - 1);
+ rte = copyObject(rte);
+ rte->relid = RelationGetRelid(rel);
+ rte->relkind = RELKIND_FOREIGN_TABLE;
+
/* Construct the SQL command string. */
- deparseInsertSql(&sql, root, 1, rel, targetAttrs, doNothing,
+ deparseInsertSql(&sql, rte, resultRelation, rel, targetAttrs, doNothing,
resultRelInfo->ri_returningList,
&retrieved_attrs);
/* Construct an execution state. */
fmstate = create_foreign_modify(mtstate->ps.state,
+ rte,
resultRelInfo,
CMD_INSERT,
NULL,
@@ -3255,6 +3256,7 @@ close_cursor(PGconn *conn, unsigned int cursor_number)
*/
static PgFdwModifyState *
create_foreign_modify(EState *estate,
+ RangeTblEntry *rte,
ResultRelInfo *resultRelInfo,
CmdType operation,
Plan *subplan,
@@ -3266,7 +3268,6 @@ create_foreign_modify(EState *estate,
PgFdwModifyState *fmstate;
Relation rel = resultRelInfo->ri_RelationDesc;
TupleDesc tupdesc = RelationGetDescr(rel);
- RangeTblEntry *rte;
Oid userid;
ForeignTable *table;
UserMapping *user;
@@ -3283,7 +3284,6 @@ create_foreign_modify(EState *estate,
* Identify which user to do the remote access as. This should match
what
* ExecCheckRTEPerms() does.
*/
- rte = rt_fetch(resultRelInfo->ri_RangeTableIndex,
estate->es_range_table);
userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
/* Get info about foreign table. */
diff --git a/contrib/postgres_fdw/postgres_fdw.h
b/contrib/postgres_fdw/postgres_fdw.h
index d37cc88b6e..a5d4011e8d 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -140,11 +140,11 @@ extern void classifyConditions(PlannerInfo *root,
extern bool is_foreign_expr(PlannerInfo *root,
RelOptInfo *baserel,
Expr *expr);
-extern void deparseInsertSql(StringInfo buf, PlannerInfo *root,
+extern void deparseInsertSql(StringInfo buf, RangeTblEntry *rte,
Index rtindex, Relation rel,
List *targetAttrs, bool doNothing, List
*returningList,
List **retrieved_attrs);
-extern void deparseUpdateSql(StringInfo buf, PlannerInfo *root,
+extern void deparseUpdateSql(StringInfo buf, RangeTblEntry *rte,
Index rtindex, Relation rel,
List *targetAttrs, List *returningList,
List **retrieved_attrs);
@@ -157,7 +157,7 @@ extern void deparseDirectUpdateSql(StringInfo buf,
PlannerInfo *root,
List **params_list,
List *returningList,
List **retrieved_attrs);
-extern void deparseDeleteSql(StringInfo buf, PlannerInfo *root,
+extern void deparseDeleteSql(StringInfo buf, RangeTblEntry *rte,
Index rtindex, Relation rel,
List *returningList,
List **retrieved_attrs);
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql
b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 633e9fecec..231b1e01a5 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1804,6 +1804,31 @@ insert into itrtest values (1, 'bar') on conflict (a) do
update set b = excluded
select tableoid::regclass, * FROM itrtest;
+delete from itrtest;
+
+drop index loct1_idx;
+
+-- Test that remote triggers work with insert tuple routing
+create function br_insert_trigfunc() returns trigger as $$
+begin
+ new.b := new.b || ' triggered !';
+ return new;
+end
+$$ language plpgsql;
+create trigger loct1_br_insert_trigger before insert on loct1
+ for each row execute procedure br_insert_trigfunc();
+create trigger loct2_br_insert_trigger before insert on loct2
+ for each row execute procedure br_insert_trigfunc();
+
+-- The new values are concatenated with ' triggered !'
+insert into itrtest values (1, 'foo') returning *;
+insert into itrtest values (2, 'qux') returning *;
+insert into itrtest values (1, 'test1'), (2, 'test2') returning *;
+with result as (insert into itrtest values (1, 'test1'), (2, 'test2')
returning *) select * from result;
+
+drop trigger loct1_br_insert_trigger on loct1;
+drop trigger loct2_br_insert_trigger on loct2;
+
drop table itrtest;
drop table loct1;
drop table loct2;
@@ -1836,6 +1861,30 @@ select tableoid::regclass, * FROM locp;
-- The executor should not let unexercised FDWs shut down
update utrtest set a = 1 where b = 'foo';
+-- Test that remote triggers work with update tuple routing
+create trigger loct_br_insert_trigger before insert on loct
+ for each row execute procedure br_insert_trigfunc();
+
+delete from utrtest;
+insert into utrtest values (2, 'qux');
+
+-- Check case where the foreign partition is a subplan target rel
+explain (verbose, costs off)
+update utrtest set a = 1 where a = 1 or a = 2 returning *;
+-- The new values are concatenated with ' triggered !'
+update utrtest set a = 1 where a = 1 or a = 2 returning *;
+
+delete from utrtest;
+insert into utrtest values (2, 'qux');
+
+-- Check case where the foreign partition isn't a subplan target rel
+explain (verbose, costs off)
+update utrtest set a = 1 where a = 2 returning *;
+-- The new values are concatenated with ' triggered !'
+update utrtest set a = 1 where a = 2 returning *;
+
+drop trigger loct_br_insert_trigger on loct;
+
drop table utrtest;
drop table loct;
diff --git a/src/backend/executor/execPartition.c
b/src/backend/executor/execPartition.c
index 233e3e9552..0e874857fc 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -334,6 +334,13 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
estate->es_instrument);
/*
+ * Verify result relation is a valid target for an INSERT. An UPDATE
of a
+ * partition-key becomes a DELETE+INSERT operation, so this check is
still
+ * required when the operation is CMD_UPDATE.
+ */
+ CheckValidResultRel(leaf_part_rri, CMD_INSERT);
+
+ /*
* Since we've just initialized this ResultRelInfo, it's not in any list
* attached to the estate as yet. Add it, so that it can be found
later.
*
@@ -345,9 +352,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
lappend(estate->es_tuple_routing_result_relations,
leaf_part_rri);
- /* Set up information needed for routing tuples to this partition. */
- ExecInitRoutingInfo(mtstate, estate, proute, leaf_part_rri, partidx);
-
/*
* Open partition indices. The user may have asked to check for
conflicts
* within this leaf partition and do "nothing" instead of throwing an
@@ -488,6 +492,9 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
&mtstate->ps, RelationGetDescr(partrel));
}
+ /* Set up information needed for routing tuples to the partition. */
+ ExecInitRoutingInfo(mtstate, estate, proute, leaf_part_rri, partidx);
+
/*
* If there is an ON CONFLICT clause, initialize state for it.
*/
@@ -653,8 +660,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
/*
* ExecInitRoutingInfo
- * Set up information needed for routing tuples to a leaf
partition if
- * routable; else abort the operation
+ * Set up information needed for routing tuples to a leaf partition
*/
void
ExecInitRoutingInfo(ModifyTableState *mtstate,
@@ -665,9 +671,6 @@ ExecInitRoutingInfo(ModifyTableState *mtstate,
{
MemoryContext oldContext;
- /* Verify the partition is a valid target for INSERT */
- CheckValidResultRel(partRelInfo, CMD_INSERT);
-
/*
* Switch into per-query memory context.
*/
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index 7ec2c6bcaa..fc1e5385bb 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1700,20 +1700,24 @@ ExecPrepareTupleRouting(ModifyTableState *mtstate,
partidx);
/*
- * Set up information needed for routing tuples to the partition if we
- * didn't yet (ExecInitRoutingInfo would abort the operation if the
- * partition isn't routable).
+ * Check whether the partition is routable if we didn't yet
*
* Note: an UPDATE of a partition key invokes an INSERT that moves the
- * tuple to a new partition. This setup would be needed for a subplan
+ * tuple to a new partition. This check would be applied to a subplan
* partition of such an UPDATE that is chosen as the partition to route
- * the tuple to. The reason we do this setup here rather than in
+ * the tuple to. The reason we do this check here rather than in
* ExecSetupPartitionTupleRouting is to avoid aborting such an UPDATE
* unnecessarily due to non-routable subplan partitions that may not be
* chosen for update tuple movement after all.
*/
if (!partrel->ri_PartitionReadyForRouting)
+ {
+ /* Verify the partition is a valid target for INSERT. */
+ CheckValidResultRel(partrel, CMD_INSERT);
+
+ /* Set up information needed for routing tuples to the
partition. */
ExecInitRoutingInfo(mtstate, estate, proute, partrel, partidx);
+ }
/*
* Make it look like we are inserting into the partition.
diff --git a/src/backend/executor/execPartition.c
b/src/backend/executor/execPartition.c
index b2ee92eb15..233e3e9552 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -307,7 +307,8 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
ModifyTable *node = (ModifyTable *) mtstate->ps.plan;
Relation rootrel = resultRelInfo->ri_RelationDesc,
partrel;
- Relation firstResultRel =
mtstate->resultRelInfo[0].ri_RelationDesc;
+ Relation firstResultRel =
mtstate->resultRelInfo->ri_RelationDesc;
+ Index firstVarno = mtstate->resultRelInfo->ri_RangeTableIndex;
ResultRelInfo *leaf_part_rri;
MemoryContext oldContext;
AttrNumber *part_attnos = NULL;
@@ -328,7 +329,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
leaf_part_rri = makeNode(ResultRelInfo);
InitResultRelInfo(leaf_part_rri,
partrel,
- node ? node->nominalRelation : 1,
+ firstVarno,
rootrel,
estate->es_instrument);
@@ -371,7 +372,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
List *wcoList;
List *wcoExprs = NIL;
ListCell *ll;
- int firstVarno =
mtstate->resultRelInfo[0].ri_RangeTableIndex;
/*
* In the case of INSERT on a partitioned table, there is only
one
@@ -437,7 +437,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
TupleTableSlot *slot;
ExprContext *econtext;
List *returningList;
- int firstVarno =
mtstate->resultRelInfo[0].ri_RangeTableIndex;
/* See the comment above for WCO lists. */
Assert((node->operation == CMD_INSERT &&
@@ -495,7 +494,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
if (node && node->onConflictAction != ONCONFLICT_NONE)
{
TupleConversionMap *map =
proute->parent_child_tupconv_maps[partidx];
- int firstVarno =
mtstate->resultRelInfo[0].ri_RangeTableIndex;
TupleDesc partrelDesc = RelationGetDescr(partrel);
ExprContext *econtext = mtstate->ps.ps_ExprContext;
ListCell *lc;
