Fix some issues in contrib/spi/refint.c.

check_foreign_key incorrectly used a single cache entry for its saved
plans for a 'c' (cascade) trigger, although there are two different
queries to execute depending on whether it fires for an update or a
delete.  This caused the wrong things to be done if both types of
event occur in one session.  (This was indeed visible in the triggers
regression test, but apparently nobody ever questioned it.)  To fix,
add the operation type to the cache key.

Its debug log output failed to distinguish update from delete
events, too.

Also, change the intended trigger usage from BEFORE ROW to AFTER ROW,
and add checks insisting on that usage.  BEFORE is really rather
unsafe, since if there are other BEFORE triggers they might change or
cancel the operation we are trying to check.  AFTER triggers are the
standard way to propagate changes to other rows, so we should follow
that way here.

In passing, remove a useless duplicate lookup of the cache entry.

This code is mostly intended as a documentation example, so we
won't consider a back-patch.

Author: Dmitrii Bondar <d.bondar@postgrespro.ru>
Reviewed-by: Paul Jungwirth <pj@illuminatedcomputing.com>
Reviewed-by: Lilian Ontowhee <ontowhee@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/79755a2b18ed4fe5e29da6a87a1e00d1@postgrespro.ru

1 files changed, 19 insertions, 7 deletions

diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c
index d954f5c838f..d5e25e07ae9 100644
--- a/contrib/spi/refint.c
+++ b/contrib/spi/refint.c
@@ -84,6 +84,10 @@ check_primary_key(PG_FUNCTION_ARGS)
 		/* internal error */
 		elog(ERROR, "check_primary_key: must be fired for row");
 
+	if (!TRIGGER_FIRED_AFTER(trigdata->tg_event))
+		/* internal error */
+		elog(ERROR, "check_primary_key: must be fired by AFTER trigger");
+
 	/* If INSERTion then must check Tuple to being inserted */
 	if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
 		tuple = trigdata->tg_trigtuple;
@@ -287,6 +291,10 @@ check_foreign_key(PG_FUNCTION_ARGS)
 		/* internal error */
 		elog(ERROR, "check_foreign_key: cannot process INSERT events");
 
+	if (!TRIGGER_FIRED_AFTER(trigdata->tg_event))
+		/* internal error */
+		elog(ERROR, "check_foreign_key: must be fired by AFTER trigger");
+
 	/* Have to check tg_trigtuple - tuple being deleted */
 	trigtuple = trigdata->tg_trigtuple;
 
@@ -338,10 +346,10 @@ check_foreign_key(PG_FUNCTION_ARGS)
 	kvals = (Datum *) palloc(nkeys * sizeof(Datum));
 
 	/*
-	 * Construct ident string as TriggerName $ TriggeredRelationId and try to
-	 * find prepared execution plan(s).
+	 * Construct ident string as TriggerName $ TriggeredRelationId $
+	 * OperationType and try to find prepared execution plan(s).
 	 */
-	snprintf(ident, sizeof(ident), "%s$%u", trigger->tgname, rel->rd_id);
+	snprintf(ident, sizeof(ident), "%s$%u$%c", trigger->tgname, rel->rd_id, is_update ? 'U' : 'D');
 	plan = find_plan(ident, &FPlans, &nFPlans);
 
 	/* if there is no plan(s) then allocate argtypes for preparation */
@@ -573,8 +581,6 @@ check_foreign_key(PG_FUNCTION_ARGS)
 
 		relname = args[0];
 
-		snprintf(ident, sizeof(ident), "%s$%u", trigger->tgname, rel->rd_id);
-		plan = find_plan(ident, &FPlans, &nFPlans);
 		ret = SPI_execp(plan->splan[r], kvals, NULL, tcount);
 		/* we have no NULLs - so we pass   ^^^^  here */
 
@@ -596,9 +602,15 @@ check_foreign_key(PG_FUNCTION_ARGS)
 		else
 		{
 #ifdef REFINT_VERBOSE
+			const char *operation;
+
+			if (action == 'c')
+				operation = is_update ? "updated" : "deleted";
+			else
+				operation = "set to null";
+
 			elog(NOTICE, "%s: " UINT64_FORMAT " tuple(s) of %s are %s",
-				 trigger->tgname, SPI_processed, relname,
-				 (action == 'c') ? "deleted" : "set to null");
+				 trigger->tgname, SPI_processed, relname, operation);
 #endif
 		}
 		args += nkeys + 1;		/* to the next relation */