Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Ildus Kurbangaliev]

On Mon, 24 Jul 2017 11:59:13 +0900
Etsuro Fujita  wrote:

> On 2017/07/21 19:16, Etsuro Fujita wrote:
> > On 2017/07/20 11:21, Etsuro Fujita wrote:  
> >> On 2017/07/19 23:36, Tom Lane wrote:  
> >>> Please put the responsibility of doing const-expression
> >>> simplification in these cases somewhere closer to where the
> >>> problem is being created.  
> >>
> >> It would be reasonable that it's the FDW's responsibility to do
> >> that const-simplification if necessary?  
> > There seems to be no objections, so I removed the const-expression 
> > simplification from the patch and I added the note to the docs for 
> > AddForeignUpdateTargets.
> > 
> > Attached is an updated version of the patch.  
> 
> I cleaned up the patch a bit.  PFA a new version of the patch.
> 
> Best regards,
> Etsuro Fujita

Checked, looks good to me. Changed status to 'Ready for Commiter'.

-- 
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

On 2017/07/21 19:16, Etsuro Fujita wrote:

On 2017/07/20 11:21, Etsuro Fujita wrote:

On 2017/07/19 23:36, Tom Lane wrote:

Please put the responsibility of doing const-expression simplification
in these cases somewhere closer to where the problem is being created.


It would be reasonable that it's the FDW's responsibility to do that 
const-simplification if necessary?
There seems to be no objections, so I removed the const-expression 
simplification from the patch and I added the note to the docs for 
AddForeignUpdateTargets.


Attached is an updated version of the patch.


I cleaned up the patch a bit.  PFA a new version of the patch.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 6962,6967  update bar set f2 = f2 + 100 returning *;
--- 6962,7026 
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)
+ 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
***
*** 1632,1637  explain (verbose, costs off)
--- 1632,1657 
  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)
+ 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/doc/src/sgml/fdwhandler.sgml
--- b/doc/src/sgml/fdwhandler.sgml
***
*** 428,438  AddForeignUpdateTargets (Query *parsetree,
   Avoid using names matching ctidN,
   wholerow, or
   wholerowN, as the core system can
!  generate junk columns of these names.
  
  
 

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

On 2017/07/20 11:21, Etsuro Fujita wrote:

On 2017/07/19 23:36, Tom Lane wrote:

Please put the responsibility of doing const-expression simplification
in these cases somewhere closer to where the problem is being created.


It would be reasonable that it's the FDW's responsibility to do that 
const-simplification if necessary?
There seems to be no objections, so I removed the const-expression 
simplification from the patch and I added the note to the docs for 
AddForeignUpdateTargets.


Attached is an updated version of the patch.

Best regards,
Etsuro Fujita
*** 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,6988 
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)
+ 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,1634 
  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)
+ 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/doc/src/sgml/fdwhandler.sgml
--- b/doc/src/sgml/fdwhandler.sgml
***
*** 428,438  AddForeignUpdateTargets (Query *parsetree,
   Avoid using names matching ctidN,
   wholerow, or
   wholerowN, as the core system can
!  generate junk columns of these names.
  
  
  
!  This function is called in the rewriter, not the planner, so the
   information available 

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

On 2017/07/19 23:36, Tom Lane wrote:

Etsuro Fujita  writes:

* Modified rewrite_targetlist(), which is a new function added to
preptlist.c, so that we do const-simplification to junk TLEs that
AddForeignUpdateTargets() added, as that API allows the FDW to add junk
TLEs containing non-Var expressions to the query's targetlist.


This does not seem like a good idea to me.  eval_const_expressions is not
a cheap thing, and for most use-cases those cycles will be wasted, and it
has never been the responsibility of preprocess_targetlist to do this sort
of thing.


Hm, I added that const-simplification to that function so that the 
existing FDWs that append junk TLEs that need const-simplification, 
which I don't know really exist, would work well for this fix, without 
any changes, but I agree on that point.



Please put the responsibility of doing const-expression simplification
in these cases somewhere closer to where the problem is being created.


It would be reasonable that it's the FDW's responsibility to do that 
const-simplification if necessary?


Best regards,
Etsuro Fujita



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Tom Lane]

Etsuro Fujita  writes:
> * Modified rewrite_targetlist(), which is a new function added to 
> preptlist.c, so that we do const-simplification to junk TLEs that 
> AddForeignUpdateTargets() added, as that API allows the FDW to add junk 
> TLEs containing non-Var expressions to the query's targetlist.

This does not seem like a good idea to me.  eval_const_expressions is not
a cheap thing, and for most use-cases those cycles will be wasted, and it
has never been the responsibility of preprocess_targetlist to do this sort
of thing.

