public inbox for [email protected]
help / color / mirror / Atom feedFrom: Marko Tiikkaja <[email protected]>
To: PostgreSQL-development <[email protected]>
Subject: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
Date: Fri, 1 Jul 2016 02:12:56 +0200
Message-ID: <[email protected]> (raw)
List-Unsubscribe: <mailto:[email protected]?body=unsub%20pgsql-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,
view thread (7+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected]
Subject: Re: Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox