On 2017/06/02 18:10, Etsuro Fujita wrote:
On 2017/05/16 21:36, Etsuro Fujita wrote:
One approach I came up with to fix this issue is to rewrite the
targetList entries of an inherited UPDATE/DELETE properly using
rewriteTargetListUD, when generating a plan for each child table in
inheritance_planner. Attached is a WIP patch for that. Maybe I am
missing something, though.
While updating the patch, I noticed the patch rewrites the UPDATE
targetList incorrectly in some cases; rewrite_inherited_tlist I added to
adjust_appendrel_attrs (1) removes all junk items from the targetList
and (2) adds junk items for the child table using rewriteTargetListUD,
but it's wrong to drop all junk items in cases where there are junk
items for some other reasons than rewriteTargetListUD. Consider junk
items containing MULTIEXPR SubLink. One way I came up with to fix this
is to change (1) to only remove junk items with resname; since junk
items added by rewriteTargetListUD should have resname (note: we would
need resname to call ExecFindJunkAttributeInTlist at execution time!)
while other junk items wouldn't have resname (see
transformUpdateTargetList), we could correctly replace junk items added
by rewriteTargetListUD for the parent with ones for the child, by that
change. I might be missing something, though. Comments welcome.
I updated the patch that way. Please find attached an updated version.
Other changes:
* Moved the initialization for "tupleid" I added in ExecModifyTable as
discussed before, which I think is essentially the same as proposed by
Ildus in [1], since I think that would be more consistent with "oldtuple".
* Added regression tests.
Anyway I'll add this to the next commitfest.
Best regards,
Etsuro Fujita
[1]
https://www.postgresql.org/message-id/20170514150525.0346ba72%40postgrespro.ru
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 6924,6929 **** update bar set f2 = f2 + 100 returning *;
--- 6924,7038 ----
7 | 277
(6 rows)
+ -- Test that UPDATE/DELETE with inherited target works with row-level triggers
+ CREATE TRIGGER trig_row_before
+ BEFORE UPDATE OR DELETE ON bar2
+ FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
+ CREATE TRIGGER trig_row_after
+ AFTER UPDATE OR DELETE ON bar2
+ FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
+ explain (verbose, costs off)
+ update bar set f2 = f2 + 100;
+ QUERY PLAN
+
--------------------------------------------------------------------------------------
+ Update on public.bar
+ Update on public.bar
+ Foreign Update on public.bar2
+ Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1 RETURNING
f1, f2, f3
+ -> Seq Scan on public.bar
+ Output: bar.f1, (bar.f2 + 100), bar.ctid
+ -> Foreign Scan on public.bar2
+ Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, bar2.*
+ Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE
+ (9 rows)
+
+ update bar set f2 = f2 + 100;
+ NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
+ NOTICE: OLD: (3,333,33),NEW: (3,433,33)
+ NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
+ NOTICE: OLD: (4,344,44),NEW: (4,444,44)
+ NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
+ NOTICE: OLD: (7,277,77),NEW: (7,377,77)
+ NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
+ NOTICE: OLD: (3,333,33),NEW: (3,433,33)
+ NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
+ NOTICE: OLD: (4,344,44),NEW: (4,444,44)
+ NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
+ NOTICE: OLD: (7,277,77),NEW: (7,377,77)
+ explain (verbose, costs off)
+ update bar set (f2,f1) = (select f1, f2 from bar2 where f3 = 77) where f1 = 7;
+ QUERY PLAN
+
-----------------------------------------------------------------------------------------------
+ Update on public.bar
+ Update on public.bar
+ Foreign Update on public.bar2
+ Remote SQL: UPDATE public.loct2 SET f1 = $2, f2 = $3 WHERE ctid = $1
RETURNING f1, f2, f3
+ InitPlan 1 (returns $0,$1)
+ -> Foreign Scan on public.bar2 bar2_1
+ Output: bar2_1.f1, bar2_1.f2
+ Remote SQL: SELECT f1, f2 FROM public.loct2 WHERE ((f3 = 77))
+ -> Seq Scan on public.bar
+ Output: $1, $0, NULL::record, bar.ctid
+ Filter: (bar.f1 = 7)
+ -> Foreign Scan on public.bar2
+ Output: $1, $0, bar2.f3, NULL::record, bar2.ctid, bar2.*
+ Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 WHERE ((f1 =
7)) FOR UPDATE
+ (14 rows)
+
+ update bar set (f2,f1) = (select f1, f2 from bar2 where f3 = 77) where f1 = 7;
+ NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
+ NOTICE: OLD: (7,377,77),NEW: (377,7,77)
+ NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
+ NOTICE: OLD: (7,377,77),NEW: (377,7,77)
+ explain (verbose, costs off)
+ update bar o set (f2,f1) = (select f1, f2 from bar2 i where i.f1 = o.f1 and
i.f2 = o.f2 and i.f3 = 77) where f2 = 7;
+ QUERY PLAN
+
---------------------------------------------------------------------------------------------------------------------------------
+ Update on public.bar o
+ Update on public.bar o
+ Foreign Update on public.bar2 o_1
+ Remote SQL: UPDATE public.loct2 SET f1 = $2, f2 = $3 WHERE ctid = $1
RETURNING f1, f2, f3
+ -> Seq Scan on public.bar o
+ Output: $3, $2, (SubPlan 1 (returns $2,$3)), o.ctid
+ Filter: (o.f2 = 7)
+ SubPlan 1 (returns $2,$3)
+ -> Foreign Scan on public.bar2 i
+ Output: i.f1, i.f2
+ Remote SQL: SELECT f1, f2 FROM public.loct2 WHERE ((f1 =
$1::integer)) AND ((f2 = $2::integer)) AND ((f3 = 77))
+ -> Foreign Scan on public.bar2 o_1
+ Output: $3, $2, o_1.f3, (SubPlan 1 (returns $2,$3)), o_1.ctid, o_1.*
+ Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 WHERE ((f2 =
7)) FOR UPDATE
+ (14 rows)
+
+ update bar o set (f2,f1) = (select f1, f2 from bar2 i where i.f1 = o.f1 and
i.f2 = o.f2 and i.f3 = 77) where f2 = 7;
+ NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
+ NOTICE: OLD: (377,7,77),NEW: (7,377,77)
+ NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
+ NOTICE: OLD: (377,7,77),NEW: (7,377,77)
+ explain (verbose, costs off)
+ delete from bar where f2 < 400;
+ QUERY PLAN
+
---------------------------------------------------------------------------------------------
+ Delete on public.bar
+ Delete on public.bar
+ Foreign Delete on public.bar2
+ Remote SQL: DELETE FROM public.loct2 WHERE ctid = $1 RETURNING f1, f2, f3
+ -> Seq Scan on public.bar
+ Output: bar.ctid
+ Filter: (bar.f2 < 400)
+ -> Foreign Scan on public.bar2
+ Output: bar2.ctid, bar2.*
+ Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 WHERE ((f2 <
400)) FOR UPDATE
+ (10 rows)
+
+ delete from bar where f2 < 400;
+ NOTICE: trig_row_before(23, skidoo) BEFORE ROW DELETE ON bar2
+ NOTICE: OLD: (7,377,77)
+ NOTICE: trig_row_after(23, skidoo) AFTER ROW DELETE ON bar2
+ NOTICE: OLD: (7,377,77)
+ -- cleanup
+ DROP TRIGGER trig_row_before ON bar2;
+ DROP TRIGGER trig_row_after ON bar2;
drop table foo cascade;
NOTICE: drop cascades to foreign table foo2
drop table bar cascade;
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 1609,1614 **** explain (verbose, costs off)
--- 1609,1642 ----
update bar set f2 = f2 + 100 returning *;
update bar set f2 = f2 + 100 returning *;
+ -- Test that UPDATE/DELETE with inherited target works with row-level triggers
+ CREATE TRIGGER trig_row_before
+ BEFORE UPDATE OR DELETE ON bar2
+ FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
+
+ CREATE TRIGGER trig_row_after
+ AFTER UPDATE OR DELETE ON bar2
+ FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
+
+ explain (verbose, costs off)
+ update bar set f2 = f2 + 100;
+ update bar set f2 = f2 + 100;
+
+ explain (verbose, costs off)
+ update bar set (f2,f1) = (select f1, f2 from bar2 where f3 = 77) where f1 = 7;
+ update bar set (f2,f1) = (select f1, f2 from bar2 where f3 = 77) where f1 = 7;
+
+ explain (verbose, costs off)
+ update bar o set (f2,f1) = (select f1, f2 from bar2 i where i.f1 = o.f1 and
i.f2 = o.f2 and i.f3 = 77) where f2 = 7;
+ update bar o set (f2,f1) = (select f1, f2 from bar2 i where i.f1 = o.f1 and
i.f2 = o.f2 and i.f3 = 77) where f2 = 7;
+
+ explain (verbose, costs off)
+ delete from bar where f2 < 400;
+ delete from bar where f2 < 400;
+
+ -- cleanup
+ DROP TRIGGER trig_row_before ON bar2;
+ DROP TRIGGER trig_row_after ON bar2;
drop table foo cascade;
drop table bar cascade;
drop table loct1;
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***************
*** 1521,1526 **** ExecModifyTable(ModifyTableState *node)
--- 1521,1527 ----
EvalPlanQualSetSlot(&node->mt_epqstate, planSlot);
slot = planSlot;
+ tupleid = NULL;
oldtuple = NULL;
if (junkfilter != NULL)
{
*** a/src/backend/optimizer/prep/prepunion.c
--- b/src/backend/optimizer/prep/prepunion.c
***************
*** 47,52 ****
--- 47,53 ----
#include "optimizer/tlist.h"
#include "parser/parse_coerce.h"
#include "parser/parsetree.h"
+ #include "rewrite/rewriteHandler.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
#include "utils/selfuncs.h"
***************
*** 110,115 **** static Node *adjust_appendrel_attrs_mutator(Node *node,
--- 111,117 ----
static Relids adjust_relid_set(Relids relids, Index oldrelid, Index newrelid);
static List *adjust_inherited_tlist(List *tlist,
AppendRelInfo *context);
+ static void rewrite_inherited_tlist(Query *parse, RangeTblEntry *target_rte);
/*
***************
*** 1807,1812 **** adjust_appendrel_attrs(PlannerInfo *root, Node *node,
AppendRelInfo *appinfo)
--- 1809,1827 ----
newnode->targetList =
adjust_inherited_tlist(newnode->targetList,
appinfo);
+ /* And fix UPDATE/DELETE junk tlist entries too, if
needed */
+ if (newnode->commandType == CMD_UPDATE ||
+ newnode->commandType == CMD_DELETE)
+ {
+ RangeTblEntry *childrte;
+ RangeTblEntry *parentrte;
+
+ childrte =
planner_rt_fetch(appinfo->child_relid, root);
+ parentrte =
planner_rt_fetch(appinfo->parent_relid, root);
+ if (childrte->relkind == RELKIND_FOREIGN_TABLE
||
+ parentrte->relkind ==
RELKIND_FOREIGN_TABLE)
+ rewrite_inherited_tlist(newnode,
childrte);
+ }
}
result = (Node *) newnode;
}
***************
*** 2140,2145 **** adjust_inherited_tlist(List *tlist, AppendRelInfo *context)
--- 2155,2231 ----
}
/*
+ * Rewrite the targetlist entries of an inherited UPDATE/DELETE operation
+ * Replace resjunk entry(s) added by rewriteTargetListUD for the parent
+ * table update with one(s) for the child table update.
+ */
+ static void
+ rewrite_inherited_tlist(Query *parse, RangeTblEntry *target_rte)
+ {
+ ListCell *tl;
+ List *new_tlist = NIL;
+ List *junk_tlist = NIL;
+ Relation target_relation;
+ int next_junk_attrno;
+
+ /*
+ * Open the target relation; we need not lock it since it was already
+ * locked by expand_inherited_rtentry().
+ */
+ target_relation = heap_open(target_rte->relid, NoLock);
+
+ /*
+ * Create a new tlist without resjunk entries; resjunk entries other
+ * than one(s) added by rewriteTargetListUD are tossed into a separate
+ * list; then appended to the new tlist.
+ */
+ foreach(tl, parse->targetList)
+ {
+ TargetEntry *tle = (TargetEntry *) lfirst(tl);
+
+ if (tle->resjunk)
+ {
+ /*
+ * If the resjunk entry has a resname, it should have
been added
+ * by rewriteTargetListUD, so ignore it.
+ */
+ if (!tle->resname)
+ junk_tlist = lappend(junk_tlist, tle);
+ continue;
+ }
+
+ new_tlist = lappend(new_tlist, tle);
+ }
+
+ next_junk_attrno = list_length(new_tlist) + 1;
+ foreach(tl, junk_tlist)
+ {
+ TargetEntry *tle = (TargetEntry *) lfirst(tl);
+
+ /* Get the resno right */
+ if (tle->resno != next_junk_attrno)
+ {
+ /*
+ * We assume here that the TargetEntry node is a copy,
so okay to
+ * scribble on it (see the comment for
adjust_inherited_tlist).
+ */
+ tle->resno = next_junk_attrno;
+ }
+ new_tlist = lappend(new_tlist, tle);
+ next_junk_attrno++;
+ }
+
+ /* Replace the query's targetlist with the new tlist */
+ parse->targetList = new_tlist;
+
+ /* And add resjunk entry(s) needed for the child table update */
+ rewriteTargetListUD(parse, target_rte, target_relation);
+
+ /* Close the target relation, but keep the lock */
+ heap_close(target_relation, NoLock);
+ }
+
+ /*
* adjust_appendrel_attrs_multilevel
* Apply Var translations from a toplevel appendrel parent down to a
child.
*
*** a/src/backend/rewrite/rewriteHandler.c
--- b/src/backend/rewrite/rewriteHandler.c
***************
*** 72,79 **** static TargetEntry *process_matched_tle(TargetEntry *src_tle,
static Node *get_assignment_input(Node *node);
static void rewriteValuesRTE(RangeTblEntry *rte, Relation target_relation,
List *attrnos);
- static void rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte,
- Relation target_relation);
static void markQueryForLocking(Query *qry, Node *jtnode,
LockClauseStrength strength,
LockWaitPolicy waitPolicy,
bool pushedDown);
--- 72,77 ----
***************
*** 1269,1275 **** rewriteValuesRTE(RangeTblEntry *rte, Relation
target_relation, List *attrnos)
* For UPDATE queries, this is applied after rewriteTargetListIU. The
* ordering isn't actually critical at the moment.
*/
! static void
rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte,
Relation target_relation)
{
--- 1267,1273 ----
* For UPDATE queries, this is applied after rewriteTargetListIU. The
* ordering isn't actually critical at the moment.
*/
! void
rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte,
Relation target_relation)
{
*** a/src/include/rewrite/rewriteHandler.h
--- b/src/include/rewrite/rewriteHandler.h
***************
*** 23,28 **** extern void AcquireRewriteLocks(Query *parsetree,
--- 23,30 ----
bool forUpdatePushedDown);
extern Node *build_column_default(Relation rel, int attrno);
+ extern void rewriteTargetListUD(Query *parsetree, RangeTblEntry *target_rte,
+ Relation target_relation);
extern Query *get_view_query(Relation view);
extern const char *view_query_is_auto_updatable(Query *viewquery,
bool check_cols);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