Please put the responsibility of doing const-expression simplification
in these cases somewhere closer to where the problem is being created.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

On 2017/07/13 21:10, Etsuro Fujita wrote:

Attached is an updated version of the patch.

Here is an updated version of the patch.  Changes are:

* Modified rewrite_targetlist(), which is a new function added to 
preptlist.c, so that we do const-simplification to junk TLEs that 
AddForeignUpdateTargets() added, as that API allows the FDW to add junk 
TLEs containing non-Var expressions to the query's targetlist.

* Updated docs in fdwhandler.sgml.

Best regards,
Etsuro Fujita
*** 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,6988 
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)
+ 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,1634 
  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)
+ 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/doc/src/sgml/fdwhandler.sgml
--- b/doc/src/sgml/fdwhandler.sgml
***
*** 432,438  AddForeignUpdateTargets (Query *parsetree,
  
  
  
!  This function is called in the rewriter, not the planner, so the
   information available is a bit different from that available to the
   planning routines.
   parsetree is the parse tree for the UPDATE or
--- 432,438 
  
  
  
!  Although this function is called in the planner, the
   

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

On 2017/06/30 18:44, Etsuro Fujita wrote:

On 2017/06/16 21:29, Etsuro Fujita wrote:
I'll have second thought about this, so I'll mark this as waiting on 
author.


I spent quite a bit of time on this and came up with a solution for 
addressing the concern mentioned by Ashutosh [1].  The basic idea is, as 
I said before, to move rewriteTargetListUD from the rewriter to the 
planner (whether the update or delete is inherited or not), except for 
the view case.  In case of inherited UPDATE/DELETE, the planner adds a 
necessary junk column needed for the update or delete for each child 
relation, without the assumption I made before about junk tlist entries, 
so I think this would be more robust for future changes as mentioned in 
[1].  It wouldn't work well that the planner does the same thing for the 
view case (ie, add a whole-row reference to the given target relation), 
unlike other cases, because what we need to do for that case is to add a 
whole-row reference to the view as the source for an INSTEAD OF trigger, 
not the target.  So, ISTM that it's reasonable to handle that case in 
the rewriter as-is, not in the planner, but one thing I'd like to 
propose to simplify the code in rewriteHandler.c is to move the code for 
the view case in rewriteTargetListUD to ApplyRetrieveRule.  By that 
change, we won't add a whole-row reference to the view in RewriteQuery, 
so we don't need this annoying thing in rewriteTargetView any more:


/*
 * For UPDATE/DELETE, rewriteTargetListUD will have added a 
wholerow junk
 * TLE for the view to the end of the targetlist, which we no 
longer need.

 * Remove it to avoid unnecessary work when we process the targetlist.
 * Note that when we recurse through rewriteQuery a new junk TLE 
will be

 * added to allow the executor to find the proper row in the new target
 * relation.  (So, if we failed to do this, we might have multiple junk
 * TLEs with the same name, which would be disastrous.)
 */
if (parsetree->commandType != CMD_INSERT)
{
TargetEntry *tle = (TargetEntry *) llast(parsetree->targetList);

Assert(tle->resjunk);
Assert(IsA(tle->expr, Var) &&
   ((Var *) tle->expr)->varno == parsetree->resultRelation &&
   ((Var *) tle->expr)->varattno == 0);
parsetree->targetList = list_delete_ptr(parsetree->targetList, 
tle);

}

Attached is an updated version of the patch.  Comments are welcome!

Best regards,
Etsuro Fujita

[1] 
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,6988 
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)
+ 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 

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

On 2017/06/16 21:29, Etsuro Fujita wrote:

On 2017/06/16 19:26, Ashutosh Bapat wrote:



That issue has not been addressed. The reason stated was that it would
make code complicated. But I have not had chance to look at how
complicated would be and assess myself whether that's worth the
trouble.
I have to admit that what I proposed upthread is a quick-and-dirty 
kluge.  One thing I thought to address your concern was to move 
rewriteTargetListUD entirely from the rewriter to the planner when doing 
inherited UPDATE/DELETE, but I'm not sure that's a good idea, because at 
least I think that would need a lot more changes to the rewriter.


I'll have second thought about this, so I'll mark this as waiting on author.

Best regards,
Etsuro Fujita



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

On 2017/06/16 19:26, Ashutosh Bapat wrote:

On Fri, Jun 16, 2017 at 3:35 PM, Etsuro Fujita

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


