public inbox for [email protected]
help / color / mirror / Atom feedAllow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
7+ messages / 4 participants
[nested] [flat]
* Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
@ 2016-07-01 00:12 Marko Tiikkaja <[email protected]>
2017-08-13 20:48 ` Re: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING Marko Tiikkaja <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Marko Tiikkaja @ 2016-07-01 00:12 UTC (permalink / raw)
To: pgsql-hackers
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
BEGIN;
CREATE OR REPLACE FUNCTION foof() RETURNS TRIGGER AS $$
BEGIN
-- imagine someone concurrently incremented counter here
OLD.counter := OLD.counter + 1;
RETURN OLD;
END
$$ LANGUAGE plpgsql;
CREATE TABLE foo(counter int NOT NULL);
CREATE VIEW foov AS SELECT * FROM foo;
CREATE TRIGGER foov_instead
INSTEAD OF DELETE ON foov
FOR EACH ROW
EXECUTE PROCEDURE foof();
INSERT INTO foo VALUES (0);
DELETE FROM foov RETURNING counter;
ROLLBACK;
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
***************
*** 2295,2307 **** ExecARDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
}
}
! bool
ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
! HeapTuple trigtuple)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
TriggerData LocTriggerData;
! HeapTuple rettuple;
int i;
LocTriggerData.type = T_TriggerData;
--- 2295,2307 ----
}
}
! TupleTableSlot *
ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
! HeapTuple trigtuple, TupleTableSlot *slot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
TriggerData LocTriggerData;
! HeapTuple rettuple = trigtuple;
int i;
LocTriggerData.type = T_TriggerData;
***************
*** 2333,2343 **** ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
relinfo->ri_TrigInstrument,
GetPerTupleMemoryContext(estate));
if (rettuple == NULL)
! return false; /* Delete was suppressed */
! if (rettuple != trigtuple)
! heap_freetuple(rettuple);
}
! return true;
}
void
--- 2333,2359 ----
relinfo->ri_TrigInstrument,
GetPerTupleMemoryContext(estate));
if (rettuple == NULL)
! return NULL; /* Delete was suppressed */
}
!
! if (rettuple != trigtuple)
! {
! /*
! * 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;
! TupleDesc tupdesc = RelationGetDescr(relinfo->ri_RelationDesc);
!
! if (newslot->tts_tupleDescriptor != tupdesc)
! ExecSetSlotDescriptor(newslot, tupdesc);
! ExecStoreTuple(rettuple, newslot, InvalidBuffer, false);
! slot = newslot;
! }
!
! return slot;
}
void
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***************
*** 573,585 **** ExecDelete(ItemPointer tupleid,
if (resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_delete_instead_row)
{
! bool dodelete;
! Assert(oldtuple != NULL);
! dodelete = ExecIRDeleteTriggers(estate, resultRelInfo, oldtuple);
! if (!dodelete) /* "do nothing" */
return NULL;
}
else if (resultRelInfo->ri_FdwRoutine)
{
--- 573,595 ----
if (resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_delete_instead_row)
{
! /*
! * Store the heap tuple into the tuple table slot, making sure we have a
! * writable copy. We can use the trigger tuple slot.
! */
! slot = estate->es_trig_tuple_slot;
! if (slot->tts_tupleDescriptor != RelationGetDescr(resultRelationDesc))
! ExecSetSlotDescriptor(slot, RelationGetDescr(resultRelationDesc));
! ExecStoreTuple(oldtuple, slot, InvalidBuffer, false);
! oldtuple = ExecMaterializeSlot(slot);
! slot = ExecIRDeleteTriggers(estate, resultRelInfo, oldtuple, slot);
! if (slot == NULL) /* "do nothing" */
return NULL;
+
+ /* trigger might have changed tuple */
+ oldtuple = ExecMaterializeSlot(slot);
}
else if (resultRelInfo->ri_FdwRoutine)
{
***************
*** 719,732 **** ldelete:;
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
{
- /*
- * We have to put the target tuple into a slot, which means first we
- * gotta fetch it. We can use the trigger tuple slot.
- */
TupleTableSlot *rslot;
HeapTupleData deltuple;
Buffer delbuffer;
if (resultRelInfo->ri_FdwRoutine)
{
/* FDW must have provided a slot containing the deleted row */
--- 729,750 ----
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
{
TupleTableSlot *rslot;
HeapTupleData deltuple;
Buffer delbuffer;
+ /*
+ * If we fired an INSTEAD OF trigger, we should use the tuple returned
+ * from said trigger for the RETURNING projections.
+ */
+ if (resultRelInfo->ri_TrigDesc &&
+ resultRelInfo->ri_TrigDesc->trig_delete_instead_row)
+ return ExecProcessReturning(resultRelInfo, slot, planSlot);
+
+ /*
+ * Otherwise we have to to fetch the target tuple into a slot. We can
+ * use the trigger tuple slot here as well.
+ */
if (resultRelInfo->ri_FdwRoutine)
{
/* FDW must have provided a slot containing the deleted row */
*** a/src/include/commands/trigger.h
--- b/src/include/commands/trigger.h
***************
*** 154,162 **** extern void ExecARDeleteTriggers(EState *estate,
ResultRelInfo *relinfo,
ItemPointer tupleid,
HeapTuple fdw_trigtuple);
! extern bool ExecIRDeleteTriggers(EState *estate,
ResultRelInfo *relinfo,
! HeapTuple trigtuple);
extern void ExecBSUpdateTriggers(EState *estate,
ResultRelInfo *relinfo);
extern void ExecASUpdateTriggers(EState *estate,
--- 154,163 ----
ResultRelInfo *relinfo,
ItemPointer tupleid,
HeapTuple fdw_trigtuple);
! extern TupleTableSlot *ExecIRDeleteTriggers(EState *estate,
ResultRelInfo *relinfo,
! HeapTuple trigtuple,
! TupleTableSlot *slot);
extern void ExecBSUpdateTriggers(EState *estate,
ResultRelInfo *relinfo);
extern void ExecASUpdateTriggers(EState *estate,
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
[text/plain] instead_delete.sql (448B, ../../[email protected]/2-instead_delete.sql)
download | inline:
BEGIN;
CREATE OR REPLACE FUNCTION foof() RETURNS TRIGGER AS $$
BEGIN
-- imagine someone concurrently incremented counter here
OLD.counter := OLD.counter + 1;
RETURN OLD;
END
$$ LANGUAGE plpgsql;
CREATE TABLE foo(counter int NOT NULL);
CREATE VIEW foov AS SELECT * FROM foo;
CREATE TRIGGER foov_instead
INSTEAD OF DELETE ON foov
FOR EACH ROW
EXECUTE PROCEDURE foof();
INSERT INTO foo VALUES (0);
DELETE FROM foov RETURNING counter;
ROLLBACK;
[text/plain] instead_delete_returning_v0.patch (5.3K, ../../[email protected]/3-instead_delete_returning_v0.patch)
download | inline diff:
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
***************
*** 2295,2307 **** ExecARDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
}
}
! bool
ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
! HeapTuple trigtuple)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
TriggerData LocTriggerData;
! HeapTuple rettuple;
int i;
LocTriggerData.type = T_TriggerData;
--- 2295,2307 ----
}
}
! TupleTableSlot *
ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
! HeapTuple trigtuple, TupleTableSlot *slot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
TriggerData LocTriggerData;
! HeapTuple rettuple = trigtuple;
int i;
LocTriggerData.type = T_TriggerData;
***************
*** 2333,2343 **** ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
relinfo->ri_TrigInstrument,
GetPerTupleMemoryContext(estate));
if (rettuple == NULL)
! return false; /* Delete was suppressed */
! if (rettuple != trigtuple)
! heap_freetuple(rettuple);
}
! return true;
}
void
--- 2333,2359 ----
relinfo->ri_TrigInstrument,
GetPerTupleMemoryContext(estate));
if (rettuple == NULL)
! return NULL; /* Delete was suppressed */
}
!
! if (rettuple != trigtuple)
! {
! /*
! * 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;
! TupleDesc tupdesc = RelationGetDescr(relinfo->ri_RelationDesc);
!
! if (newslot->tts_tupleDescriptor != tupdesc)
! ExecSetSlotDescriptor(newslot, tupdesc);
! ExecStoreTuple(rettuple, newslot, InvalidBuffer, false);
! slot = newslot;
! }
!
! return slot;
}
void
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***************
*** 573,585 **** ExecDelete(ItemPointer tupleid,
if (resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_delete_instead_row)
{
! bool dodelete;
! Assert(oldtuple != NULL);
! dodelete = ExecIRDeleteTriggers(estate, resultRelInfo, oldtuple);
! if (!dodelete) /* "do nothing" */
return NULL;
}
else if (resultRelInfo->ri_FdwRoutine)
{
--- 573,595 ----
if (resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_delete_instead_row)
{
! /*
! * Store the heap tuple into the tuple table slot, making sure we have a
! * writable copy. We can use the trigger tuple slot.
! */
! slot = estate->es_trig_tuple_slot;
! if (slot->tts_tupleDescriptor != RelationGetDescr(resultRelationDesc))
! ExecSetSlotDescriptor(slot, RelationGetDescr(resultRelationDesc));
! ExecStoreTuple(oldtuple, slot, InvalidBuffer, false);
! oldtuple = ExecMaterializeSlot(slot);
! slot = ExecIRDeleteTriggers(estate, resultRelInfo, oldtuple, slot);
! if (slot == NULL) /* "do nothing" */
return NULL;
+
+ /* trigger might have changed tuple */
+ oldtuple = ExecMaterializeSlot(slot);
}
else if (resultRelInfo->ri_FdwRoutine)
{
***************
*** 719,732 **** ldelete:;
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
{
- /*
- * We have to put the target tuple into a slot, which means first we
- * gotta fetch it. We can use the trigger tuple slot.
- */
TupleTableSlot *rslot;
HeapTupleData deltuple;
Buffer delbuffer;
if (resultRelInfo->ri_FdwRoutine)
{
/* FDW must have provided a slot containing the deleted row */
--- 729,750 ----
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
{
TupleTableSlot *rslot;
HeapTupleData deltuple;
Buffer delbuffer;
+ /*
+ * If we fired an INSTEAD OF trigger, we should use the tuple returned
+ * from said trigger for the RETURNING projections.
+ */
+ if (resultRelInfo->ri_TrigDesc &&
+ resultRelInfo->ri_TrigDesc->trig_delete_instead_row)
+ return ExecProcessReturning(resultRelInfo, slot, planSlot);
+
+ /*
+ * Otherwise we have to to fetch the target tuple into a slot. We can
+ * use the trigger tuple slot here as well.
+ */
if (resultRelInfo->ri_FdwRoutine)
{
/* FDW must have provided a slot containing the deleted row */
*** a/src/include/commands/trigger.h
--- b/src/include/commands/trigger.h
***************
*** 154,162 **** extern void ExecARDeleteTriggers(EState *estate,
ResultRelInfo *relinfo,
ItemPointer tupleid,
HeapTuple fdw_trigtuple);
! extern bool ExecIRDeleteTriggers(EState *estate,
ResultRelInfo *relinfo,
! HeapTuple trigtuple);
extern void ExecBSUpdateTriggers(EState *estate,
ResultRelInfo *relinfo);
extern void ExecASUpdateTriggers(EState *estate,
--- 154,163 ----
ResultRelInfo *relinfo,
ItemPointer tupleid,
HeapTuple fdw_trigtuple);
! extern TupleTableSlot *ExecIRDeleteTriggers(EState *estate,
ResultRelInfo *relinfo,
! HeapTuple trigtuple,
! TupleTableSlot *slot);
extern void ExecBSUpdateTriggers(EState *estate,
ResultRelInfo *relinfo);
extern void ExecASUpdateTriggers(EState *estate,
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
2016-07-01 00:12 Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING Marko Tiikkaja <[email protected]>
@ 2017-08-13 20:48 ` Marko Tiikkaja <[email protected]>
2017-09-05 08:44 ` Re: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING Haribabu Kommi <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Marko Tiikkaja @ 2017-08-13 20:48 UTC (permalink / raw)
To: pgsql-hackers
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
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
[application/octet-stream] instead_of_delete_returning_v2.patch (8.6K, ../../CAL9smLB9NW-g4obNLWO-DpBFE15ZkGn0gLff_3HeGGPKN7ahtQ@mail.gmail.com/3-instead_of_delete_returning_v2.patch)
download | inline diff:
diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index 950245d..e8437ac 100644
*** a/doc/src/sgml/trigger.sgml
--- b/doc/src/sgml/trigger.sgml
***************
*** 212,218 ****
change the data returned by
<command>INSERT RETURNING</> or <command>UPDATE RETURNING</>,
and is useful when the view will not show exactly the same data
! that was provided.
</para>
<para>
--- 212,220 ----
change the data returned by
<command>INSERT RETURNING</> or <command>UPDATE RETURNING</>,
and is useful when the view will not show exactly the same data
! 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.
</para>
<para>
diff --git a/src/backend/commandindex b502941..645c216 100644
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
***************
*** 2654,2666 **** ExecARDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
}
}
! bool
ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
! HeapTuple trigtuple)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
TriggerData LocTriggerData;
! HeapTuple rettuple;
int i;
LocTriggerData.type = T_TriggerData;
--- 2654,2666 ----
}
}
! TupleTableSlot *
ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
! HeapTuple trigtuple, TupleTableSlot *slot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
TriggerData LocTriggerData;
! HeapTuple rettuple = trigtuple;
int i;
LocTriggerData.type = T_TriggerData;
***************
*** 2694,2704 **** ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
relinfo->ri_TrigInstrument,
GetPerTupleMemoryContext(estate));
if (rettuple == NULL)
! return false; /* Delete was suppressed */
! if (rettuple != trigtuple)
! heap_freetuple(rettuple);
}
! return true;
}
void
--- 2694,2720 ----
relinfo->ri_TrigInstrument,
GetPerTupleMemoryContext(estate));
if (rettuple == NULL)
! return NULL; /* Delete was suppressed */
}
!
! if (rettuple != trigtuple)
! {
! /*
! * 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;
! TupleDesc tupdesc = RelationGetDescr(relinfo->ri_RelationDesc);
!
! if (newslot->tts_tupleDescriptor != tupdesc)
! ExecSetSlotDescriptor(newslot, tupdesc);
! ExecStoreTuple(rettuple, newslot, InvalidBuffer, false);
! slot = newslot;
! }
!
! return slot;
}
void
diff --git a/src/backend/executor/nodindex 30add8e..d81c5a4 100644
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***************
*** 704,716 **** ExecDelete(ModifyTableState *mtstate,
if (resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_delete_instead_row)
{
! bool dodelete;
! Assert(oldtuple != NULL);
! dodelete = ExecIRDeleteTriggers(estate, resultRelInfo, oldtuple);
! if (!dodelete) /* "do nothing" */
return NULL;
}
else if (resultRelInfo->ri_FdwRoutine)
{
--- 704,726 ----
if (resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_delete_instead_row)
{
! /*
! * Store the heap tuple into the tuple table slot, making sure we have a
! * writable copy. We can use the trigger tuple slot.
! */
! slot = estate->es_trig_tuple_slot;
! if (slot->tts_tupleDescriptor != RelationGetDescr(resultRelationDesc))
! ExecSetSlotDescriptor(slot, RelationGetDescr(resultRelationDesc));
! ExecStoreTuple(oldtuple, slot, InvalidBuffer, false);
! oldtuple = ExecMaterializeSlot(slot);
! slot = ExecIRDeleteTriggers(estate, resultRelInfo, oldtuple, slot);
! if (slot == NULL) /* "do nothing" */
return NULL;
+
+ /* trigger might have changed tuple */
+ oldtuple = ExecMaterializeSlot(slot);
}
else if (resultRelInfo->ri_FdwRoutine)
{
***************
*** 851,864 **** ldelete:;
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
{
- /*
- * We have to put the target tuple into a slot, which means first we
- * gotta fetch it. We can use the trigger tuple slot.
- */
TupleTableSlot *rslot;
HeapTupleData deltuple;
Buffer delbuffer;
if (resultRelInfo->ri_FdwRoutine)
{
/* FDW must have provided a slot containing the deleted row */
--- 861,882 ----
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
{
TupleTableSlot *rslot;
HeapTupleData deltuple;
Buffer delbuffer;
+ /*
+ * If we fired an INSTEAD OF trigger, we should use the tuple returned
+ * from said trigger for the RETURNING projections.
+ */
+ if (resultRelInfo->ri_TrigDesc &&
+ resultRelInfo->ri_TrigDesc->trig_delete_instead_row)
+ return ExecProcessReturning(resultRelInfo, slot, planSlot);
+
+ /*
+ * Otherwise we have to to fetch the target tuple into a slot. We can
+ * use the trigger tuple slot here as well.
+ */
if (resultRelInfo->ri_FdwRoutine)
{
/* FDW must have provided a slot containing the deleted row */
diff --git a/src/include/commands/trigger.h bindex 36c1134..8582177 100644
*** a/src/include/commands/trigger.h
--- b/src/include/commands/trigger.h
***************
*** 209,217 **** extern void ExecARDeleteTriggers(EState *estate,
ItemPointer tupleid,
HeapTuple fdw_trigtuple,
TransitionCaptureState *transition_capture);
! extern bool ExecIRDeleteTriggers(EState *estate,
ResultRelInfo *relinfo,
! HeapTuple trigtuple);
extern void ExecBSUpdateTriggers(EState *estate,
ResultRelInfo *relinfo);
extern void ExecASUpdateTriggers(EState *estate,
--- 209,218 ----
ItemPointer tupleid,
HeapTuple fdw_trigtuple,
TransitionCaptureState *transition_capture);
! extern TupleTableSlot *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/expecteindex ac132b0..b445e26 100644
*** a/src/test/regress/expected/triggers.out
--- b/src/test/regress/expected/triggers.out
***************
*** 1309,1314 **** UPDATE 0
--- 1309,1341 ----
DELETE FROM european_city_view;
DELETE 0
\set QUIET true
+ -- modifying RETURNING from INSTEAD OF triggers on DELETEs
+ CREATE VIEW instead_of_delete_returning AS SELECT 1 AS fff;
+ CREATE FUNCTION instead_of_delete_returning_f() RETURNS trigger LANGUAGE plpgsql AS $$
+ BEGIN
+ RETURN NULL;
+ END;
+ $$;
+ CREATE TRIGGER instead_of_delete_returning_t INSTEAD OF DELETE ON instead_of_delete_returning
+ FOR EACH ROW EXECUTE PROCEDURE instead_of_delete_returning_f();
+ DELETE FROM instead_of_delete_returning RETURNING *;
+ fff
+ -----
+ (0 rows)
+
+ CREATE OR REPLACE FUNCTION instead_of_delete_returning_f() RETURNS trigger LANGUAGE plpgsql AS $$
+ BEGIN
+ OLD.fff := 3;
+ RETURN OLD;
+ END;
+ $$;
+ DELETE FROM instead_of_delete_returning RETURNING *;
+ fff
+ -----
+ 3
+ (1 row)
+
+ DROP VIEW instead_of_delete_returning CASCADE;
-- rules bypassing no-op triggers
CREATE RULE european_city_insert_rule AS ON INSERT TO european_city_view
DO INSTEAD INSERT INTO city_view
diff --git a/src/test/regress/sql/triggers.sqindex b10159a..636e387 100644
*** a/src/test/regress/sql/triggers.sql
--- b/src/test/regress/sql/triggers.sql
***************
*** 916,921 **** DELETE FROM european_city_view;
--- 916,941 ----
\set QUIET true
+ -- modifying RETURNING from INSTEAD OF triggers on DELETEs
+ CREATE VIEW instead_of_delete_returning AS SELECT 1 AS fff;
+ CREATE FUNCTION instead_of_delete_returning_f() RETURNS trigger LANGUAGE plpgsql AS $$
+ BEGIN
+ RETURN NULL;
+ END;
+ $$;
+ CREATE TRIGGER instead_of_delete_returning_t INSTEAD OF DELETE ON instead_of_delete_returning
+ FOR EACH ROW EXECUTE PROCEDURE instead_of_delete_returning_f();
+
+ DELETE FROM instead_of_delete_returning RETURNING *;
+ CREATE OR REPLACE FUNCTION instead_of_delete_returning_f() RETURNS trigger LANGUAGE plpgsql AS $$
+ BEGIN
+ OLD.fff := 3;
+ RETURN OLD;
+ END;
+ $$;
+ DELETE FROM instead_of_delete_returning RETURNING *;
+ DROP VIEW instead_of_delete_returning CASCADE;
+
-- rules bypassing no-op triggers
CREATE RULE european_city_insert_rule AS ON INSERT TO european_city_view
DO INSTEAD INSERT INTO city_view
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
2016-07-01 00:12 Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING Marko Tiikkaja <[email protected]>
2017-08-13 20:48 ` Re: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING Marko Tiikkaja <[email protected]>
@ 2017-09-05 08:44 ` Haribabu Kommi <[email protected]>
2017-09-15 14:07 ` Re: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING Daniel Gustafsson <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Haribabu Kommi @ 2017-09-05 08:44 UTC (permalink / raw)
To: Marko Tiikkaja <[email protected]>; +Cc: pgsql-hackers
On Mon, Aug 14, 2017 at 6:48 AM, Marko Tiikkaja <[email protected]> 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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
2016-07-01 00:12 Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING Marko Tiikkaja <[email protected]>
2017-08-13 20:48 ` Re: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING Marko Tiikkaja <[email protected]>
2017-09-05 08:44 ` Re: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING Haribabu Kommi <[email protected]>
@ 2017-09-15 14:07 ` Daniel Gustafsson <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Gustafsson @ 2017-09-15 14:07 UTC (permalink / raw)
To: Marko Tiikkaja <[email protected]>; Haribabu Kommi <[email protected]>; +Cc: pgsql-hackers
> On 05 Sep 2017, at 10:44, Haribabu Kommi <[email protected]> wrote:
>
> On Mon, Aug 14, 2017 at 6:48 AM, Marko Tiikkaja <[email protected] <mailto:[email protected]>> 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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v2 1/1] add note about re-archiving in docs
@ 2022-07-07 17:43 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Nathan Bossart @ 2022-07-07 17:43 UTC (permalink / raw)
---
doc/src/sgml/backup.sgml | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 73a774d3d7..4462bfe7a9 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -681,14 +681,28 @@ test ! -f /mnt/server/archivedir/00000001000000A900000065 && cp pg_wal/0
any pre-existing archive file. This is an important safety feature to
preserve the integrity of your archive in case of administrator error
(such as sending the output of two different servers to the same archive
- directory).
+ directory). It is advisable to test your proposed archive library to ensure
+ that it does not overwrite an existing file.
</para>
<para>
- It is advisable to test your proposed archive library to ensure that it
- indeed does not overwrite an existing file, <emphasis>and that it returns
- <literal>false</literal> in this case</emphasis>.
- The example command above for Unix ensures this by including a separate
+ In rare cases, <productname>PostgreSQL</productname> may attempt to
+ re-archive a WAL file that was previously archived. For example, if the
+ system crashes before the server makes a durable record of archival success,
+ the server will attempt to archive the file again after restarting (provided
+ archiving is still enabled). When an archive library encounters a
+ pre-existing file, it may return <literal>true</literal> if the WAL file has
+ identical contents to the pre-existing archive and the pre-existing archive
+ is fully persisted to storage. Alternatively, the archive library can
+ return <literal>false</literal> anytime a pre-existing file is encountered,
+ but this will require manual action by an administrator to resolve. If a
+ pre-existing file contains different contents than the WAL file being
+ archived, the archive library <emphasis>must</emphasis> return false.
+ </para>
+
+ <para>
+ The example command above for Unix avoids overwriting a pre-existing archive
+ by including a separate
<command>test</command> step. On some Unix platforms, <command>cp</command> has
switches such as <option>-i</option> that can be used to do the same thing
less verbosely, but you should not rely on these without verifying that
--
2.25.1
--3V7upXqbjpZ4EhLz--
^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v4 2/2] add note about re-archiving in docs
@ 2022-07-07 17:43 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Nathan Bossart @ 2022-07-07 17:43 UTC (permalink / raw)
---
doc/src/sgml/backup.sgml | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 73a774d3d7..04a1f94ad0 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -681,14 +681,28 @@ test ! -f /mnt/server/archivedir/00000001000000A900000065 && cp pg_wal/0
any pre-existing archive file. This is an important safety feature to
preserve the integrity of your archive in case of administrator error
(such as sending the output of two different servers to the same archive
- directory).
+ directory). It is advisable to test your proposed archive library to ensure
+ that it does not overwrite an existing file.
</para>
<para>
- It is advisable to test your proposed archive library to ensure that it
- indeed does not overwrite an existing file, <emphasis>and that it returns
- <literal>false</literal> in this case</emphasis>.
- The example command above for Unix ensures this by including a separate
+ In rare cases, <productname>PostgreSQL</productname> may attempt to
+ re-archive a WAL file that was previously archived. For example, if the
+ system crashes before the server makes a durable record of archival success,
+ the server will attempt to archive the file again after restarting (provided
+ archiving is still enabled). When an archive library encounters a
+ pre-existing file, it may return <literal>true</literal> if the WAL file has
+ identical contents to the pre-existing archive and the pre-existing archive
+ is fully persisted to storage. Alternatively, the archive library may
+ return <literal>false</literal> anytime a pre-existing file is encountered,
+ but this will require manual action by an administrator to resolve. If a
+ pre-existing file contains different contents than the WAL file being
+ archived, the archive library <emphasis>must</emphasis> return false.
+ </para>
+
+ <para>
+ The example command above for Unix avoids overwriting a pre-existing archive
+ by including a separate
<command>test</command> step. On some Unix platforms, <command>cp</command> has
switches such as <option>-i</option> that can be used to do the same thing
less verbosely, but you should not rely on these without verifying that
--
2.25.1
--X1bOJ3K7DJ5YkBrT--
^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v1 1/1] add note about re-archiving in docs
@ 2022-07-07 17:43 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Nathan Bossart @ 2022-07-07 17:43 UTC (permalink / raw)
---
doc/src/sgml/backup.sgml | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 73a774d3d7..dd1c360455 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -685,10 +685,24 @@ test ! -f /mnt/server/archivedir/00000001000000A900000065 && cp pg_wal/0
</para>
<para>
- It is advisable to test your proposed archive library to ensure that it
- indeed does not overwrite an existing file, <emphasis>and that it returns
- <literal>false</literal> in this case</emphasis>.
- The example command above for Unix ensures this by including a separate
+ In rare cases, <productname>PostgreSQL</productname> may attempt to
+ re-archive a WAL file that was previously archived. For example, if the
+ system crashes before the server makes a durable record of archival success,
+ the server will attempt to archive the file again after restarting (provided
+ archiving is still enabled). It is advisable to test your proposed archive
+ library to ensure that it indeed does not overwrite an existing file. When
+ a pre-existing file is encountered, the archive library may return
+ <literal>true</literal> if the WAL file has identical contents to the
+ pre-existing archive. Alternatively, the archive library can return
+ <literal>false</literal> anytime a pre-existing file is encountered, but
+ this will require manual action by an administrator to resolve. If a
+ pre-existing file contains different contents than the WAL file being
+ archived, the archive library <emphasis>must</emphasis> return false.
+ </para>
+
+ <para>
+ The example command above for Unix avoids overwriting a pre-existing archive
+ by including a separate
<command>test</command> step. On some Unix platforms, <command>cp</command> has
switches such as <option>-i</option> that can be used to do the same thing
less verbosely, but you should not rely on these without verifying that
--
2.25.1
--SLDf9lqlvOQaIe6s--
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2022-07-07 17:43 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2016-07-01 00:12 Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING Marko Tiikkaja <[email protected]>
2017-08-13 20:48 ` Marko Tiikkaja <[email protected]>
2017-09-05 08:44 ` Haribabu Kommi <[email protected]>
2017-09-15 14:07 ` Daniel Gustafsson <[email protected]>
2022-07-07 17:43 [PATCH v2 1/1] add note about re-archiving in docs Nathan Bossart <[email protected]>
2022-07-07 17:43 [PATCH v4 2/2] add note about re-archiving in docs Nathan Bossart <[email protected]>
2022-07-07 17:43 [PATCH v1 1/1] add note about re-archiving in docs Nathan Bossart <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox