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

On Tue, Jan 9, 2024 at 6:21 PM vignesh C <vignesh21(at)gmail(dot)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.