Thread

1. Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

   Marko Tiikkaja <marko@joh.to> — 2016-07-01T00:12:56Z

   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
   

2. Re: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

   Marko Tiikkaja <marko@joh.to> — 2017-08-13T20:48:56Z

   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
   

3. Re: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

   Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-09-05T08:44:44Z

   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.
   >
   
   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.
   
   
   Regards,
   Hari Babu
   Fujitsu Australia
   

4. Re: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

   Daniel Gustafsson <daniel@yesql.se> — 2017-09-15T14:07:02Z

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

5. Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

   Marko Tiikkaja <marko@joh.to> — 2023-10-19T11:03:42Z

   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
   

6. Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

   Shlok Kyal <shlok.kyal.oss@gmail.com> — 2023-11-02T10:24:19Z

   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
   
   
   
   

7. Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

   Marko Tiikkaja <marko@joh.to> — 2023-11-02T16:33:50Z

   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
   

8. Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

   jian he <jian.universality@gmail.com> — 2023-11-16T00:00:00Z

   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.
   
   
   
   

9. Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

   vignesh C <vignesh21@gmail.com> — 2024-01-09T10:21:33Z

   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
   
   
   
   

10. Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

    jian he <jian.universality@gmail.com> — 2024-01-16T06:50:00Z

    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.
    

11. Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

    Aleksander Alekseev <aleksander@timescale.com> — 2024-03-15T14:27:18Z

    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)
    
    
    
    

12. Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

    Rahila Syed <rahilasyed90@gmail.com> — 2024-09-19T08:41:20Z

    Hi,
    
    > 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.
    >
    I am trying to understand the basic idea behind the change.  What is the
    use-case for allowing the OLD tuple to be modified in the INSTEAD of DELETE
    triggers.
    AFAIU no matter what we return from the trigger OLD/NEW or NULL, the delete
    operation is skipped, so what is the point of modifying the OLD row in your
    test
    for example.
    
    If the requirement is to show the recent row in the returning clause, is
    fetching the
    row from heap and copying into the returning slot not enough?
    
    In the tests shown as follows, inspite of the output showing 'DELETE 1',
    no row is actually deleted from the table or the view. Probably this
    is an existing behavior but I think this is misleading.
    
    ```
    DELETE FROM test_ins_del_view where a = 1 returning a;
    NOTICE:  old.a 4
     a
    ---
     4
    (1 row)
    
    DELETE 1
    ```
    
    FWIW, it might be good to include in tests the example
    to demonstrate the problem regarding the concurrent transactions,
    one of which is modifying the row and the other returning it
    after attempting DELETE.
    
    
    
    Thank you,
    Rahila Syed
    
    
    
    
    On Fri, Mar 15, 2024 at 7:57 PM Aleksander Alekseev <
    aleksander@timescale.com> wrote:
    
    > 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)
    >
    >
    >