That issue has not been addressed. The reason stated was that it would
make code complicated. But I have not had chance to look at how
complicated would be and assess myself whether that's worth the
trouble.
I have to admit that what I proposed upthread is a quick-and-dirty 
kluge.  One thing I thought to address your concern was to move 
rewriteTargetListUD entirely from the rewriter to the planner when doing 
inherited UPDATE/DELETE, but I'm not sure that's a good idea, because at 
least I think that would need a lot more changes to the rewriter.


Best regards,
Etsuro Fujita



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

On 2017/06/16 19:26, Ashutosh Bapat wrote:

Also, I don't see any discussion about my concern [3] about a parent
with child from multiple foreign servers with different FDWs. So, I am
not sure whether the patch really fixes the problem in its entirety.


The patch would allow child tables to have different foreign servers 
with different FDWs since it applies rewriteTargetListUD to each child 
table when generating a modified query with that child table with the 
target in inheritance_planner.  I didn't any regression tests for that, 
though.


Best regards,
Etsuro Fujita



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Ashutosh Bapat]

On Fri, Jun 16, 2017 at 3:35 PM, Etsuro Fujita
 wrote:
> 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).

That issue has not been addressed. The reason stated was that it would
make code complicated. But I have not had chance to look at how
complicated would be and assess myself whether that's worth the
trouble. There was another issue

Also, I don't see any discussion about my concern [3] about a parent
with child from multiple foreign servers with different FDWs. So, I am
not sure whether the patch really fixes the problem in its entirety.

[3] 
https://www.postgresql.org/message-id/cafjfprfq1pkcjnqfvop_bpjfc7or3596nqvvfbgaidezqb4...@mail.gmail.com

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

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 

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Ildus Kurbangaliev]

On Mon, 5 Jun 2017 17:25:27 +0900
Etsuro Fujita  wrote:

> 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

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

-- 
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

On 2017/06/05 17:39, Ashutosh Bapat wrote:

On Fri, Jun 2, 2017 at 2:40 PM, Etsuro Fujita



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 haven't looked at the patch, but that doesn't look right. In future
some code path other than rewriteTargetListUD() may add junk items
with resname and this fix will remove those junk items as well.


Yeah, I thought that too.  I think we could replace junk tlist entries 
added by rewriteTargetListUD() more safely, by adding a lot more code, 
but I'm not sure it's worth complicating the code at the current stage.


Best regards,
Etsuro Fujita



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Ashutosh Bapat]

On Fri, Jun 2, 2017 at 2:40 PM, 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 haven't looked at the patch, but that doesn't look right. In future
some code path other than rewriteTargetListUD() may add junk items
with resname and this fix will remove those junk items as well.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

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)
+ 

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

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.


Best regards,
Etsuro Fujita



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

On 2017/05/17 17:54, Ildus Kurbangaliev wrote:

On Wed, 17 May 2017 15:28:24 +0900
Michael Paquier  wrote:


On Tue, May 16, 2017 at 11:26 PM, Ildus Kurbangaliev
 wrote:

On Tue, 16 May 2017 21:36:11 +0900
Etsuro Fujita  wrote:

On 2017/05/16 21:11, Ashutosh Bapat wrote:

On Tue, May 16, 2017 at 5:35 PM, Ildus Kurbangaliev
 wrote:



I agree. Maybe this issue should be added to Postgresql Open
Items? I think there should be some complex solution that fixes
not only triggers for foreign tables at table partitioning, but
covers other possible not working cases.



I doubt if this is an open item, since DMLs on foreign tables are
supported since 9.3 and support to add foreign tables to
inheritance was added back in 9.5.


I think this issue was introduced by the latter, so that was my
fault.

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.


Could this patch include some regression tests to see at what extent
it has been tested? We surely don't want to see that broken again in
the future as well. (Nit: I did not look at the patch in details yet)


OK, I'll include regression tests in the next version of the patch.


I tested the patch, looks good.


What kind of tests did you do?


I tested update triggers for foreign table when regular table is a
parent and foreign table is a child. Case like this:

explain verbose update parent set val = random();
  QUERY PLAN
---
 Update on public.parent  (cost=0.00..258.63 rows=5535 width=10)
   Update on public.parent
   Update on public.child1
   Foreign Update on public.ftable // we have triggers on ftable here



  junkfilter = resultRelInfo->ri_junkFilter;
+ tupleid = NULL;
  estate->es_result_relation_info = resultRelInfo;
Er, what is that?


That fixes the bug when tupleid from regular tuple is used to get
oldtuple for triggers of foreign table.


