On Tue, Jan 9, 2024 at 6:21 PM vignesh C <vignes...@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.
From 5f41738b3c7dc7bf3d849539d16a568b52cedc55 Mon Sep 17 00:00:00 2001
From: jian he <jian.universal...@gmail.com>
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
<command>INSERT</command> and <command>UPDATE</command> operations only, the trigger
- may modify the <varname>NEW</varname> row before returning it. This will
+ may modify the <varname>NEW</varname> row before returning it.
+ For <command>DELETE</command> operations, the trigger
+ may modify the <varname>OLD</varname> row before returning it.
+ This will
change the data returned by
- <command>INSERT RETURNING</command> or <command>UPDATE RETURNING</command>,
+ <command>INSERT RETURNING</command>, <command>UPDATE RETURNING</command>, <command>DELETE RETURNING</command>
and is useful when the view will not show exactly the same data
that was provided.
</para>
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;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
- ExecForceStoreHeapTuple(trigtuple, slot, false);
-
for (i = 0; i < trigdesc->numtriggers; i++)
{
- HeapTuple rettuple;
Trigger *trigger = &trigdesc->triggers[i];
+ HeapTuple oldtuple;
if (!TRIGGER_TYPE_MATCHES(trigger->tgtype,
TRIGGER_TYPE_ROW,
@@ -2844,18 +2843,33 @@ ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
NULL, slot, NULL))
continue;
+ if (!newtuple)
+ newtuple = ExecFetchSlotHeapTuple(slot, true, &should_free);
+
LocTriggerData.tg_trigslot = slot;
- LocTriggerData.tg_trigtuple = trigtuple;
+ LocTriggerData.tg_trigtuple = oldtuple = newtuple;
LocTriggerData.tg_trigger = trigger;
- rettuple = ExecCallTriggerFunc(&LocTriggerData,
+ newtuple = ExecCallTriggerFunc(&LocTriggerData,
i,
relinfo->ri_TrigFunctions,
relinfo->ri_TrigInstrument,
GetPerTupleMemoryContext(estate));
- if (rettuple == NULL)
+ if (newtuple == NULL)
+ {
+ if (should_free)
+ heap_freetuple(oldtuple);
return false; /* Delete was suppressed */
- if (rettuple != trigtuple)
- heap_freetuple(rettuple);
+ }
+ else if (newtuple != oldtuple)
+ {
+ ExecForceStoreHeapTuple(newtuple, slot, false);
+
+ if (should_free)
+ heap_freetuple(oldtuple);
+ heap_freetuple(newtuple);
+ /* signal tuple should be re-fetched if used */
+ newtuple = NULL;
+ }
}
return true;
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 9fc5abff..103553b2 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1452,7 +1452,11 @@ ExecDelete(ModifyTableContext *context,
bool dodelete;
Assert(oldtuple != NULL);
- dodelete = ExecIRDeleteTriggers(estate, resultRelInfo, oldtuple);
+
+ slot = ExecGetReturningSlot(estate, resultRelInfo);
+ ExecForceStoreHeapTuple(oldtuple, slot, false);
+
+ dodelete = ExecIRDeleteTriggers(estate, resultRelInfo, slot);
if (!dodelete) /* "do nothing" */
return NULL;
@@ -1676,7 +1680,13 @@ ldelete:
*/
TupleTableSlot *rslot;
- if (resultRelInfo->ri_FdwRoutine)
+ if (resultRelInfo->ri_TrigDesc &&
+ resultRelInfo->ri_TrigDesc->trig_delete_instead_row)
+ {
+ /* INSTEAD OF trigger handling should have provided a slot */
+ Assert(!TupIsNull(slot));
+ }
+ else if (resultRelInfo->ri_FdwRoutine)
{
/* FDW must have provided a slot containing the deleted row */
Assert(!TupIsNull(slot));
diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 8a5a9fe6..e853da57 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -222,7 +222,7 @@ extern void ExecARDeleteTriggers(EState *estate,
bool is_crosspart_update);
extern bool ExecIRDeleteTriggers(EState *estate,
ResultRelInfo *relinfo,
- HeapTuple trigtuple);
+ TupleTableSlot *slot);
extern void ExecBSUpdateTriggers(EState *estate,
ResultRelInfo *relinfo);
extern void ExecASUpdateTriggers(EState *estate,
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 78e90309..df557f5b 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -1385,6 +1385,22 @@ end;
$$;
CREATE TRIGGER city_update_trig INSTEAD OF UPDATE ON city_view
FOR EACH ROW EXECUTE PROCEDURE city_update();
+-- DELETE .. RETURNING setup.
+CREATE TABLE test_ins_del(a int, b text);
+INSERT INTO test_ins_del VALUES (1,repeat('x', 1000));
+CREATE VIEW test_ins_del_view AS SELECT a as a,b as b from test_ins_del;
+CREATE OR REPLACE FUNCTION view_ins_del_trig_trig() RETURNS trigger LANGUAGE plpgsql AS $$
+BEGIN
+ if (TG_OP = 'DELETE') then
+ OLD.a := OLD.a + 3;
+ raise notice 'old.a %',old.a;
+ RETURN OLD;
+ end if;
+end
+$$;
+CREATE OR REPLACE TRIGGER tv_delete_trig
+INSTEAD OF DELETE ON test_ins_del_view
+FOR EACH ROW EXECUTE PROCEDURE view_ins_del_trig_trig();
\set QUIET false
-- INSERT .. RETURNING
INSERT INTO city_view(city_name) VALUES('Tokyo') RETURNING *;
@@ -1478,8 +1494,19 @@ DELETE FROM city_view WHERE city_name = 'Birmingham' RETURNING *;
234567 | Birmingham | 1016800 | UK | Europe
(1 row)
+DELETE 1
+DELETE FROM test_ins_del_view where a = 1 returning a;
+NOTICE: old.a 4
+ a
+---
+ 4
+(1 row)
+
DELETE 1
\set QUIET true
+DROP TABLE test_ins_del cascade;
+NOTICE: drop cascades to view test_ins_del_view
+DROP FUNCTION view_ins_del_trig_trig;
-- read-only view with WHERE clause
CREATE VIEW european_city_view AS
SELECT * FROM city_view WHERE continent = 'Europe';
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 46795a9c..e2dd702d 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -960,6 +960,23 @@ $$;
CREATE TRIGGER city_update_trig INSTEAD OF UPDATE ON city_view
FOR EACH ROW EXECUTE PROCEDURE city_update();
+-- DELETE .. RETURNING setup.
+CREATE TABLE test_ins_del(a int, b text);
+INSERT INTO test_ins_del VALUES (1,repeat('x', 1000));
+CREATE VIEW test_ins_del_view AS SELECT a as a,b as b from test_ins_del;
+CREATE OR REPLACE FUNCTION view_ins_del_trig_trig() RETURNS trigger LANGUAGE plpgsql AS $$
+BEGIN
+ if (TG_OP = 'DELETE') then
+ OLD.a := OLD.a + 3;
+ raise notice 'old.a %',old.a;
+ RETURN OLD;
+ end if;
+end
+$$;
+
+CREATE OR REPLACE TRIGGER tv_delete_trig
+INSTEAD OF DELETE ON test_ins_del_view
+FOR EACH ROW EXECUTE PROCEDURE view_ins_del_trig_trig();
\set QUIET false
-- INSERT .. RETURNING
@@ -983,9 +1000,12 @@ UPDATE city_view v1 SET country_name = v2.country_name FROM city_view v2
-- DELETE .. RETURNING
DELETE FROM city_view WHERE city_name = 'Birmingham' RETURNING *;
+DELETE FROM test_ins_del_view where a = 1 returning a;
\set QUIET true
+DROP TABLE test_ins_del cascade;
+DROP FUNCTION view_ins_del_trig_trig;
-- read-only view with WHERE clause
CREATE VIEW european_city_view AS
SELECT * FROM city_view WHERE continent = 'Europe';
--
2.34.1
