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

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

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