Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

Started by Marko Tiikkajaover 9 years ago12 messages

marko@joh.to

over 9 years ago

2 attachment(s)

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?

Attachments:

instead_delete.sqltext/plain; charset=UTF-8; name=instead_delete.sqlDownload

instead_delete_returning_v0.patchtext/plain; charset=UTF-8; name=instead_delete_returning_v0.patchDownload

*** 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,

Marko Tiikkaja

marko@joh.to

over 8 years ago

In reply to: Marko Tiikkaja (#1)

1 attachment(s)

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

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.

Attachments:

instead_of_delete_returning_v2.patchapplication/octet-stream; name=instead_of_delete_returning_v2.patchDownload

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

Haribabu Kommi

kommi.haribabu@gmail.com

over 8 years ago

In reply to: Marko Tiikkaja (#2)

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

On Mon, Aug 14, 2017 at 6:48 AM, Marko Tiikkaja <marko@joh.to> 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.

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

Daniel Gustafsson

daniel@yesql.se

over 8 years ago

In reply to: Haribabu Kommi (#3)

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

On 05 Sep 2017, at 10:44, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:

On Mon, Aug 14, 2017 at 6:48 AM, Marko Tiikkaja <marko@joh.to <mailto:marko@joh.to>> 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Marko Tiikkaja

marko@joh.to

about 2 years ago

In reply to: Haribabu Kommi (#3)

1 attachment(s)

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

Hi,

Thank you for the feedback.

Apparently it took me six years, but I've attached the latest version
of the patch which I believe addresses all issues. I'll add it to the
open commitfest.

Attachments:

instead_of_delete_returning_v3.patchapplication/octet-stream; name=instead_of_delete_returning_v3.patchDownload

diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index 6e1f370b21..332075daa6 100644
--- a/doc/src/sgml/trigger.sgml
+++ b/doc/src/sgml/trigger.sgml
@@ -262,13 +262,15 @@
     used to signal that the trigger performed the necessary data
     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
-    change the data returned by
-    <command>INSERT RETURNING</command> or <command>UPDATE RETURNING</command>,
-    and is useful when the view will not show exactly the same data
-    that was provided.
-   </para>
+    <command>INSERT</command>, <command>UPDATE</command>, and
+    <command>DELETE</command> operations, <command>INSTEAD OF</COMMAND>
+    triggers can modify the data returned by <command>RETURNING</command>.  In
+    the case of <command>INSERT</command> and <command>UPDATE</command>,
+    triggers can modify the <varname>NEW</varname> row before returning it,
+    while for <command>DELETE</command>, triggers can modify the
+    <varname>OLD</varname> row before returning it. This feature is useful when
+    the returned data needs to be adjusted to match the view or other
+    requirements.
 
    <para>
     The return value is ignored for row-level triggers fired after an
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 52177759ab..383f8021a5 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);
+
+			/* 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 f62d28ac60..5c3717442c 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1451,7 +1451,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;
@@ -1672,7 +1676,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 430e3ca7dd..09d3d66447 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 e8de916dfe..657dc8109f 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -1385,6 +1385,15 @@ end;
 $$;
 CREATE TRIGGER city_update_trig INSTEAD OF UPDATE ON city_view
 FOR EACH ROW EXECUTE PROCEDURE city_update();
+CREATE VIEW static_view AS SELECT 1 AS a;
+CREATE FUNCTION static_view_delete() RETURNS trigger LANGUAGE plpgsql AS $$
+begin
+    OLD.a := OLD.a + 3;
+    RETURN OLD;
+end;
+$$;
+CREATE TRIGGER static_delete_trig INSTEAD OF DELETE ON static_view
+FOR EACH ROW EXECUTE PROCEDURE static_view_delete();
 \set QUIET false
 -- INSERT .. RETURNING
 INSERT INTO city_view(city_name) VALUES('Tokyo') RETURNING *;
@@ -1478,6 +1487,13 @@ DELETE FROM city_view WHERE city_name = 'Birmingham' RETURNING *;
   234567 | Birmingham |    1016800 | UK           | Europe
 (1 row)
 
+DELETE 1
+DELETE FROM static_view RETURNING *;
+ a 
+---
+ 4
+(1 row)
+
 DELETE 1
 \set QUIET true
 -- read-only view with WHERE clause
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index d29e98d2ac..80f9512ec8 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -960,6 +960,18 @@ $$;
 CREATE TRIGGER city_update_trig INSTEAD OF UPDATE ON city_view
 FOR EACH ROW EXECUTE PROCEDURE city_update();
 
+CREATE VIEW static_view AS SELECT 1 AS a;
+
+CREATE FUNCTION static_view_delete() RETURNS trigger LANGUAGE plpgsql AS $$
+begin
+    OLD.a := OLD.a + 3;
+    RETURN OLD;
+end;
+$$;
+
+CREATE TRIGGER static_delete_trig INSTEAD OF DELETE ON static_view
+FOR EACH ROW EXECUTE PROCEDURE static_view_delete();
+
 \set QUIET false
 
 -- INSERT .. RETURNING
@@ -983,6 +995,7 @@ 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 static_view RETURNING *;
 
 \set QUIET true

Shlok Kyal

shlok.kyal.oss@gmail.com

about 2 years ago

In reply to: Marko Tiikkaja (#5)

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

Hi,

On Thu, 19 Oct 2023 at 16:35, Marko Tiikkaja <marko@joh.to> wrote:

Hi,

Thank you for the feedback.

Apparently it took me six years, but I've attached the latest version
of the patch which I believe addresses all issues. I'll add it to the
open commitfest.

.m

I went through the CFbot and found that docs build run is giving some
error (link: https://cirrus-ci.com/task/5712582359646208):
[07:37:19.769] trigger.sgml:266: parser error : Opening and ending tag
mismatch: command line 266 and COMMAND
[07:37:19.769] <command>DELETE</command> operations, <command>INSTEAD
OF</COMMAND>
[07:37:19.769] ^
[07:37:19.769] trigger.sgml:408: parser error : Opening and ending tag
mismatch: para line 266 and sect1
[07:37:19.769] </sect1>
[07:37:19.769] ^
[07:37:19.769] trigger.sgml:1034: parser error : Opening and ending
tag mismatch: sect1 line 266 and chapter
[07:37:19.769] </chapter>
[07:37:19.769] ^
[07:37:19.769] trigger.sgml:1035: parser error : chunk is not well balanced
[07:37:19.769]
[07:37:19.769] ^
[07:37:19.769] postgres.sgml:222: parser error : Failure to process
entity trigger
[07:37:19.769] &trigger;
[07:37:19.769] ^
[07:37:19.769] postgres.sgml:222: parser error : Entity 'trigger' not defined
[07:37:19.769] &trigger;
[07:37:19.769] ^
[07:37:19.956] make[2]: *** [Makefile:73: postgres-full.xml] Error 1
[07:37:19.957] make[1]: *** [Makefile:8: all] Error 2
[07:37:19.957] make: *** [Makefile:16: all] Error 2
[07:37:19.957]
[07:37:19.957] real 0m0.529s
[07:37:19.957] user 0m0.493s
[07:37:19.957] sys 0m0.053s
[07:37:19.957]
[07:37:19.957] Exit status: 2

Just wanted to make sure you are aware of the issue.

Thanks
Shlok Kumar Kyal

Marko Tiikkaja

marko@joh.to

about 2 years ago

In reply to: Shlok Kyal (#6)

1 attachment(s)

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

On Thu, Nov 2, 2023 at 12:24 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:

I went through the CFbot and found that docs build run is giving some
error (link: https://cirrus-ci.com/task/5712582359646208):

Just wanted to make sure you are aware of the issue.

I am now. Thanks! :-) Will try to keep an eye on the builds in the future.

Attached v4 of the patch which should fix the issue.

Attachments:

instead_of_delete_returning_v4.patchapplication/octet-stream; name=instead_of_delete_returning_v4.patchDownload

diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index 6e1f370b21..1d6e58312b 100644
--- a/doc/src/sgml/trigger.sgml
+++ b/doc/src/sgml/trigger.sgml
@@ -262,13 +262,15 @@
     used to signal that the trigger performed the necessary data
     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
-    change the data returned by
-    <command>INSERT RETURNING</command> or <command>UPDATE RETURNING</command>,
-    and is useful when the view will not show exactly the same data
-    that was provided.
-   </para>
+    <command>INSERT</command>, <command>UPDATE</command>, and
+    <command>DELETE</command> operations, <command>INSTEAD OF</command>
+    triggers can modify the data returned by <command>RETURNING</command>.  In
+    the case of <command>INSERT</command> and <command>UPDATE</command>,
+    triggers can modify the <varname>NEW</varname> row before returning it,
+    while for <command>DELETE</command>, triggers can modify the
+    <varname>OLD</varname> row before returning it. This feature is useful when
+    the returned data needs to be adjusted to match the view or other
+    requirements.
 
    <para>
     The return value is ignored for row-level triggers fired after an
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 52177759ab..383f8021a5 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);
+
+			/* 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 f62d28ac60..5c3717442c 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1451,7 +1451,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;
@@ -1672,7 +1676,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 430e3ca7dd..09d3d66447 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 e8de916dfe..657dc8109f 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -1385,6 +1385,15 @@ end;
 $$;
 CREATE TRIGGER city_update_trig INSTEAD OF UPDATE ON city_view
 FOR EACH ROW EXECUTE PROCEDURE city_update();
+CREATE VIEW static_view AS SELECT 1 AS a;
+CREATE FUNCTION static_view_delete() RETURNS trigger LANGUAGE plpgsql AS $$
+begin
+    OLD.a := OLD.a + 3;
+    RETURN OLD;
+end;
+$$;
+CREATE TRIGGER static_delete_trig INSTEAD OF DELETE ON static_view
+FOR EACH ROW EXECUTE PROCEDURE static_view_delete();
 \set QUIET false
 -- INSERT .. RETURNING
 INSERT INTO city_view(city_name) VALUES('Tokyo') RETURNING *;
@@ -1478,6 +1487,13 @@ DELETE FROM city_view WHERE city_name = 'Birmingham' RETURNING *;
   234567 | Birmingham |    1016800 | UK           | Europe
 (1 row)
 
+DELETE 1
+DELETE FROM static_view RETURNING *;
+ a 
+---
+ 4
+(1 row)
+
 DELETE 1
 \set QUIET true
 -- read-only view with WHERE clause
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index d29e98d2ac..80f9512ec8 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -960,6 +960,18 @@ $$;
 CREATE TRIGGER city_update_trig INSTEAD OF UPDATE ON city_view
 FOR EACH ROW EXECUTE PROCEDURE city_update();
 
+CREATE VIEW static_view AS SELECT 1 AS a;
+
+CREATE FUNCTION static_view_delete() RETURNS trigger LANGUAGE plpgsql AS $$
+begin
+    OLD.a := OLD.a + 3;
+    RETURN OLD;
+end;
+$$;
+
+CREATE TRIGGER static_delete_trig INSTEAD OF DELETE ON static_view
+FOR EACH ROW EXECUTE PROCEDURE static_view_delete();
+
 \set QUIET false
 
 -- INSERT .. RETURNING
@@ -983,6 +995,7 @@ 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 static_view RETURNING *;
 
 \set QUIET true

jian he

jian.universality@gmail.com

about 2 years ago

In reply to: Marko Tiikkaja (#7)

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

On Fri, Nov 3, 2023 at 12:34 AM Marko Tiikkaja <marko@joh.to> wrote:

I am now. Thanks! :-) Will try to keep an eye on the builds in the future.

Attached v4 of the patch which should fix the issue.

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.

vignesh C

vignesh21@gmail.com

about 2 years ago

In reply to: jian he (#8)

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

On Thu, 16 Nov 2023 at 05:30, jian he <jian.universality@gmail.com> wrote:

On Fri, Nov 3, 2023 at 12:34 AM Marko Tiikkaja <marko@joh.to> wrote:

I am now. Thanks! :-) Will try to keep an eye on the builds in the future.

Attached v4 of the patch which should fix the issue.

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.

Regards,
Vignesh

#10

jian he

jian.universality@gmail.com

almost 2 years ago

In reply to: vignesh C (#9)

1 attachment(s)

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@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.

Attachments:

v5-0001-allow-INSTEAD-OF-DELETE-triggers-to-modify-the-OL.patchtext/x-patch; charset=US-ASCII; name=v5-0001-allow-INSTEAD-OF-DELETE-triggers-to-modify-the-OL.patchDownload

From 5f41738b3c7dc7bf3d849539d16a568b52cedc55 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@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

#11

Aleksander Alekseev

aleksander@timescale.com

almost 2 years ago

In reply to: jian he (#10)

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

Hi,

it took me a while to figure out why the doc build fails.

[...]

Anyway, based on your patch, I modified it, also added a slightly more complicated test.

Thank you for keeping the patch up-to-date. Unfortunately, it seems to
be needing another rebase, according to cfbot.

Best regards,
Aleksander Alekseev (wearing co-CFM hat)

#12

Rahila Syed

rahilasyed90@gmail.com

over 1 year ago

In reply to: Aleksander Alekseev (#11)

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

Hi,

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.

I am trying to understand the basic idea behind the change. What is the
use-case for allowing the OLD tuple to be modified in the INSTEAD of DELETE
triggers.
AFAIU no matter what we return from the trigger OLD/NEW or NULL, the delete
operation is skipped, so what is the point of modifying the OLD row in your
test
for example.

If the requirement is to show the recent row in the returning clause, is
fetching the
row from heap and copying into the returning slot not enough?

In the tests shown as follows, inspite of the output showing 'DELETE 1',
no row is actually deleted from the table or the view. Probably this
is an existing behavior but I think this is misleading.

```
DELETE FROM test_ins_del_view where a = 1 returning a;
NOTICE: old.a 4
a
---
4
(1 row)

DELETE 1
```

FWIW, it might be good to include in tests the example
to demonstrate the problem regarding the concurrent transactions,
one of which is modifying the row and the other returning it
after attempting DELETE.

Thank you,
Rahila Syed

On Fri, Mar 15, 2024 at 7:57 PM Aleksander Alekseev <
aleksander@timescale.com> wrote:

Show quoted text

Hi,

it took me a while to figure out why the doc build fails.

[...]

Anyway, based on your patch, I modified it, also added a slightly more

complicated test.

Thank you for keeping the patch up-to-date. Unfortunately, it seems to
be needing another rebase, according to cfbot.

Best regards,
Aleksander Alekseev (wearing co-CFM hat)