That's right.  Let me explain in more detail.  Currently, tupleid is 
only initialized at the top of ExecModifyTable, so if we just loop 
within the for(;;) code in that function (without returning RETURNING to 
caller), tupleid won't be initialized even when advancing to next 
subplan in case of inherited UPDATE/DELETE.  This would cause a problem. 
 Assume that the current subplan is for the parent, which is a plain 
table, that the next subplan is for the child, which is a foreign table, 
and that the foreign table has a BEFORE trigger, as tested by Ildus.  In 
that case the current subplan would set tupleid to ctid for each row 
from the plain table, and after advancing to the next subplan, the 
subplan would set oldtuple to wholerow for the first row from the 
foreign table, *without initializing tupleid to NULL*, and then call 
ExecBRUpdateTriggers/ExecBRDeleteTriggers during ExecUpdate/ExecDelete, 
which would cause an assertion error for 
Assert(HeapTupleIsValid(fdw_trigtuple) ^ ItemPointerIsValid(tupleid)) in 
those trigger functions, because oldtuple and tupleid are both valid. 
So, tupleid should be initialized at least when advancing to next 
subplan.  It might be better to initialize that at each iteration of the 
for(;;) code, like oldtuple, though.


Best regards,
Etsuro Fujita



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Ildus Kurbangaliev]

On Wed, 17 May 2017 15:28:24 +0900
Michael Paquier  wrote:

> On Tue, May 16, 2017 at 11:26 PM, Ildus Kurbangaliev
>  wrote:
> > On Tue, 16 May 2017 21:36:11 +0900
> > Etsuro Fujita  wrote:  
> >> On 2017/05/16 21:11, Ashutosh Bapat wrote:  
> >> > On Tue, May 16, 2017 at 5:35 PM, Ildus Kurbangaliev
> >> >  wrote:  
> >>  
> >> >> I agree. Maybe this issue should be added to Postgresql Open
> >> >> Items? I think there should be some complex solution that fixes
> >> >> not only triggers for foreign tables at table partitioning, but
> >> >> covers other possible not working cases.  
> >>  
> >> > I doubt if this is an open item, since DMLs on foreign tables are
> >> > supported since 9.3 and support to add foreign tables to
> >> > inheritance was added back in 9.5.  
> >>
> >> I think this issue was introduced by the latter, so that was my
> >> fault.
> >>
> >> 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.  
> 
> Could this patch include some regression tests to see at what extent
> it has been tested? We surely don't want to see that broken again in
> the future as well. (Nit: I did not look at the patch in details yet)
> 
> > I tested the patch, looks good.  
> 
> What kind of tests did you do?

I tested update triggers for foreign table when regular table is a
parent and foreign table is a child. Case like this:

explain verbose update parent set val = random();
  QUERY PLAN
---
 Update on public.parent  (cost=0.00..258.63 rows=5535 width=10)
   Update on public.parent
   Update on public.child1
   Foreign Update on public.ftable // we have triggers on ftable here

> 
>   junkfilter = resultRelInfo->ri_junkFilter;
> + tupleid = NULL;
>   estate->es_result_relation_info = resultRelInfo;
> Er, what is that?

That fixes the bug when tupleid from regular tuple is used to get
oldtuple for triggers of foreign table.

-- 
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Michael Paquier]

On Tue, May 16, 2017 at 11:26 PM, Ildus Kurbangaliev
 wrote:
> On Tue, 16 May 2017 21:36:11 +0900
> Etsuro Fujita  wrote:
>> On 2017/05/16 21:11, Ashutosh Bapat wrote:
>> > On Tue, May 16, 2017 at 5:35 PM, Ildus Kurbangaliev
>> >  wrote:
>>
>> >> I agree. Maybe this issue should be added to Postgresql Open Items?
>> >> I think there should be some complex solution that fixes not only
>> >> triggers for foreign tables at table partitioning, but covers other
>> >> possible not working cases.
>>
>> > I doubt if this is an open item, since DMLs on foreign tables are
>> > supported since 9.3 and support to add foreign tables to inheritance
>> > was added back in 9.5.
>>
>> I think this issue was introduced by the latter, so that was my fault.
>>
>> 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.

Could this patch include some regression tests to see at what extent
it has been tested? We surely don't want to see that broken again in
the future as well. (Nit: I did not look at the patch in details yet)

> I tested the patch, looks good.

What kind of tests did you do?

  junkfilter = resultRelInfo->ri_junkFilter;
+ tupleid = NULL;
  estate->es_result_relation_info = resultRelInfo;
Er, what is that?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Ildus Kurbangaliev]

On Tue, 16 May 2017 21:36:11 +0900
Etsuro Fujita  wrote:

