Hi,
While updating the fix-postgres_fdw-WCO-handling patch, I noticed that
the patch for tuple routing for foreign partitions doesn't work well
with remote triggers. Here is an example:
postgres=# create table loct1 (a int check (a in (1)), b text);
postgres=# create function br_insert_trigfunc() returns trigger as
$$begin new.b := new.b || ' triggered !'; return new; end$$ language
plpgsql;
postgres=# create trigger loct1_br_insert_trigger before insert on loct1
for each row execute procedure br_insert_trigfunc();
postgres=# create table itrtest (a int, b text) partition by list (a);
postgres=# create foreign table remp1 (a int check (a in (1)), b text)
server loopback options (table_name 'loct1');
postgres=# alter table itrtest attach partition remp1 for values in (1);
This behaves oddly:
postgres=# insert into itrtest values (1, 'foo') returning *;
a | b
---+-----
1 | foo
(1 row)
INSERT 0 1
The new value of b in the RETURNING result should be concatenated with '
triggered !', as shown below:
postgres=# select tableoid::regclass, * from itrtest ;
tableoid | a | b
----------+---+-----------------
remp1 | 1 | foo triggered !
(1 row)
The reason for that is: since that in ExecInitPartitionInfo, the core
calls BeginForeignInsert via ExecInitRoutingInfo before initializing
ri_returningList of ResultRelInfo for the partition, BeginForeignInsert
sees that the ri_returningList is NULL and incorrectly recognizes that
the local query doesn't have a RETURNING list. So, I moved the
ExecInitRoutingInfo call after building the ri_returningList (and before
handling ON CONFLICT because we reference the conversion map created by
that when handling that clause). The first version of the patch called
BeginForeignInsert that order, but I updated the patch incorrectly. :(
Also, I removed the CheckValidResultRel check from ExecInitRoutingInfo
and added that to ExecInitPartitionInfo, right after the
InitResultRelInfo call, because it would be better to abort the
operation as soon as we find the partition to be non-routable. Another
thing I noticed is the RT index of the foreign partition set to 1 in
postgresBeginForeignInsert to create a remote query; that would not work
for cases where the partitioned table has an RT index larger than 1 (eg,
the partitioned table is not the primary ModifyTable node); in which
cases, since the varno of Vars belonging to the foreign partition in the
RETURNING expressions is the same as the partitioned table, calling
deparseReturningList with the RT index set to 1 against the RETURNING
expressions would produce attrs_used to be NULL, leading to postgres_fdw
not retrieving actually inserted data from the remote, even if remote
triggers might change those data. So, I fixed this as well, by setting
the RT index accordingly to the partitioned table. Attached is a patch
for fixing these issues. I'll add this to the open items list for PG11.
(After addressing this, I'll post an updated version of the
fix-postgres_fdw-WCO-handling patch.)
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 7454,7459 **** select tableoid::regclass, * FROM itrtest;
--- 7454,7501 ----
remp1 | 1 | foo
(1 row)
+ delete from itrtest;
+ drop index loct1_idx;
+ -- Test that remote triggers work as expected
+ 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;
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 1975,1982 **** postgresBeginForeignInsert(ModifyTableState *mtstate,
{
PgFdwModifyState *fmstate;
Plan *plan = mtstate->ps.plan;
Relation rel = resultRelInfo->ri_RelationDesc;
- RangeTblEntry *rte;
Query *query;
PlannerInfo *root;
TupleDesc tupdesc = RelationGetDescr(rel);
--- 1975,1983 ----
{
PgFdwModifyState *fmstate;
Plan *plan = mtstate->ps.plan;
+ EState *estate = mtstate->ps.state;
+ Index resultRelation = resultRelInfo->ri_RangeTableIndex;
Relation rel = resultRelInfo->ri_RelationDesc;
Query *query;
PlannerInfo *root;
TupleDesc tupdesc = RelationGetDescr(rel);
***************
*** 1985,2002 **** postgresBeginForeignInsert(ModifyTableState *mtstate,
List *targetAttrs = NIL;
List *retrieved_attrs = NIL;
bool doNothing = 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;
--- 1986,2018 ----
List *targetAttrs = NIL;
List *retrieved_attrs = NIL;
bool doNothing = false;
+ ListCell *lc = NULL;
+ RangeTblEntry *parent_rte = NULL;
initStringInfo(&sql);
+ /*
+ * If the foreign table is a partition, temporarily replace the parent's
+ * RTE in the range table with a new target RTE describing the foreign
+ * table for use by deparseInsertSql() and create_foreign_modify() below.
+ */
+ if (resultRelInfo->ri_PartitionRoot)
+ {
+ RangeTblEntry *child_rte;
+
+ lc = list_nth_cell(estate->es_range_table, resultRelation - 1);
+ parent_rte = lfirst(lc);
+ child_rte = copyObject(parent_rte);
+ child_rte->relid = RelationGetRelid(rel);
+ child_rte->relkind = RELKIND_FOREIGN_TABLE;
+ lfirst(lc) = child_rte;
+ }
+
/* Set up largely-dummy planner state. */
query = makeNode(Query);
query->commandType = CMD_INSERT;
! query->resultRelation = resultRelation;
! query->rtable = estate->es_range_table;
root = makeNode(PlannerInfo);
root->parse = query;
***************
*** 2023,2033 **** postgresBeginForeignInsert(ModifyTableState *mtstate,
}
/* Construct the SQL command string. */
! deparseInsertSql(&sql, root, 1, rel, targetAttrs, doNothing,
resultRelInfo->ri_returningList, &retrieved_attrs);
/* Construct an execution state. */
! fmstate = create_foreign_modify(mtstate->ps.state,
resultRelInfo,
CMD_INSERT,
NULL,
--- 2039,2049 ----
}
/* Construct the SQL command string. */
! deparseInsertSql(&sql, root, resultRelation, rel, targetAttrs, doNothing,
resultRelInfo->ri_returningList, &retrieved_attrs);
/* Construct an execution state. */
! fmstate = create_foreign_modify(estate,
resultRelInfo,
CMD_INSERT,
NULL,
***************
*** 2036,2041 **** postgresBeginForeignInsert(ModifyTableState *mtstate,
--- 2052,2061 ----
retrieved_attrs != NIL,
retrieved_attrs);
+ /* Restore the saved RTE. */
+ if (resultRelInfo->ri_PartitionRoot)
+ lfirst(lc) = parent_rte;
+
resultRelInfo->ri_FdwState = fmstate;
}
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 1804,1809 **** insert into itrtest values (1, 'bar') on conflict (a) do update set b = excluded
--- 1804,1834 ----
select tableoid::regclass, * FROM itrtest;
+ delete from itrtest;
+
+ drop index loct1_idx;
+
+ -- Test that remote triggers work as expected
+ 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;
*** a/src/backend/executor/execPartition.c
--- b/src/backend/executor/execPartition.c
***************
*** 330,335 **** ExecInitPartitionInfo(ModifyTableState *mtstate,
--- 330,342 ----
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.
*
***************
*** 341,349 **** 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
--- 348,353 ----
***************
*** 466,471 **** ExecInitPartitionInfo(ModifyTableState *mtstate,
--- 470,478 ----
&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.
*/
***************
*** 612,619 **** ExecInitPartitionInfo(ModifyTableState *mtstate,
/*
* ExecInitRoutingInfo
! * Set up information needed for routing tuples to a leaf partition if
! * routable; else abort the operation
*/
void
ExecInitRoutingInfo(ModifyTableState *mtstate,
--- 619,625 ----
/*
* ExecInitRoutingInfo
! * Set up information needed for routing tuples to a leaf partition
*/
void
ExecInitRoutingInfo(ModifyTableState *mtstate,
***************
*** 624,632 **** ExecInitRoutingInfo(ModifyTableState *mtstate,
{
MemoryContext oldContext;
- /* Verify the partition is a valid target for INSERT */
- CheckValidResultRel(partRelInfo, CMD_INSERT);
-
/*
* Switch into per-query memory context.
*/
--- 630,635 ----
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***************
*** 1700,1719 **** 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).
*
* 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
* 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
* 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)
ExecInitRoutingInfo(mtstate, estate, proute, partrel, partidx);
/*
* Make it look like we are inserting into the partition.
--- 1700,1723 ----
partidx);
/*
! * 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 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 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.
