From 2e91865f67218bda428d2ccd87ca1f2d4d292992 Mon Sep 17 00:00:00 2001 From: Ayush Tiwari Date: Fri, 15 May 2026 14:53:54 +0000 Subject: [PATCH v3] refint: Remove private SPI plan cache. refint maintained a private per-trigger SPI plan cache without any invalidation mechanism. For check_foreign_key(), this was unsafe both because cascade UPDATE query text embeds the current NEW key values in its SET clause, and because dropping and recreating a trigger with the same name but different arguments could find stale plans. check_primary_key() did not have the cascade UPDATE value-reuse bug, but it still had the same invalidation problem. Since refint is sample code, remove the cache altogether instead of keeping partial cache invalidation semantics that are easy to copy incorrectly. Prepare the SPI plans afresh on each trigger invocation and let SPI_finish() release them. Add minimal regression coverage for the cascade UPDATE value-reuse case. --- contrib/spi/expected/refint.out | 21 +++++ contrib/spi/refint.c | 153 ++++++-------------------------- contrib/spi/sql/refint.sql | 17 ++++ 3 files changed, 66 insertions(+), 125 deletions(-) diff --git a/contrib/spi/expected/refint.out b/contrib/spi/expected/refint.out index 79633603217..55cce40179b 100644 --- a/contrib/spi/expected/refint.out +++ b/contrib/spi/expected/refint.out @@ -88,6 +88,27 @@ NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are updated update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 10 and pkey2 = '1'; ERROR: duplicate key value violates unique constraint "pkeys_i" DETAIL: Key (pkey1, pkey2)=(7, 70) already exists. +-- Cascade UPDATE plans must use the current new key values. +create table pkeys3 (pkey int4); +create table fkeys3 (fkey int4); +create trigger check_pkeys3_fkey_cascade + after delete or update on pkeys3 + for each row + execute function check_foreign_key (1, 'cascade', 'pkey', 'fkeys3', 'fkey'); +insert into pkeys3 values (1), (2); +insert into fkeys3 values (1), (2); +update pkeys3 set pkey = 11 where pkey = 1; +NOTICE: check_pkeys3_fkey_cascade: 1 tuple(s) of fkeys3 are updated +update pkeys3 set pkey = 22 where pkey = 2; +NOTICE: check_pkeys3_fkey_cascade: 1 tuple(s) of fkeys3 are updated +select * from fkeys3 order by fkey; + fkey +------ + 11 + 22 +(2 rows) + +drop table pkeys3, fkeys3; SELECT trigger_name, event_manipulation, event_object_schema, event_object_table, action_order, action_condition, action_orientation, action_timing, action_reference_old_table, action_reference_new_table diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c index 48512a664d2..4963df3ce34 100644 --- a/contrib/spi/refint.c +++ b/contrib/spi/refint.c @@ -12,7 +12,6 @@ #include "commands/trigger.h" #include "executor/spi.h" #include "utils/builtins.h" -#include "utils/memutils.h" #include "utils/rel.h" PG_MODULE_MAGIC_EXT( @@ -20,20 +19,6 @@ PG_MODULE_MAGIC_EXT( .version = PG_VERSION ); -typedef struct -{ - char *ident; - int nplans; - SPIPlanPtr *splan; -} EPlan; - -static EPlan *FPlans = NULL; -static int nFPlans = 0; -static EPlan *PPlans = NULL; -static int nPPlans = 0; - -static EPlan *find_plan(char *ident, EPlan **eplan, int *nplans); - /* * check_primary_key () -- check that key in tuple being inserted/updated * references existing tuple in "primary" table. @@ -59,10 +44,9 @@ check_primary_key(PG_FUNCTION_ARGS) Relation rel; /* triggered relation */ HeapTuple tuple = NULL; /* tuple to return */ TupleDesc tupdesc; /* tuple description */ - EPlan *plan; /* prepared plan */ - Oid *argtypes = NULL; /* key types to prepare execution plan */ + SPIPlanPtr splan; /* prepared plan for this call */ + Oid *argtypes; /* key types to prepare execution plan */ bool isnull; /* to know is some column NULL or not */ - char ident[2 * NAMEDATALEN]; /* to identify myself */ int ret; int i; @@ -124,15 +108,10 @@ check_primary_key(PG_FUNCTION_ARGS) kvals = (Datum *) palloc(nkeys * sizeof(Datum)); /* - * Construct ident string as TriggerName $ TriggeredRelationId and try to - * find prepared execution plan. + * Prepare an SPI plan per trigger invocation. refint is example code and + * does not need a private cache with incomplete invalidation semantics. */ - snprintf(ident, sizeof(ident), "%s$%u", trigger->tgname, rel->rd_id); - plan = find_plan(ident, &PPlans, &nPPlans); - - /* if there is no plan then allocate argtypes for preparation */ - if (plan->nplans <= 0) - argtypes = (Oid *) palloc(nkeys * sizeof(Oid)); + argtypes = (Oid *) palloc(nkeys * sizeof(Oid)); /* For each column in key ... */ for (i = 0; i < nkeys; i++) @@ -161,16 +140,14 @@ check_primary_key(PG_FUNCTION_ARGS) return PointerGetDatum(tuple); } - if (plan->nplans <= 0) /* Get typeId of column */ - argtypes[i] = SPI_gettypeid(tupdesc, fnumber); + /* Get typeId of column */ + argtypes[i] = SPI_gettypeid(tupdesc, fnumber); } /* - * If we have to prepare plan ... + * Prepare plan. */ - if (plan->nplans <= 0) { - SPIPlanPtr pplan; StringInfoData sql; initStringInfo(&sql); @@ -188,30 +165,18 @@ check_primary_key(PG_FUNCTION_ARGS) } /* Prepare plan for query */ - pplan = SPI_prepare(sql.data, nkeys, argtypes); - if (pplan == NULL) + splan = SPI_prepare(sql.data, nkeys, argtypes); + if (splan == NULL) /* internal error */ elog(ERROR, "check_primary_key: SPI_prepare returned %s", SPI_result_code_string(SPI_result)); - /* - * Remember that SPI_prepare places plan in current memory context - - * so, we have to save plan in TopMemoryContext for later use. - */ - if (SPI_keepplan(pplan)) - /* internal error */ - elog(ERROR, "check_primary_key: SPI_keepplan failed"); - plan->splan = (SPIPlanPtr *) MemoryContextAlloc(TopMemoryContext, - sizeof(SPIPlanPtr)); - *(plan->splan) = pplan; - plan->nplans = 1; - pfree(sql.data); } /* * Ok, execute prepared plan. */ - ret = SPI_execp(*(plan->splan), kvals, NULL, 1); + ret = SPI_execp(splan, kvals, NULL, 1); /* we have no NULLs - so we pass ^^^^ here */ if (ret < 0) @@ -263,11 +228,10 @@ check_foreign_key(PG_FUNCTION_ARGS) HeapTuple trigtuple = NULL; /* tuple to being changed */ HeapTuple newtuple = NULL; /* tuple to return */ TupleDesc tupdesc; /* tuple description */ - EPlan *plan; /* prepared plan(s) */ - Oid *argtypes = NULL; /* key types to prepare execution plan */ + SPIPlanPtr *splan; /* prepared plan(s) for this call */ + Oid *argtypes; /* key types to prepare execution plan */ bool isnull; /* to know is some column NULL or not */ bool isequal = true; /* are keys in both tuples equal (in UPDATE) */ - char ident[2 * NAMEDATALEN]; /* to identify myself */ int is_update = 0; int ret; int i, @@ -351,23 +315,16 @@ check_foreign_key(PG_FUNCTION_ARGS) kvals = (Datum *) palloc(nkeys * sizeof(Datum)); /* - * Construct ident string as TriggerName $ TriggeredRelationId $ - * OperationType and try to find prepared execution plan(s). + * SPI plans are prepared per trigger invocation and released by + * SPI_finish() below. Earlier versions of this code cached plans across + * invocations keyed by trigger name, but that was unsafe in two ways: the + * cascade UPDATE query text embeds the current NEW key values in its SET + * clause and so cannot be reused for later UPDATEs, and the cache had no + * invalidation mechanism, so a trigger dropped and recreated with the same + * name but different arguments would still find the stale plans. refint + * is example code, so we just keep things simple here. */ - 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 */ - if (plan->nplans <= 0) - argtypes = (Oid *) palloc(nkeys * sizeof(Oid)); - - /* - * else - check that we have exactly nrefs plan(s) ready - */ - else if (plan->nplans != nrefs) - /* internal error */ - elog(ERROR, "%s: check_foreign_key: # of plans changed in meantime", - trigger->tgname); + argtypes = (Oid *) palloc(nkeys * sizeof(Oid)); /* For each column in key ... */ for (i = 0; i < nkeys; i++) @@ -415,23 +372,21 @@ check_foreign_key(PG_FUNCTION_ARGS) isequal = false; } - if (plan->nplans <= 0) /* Get typeId of column */ - argtypes[i] = SPI_gettypeid(tupdesc, fnumber); + /* Get typeId of column */ + argtypes[i] = SPI_gettypeid(tupdesc, fnumber); } args_temp = args; nargs -= nkeys; args += nkeys; /* - * If we have to prepare plans ... + * Prepare an SPI plan for each referencing relation. */ - if (plan->nplans <= 0) { SPIPlanPtr pplan; char **args2 = args; - plan->splan = (SPIPlanPtr *) MemoryContextAlloc(TopMemoryContext, - nrefs * sizeof(SPIPlanPtr)); + splan = (SPIPlanPtr *) palloc(nrefs * sizeof(SPIPlanPtr)); for (r = 0; r < nrefs; r++) { @@ -531,15 +486,7 @@ check_foreign_key(PG_FUNCTION_ARGS) /* internal error */ elog(ERROR, "check_foreign_key: SPI_prepare returned %s", SPI_result_code_string(SPI_result)); - /* - * Remember that SPI_prepare places plan in current memory context - * - so, we have to save plan in Top memory context for later use. - */ - if (SPI_keepplan(pplan)) - /* internal error */ - elog(ERROR, "check_foreign_key: SPI_keepplan failed"); - - plan->splan[r] = pplan; + splan[r] = pplan; args2 += nkeys + 1; /* to the next relation */ @@ -549,7 +496,6 @@ check_foreign_key(PG_FUNCTION_ARGS) pfree(sql.data); } - plan->nplans = nrefs; } /* @@ -574,7 +520,7 @@ check_foreign_key(PG_FUNCTION_ARGS) relname = args[0]; - ret = SPI_execp(plan->splan[r], kvals, NULL, tcount); + ret = SPI_execp(splan[r], kvals, NULL, tcount); /* we have no NULLs - so we pass ^^^^ here */ if (ret < 0) @@ -613,46 +559,3 @@ check_foreign_key(PG_FUNCTION_ARGS) return PointerGetDatum((newtuple == NULL) ? trigtuple : newtuple); } - -static EPlan * -find_plan(char *ident, EPlan **eplan, int *nplans) -{ - EPlan *newp; - int i; - MemoryContext oldcontext; - - /* - * All allocations done for the plans need to happen in a session-safe - * context. - */ - oldcontext = MemoryContextSwitchTo(TopMemoryContext); - - if (*nplans > 0) - { - for (i = 0; i < *nplans; i++) - { - if (strcmp((*eplan)[i].ident, ident) == 0) - break; - } - if (i != *nplans) - { - MemoryContextSwitchTo(oldcontext); - return (*eplan + i); - } - *eplan = (EPlan *) repalloc(*eplan, (i + 1) * sizeof(EPlan)); - newp = *eplan + i; - } - else - { - newp = *eplan = palloc_object(EPlan); - (*nplans) = i = 0; - } - - newp->ident = pstrdup(ident); - newp->nplans = 0; - newp->splan = NULL; - (*nplans)++; - - MemoryContextSwitchTo(oldcontext); - return newp; -} diff --git a/contrib/spi/sql/refint.sql b/contrib/spi/sql/refint.sql index 63458127917..3b855f6db67 100644 --- a/contrib/spi/sql/refint.sql +++ b/contrib/spi/sql/refint.sql @@ -85,6 +85,23 @@ delete from pkeys where pkey1 = 40 and pkey2 = '4'; update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 50 and pkey2 = '5'; update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 10 and pkey2 = '1'; +-- Cascade UPDATE plans must use the current new key values. +create table pkeys3 (pkey int4); +create table fkeys3 (fkey int4); + +create trigger check_pkeys3_fkey_cascade + after delete or update on pkeys3 + for each row + execute function check_foreign_key (1, 'cascade', 'pkey', 'fkeys3', 'fkey'); + +insert into pkeys3 values (1), (2); +insert into fkeys3 values (1), (2); +update pkeys3 set pkey = 11 where pkey = 1; +update pkeys3 set pkey = 22 where pkey = 2; +select * from fkeys3 order by fkey; + +drop table pkeys3, fkeys3; + SELECT trigger_name, event_manipulation, event_object_schema, event_object_table, action_order, action_condition, action_orientation, action_timing, action_reference_old_table, action_reference_new_table -- 2.43.0