> On 2017/05/16 21:11, Ashutosh Bapat wrote:
> > On Tue, May 16, 2017 at 5:35 PM, Ildus Kurbangaliev
> >  wrote:  
> 
> >> I agree. Maybe this issue should be added to Postgresql Open Items?
> >> I think there should be some complex solution that fixes not only
> >> triggers for foreign tables at table partitioning, but covers other
> >> possible not working cases.  
> 
> > I doubt if this is an open item, since DMLs on foreign tables are
> > supported since 9.3 and support to add foreign tables to inheritance
> > was added back in 9.5.  
> 
> I think this issue was introduced by the latter, so that was my fault.
> 
> 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.
> 
> Best regards,
> Etsuro Fujita

I tested the patch, looks good.

-- 
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

On 2017/05/16 21:11, Ashutosh Bapat wrote:

On Tue, May 16, 2017 at 5:35 PM, Ildus Kurbangaliev
 wrote:



I agree. Maybe this issue should be added to Postgresql Open Items?
I think there should be some complex solution that fixes not only
triggers for foreign tables at table partitioning, but covers other
possible not working cases.



I doubt if this is an open item, since DMLs on foreign tables are
supported since 9.3 and support to add foreign tables to inheritance
was added back in 9.5.


I think this issue was introduced by the latter, so that was my fault.

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.


Best regards,
Etsuro Fujita
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***
*** 1469,1474  ExecModifyTable(ModifyTableState *node)
--- 1469,1475 
  resultRelInfo++;
  subplanstate = node->mt_plans[node->mt_whichplan];
  junkfilter = resultRelInfo->ri_junkFilter;
+ tupleid = NULL;
  estate->es_result_relation_info = resultRelInfo;
  EvalPlanQualSetPlan(>mt_epqstate, subplanstate->plan,
  	node->mt_arowmarks[node->mt_whichplan]);
*** 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);
  
  
  /*
***
*** 1806,1811  adjust_appendrel_attrs(PlannerInfo *root, Node *node, AppendRelInfo *appinfo)
--- 1808,1826 
  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;
  	}
***
*** 2139,2144  adjust_inherited_tlist(List *tlist, AppendRelInfo *context)
--- 2154,2196 
  }
  
  /*
+  * Rewrite the targetlist entries of an inherited UPDATE/DELETE operation
+  */
+ static void
+ rewrite_inherited_tlist(Query *parse, RangeTblEntry *target_rte)
+ {
+ 	ListCell   *tl;
+ 	List	   *new_tlist = NIL;
+ 	Relation	target_relation;
+ 
+ 	/*
+ 	 * 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 junk tlist entries */
+ 	foreach(tl, parse->targetList)
+ 	{
+ 		TargetEntry *tle = (TargetEntry *) lfirst(tl);
+ 
+ 		if (tle->resjunk)
+ 			continue;			/* don't need junk items */
+ 
+ 		new_tlist = lappend(new_tlist, tle);
+ 	}
+ 
+ 	/* Replace the targetList with the new one */
+ 	parse->targetList = new_tlist;
+ 
+ 	/* And add junk tlist entries needed for UPDATE/DELETE */
+ 	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 

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Ashutosh Bapat]

On Tue, May 16, 2017 at 5:35 PM, Ildus Kurbangaliev
 wrote:

>
> I agree. Maybe this issue should be added to Postgresql Open Items?
> I think there should be some complex solution that fixes not only
> triggers for foreign tables at table partitioning, but covers other
> possible not working cases.
>

I doubt if this is an open item, since DMLs on foreign tables are
supported since 9.3 and support to add foreign tables to inheritance
was added back in 9.5.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Ildus Kurbangaliev]

On Tue, 16 May 2017 15:21:27 +0530
Ashutosh Bapat  wrote:

