On 2017/06/16 0:05, Ildus Kurbangaliev wrote:

I 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.

Checked the latest patch. Looks good.
Shouldn't this patch be backported to 9.6 and 10beta? The bug
affects them too.

Thank you for the review!

The bug is in foreign table inheritance, which was supported in 9.5, so I think this patch should be backported to 9.5.

Ashutosh mentioned his concern about what I proposed above before [2], but I'm not sure we should address that. And there have been no opinions from him (or anyone else) since then. So, I'd like to leave that for committer (ie, +1 for Ready for Committer).

Attached is a slightly-updated version; I renamed some variables used in rewrite_inherited_tlist() to match other existing code in prepunion.c and revised some comments a bit. I didn't make any functional changes, so I'll keep this Ready for Committer.

Best regards,
Etsuro Fujita

[2] https://www.postgresql.org/message-id/CAFjFpRfTpamoUz6fNyk6gPh%3DecfBJjbUHJNKbDxscpyPJ3FfjQ%40mail.gmail.com
*** 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
***************
*** 1538,1543 **** ExecModifyTable(ModifyTableState *node)
--- 1538,1544 ----
                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 *child_rte);
  
  
  /*
***************
*** 1807,1812 **** adjust_appendrel_attrs(PlannerInfo *root, Node *node, 
AppendRelInfo *appinfo)
--- 1809,1827 ----
                                newnode->targetList =
                                        
adjust_inherited_tlist(newnode->targetList,
                                                                                
   appinfo);
+                       /* And fix junk tlist entries for UPDATE/DELETE 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 of the specified query
+  *      Replace resjunk entry(s) in the targetlist added by 
rewriteTargetListUD
+  *      for the parent table with one(s) for the child table.
+  *
+  * The targetlist has already been through expression_tree_mutator;
+  * therefore the TargetEntry nodes are fresh copies that it's okay to
+  * scribble on.
+  */
+ static void
+ rewrite_inherited_tlist(Query *parse, RangeTblEntry *child_rte)
+ {
+       ListCell   *tl;
+       List       *new_tlist = NIL;
+       List       *junk_tlist = NIL;
+       Relation        child_relation;
+       int                     next_junk_attrno;
+ 
+       /*
+        * Open the child relation; we need not lock it since it was already
+        * locked by expand_inherited_rtentry.
+        */
+       child_relation = heap_open(child_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)
+               {
+                       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 */
+       rewriteTargetListUD(parse, child_rte, child_relation);
+ 
+       /* Close the child relation, but keep the lock */
+       heap_close(child_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