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

Reply via email to