On Tue, Jan 9, 2024 at 6:21 PM vignesh C 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.
From 5f41738b3c7dc7bf3d849539d16a568b52cedc55 Mon Sep 17 00:00:00 2001
From: jian he
Date: Mon, 15 Jan 2024 16:20:28 +0800
Subject: [PATCH v5 1/1] allow INSTEAD OF DELETE triggers to modify the OLD
tuple and use that for RETURNING instead of returning the tuple in planSlot.
Currently the tuple returned by INSTEAD OF triggers on DELETEs is only
used to determine whether to pretend that the DELETE happened or not.
now we can modified the old value returned by INSTEAD OF DELETE trigger.
---
doc/src/sgml/trigger.sgml | 7 --
src/backend/commands/trigger.c | 34 ++
src/backend/executor/nodeModifyTable.c | 14 +--
src/include/commands/trigger.h | 2 +-
src/test/regress/expected/triggers.out | 27
src/test/regress/sql/triggers.sql | 20 +++
6 files changed, 89 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index a5390ff6..9813e323 100644
--- a/doc/src/sgml/trigger.sgml
+++ b/doc/src/sgml/trigger.sgml
@@ -261,9 +261,12 @@
modifications in the view. This will cause the count of the number
of rows affected by the command to be incremented. For
INSERT and UPDATE operations only, the trigger
-may modify the NEW row before returning it. This will
+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 or UPDATE RETURNING,
+INSERT RETURNING, UPDATE RETURNING, DELETE RETURNING
and is useful when the view will not show exactly the same data
that was provided.
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 0880ca51..d8e95286 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2815,10 +2815,11 @@ ExecARDeleteTriggers(EState *estate,
bool
ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
- HeapTuple trigtuple)
+ TupleTableSlot *slot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
- TupleTableSlot *slot = ExecGetTriggerOldSlot(estate, relinfo);
+ HeapTuple newtuple = NULL;
+ bool should_free;
TriggerData LocTriggerData = {0};
int i;
@@ -2828,12 +2829,10 @@ ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
TRIGGER_EVENT_INSTEAD;
