From: Nathan Bossart Date: Fri, 5 Jun 2026 17:08:05 +0000 (-0500) Subject: refint: Remove plan cache. X-Git-Url: http://git.ipfire.org/gitweb/index.cgi?a=commitdiff_plain;h=b7b513d9afabf43124edda3dff7dd8af39906951;p=thirdparty%2Fpostgresql.git refint: Remove plan cache. Presently, refint stores plans in a per-backend cache to avoid re-preparing in each call. This has a few problems. For one, check_foreign_key() embeds the new key values in its cascade-UPDATE queries, so a cached plan reuses the values from preparation. Also, the cache is never invalidated, so it can return stale entries that cause other problems. There may very well be more bugs lurking. We could spend a lot of time trying to address all these problems, but this module is primarily intended as sample code, and by all indications, it sees minimal use. Furthermore, there is a growing consensus for removing refint in v20. However, since we'll need to support it on the back-branches for a while longer, it probably still makes sense to fix some of the more egregious bugs. Therefore, let's just remove refint's plan cache entirely. That means we'll re-prepare on every call, but that seems quite unlikely to bother anyone. On v17 and older versions, the regression test for triggers fails after this change, so I've borrowed pieces of commit 8cfbdf8f4d to fix it. Author: Ayush Tiwari Discussion: https://postgr.es/m/CAJTYsWXU%2BfhuzrEd_bnrxyGH3%2Bny8QRQC2QHf3ws6s9iki3c2Q%40mail.gmail.com Backpatch-through: 14 --- diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c index 2fa96fe092b..80b9ef650a1 100644 --- a/contrib/spi/refint.c +++ b/contrib/spi/refint.c @@ -12,25 +12,10 @@ #include "commands/trigger.h" #include "executor/spi.h" #include "utils/builtins.h" -#include "utils/memutils.h" #include "utils/rel.h" PG_MODULE_MAGIC; -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. @@ -56,12 +41,12 @@ check_primary_key(PG_FUNCTION_ARGS) Relation rel; /* triggered relation */ HeapTuple tuple = NULL; /* tuple to return */ TupleDesc tupdesc; /* tuple description */ - EPlan *plan; /* prepared plan */ + SPIPlanPtr pplan; /* prepared plan */ Oid *argtypes = NULL; /* 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; + StringInfoData sql; #ifdef DEBUG_QUERY elog(DEBUG4, "check_primary_key: Enter Function"); @@ -118,16 +103,8 @@ 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. - */ - 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)); + /* allocate argtypes for preparation */ + argtypes = (Oid *) palloc(nkeys * sizeof(Oid)); /* For each column in key ... */ for (i = 0; i < nkeys; i++) @@ -156,57 +133,36 @@ 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); } + initStringInfo(&sql); + /* - * If we have to prepare plan ... + * Construct query: SELECT 1 FROM _referenced_relation_ WHERE Pkey1 = $1 + * [AND Pkey2 = $2 [...]] */ - if (plan->nplans <= 0) + appendStringInfo(&sql, "select 1 from %s where ", relname); + for (i = 1; i <= nkeys; i++) { - SPIPlanPtr pplan; - StringInfoData sql; - - initStringInfo(&sql); - - /* - * Construct query: SELECT 1 FROM _referenced_relation_ WHERE Pkey1 = - * $1 [AND Pkey2 = $2 [...]] - */ - appendStringInfo(&sql, "select 1 from %s where ", relname); - for (i = 1; i <= nkeys; i++) - { - appendStringInfo(&sql, "%s = $%d ", args[i + nkeys], i); - if (i < nkeys) - appendStringInfoString(&sql, "and "); - } - - /* Prepare plan for query */ - pplan = SPI_prepare(sql.data, nkeys, argtypes); - if (pplan == NULL) - /* internal error */ - elog(ERROR, "check_primary_key: SPI_prepare returned %s", SPI_result_code_string(SPI_result)); + appendStringInfo(&sql, "%s = $%d ", args[i + nkeys], i); + if (i < nkeys) + appendStringInfoString(&sql, "and "); + } - /* - * 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; + /* Prepare plan for query */ + pplan = SPI_prepare(sql.data, nkeys, argtypes); + if (pplan == NULL) + /* internal error */ + elog(ERROR, "check_primary_key: SPI_prepare returned %s", SPI_result_code_string(SPI_result)); - pfree(sql.data); - } + pfree(sql.data); /* * Ok, execute prepared plan. */ - ret = SPI_execp(*(plan->splan), kvals, NULL, 1); + ret = SPI_execp(pplan, kvals, NULL, 1); /* we have no NULLs - so we pass ^^^^ here */ if (ret < 0) @@ -258,15 +214,15 @@ 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) */ + SPIPlanPtr *splan; /* prepared plan(s) */ Oid *argtypes = NULL; /* 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, r; + char **args2; #ifdef DEBUG_QUERY elog(DEBUG4, "check_foreign_key: Enter Function"); @@ -343,24 +299,8 @@ 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). - */ - snprintf(ident, sizeof(ident), "%s$%u", trigger->tgname, rel->rd_id); - 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); + /* allocate argtypes for preparation */ + argtypes = (Oid *) palloc(nkeys * sizeof(Oid)); /* For each column in key ... */ for (i = 0; i < nkeys; i++) @@ -408,141 +348,124 @@ 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; + args2 = args; - /* - * If we have to prepare plans ... - */ - if (plan->nplans <= 0) + splan = (SPIPlanPtr *) palloc(nrefs * sizeof(SPIPlanPtr)); + + for (r = 0; r < nrefs; r++) { + StringInfoData sql; SPIPlanPtr pplan; - char **args2 = args; - plan->splan = (SPIPlanPtr *) MemoryContextAlloc(TopMemoryContext, - nrefs * sizeof(SPIPlanPtr)); + initStringInfo(&sql); + + relname = args2[0]; + + /*--------- + * For 'R'estrict action we construct SELECT query: + * + * SELECT 1 + * FROM _referencing_relation_ + * WHERE Fkey1 = $1 [AND Fkey2 = $2 [...]] + * + * to check is tuple referenced or not. + *--------- + */ + if (action == 'r') + appendStringInfo(&sql, "select 1 from %s where ", relname); + + /*--------- + * For 'C'ascade action we construct DELETE query + * + * DELETE + * FROM _referencing_relation_ + * WHERE Fkey1 = $1 [AND Fkey2 = $2 [...]] + * + * to delete all referencing tuples. + *--------- + */ + + /* + * Max : Cascade with UPDATE query i create update query that updates + * new key values in referenced tables + */ + - for (r = 0; r < nrefs; r++) + else if (action == 'c') { - StringInfoData sql; - - initStringInfo(&sql); - - relname = args2[0]; - - /*--------- - * For 'R'estrict action we construct SELECT query: - * - * SELECT 1 - * FROM _referencing_relation_ - * WHERE Fkey1 = $1 [AND Fkey2 = $2 [...]] - * - * to check is tuple referenced or not. - *--------- - */ - if (action == 'r') - appendStringInfo(&sql, "select 1 from %s where ", relname); - - /*--------- - * For 'C'ascade action we construct DELETE query - * - * DELETE - * FROM _referencing_relation_ - * WHERE Fkey1 = $1 [AND Fkey2 = $2 [...]] - * - * to delete all referencing tuples. - *--------- - */ - - /* - * Max : Cascade with UPDATE query i create update query that - * updates new key values in referenced tables - */ - - - else if (action == 'c') + if (is_update == 1) { - if (is_update == 1) - { - int fn; - char *nv; - int k; - - appendStringInfo(&sql, "update %s set ", relname); - for (k = 1; k <= nkeys; k++) - { - fn = SPI_fnumber(tupdesc, args_temp[k - 1]); - Assert(fn > 0); /* already checked above */ - nv = SPI_getvalue(newtuple, tupdesc, fn); - - appendStringInfo(&sql, " %s = %s ", - args2[k], - nv ? quote_literal_cstr(nv) : "NULL"); - if (k < nkeys) - appendStringInfoString(&sql, ", "); - } - appendStringInfoString(&sql, " where "); - } - else - /* DELETE */ - appendStringInfo(&sql, "delete from %s where ", relname); - } + int fn; + char *nv; + int k; - /* - * For 'S'etnull action we construct UPDATE query - UPDATE - * _referencing_relation_ SET Fkey1 null [, Fkey2 null [...]] - * WHERE Fkey1 = $1 [AND Fkey2 = $2 [...]] - to set key columns in - * all referencing tuples to NULL. - */ - else if (action == 's') - { appendStringInfo(&sql, "update %s set ", relname); - for (i = 1; i <= nkeys; i++) + for (k = 1; k <= nkeys; k++) { - appendStringInfo(&sql, "%s = null", args2[i]); - if (i < nkeys) + fn = SPI_fnumber(tupdesc, args_temp[k - 1]); + Assert(fn > 0); /* already checked above */ + nv = SPI_getvalue(newtuple, tupdesc, fn); + + appendStringInfo(&sql, " %s = %s ", + args2[k], + nv ? quote_literal_cstr(nv) : "NULL"); + if (k < nkeys) appendStringInfoString(&sql, ", "); } appendStringInfoString(&sql, " where "); } + else + /* DELETE */ + appendStringInfo(&sql, "delete from %s where ", relname); + } - /* Construct WHERE qual */ + /* + * For 'S'etnull action we construct UPDATE query - UPDATE + * _referencing_relation_ SET Fkey1 null [, Fkey2 null [...]] WHERE + * Fkey1 = $1 [AND Fkey2 = $2 [...]] - to set key columns in all + * referencing tuples to NULL. + */ + else if (action == 's') + { + appendStringInfo(&sql, "update %s set ", relname); for (i = 1; i <= nkeys; i++) { - appendStringInfo(&sql, "%s = $%d ", args2[i], i); + appendStringInfo(&sql, "%s = null", args2[i]); if (i < nkeys) - appendStringInfoString(&sql, "and "); + appendStringInfoString(&sql, ", "); } + appendStringInfoString(&sql, " where "); + } - /* Prepare plan for query */ - pplan = SPI_prepare(sql.data, nkeys, argtypes); - if (pplan == NULL) - /* internal error */ - elog(ERROR, "check_foreign_key: SPI_prepare returned %s", SPI_result_code_string(SPI_result)); + /* Construct WHERE qual */ + for (i = 1; i <= nkeys; i++) + { + appendStringInfo(&sql, "%s = $%d ", args2[i], i); + if (i < nkeys) + appendStringInfoString(&sql, "and "); + } - /* - * 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"); + /* Prepare plan for query */ + pplan = SPI_prepare(sql.data, nkeys, argtypes); + if (pplan == NULL) + /* internal error */ + elog(ERROR, "check_foreign_key: SPI_prepare returned %s", SPI_result_code_string(SPI_result)); - plan->splan[r] = pplan; + splan[r] = pplan; - args2 += nkeys + 1; /* to the next relation */ + args2 += nkeys + 1; /* to the next relation */ #ifdef DEBUG_QUERY - elog(DEBUG4, "check_foreign_key Debug Query is : %s ", sql.data); + elog(DEBUG4, "check_foreign_key Debug Query is : %s ", sql.data); #endif - pfree(sql.data); - } - plan->nplans = nrefs; + pfree(sql.data); } /* @@ -567,9 +490,7 @@ 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); + ret = SPI_execp(splan[r], kvals, NULL, tcount); /* we have no NULLs - so we pass ^^^^ here */ if (ret < 0) @@ -602,46 +523,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 = (EPlan *) palloc(sizeof(EPlan)); - (*nplans) = i = 0; - } - - newp->ident = pstrdup(ident); - newp->nplans = 0; - newp->splan = NULL; - (*nplans)++; - - MemoryContextSwitchTo(oldcontext); - return newp; -} diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index efecd3fa2dd..b71284d7973 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -46,12 +46,12 @@ create unique index pkeys_i on pkeys (pkey1, pkey2); -- (fkey3) --> fkeys2 (pkey23) -- create trigger check_fkeys_pkey_exist - before insert or update on fkeys + after insert or update on fkeys for each row execute function check_primary_key ('fkey1', 'fkey2', 'pkeys', 'pkey1', 'pkey2'); create trigger check_fkeys_pkey2_exist - before insert or update on fkeys + after insert or update on fkeys for each row execute function check_primary_key ('fkey3', 'fkeys2', 'pkey23'); -- @@ -59,7 +59,7 @@ create trigger check_fkeys_pkey2_exist -- (fkey21, fkey22) --> pkeys (pkey1, pkey2) -- create trigger check_fkeys2_pkey_exist - before insert or update on fkeys2 + after insert or update on fkeys2 for each row execute procedure check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2'); @@ -74,7 +74,7 @@ COMMENT ON TRIGGER check_fkeys2_pkey_exist ON fkeys2 IS NULL; -- fkeys (fkey1, fkey2) and fkeys2 (fkey21, fkey22) -- create trigger check_pkeys_fkey_cascade - before delete or update on pkeys + after delete or update on pkeys for each row execute procedure check_foreign_key (2, 'cascade', 'pkey1', 'pkey2', @@ -85,7 +85,7 @@ create trigger check_pkeys_fkey_cascade -- fkeys (fkey3) -- create trigger check_fkeys2_fkey_restrict - before delete or update on fkeys2 + after delete or update on fkeys2 for each row execute procedure check_foreign_key (1, 'restrict', 'pkey23', 'fkeys', 'fkey3'); insert into fkeys2 values (10, '1', 1); @@ -117,11 +117,10 @@ NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are deleted update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 50 and pkey2 = '5'; NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted -ERROR: "check_fkeys2_fkey_restrict": tuple is referenced in "fkeys" -CONTEXT: SQL statement "delete from fkeys2 where fkey21 = $1 and fkey22 = $2 " -update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 10 and pkey2 = '1'; -NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are deleted +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. 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 @@ -130,16 +129,16 @@ SELECT trigger_name, event_manipulation, event_object_schema, event_object_table ORDER BY trigger_name COLLATE "C", 2; 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 ----------------------------+--------------------+---------------------+--------------------+--------------+------------------+--------------------+---------------+----------------------------+---------------------------- - check_fkeys2_fkey_restrict | DELETE | public | fkeys2 | 1 | | ROW | BEFORE | | - check_fkeys2_fkey_restrict | UPDATE | public | fkeys2 | 1 | | ROW | BEFORE | | - check_fkeys2_pkey_exist | INSERT | public | fkeys2 | 1 | | ROW | BEFORE | | - check_fkeys2_pkey_exist | UPDATE | public | fkeys2 | 2 | | ROW | BEFORE | | - check_fkeys_pkey2_exist | INSERT | public | fkeys | 1 | | ROW | BEFORE | | - check_fkeys_pkey2_exist | UPDATE | public | fkeys | 1 | | ROW | BEFORE | | - check_fkeys_pkey_exist | INSERT | public | fkeys | 2 | | ROW | BEFORE | | - check_fkeys_pkey_exist | UPDATE | public | fkeys | 2 | | ROW | BEFORE | | - check_pkeys_fkey_cascade | DELETE | public | pkeys | 1 | | ROW | BEFORE | | - check_pkeys_fkey_cascade | UPDATE | public | pkeys | 1 | | ROW | BEFORE | | + check_fkeys2_fkey_restrict | DELETE | public | fkeys2 | 1 | | ROW | AFTER | | + check_fkeys2_fkey_restrict | UPDATE | public | fkeys2 | 1 | | ROW | AFTER | | + check_fkeys2_pkey_exist | INSERT | public | fkeys2 | 1 | | ROW | AFTER | | + check_fkeys2_pkey_exist | UPDATE | public | fkeys2 | 2 | | ROW | AFTER | | + check_fkeys_pkey2_exist | INSERT | public | fkeys | 1 | | ROW | AFTER | | + check_fkeys_pkey2_exist | UPDATE | public | fkeys | 1 | | ROW | AFTER | | + check_fkeys_pkey_exist | INSERT | public | fkeys | 2 | | ROW | AFTER | | + check_fkeys_pkey_exist | UPDATE | public | fkeys | 2 | | ROW | AFTER | | + check_pkeys_fkey_cascade | DELETE | public | pkeys | 1 | | ROW | AFTER | | + check_pkeys_fkey_cascade | UPDATE | public | pkeys | 1 | | ROW | AFTER | | (10 rows) DROP TABLE pkeys; diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index b6a5bd3d95f..d880006199c 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -57,13 +57,13 @@ create unique index pkeys_i on pkeys (pkey1, pkey2); -- (fkey3) --> fkeys2 (pkey23) -- create trigger check_fkeys_pkey_exist - before insert or update on fkeys + after insert or update on fkeys for each row execute function check_primary_key ('fkey1', 'fkey2', 'pkeys', 'pkey1', 'pkey2'); create trigger check_fkeys_pkey2_exist - before insert or update on fkeys + after insert or update on fkeys for each row execute function check_primary_key ('fkey3', 'fkeys2', 'pkey23'); @@ -72,7 +72,7 @@ create trigger check_fkeys_pkey2_exist -- (fkey21, fkey22) --> pkeys (pkey1, pkey2) -- create trigger check_fkeys2_pkey_exist - before insert or update on fkeys2 + after insert or update on fkeys2 for each row execute procedure check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2'); @@ -88,7 +88,7 @@ COMMENT ON TRIGGER check_fkeys2_pkey_exist ON fkeys2 IS NULL; -- fkeys (fkey1, fkey2) and fkeys2 (fkey21, fkey22) -- create trigger check_pkeys_fkey_cascade - before delete or update on pkeys + after delete or update on pkeys for each row execute procedure check_foreign_key (2, 'cascade', 'pkey1', 'pkey2', @@ -100,7 +100,7 @@ create trigger check_pkeys_fkey_cascade -- fkeys (fkey3) -- create trigger check_fkeys2_fkey_restrict - before delete or update on fkeys2 + after delete or update on fkeys2 for each row execute procedure check_foreign_key (1, 'restrict', 'pkey23', 'fkeys', 'fkey3');