(2018/04/26 12:43), Etsuro Fujita wrote:
(2018/04/25 17:29), Amit Langote wrote:
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.
That's right. I took care of this in my version, but unfortuneately,
that was ignored in the updated versions. Maybe the comments I added to
the patch were not enough, though.
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;
As I said upthread, we can use the RTE in the range table as-is if the
foreign table is one of the UPDATE subplan partitions or the target
specified in a COPY command. So, I created the patch that way because
that would save cycles. Why not just use that RTE in those cases?
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?
Actually, I also thought the former when creating the patch, but I left
that as-is because I'm not sure that would be really safe;
ExecConstraints looks at the RT index via
GetInsertedColumns/GetUpdatedColumns, so what I thought was: that might
cause unexpected behavior. Anyway, I think that the former is more like
an improvement rather than a fix, so it would be better to leave that
for another patch for PG12?
Here is a new version I'd like to propose for fixing this issue without
the former patch.
Thanks for working on this!
Best regards,
Etsuro Fujita
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 6e2fa14..d272719 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 53ed285..bb6b1a8 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 a46160d..1285e39 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,32 +1981,21 @@ 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;
+ RangeTblEntry *rte = NULL;
TupleDesc tupdesc = RelationGetDescr(rel);
int attnum;
StringInfoData sql;
List *targetAttrs = NIL;
List *retrieved_attrs = NIL;
bool doNothing = false;
+ bool dostuff = false;
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++)
{
@@ -2012,7 +2008,7 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
/* Check if we add the ON CONFLICT clause to the remote query. */
if (plan)
{
- OnConflictAction onConflictAction = ((ModifyTable *) plan)->onConflictAction;
+ OnConflictAction onConflictAction = plan->onConflictAction;
/* We only support DO NOTHING without an inference specification. */
if (onConflictAction == ONCONFLICT_NOTHING)
@@ -2022,12 +2018,34 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
(int) onConflictAction);
}
+ /* Find or create the RTE for the foreign table. */
+ if (resultRelInfo->ri_PartitionRoot)
+ {
+ dostuff = true;
+
+ if (plan && plan->operation == CMD_UPDATE)
+ {
+ if (resultRelation != plan->nominalRelation)
+ dostuff = false;
+ else
+ resultRelation = mtstate->resultRelInfo[0].ri_RangeTableIndex;
+ }
+ }
+ rte = list_nth(estate->es_range_table, resultRelation - 1);
+ if (dostuff)
+ {
+ 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 +3273,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 +3285,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 +3301,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 d37cc88..a5d4011 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 633e9fe..231b1e0 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 b2ee92e..954a96c 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -333,6 +333,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.
*
@@ -344,9 +351,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
@@ -489,6 +493,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.
*/
@@ -655,8 +662,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,
@@ -667,9 +673,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 7ec2c6b..fc1e538 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.
