> On 05 Sep 2017, at 10:44, Haribabu Kommi <kommi.harib...@gmail.com> wrote: > > On Mon, Aug 14, 2017 at 6:48 AM, Marko Tiikkaja <ma...@joh.to > <mailto:ma...@joh.to>> wrote: > On Fri, Jul 1, 2016 at 2:12 AM, I wrote: > Currently the tuple returned by INSTEAD OF triggers on DELETEs is only used > to determine whether to pretend that the DELETE happened or not, which is > often not helpful enough; for example, the actual tuple might have been > concurrently UPDATEd by another transaction and one or more of the columns > now hold values different from those in the planSlot tuple. Attached is an > example case which is impossible to properly implement under the current > behavior. For what it's worth, the current behavior seems to be an accident; > before INSTEAD OF triggers either the tuple was already locked (in case of > BEFORE triggers) or the actual pre-DELETE version of the tuple was fetched > from the heap. > > So I'm suggesting to change this behavior and allow INSTEAD OF DELETE > triggers to modify the OLD tuple and use that for RETURNING instead of > returning the tuple in planSlot. Attached is a WIP patch implementing that. > > Is there any reason why we wouldn't want to change the current behavior? > > Since nobody seems to have came up with a reason, here's a patch for that > with test cases and some documentation changes. I'll also be adding this to > the open commit fest, as is customary. > > Thanks for the patch. This patch improves the DELETE returning > clause with the actual row. > > Compilation and tests are passed. I have some review comments. > > ! that was provided. Likewise, for <command>DELETE</> operations the > ! <varname>OLD</> variable can be modified before returning it, and > ! the changes will be reflected in the output data. > > The above explanation is not very clear, how about the following? > > Likewise, for <command>DELETE</> operations the trigger may > modify the <varname>OLD</> row before returning it, and the > change will be reflected in the output data of <command>DELETE RETURNING</>. > > > ! TupleTableSlot * > ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo, > ! HeapTuple trigtuple, TupleTableSlot > *slot) > > ! oldtuple = ExecMaterializeSlot(slot); --nodeModifyTable.c > > The trigtuple is part of the slot anyway, I feel there is no need to pass > the trigtuple seperately. The tuple can be formed internaly by Materializing > slot. > > Or > > Don't materialize the slot before the ExecIRDeleteTriggers function > call. > > ! /* > ! * Return the modified tuple using the es_trig_tuple_slot. We > assume > ! * the tuple was allocated in per-tuple memory context, and > therefore > ! * will go away by itself. The tuple table slot should not try > to > ! * clear it. > ! */ > ! TupleTableSlot *newslot = estate->es_trig_tuple_slot; > > The input slot that is passed to the function ExecIRDeleteTriggers > is same as estate->es_trig_tuple_slot. And also the tuple descriptor > is same. Instead of using the newslot, directly use the slot is fine. > > > + /* trigger might have changed tuple */ > + oldtuple = ExecMaterializeSlot(slot); > > > + if (resultRelInfo->ri_TrigDesc && > + resultRelInfo->ri_TrigDesc->trig_delete_instead_row) > + return ExecProcessReturning(resultRelInfo, slot, > planSlot); > > > Views cannot have before/after triggers, Once the call enters into > Instead of triggers flow, the oldtuple is used to frame the slot, if the > returning clause is present. But in case of instead of triggers, the call > is returned early as above and the framed old tuple is not used. > > Either change the logic of returning for instead of triggers, or remove > the generation of oldtuple after instead triggers call execution.
Have you had a chance to work on this patch to address the above review? cheers ./daniel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
