Обсуждение: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

Поиск
Список
Период
Сортировка

Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

От
Marko Tiikkaja
Дата:
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.

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

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
Вложения

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

От
jian he
Дата:
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.

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

От
vignesh C
Дата:
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

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

От
jian he
Дата:

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.

Вложения

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)

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

От
Rahila Syed
Дата:
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)