Re: Bug in ExecModifyTable function and trigger issues for foreign 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