> On Mon, May 15, 2017 at 7:24 PM, Ildus Kurbangaliev
>  wrote:
> > On Mon, 15 May 2017 17:43:52 +0530
> > Ashutosh Bapat  wrote:
> >  
> >> On Mon, May 15, 2017 at 2:46 PM, Ildus Kurbangaliev
> >>  wrote:  
> >> > On Mon, 15 May 2017 10:34:58 +0530
> >> > Dilip Kumar  wrote:
> >> >  
> >> >> On Sun, May 14, 2017 at 9:54 PM, Dilip Kumar
> >> >>  wrote:  
> >> >> > After your fix, now tupleid is invalid which is expected, but
> >> >> > seems like we need to do something more. As per the comments
> >> >> > seems like it is expected to get the oldtuple from planSlot.
> >> >> > But I don't see any code for handling that part.  
> >> >>
> >> >> Maybe we should do something like attached patch.
> >> >>  
> >> >
> >> > Hi,
> >> > planSlot contains already projected tuple, you can't use it as
> >> > oldtuple. I think problem is that `rewriteTargetListUD` called
> >> > only for parent relation, so there is no `wholerow` attribute for
> >> > foreign tables.  
> >>
> >> Yes. postgresAddForeignUpdateTargets() which is called by
> >> rewriteTargetListUD injects "ctid". "wholerow" is always there. Not
> >> for postgres_fdw but for other wrappers it might be a bad news.
> >> ctid, whole row obtained from the remote postgres server will fit
> >> the tuple descriptor of parent, but for other FDWs the column
> >> injected by rewriteTargetListUD() may make the child tuple look
> >> different from that of the parent, so we may not pass that column
> >> down to the child. 
> >
> > I'm trying to say that when we have a regular table as parent, and
> > foreign table as child, in rewriteTargetListUD `wholerow` won't be
> > added, because rewriteTargetListUD will be called only for parent
> > relation. You can see that by running the script i provided in the
> > first message of this thread.  
> 
> 
> You are right.
> 
> explain verbose update parent set val = random();
>   QUERY PLAN
> ---
>  Update on public.parent  (cost=0.00..258.63 rows=5535 width=10)
>Update on public.parent
>Update on public.child1
>Foreign Update on public.ftable
>  Remote SQL: UPDATE public.child1 SET val = $2 WHERE ctid = $1
>->  Seq Scan on public.parent  (cost=0.00..4.83 rows=255
> width=10) Output: random(), parent.ctid
>->  Seq Scan on public.child1  (cost=0.00..48.25 rows=2550
> width=10) Output: random(), child1.ctid
>->  Foreign Scan on public.ftable  (cost=100.00..205.55 rows=2730
> width=10) Output: random(), ftable.ctid
>  Remote SQL: SELECT ctid FROM public.child1 FOR UPDATE
> (12 rows)
> 
> explain verbose update ftable set val = random();
>   QUERY PLAN
> ---
>  Update on public.ftable  (cost=100.00..159.42 rows=1412 width=38)
>Remote SQL: UPDATE public.child1 SET val = $2 WHERE ctid = $1
>->  Foreign Scan on public.ftable  (cost=100.00..159.42 rows=1412
> width=38) Output: random(), ctid, ftable.*
>  Remote SQL: SELECT val, ctid FROM public.child1 FOR UPDATE
> (5 rows)
> 
> Anyway, the problem I am stating, i.e. we have a bigger problem to fix
> when there are FDWs other than postgres_fdw involved seems to be still
> valid.

I agree. Maybe this issue should be added to Postgresql Open Items? 
I think there should be some complex solution that fixes not only
triggers for foreign tables at table partitioning, but covers other
possible not working cases.

-- 
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Ashutosh Bapat]

On Mon, May 15, 2017 at 7:24 PM, Ildus Kurbangaliev
 wrote:
> On Mon, 15 May 2017 17:43:52 +0530
> Ashutosh Bapat  wrote:
>
>> On Mon, May 15, 2017 at 2:46 PM, Ildus Kurbangaliev
>>  wrote:
>> > On Mon, 15 May 2017 10:34:58 +0530
>> > Dilip Kumar  wrote:
>> >
>> >> On Sun, May 14, 2017 at 9:54 PM, Dilip Kumar
>> >>  wrote:
>> >> > After your fix, now tupleid is invalid which is expected, but
>> >> > seems like we need to do something more. As per the comments
>> >> > seems like it is expected to get the oldtuple from planSlot.
>> >> > But I don't see any code for handling that part.
>> >>
>> >> Maybe we should do something like attached patch.
>> >>
>> >
>> > Hi,
>> > planSlot contains already projected tuple, you can't use it as
>> > oldtuple. I think problem is that `rewriteTargetListUD` called only
>> > for parent relation, so there is no `wholerow` attribute for
>> > foreign tables.
>>
>> Yes. postgresAddForeignUpdateTargets() which is called by
>> rewriteTargetListUD injects "ctid". "wholerow" is always there. Not
>> for postgres_fdw but for other wrappers it might be a bad news. ctid,
>> whole row obtained from the remote postgres server will fit the tuple
>> descriptor of parent, but for other FDWs the column injected by
>> rewriteTargetListUD() may make the child tuple look different from
>> that of the parent, so we may not pass that column down to the child.
>>
>
> I'm trying to say that when we have a regular table as parent, and
> foreign table as child, in rewriteTargetListUD `wholerow` won't be
> added, because rewriteTargetListUD will be called only for parent
> relation. You can see that by running the script i provided in the first
> message of this thread.


