Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

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

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

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.

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)

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: