Обсуждение: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
Hi, 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? .m
Вложения
Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
От
Marko Tiikkaja
Дата:
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.
.m
Вложения
Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuplefor RETURNING
От
Haribabu Kommi
Дата:
On Mon, Aug 14, 2017 at 6:48 AM, Marko Tiikkaja <marko@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.
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.
Regards,
Hari Babu
Fujitsu Australia
Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
От
Daniel Gustafsson
Дата:
> On 05 Sep 2017, at 10:44, Haribabu Kommi <kommi.haribabu@gmail.com> wrote: > > On Mon, Aug 14, 2017 at 6:48 AM, Marko Tiikkaja <marko@joh.to <mailto:marko@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 DELETEhappened or not, which is often not helpful enough; for example, the actual tuple might have been concurrently UPDATEdby 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 caseof 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 forRETURNING 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
Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
От
Marko Tiikkaja
Дата:
Hi, Thank you for the feedback. Apparently it took me six years, but I've attached the latest version of the patch which I believe addresses all issues. I'll add it to the open commitfest. .m
Вложения
Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
От
Shlok Kyal
Дата:
Hi, On Thu, 19 Oct 2023 at 16:35, Marko Tiikkaja <marko@joh.to> wrote: > > Hi, > > Thank you for the feedback. > > Apparently it took me six years, but I've attached the latest version > of the patch which I believe addresses all issues. I'll add it to the > open commitfest. > > > .m I went through the CFbot and found that docs build run is giving some error (link: https://cirrus-ci.com/task/5712582359646208): [07:37:19.769] trigger.sgml:266: parser error : Opening and ending tag mismatch: command line 266 and COMMAND [07:37:19.769] <command>DELETE</command> operations, <command>INSTEAD OF</COMMAND> [07:37:19.769] ^ [07:37:19.769] trigger.sgml:408: parser error : Opening and ending tag mismatch: para line 266 and sect1 [07:37:19.769] </sect1> [07:37:19.769] ^ [07:37:19.769] trigger.sgml:1034: parser error : Opening and ending tag mismatch: sect1 line 266 and chapter [07:37:19.769] </chapter> [07:37:19.769] ^ [07:37:19.769] trigger.sgml:1035: parser error : chunk is not well balanced [07:37:19.769] [07:37:19.769] ^ [07:37:19.769] postgres.sgml:222: parser error : Failure to process entity trigger [07:37:19.769] &trigger; [07:37:19.769] ^ [07:37:19.769] postgres.sgml:222: parser error : Entity 'trigger' not defined [07:37:19.769] &trigger; [07:37:19.769] ^ [07:37:19.956] make[2]: *** [Makefile:73: postgres-full.xml] Error 1 [07:37:19.957] make[1]: *** [Makefile:8: all] Error 2 [07:37:19.957] make: *** [Makefile:16: all] Error 2 [07:37:19.957] [07:37:19.957] real 0m0.529s [07:37:19.957] user 0m0.493s [07:37:19.957] sys 0m0.053s [07:37:19.957] [07:37:19.957] Exit status: 2 Just wanted to make sure you are aware of the issue. Thanks Shlok Kumar Kyal
Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
От
Marko Tiikkaja
Дата:
On Thu, Nov 2, 2023 at 12:24 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote: > I went through the CFbot and found that docs build run is giving some > error (link: https://cirrus-ci.com/task/5712582359646208): > > Just wanted to make sure you are aware of the issue. I am now. Thanks! :-) Will try to keep an eye on the builds in the future. Attached v4 of the patch which should fix the issue. .m
Вложения
On Fri, Nov 3, 2023 at 12:34 AM Marko Tiikkaja <marko@joh.to> wrote: > > I am now. Thanks! :-) Will try to keep an eye on the builds in the future. > > Attached v4 of the patch which should fix the issue. > doc seems to still have an issue. https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F45%2F4617 In the regress test, do we need to clean up the created object after we use it. tested passed, looking at ExecIRInsertTriggers, your changes look sane.
On Thu, 16 Nov 2023 at 05:30, jian he <jian.universality@gmail.com> wrote: > > On Fri, Nov 3, 2023 at 12:34 AM Marko Tiikkaja <marko@joh.to> wrote: > > > > I am now. Thanks! :-) Will try to keep an eye on the builds in the future. > > > > Attached v4 of the patch which should fix the issue. > > > > doc seems to still have an issue. > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F45%2F4617 > > In the regress test, do we need to clean up the created object after we use it. > tested passed, looking at ExecIRInsertTriggers, your changes look sane. I have changed the status of the patch to "Waiting on Author" as the CFBot reported by jina he is not yet handled. Regards, Vignesh
On Tue, Jan 9, 2024 at 6:21 PM vignesh C <vignesh21@gmail.com> wrote:
>
> > doc seems to still have an issue.
> > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F45%2F4617
> >
> > In the regress test, do we need to clean up the created object after we use it.
> > tested passed, looking at ExecIRInsertTriggers, your changes look sane.
>
> I have changed the status of the patch to "Waiting on Author" as the
> CFBot reported by jina he is not yet handled.
Hi
it took me a while to figure out why the doc build fails.
Currently your wording is:
For INSERT, UPDATE, and DELETE operations, INSTEAD OF triggers can modify the data returned by RETURNING. In the case of INSERT and UPDATE, triggers can modify the NEW row before returning it, while for DELETE, triggers can modify the OLD row before returning it. This feature is useful when the returned data needs to be adjusted to match the view or other requirements.
The doc is:
For INSERT and UPDATE operations only, the trigger may modify the NEW row before returning it. This will change the data returned by INSERT RETURNING or UPDATE RETURNING, and is useful when the view will not show exactly the same data that was provided.
to make it a minimum change compared to doc, i think the following make sense:
For INSERT and UPDATE operations only, the trigger may modify the NEW row before returning it. For DELETE operations, the trigger may modify the OLD row before returning it.
This will change the data returned by INSERT RETURNING, UPDATE RETURNING, DELETE RETURNING and is useful when the view will not show exactly the same data that was provided.
I am not sure the following changes in the function ExecIRDeleteTriggers is right.
+ else if (newtuple != oldtuple)
+ {
+ ExecForceStoreHeapTuple(newtuple, slot, false);
+
+ if (should_free)
+ heap_freetuple(oldtuple);
+
+ /* signal tuple should be re-fetched if used */
+ newtuple = NULL;
In the ExecIRDeleteTriggers function,
all we want is to return the slot,
so that, nodeModifyTable.c `if (processReturning && resultRelInfo->ri_projectReturning) {}` can further process it, materialize it.
if newtuple != oldtuple that means DELETE INSTEAD OF changed the value.
ExecForceStoreHeapTuple already put the new values into the slot, we should just free the newtuple, since we don't use it anymore?
Also maybe we don't need the variable should_free, if (newtuple != oldtuple) then we should free oldtuple and newtuple, because the content is already in the slot.
Anyway, based on your patch, I modified it, also added a slightly more complicated test.
so that, nodeModifyTable.c `if (processReturning && resultRelInfo->ri_projectReturning) {}` can further process it, materialize it.
if newtuple != oldtuple that means DELETE INSTEAD OF changed the value.
ExecForceStoreHeapTuple already put the new values into the slot, we should just free the newtuple, since we don't use it anymore?
Also maybe we don't need the variable should_free, if (newtuple != oldtuple) then we should free oldtuple and newtuple, because the content is already in the slot.
Anyway, based on your patch, I modified it, also added a slightly more complicated test.
Вложения
Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
От
Aleksander Alekseev
Дата:
Hi, > it took me a while to figure out why the doc build fails. > > [...] > > Anyway, based on your patch, I modified it, also added a slightly more complicated test. Thank you for keeping the patch up-to-date. Unfortunately, it seems to be needing another rebase, according to cfbot. Best regards, Aleksander Alekseev (wearing co-CFM hat)