You are right.

explain verbose update parent set val = random();
  QUERY PLAN
---
 Update on public.parent  (cost=0.00..258.63 rows=5535 width=10)
   Update on public.parent
   Update on public.child1
   Foreign Update on public.ftable
 Remote SQL: UPDATE public.child1 SET val = $2 WHERE ctid = $1
   ->  Seq Scan on public.parent  (cost=0.00..4.83 rows=255 width=10)
 Output: random(), parent.ctid
   ->  Seq Scan on public.child1  (cost=0.00..48.25 rows=2550 width=10)
 Output: random(), child1.ctid
   ->  Foreign Scan on public.ftable  (cost=100.00..205.55 rows=2730 width=10)
 Output: random(), ftable.ctid
 Remote SQL: SELECT ctid FROM public.child1 FOR UPDATE
(12 rows)

explain verbose update ftable set val = random();
  QUERY PLAN
---
 Update on public.ftable  (cost=100.00..159.42 rows=1412 width=38)
   Remote SQL: UPDATE public.child1 SET val = $2 WHERE ctid = $1
   ->  Foreign Scan on public.ftable  (cost=100.00..159.42 rows=1412 width=38)
 Output: random(), ctid, ftable.*
 Remote SQL: SELECT val, ctid FROM public.child1 FOR UPDATE
(5 rows)

Anyway, the problem I am stating, i.e. we have a bigger problem to fix
when there are FDWs other than postgres_fdw involved seems to be still
valid.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Ildus Kurbangaliev]

On Mon, 15 May 2017 17:43:52 +0530
Ashutosh Bapat  wrote:

> On Mon, May 15, 2017 at 2:46 PM, Ildus Kurbangaliev
>  wrote:
> > On Mon, 15 May 2017 10:34:58 +0530
> > Dilip Kumar  wrote:
> >  
> >> On Sun, May 14, 2017 at 9:54 PM, Dilip Kumar
> >>  wrote:  
> >> > After your fix, now tupleid is invalid which is expected, but
> >> > seems like we need to do something more. As per the comments
> >> > seems like it is expected to get the oldtuple from planSlot.
> >> > But I don't see any code for handling that part.  
> >>
> >> Maybe we should do something like attached patch.
> >>  
> >
> > Hi,
> > planSlot contains already projected tuple, you can't use it as
> > oldtuple. I think problem is that `rewriteTargetListUD` called only
> > for parent relation, so there is no `wholerow` attribute for
> > foreign tables.  
> 
> Yes. postgresAddForeignUpdateTargets() which is called by
> rewriteTargetListUD injects "ctid". "wholerow" is always there. Not
> for postgres_fdw but for other wrappers it might be a bad news. ctid,
> whole row obtained from the remote postgres server will fit the tuple
> descriptor of parent, but for other FDWs the column injected by
> rewriteTargetListUD() may make the child tuple look different from
> that of the parent, so we may not pass that column down to the child.
> 

I'm trying to say that when we have a regular table as parent, and
foreign table as child, in rewriteTargetListUD `wholerow` won't be
added, because rewriteTargetListUD will be called only for parent
relation. You can see that by running the script i provided in the first
message of this thread.

-- 
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Ashutosh Bapat]

On Mon, May 15, 2017 at 6:04 PM, Dilip Kumar  wrote:
> On Mon, May 15, 2017 at 5:43 PM, Ashutosh Bapat
>  wrote:
>> Yes. postgresAddForeignUpdateTargets() which is called by
>> rewriteTargetListUD injects "ctid". "wholerow" is always there. Not
>> for postgres_fdw but for other wrappers it might be a bad news. ctid,
>> whole row obtained from the remote postgres server will fit the tuple
>> descriptor of parent, but for other FDWs the column injected by
>> rewriteTargetListUD() may make the child tuple look different from
>> that of the parent, so we may not pass that column down to the child.
>
> But, can't we call rewriteTargetListUD for all the inheritors if the
> inheritor is a foreign table which will intern call the
> postgresAddForeignUpdateTargets?
>
Yes. But it's not necessary to have all children use same FDW.


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Dilip Kumar]

On Mon, May 15, 2017 at 5:43 PM, Ashutosh Bapat
 wrote:
> Yes. postgresAddForeignUpdateTargets() which is called by
> rewriteTargetListUD injects "ctid". "wholerow" is always there. Not
> for postgres_fdw but for other wrappers it might be a bad news. ctid,
> whole row obtained from the remote postgres server will fit the tuple
> descriptor of parent, but for other FDWs the column injected by
> rewriteTargetListUD() may make the child tuple look different from
> that of the parent, so we may not pass that column down to the child.

