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

From: Marko Tiikkaja <[email-masked]>
Date: Fri, 1 Jul 2016 02:12:56 +0200

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
*** 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-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

---

From: Marko Tiikkaja <[email-masked]>
Date: Sun, 13 Aug 2017 22:48:56 +0200

On Fri, Jul 1, 2016 at 2:12 AM, I wrote:

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

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

---

From: Haribabu Kommi <[email-masked]>
Date: Tue, 5 Sep 2017 18:44:44 +1000

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

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

! /*

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.

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

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 15 Sep 2017 16:07:02 +0200

> On 05 Sep 2017, at 10:44, Haribabu Kommi <[email-masked]> wrote:
> 
> On Mon, Aug 14, 2017 at 6:48 AM, Marko Tiikkaja <[email-masked] <mailto:[email-masked]>> 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.
> 
> 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.
> 

Have you had a chance to work on this patch to address the above review?

cheers ./daniel