But, can't we call rewriteTargetListUD for all the inheritors if the
inheritor is a foreign table which will intern call the
postgresAddForeignUpdateTargets?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Ashutosh Bapat]

On Mon, May 15, 2017 at 2:46 PM, Ildus Kurbangaliev
 wrote:
> On Mon, 15 May 2017 10:34:58 +0530
> Dilip Kumar  wrote:
>
>> On Sun, May 14, 2017 at 9:54 PM, Dilip Kumar 
>> wrote:
>> > After your fix, now tupleid is invalid which is expected, but seems
>> > like we need to do something more. As per the comments seems like it
>> > is expected to get the oldtuple from planSlot.  But I don't see any
>> > code for handling that part.
>>
>> Maybe we should do something like attached patch.
>>
>
> Hi,
> planSlot contains already projected tuple, you can't use it as oldtuple.
> I think problem is that `rewriteTargetListUD` called only for parent
> relation, so there is no `wholerow` attribute for foreign tables.

Yes. postgresAddForeignUpdateTargets() which is called by
rewriteTargetListUD injects "ctid". "wholerow" is always there. Not
for postgres_fdw but for other wrappers it might be a bad news. ctid,
whole row obtained from the remote postgres server will fit the tuple
descriptor of parent, but for other FDWs the column injected by
rewriteTargetListUD() may make the child tuple look different from
that of the parent, so we may not pass that column down to the child.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Dilip Kumar]

On Mon, May 15, 2017 at 2:46 PM, Ildus Kurbangaliev
 wrote:
> Hi,
> planSlot contains already projected tuple, you can't use it as oldtuple.
> I think problem is that `rewriteTargetListUD` called only for parent
> relation, so there is no `wholerow` attribute for foreign tables.

Oh, right!

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Ildus Kurbangaliev]

On Mon, 15 May 2017 10:34:58 +0530
Dilip Kumar  wrote:

> On Sun, May 14, 2017 at 9:54 PM, Dilip Kumar 
> wrote:
> > After your fix, now tupleid is invalid which is expected, but seems
> > like we need to do something more. As per the comments seems like it
> > is expected to get the oldtuple from planSlot.  But I don't see any
> > code for handling that part.  
> 
> Maybe we should do something like attached patch.
> 

Hi,
planSlot contains already projected tuple, you can't use it as oldtuple.
I think problem is that `rewriteTargetListUD` called only for parent
relation, so there is no `wholerow` attribute for foreign tables.

-- 
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Michael Paquier]

On Mon, May 15, 2017 at 2:04 PM, Dilip Kumar  wrote:
> On Sun, May 14, 2017 at 9:54 PM, Dilip Kumar  wrote:
>> After your fix, now tupleid is invalid which is expected, but seems
>> like we need to do something more. As per the comments seems like it
>> is expected to get the oldtuple from planSlot.  But I don't see any
>> code for handling that part.
>
> Maybe we should do something like attached patch.

As a deficiency, shouldn't this try as well to improve regression test
coverage for FDW triggers with inheritance trees? Those tests are in
postgres_fdw. You may find other issues on the way..
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Dilip Kumar]

On Sun, May 14, 2017 at 9:54 PM, Dilip Kumar  wrote:
> After your fix, now tupleid is invalid which is expected, but seems
> like we need to do something more. As per the comments seems like it
> is expected to get the oldtuple from planSlot.  But I don't see any
> code for handling that part.

Maybe we should do something like attached patch.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


fix_fdw_trigger.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Dilip Kumar]

On Sun, May 14, 2017 at 5:35 PM, Ildus Kurbangaliev
 wrote:
> TRAP: FailedAssertion("!(((const void*)(fdw_trigtuple) != ((void *)0))
> ^ ((bool) (((const void*)(tupleid) != ((void *)0)) &&
> ((tupleid)->ip_posid != 0", File: "trigger.c", Line: 2428)
>
> I'm not sure how it should be fixed, because as I see `oldtuple` will
> be set only for AFTER ROW triggers by `wholerow` junk attribute.

There is comment on the ExecUpdate function

 *  When updating a foreign table,
 * tupleid is invalid; the FDW has to figure out which row to
 * update using data from the planSlot.  oldtuple is passed to
 * foreign table triggers; it is NULL when the foreign table has
 * no relevant triggers.

After your fix, now tupleid is invalid which is expected, but seems
like we need to do something more. As per the comments seems like it
is expected to get the oldtuple from planSlot.  But I don't see any
code for handling that part.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers