Hi hackers, I'll attach a new v5 version of my patch, I rebased so my patches can now apply cleanly in master. Reviews are welcomed. Regards, Lucas Jeffrey
El vie, 25 sept 2026 a las 1:56, Trakshan Mishra (< [email protected]>) escribió: > Hi Lucas, > > Thanks for the quick turnaround on v4. I have retested it; everything > from my review is addressed, and I could not break the new bookkeeping. > Details below. > > Test environment: > master @ 89829354de1, plus v4-0001 and v4-0002 > Linux x86_64, gcc 15.2.0 > meson, --buildtype=debug -Dcassert=true > > > == Submission review == > > Both patches apply cleanly to current master, and "git apply > --whitespace=warn" now reports no whitespace errors at all (it was 12). > typedefs.list is updated. The build is clean with no warnings. > > > == Coding review == > > 1. Fixed. I rechecked this rather than take it on trust, by rerunning > the instrumented case from my review: a parent/child pair with ON DELETE > CASCADE, a BEFORE DELETE trigger on the child that raises, and ten > cascade deletes each caught by an EXCEPTION block. Previously the count > climbed and never came back: > > RI_REFCOUNT_DEBUG entries=1 plan=0x60f5822cfd30 refcount=1 > RI_REFCOUNT_DEBUG entries=1 plan=0x60f5822cfd30 refcount=2 > ... > RI_REFCOUNT_DEBUG entries=1 plan=0x60f5822cfd30 refcount=10 > > With v4, all ten iterations look like this: > > RI_REFCOUNT_DEBUG entries=1 plan=0x5f5073f4c770 refcount=1 > > The count is given back on every error path, and the table does not > grow. The ResourceOwner approach is nicer than the PG_TRY I suggested, > and I agree with your reasoning about subtransaction boundaries. > > 2. Fixed. The entry is now removed whenever the count reaches zero, > independently of plan validity. > > 3. You kept the plan pointer as the hash key and closed the hole with an > invariant instead: a plan is never freed while it has an entry, so its > address cannot be reused while it is tracked. I went looking for a way > around that and did not find one. > > - All three SPI_freeplan() calls now sit inside the new helpers > (ri_PreparedPlanDecrementRefCount, ri_PreparedPlanReleaseASAP and > ri_PreparedPlanFreeUnused), and ri_FetchPreparedPlan() no longer > frees a plan directly. > - The RI plans are saved with SPI_keepplan(), so SPI will not free > them at transaction end either. > - Nothing else removes entries from ri_query_cache. > > I also built with a detector in ri_PreparedPlanExecutionStarted() that > warns whenever HASH_ENTER finds an existing entry whose refcount is > zero, which is what picking up a stale entry would look like. It did > not fire once across the whole regression suite. > > So I am satisfied. I still mildly prefer the refcount living on > RI_QueryHashEntry, because then the question cannot be raised at all, > but that is a preference rather than an objection, and a committer's > call. > > 4. Fixed, Assert(entry->refcount > 0) is in place. > > 5. Fixed. The ri_InitHashTables() call is now an Assert, and the table > is created alongside the others. > > 6. The style points are all addressed. > > > == Teardown ordering == > > The commit message relies on the resource owner having been released by > the time AtEOSubXact_RI() and AtEOXact_RI() run. That holds: > RESOURCE_RELEASE_AFTER_LOCKS is at xact.c:5419 and AtEOSubXact_RI() at > 5434, and on the top-level paths AtEOXact_RI() is at 2526, 2822 and > 3053, after the three ResourceOwnerRelease() calls in each. > > One thing worth stating explicitly: AtEOXact_SPI() runs immediately > before AtEOXact_RI() (xact.c:3045 and 3053 on the abort path), so > ri_PreparedPlanFreeUnused() calls SPI_freeplan() after SPI has been torn > down for the transaction. As far as I can tell that is fine, since > SPI_freeplan() does not touch _SPI_current, and it works in practice > (see below). I mention it only because it is a dependency that is not > obvious from reading the patch on its own. > > > == Test patch == > > The identifiers are English, the surviving rows are checked, and the > invalidation is deterministic now -- the GRANT/REVOKE inside the trigger > is a much better mechanism than the 1000 temp tables. I confirmed the > test still catches the bug: master with only v4-0001 applied (I checked > that the fix was absent from the binary) segfaults on > > DELETE FROM fk_self_ref WHERE id = 1; > > so it fails loudly on an unfixed backend, which was my concern before. > > Two comments. > > a) Moving it into src/test/regress answers my question about isolation, > but the blast radius is worth knowing: when it crashes, the cluster > restarts and takes the rest of the schedule with it. On my unpatched > run, foreign_key plus 51 later tests failed. I think that is acceptable > for a regression test, since it only crashes if the bug returns. > > b) The test only covers subtransaction abort, via the PL/pgSQL exception > block. The top-level abort path, where AtEOXact_RI() rather than > AtEOSubXact_RI() does the freeing, is not exercised -- and that is the > path with the AtEOXact_SPI() ordering dependency above, so it is the one > I would most like covered. I tested it by hand: same schema, the error > allowed to escape to the top level, then a further invalidation and a > successful cascade, repeated 50 times on a cassert build. It is clean, > no crash and no assertion failure. But it would be better living in the > test than as something I ran once. > > > == Regression testing == > > Full "meson test" with v4: 360 ok, 0 failed, 50 skipped. > > That run included recovery/027_stream_regress, which passed. I would > not read anything into that. It fails intermittently at roughly a third > of runs on this machine, and it is independent of your patch -- it > reproduces at c36a0df195d, which predates the RI fast-path series. A > single green run is not evidence either way. It is being tracked > separately: > > > https://www.google.com/url?q=https://postgr.es/m/cacrpqq9wihnyhjxdqp_t%2bi-i3r2fg6v%[email protected]&source=gmail&ust=1790398611321000&sa=E > > > == Nits == > > - One line in the v4-0002 commit message runs to 96 columns > ("invalidation are released already. Hence no entry survives ..."). > - Three added lines in ri_triggers.c reach 79 columns. > > > == Summary == > > Everything from my review is addressed, and the fix looks right to me. > I am marking this Ready for Committer. If you would prefer to add the > top-level abort case to the test first, I am happy to retest. > > Regards, > Trakshan Mishra > > On Fri, Sep 25, 2026 12:39 AM, Lucas Jeffrey < > [email protected]> wrote: > >> Hi, Trakshan, thanks for your review. >> >> I created a v4 of my patch which fixes items pointed out by your review. >> >> Now, my patch is using a ResourceOwner to prevent a leak if the RI check >> throws an exception, >> also, now I'm adding invalidated plans that can't be instantly deleted to >> a delete-later list: "ri_plans_to_free", >> and, now it removes the refcount table entries if the plan wasn't >> invalidated (the plan is not freed, just the use-count entry). >> >> The test case, now all names are in english and now is using a >> deterministic way of reproducing the crash, instead of relying in >> invalidations by another session by creating and destroying the same table >> in a loop. >> El jue, 24 sept 2026 a las 11:46, Trakshan Mishra (< >> [email protected]>) escribió: >> >>> Hi, >>> >>> A correction to my review upthread. I said I would report the >>> 027_stream_regress assertion separately; that was already done, four >>> days before the review went out: >>> >>> Intermittent Assert("plan->magic == _SPI_PLAN_MAGIC") in >>> 027_stream_regress >>> >>> https://www.google.com/url?q=https://postgr.es/m/cacrpqq9wihnyhjxdqp_t%2bi-i3r2fg6v%[email protected]&source=gmail&ust=1790347576108000&sa=E >>> >>> Apologies for the tense -- I wrote that paragraph before posting and >>> forgot to fix it. There is nothing new in it, I just did not want >>> anyone holding off on a report that already exists. >>> >>> Two things from that thread are worth repeating here, because they >>> bear on this patch more directly than my review made out. >>> >>> First, the assert is older than the RI work. I ran it at c36a0df195d >>> (2026-08-20), which predates the RI fast-path series, and it still >>> fails there. So it is not fallout from 6fc2a486417, e2c812f1475, >>> 2c45694a240 or c62b330912e, and it is independent of v2-0002. My >>> review only said the failure was pre-existing on current master; the >>> bisect point makes that a good deal firmer. >>> >>> Second, on the counts. The 5/15 and 9/15 in my review were a separate >>> batch from the 7 of 20 on master in the earlier thread, both at >>> 9e17d25e79d. Together that is 12 of 35 unpatched runs, 34%, which is >>> in line with the 60% I saw patched given the sample sizes. I still >>> would not read anything into the difference. >>> >>> No action needed here. #6825 stands where I left it, Waiting on >>> Author for the refcount points. >>> >>> Regards, >>> Trakshan Mishra >>> >>> On Thu, Sep 24, 2026 05:31 PM, Trakshan Mishra < >>> [email protected]> wrote: >>> >>>> Hi Lucas, >>>> >>>> I picked this up from the PG20-2 commitfest (#6825) as a first-time >>>> reviewer. Summary up front: the bug is real and reproducible, the patch >>>> does fix it, but I think the refcount bookkeeping needs another round. >>>> >>>> Test environment: >>>> master @ 9e17d25e79d >>>> Linux x86_64, gcc 15.2.0 >>>> meson, --buildtype=debug -Dcassert=true >>>> >>>> >>>> == Submission review == >>>> >>>> v3-0001 (the isolation test) applies cleanly. >>>> >>>> v2-0002 (the fix) does *not* apply to current master. It conflicts in >>>> the "Local data" block around ri_triggers.c:251 -- the RI fast-path work >>>> that landed since you posted (c62b330912e, 2c45694a240, e2c812f1475 and >>>> neighbours) restructured that area. "git apply -3" resolves it without >>>> a real conflict, so this is just a rebase, but a v4 on top of current >>>> master would help the next reviewer and cfbot. >>>> >>>> "git apply" reports 12 whitespace errors across the two patches: 4 in >>>> the .spec file and 8 in ri_triggers.c. Several are tabs immediately >>>> after an opening brace, e.g. >>>> >>>> ri_PreparedPlanExecutionStarted(SPIPlanPtr plan) >>>> {<tab> >>>> >>>> pgindent should clear these. >>>> >>>> >>>> == Feature test == >>>> >>>> I can confirm the crash on unpatched master. Applying only v3-0001 and >>>> running the new isolation test: >>>> >>>> client backend (PID 23625) was terminated by signal 11: Segmentation >>>> fault >>>> DETAIL: Failed process was running: DELETE FROM >>>> crash_reentrancia_tabla_autoreferencial WHERE id = 1; >>>> LOG: terminating any other active server processes >>>> LOG: all server processes terminated; reinitializing >>>> >>>> With v2-0002 applied the same test passes and the server stays up. So >>>> the patch does address the reported crash. Thanks for the clear >>>> reproducer -- the advisory-lock handshake to line up the invalidation >>>> was a nice touch. >>>> >>>> >>>> == Coding review == >>>> >>>> 1. The refcount is leaked whenever the RI query throws. >>>> >>>> In ri_PerformCheck() the increment and decrement bracket >>>> SPI_execute_snapshot() with no PG_TRY/PG_FINALLY: >>>> >>>> ri_PreparedPlanExecutionStarted(qplan); >>>> spi_result = SPI_execute_snapshot(qplan, ...); >>>> ri_PreparedPlanExecutionFinished(qplan); >>>> >>>> Any ereport(ERROR) from inside the RI query longjmps past the Finished() >>>> call, so the count is never given back. This is not an exotic path -- a >>>> BEFORE DELETE trigger on the referencing table that raises will do it, >>>> and so will statement_timeout, query cancel or a deadlock during the >>>> cascade. >>>> >>>> I instrumented the hash table locally to check, using a parent/child >>>> pair with ON DELETE CASCADE where the child has a BEFORE DELETE trigger >>>> that raises, and ten cascade deletes each caught by an EXCEPTION block: >>>> >>>> RI_REFCOUNT_DEBUG entries=1 plan=0x60f5822cfd30 refcount=1 >>>> RI_REFCOUNT_DEBUG entries=1 plan=0x60f5822cfd30 refcount=2 >>>> RI_REFCOUNT_DEBUG entries=1 plan=0x60f5822cfd30 refcount=3 >>>> ... >>>> RI_REFCOUNT_DEBUG entries=1 plan=0x60f5822cfd30 refcount=10 >>>> >>>> The count rises monotonically and never comes back down. Once that has >>>> happened ri_PreparedPlanCanRelease() returns false for that plan >>>> forever, so ri_FetchPreparedPlan() will never SPI_freeplan() it: on the >>>> next invalidation it sets entry->plan = NULL and the plan is orphaned in >>>> CacheMemoryContext for the life of the backend. >>>> >>>> A PG_TRY/PG_FINALLY around the execute, or tying the decrement to >>>> resource-owner or subtransaction cleanup, would fix this. >>>> >>>> >>>> 2. Entries are never removed when the refcount drops to zero on a plan >>>> that is still valid. >>>> >>>> ri_PreparedPlanExecutionFinished() only does HASH_REMOVE inside >>>> >>>> if (entry->refcount == 0 && !SPI_plan_is_valid(plan)) >>>> >>>> which is the uncommon case. Normally the entry stays behind with >>>> refcount 0 forever. I think the removal should happen whenever the >>>> count reaches 0, independently of plan validity. >>>> >>>> >>>> 3. Keying the hash table on the raw SPIPlanPtr looks fragile. >>>> >>>> Because entries outlive the plans they describe (point 2), the table >>>> accumulates entries keyed on pointers that have since been freed. That >>>> would be harmless if addresses were never reused, but they are. Driving >>>> 40 plan invalidations through ALTER TABLE, I only ever saw three >>>> distinct plan addresses, cycling: >>>> >>>> entries=1 plan=0x60f5822cf900 >>>> entries=2 plan=0x60f5822ce8d0 >>>> entries=3 plan=0x60f5822ce0b0 >>>> ... then those same three addresses repeatedly, entries stuck at 3 >>>> >>>> So a freshly created plan routinely lands on an address that already has >>>> an entry and inherits whatever refcount it was left holding. >>>> >>>> I want to be careful not to overstate this: in every path I could >>>> actually reach, the inherited value was 0, and I could not turn this >>>> into a demonstrable failure. So treat it as a design concern rather >>>> than a proven bug. But combined with point 1, which does leave counts >>>> above zero, a new plan could start life pinned and never be freed -- or >>>> a count could reach zero while an outer reentrant frame still holds the >>>> plan, which is the use-after-free this patch exists to prevent. Storing >>>> the refcount on the RI_QueryHashEntry that already owns the plan would >>>> sidestep the question entirely. >>>> >>>> >>>> 4. entry->refcount-- is unguarded. >>>> >>>> It is a uint32, so a stray extra Finished() call (or a stale entry per >>>> point 3) wraps it to 4294967295 rather than tripping anything. An >>>> Assert(entry->refcount > 0) before the decrement would catch that in >>>> cassert builds. >>>> >>>> >>>> 5. The ri_InitHashTables() call in ri_PreparedPlanExecutionStarted(). >>>> >>>> if (!ri_query_plan_cache_executing_refcount) >>>> ri_InitHashTables(); >>>> >>>> ri_InitHashTables() unconditionally recreates all four hash tables and >>>> re-runs both CacheRegisterSyscacheCallback() calls. If this branch were >>>> ever taken with the other caches already populated it would orphan >>>> ri_constraint_cache, ri_query_cache and ri_compare_cache, and register >>>> duplicate syscache callbacks against a limited pool. In practice >>>> ri_PerformCheck() is only reached after the caches exist, so the branch >>>> looks unreachable -- which argues for an Assert instead, or for >>>> splitting the refcount table's initialisation out. >>>> >>>> >>>> 6. Style points >>>> >>>> - "// Remove the entry" needs to be a /* */ comment. >>>> - "RI_QueryPlanCacheExecutingRefCountEntry* entry" should be >>>> "... *entry" (three occurrences). >>>> - "bool found" is declared in ri_PreparedPlanExecutionFinished() and >>>> ri_PreparedPlanCanRelease() but never read; both test !entry. >>>> - Several lines run to 103-131 columns. >>>> - The three new functions have no comment headers, unlike their >>>> neighbours in this file. >>>> - "this call can free the plan..." should start with a capital. >>>> >>>> All pgindent/typedefs.list territory rather than anything substantive. >>>> >>>> >>>> == Test patch == >>>> >>>> 1. The identifiers are in Spanish -- crash_reentrancia_tabla_ >>>> autoreferencial, nombre, padre_id, crash_reentrancia_segunda_tabla, >>>> valor. The rest of the tree is English, so these will need renaming. >>>> >>>> 2. The test leans on overflowing the shared invalidation queue with 1000 >>>> temp table create/drops. It does reproduce reliably here (7.2s), but >>>> nothing makes it fail loudly if that stops being enough -- it would just >>>> start passing on an unfixed backend. Is there a way to make the >>>> invalidation deterministic? >>>> >>>> 3. A crash test in the isolation schedule takes down the whole cluster >>>> when it fails, aborting the rest of the schedule. I do not know the >>>> project's preference here, but it may be worth asking whether this >>>> belongs in src/test/isolation or as a TAP test. >>>> >>>> 4. The expected output only shows that s1_delete completed; it does not >>>> check the resulting table contents. Asserting the surviving rows would >>>> turn "did not crash" into "cascaded correctly". >>>> >>>> 5. Four of the whitespace errors above are in this file. >>>> >>>> >>>> == Regression testing == >>>> >>>> Full "meson test" with the patch: 359 ok, 49 skipped, 1 failed. >>>> >>>> The failure was recovery/027_stream_regress, with >>>> >>>> TRAP: failed Assert("plan->magic == _SPI_PLAN_MAGIC"), >>>> File: "../src/backend/executor/spi.c", Line: 1951 >>>> client backend was terminated by signal 6: Aborted >>>> DETAIL: Failed process was running: UPDATE temporal_mltrng >>>> SET valid_at = >>>> datemultirange(daterange('2016-02-01','2016-03-01')) >>>> WHERE id = '[5,6)' AND ... >>>> >>>> I initially assumed the patch had caused this, since it is the same >>>> use-after-free shape the patch is about. It has not. Clean master with >>>> no patch applied reproduces the identical assertion. Counts over 15 >>>> runs each: >>>> >>>> master : 5/15 runs hit the assert (33%) >>>> patched : 9/15 runs hit the assert (60%) >>>> >>>> Fisher exact two-tailed p = 0.27, so the difference is not significant >>>> at these sample sizes and I am not claiming the patch makes it worse -- >>>> only that it does not fix it and that the failure is pre-existing. I >>>> will report that one separately rather than tangle it up with this >>>> thread. >>>> >>>> Aside from 027_stream_regress, nothing regressed. >>>> >>>> >>>> == Summary == >>>> >>>> The crash is real, easy to trigger, and the patch fixes it. I would >>>> call the direction sound but the bookkeeping not ready: point 1 is a >>>> straightforward leak on a common error path, and point 3 makes me uneasy >>>> about the choice of hash key. >>>> >>>> Marking this Waiting on Author. Happy to retest a v4. >>>> >>>> On the wider question about whether a BEFORE DELETE trigger should be >>>> deleting rows at all -- I do not have the standing to argue that either >>>> way. But a backend that segfaults seems worth closing regardless of >>>> whether the usage is advisable, and if the consensus is that it should >>>> not be allowed, an explicit error would still need this same reentrancy >>>> information to detect the situation. >>>> >>>> Regards, >>>> Trakshan Mishra >>>> >>>
From 6587a490533ebfbd3a4b5e5b980bd3662690c288 Mon Sep 17 00:00:00 2001 From: Lucas Jeffrey <[email protected]> Date: Thu, 24 Sep 2026 12:56:54 -0300 Subject: [PATCH v5 1/2] Add test for reentrant ON DELETE CASCADE on a self-referencing table Add a regression test for a crash in RI_FKey_cascade_del(): with an ON DELETE CASCADE foreign key on a self-referencing table and a BEFORE DELETE trigger that deletes from that same table, the RI cascade query can be reentered while it's executing. If the cascade's plan gets invalidated meanwhile, the nested call finds it invalid in ri_FetchPreparedPlan() and frees it, while the outer call is still executing it, leading to a use-after-free in _SPI_execute_plan(). The test also checks the rows that survive the cascade, and covers the error path: the nested cascade raises an error, caught by a PL/pgSQL exception block, several times in a row, while the invalidated plans are still being executed; a plain DELETE after a further invalidation must still work (and not trip any assertion). Signed-off-by: Lucas Jeffrey <[email protected]> --- src/test/regress/expected/foreign_key.out | 75 +++++++++++++++++++++++ src/test/regress/sql/foreign_key.sql | 55 +++++++++++++++++ 2 files changed, 130 insertions(+) diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index abafaff785a..eb1a9828146 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -4119,3 +4119,78 @@ DROP TYPE fkint CASCADE; NOTICE: drop cascades to 2 other objects DETAIL: drop cascades to function fkint_in(cstring) drop cascades to function fkint_out(fkint) +-- ON DELETE CASCADE on a self-referencing table, with a BEFORE DELETE trigger +-- that deletes from the same table. The cascade's plan is invalidated (by +-- the GRANT) while the cascade is executing it, and the nested DELETE reaches +-- the same RI query again; the outer execution must still be able to finish. +CREATE TABLE fk_self_ref ( + id int PRIMARY KEY, + parent_id int REFERENCES fk_self_ref (id) ON DELETE CASCADE +); +CREATE FUNCTION fk_self_ref_before_del() RETURNS trigger LANGUAGE plpgsql AS $$ +BEGIN + IF OLD.id = 2 THEN + GRANT SELECT ON fk_self_ref TO PUBLIC; + REVOKE SELECT ON fk_self_ref FROM PUBLIC; + DELETE FROM fk_self_ref WHERE parent_id = OLD.id; + ELSIF OLD.id = 4 AND current_setting('fk_self_ref.fail', true) = 'on' THEN + RAISE EXCEPTION 'fk_self_ref: deleting %', OLD.id; + END IF; + RETURN OLD; +END$$; +CREATE TRIGGER fk_self_ref_before_del BEFORE DELETE ON fk_self_ref + FOR EACH ROW EXECUTE PROCEDURE fk_self_ref_before_del(); +INSERT INTO fk_self_ref VALUES (1, NULL), (2, 1), (3, 2), (4, 3), (5, 4), + (10, NULL), (11, 10); +DELETE FROM fk_self_ref WHERE id = 1; +SELECT * FROM fk_self_ref ORDER BY id; + id | parent_id +----+----------- + 10 | + 11 | 10 +(2 rows) + +-- Same, but with the nested cascade failing, which aborts the executions +-- that are using the plans. Their pins must be released on error, so that +-- the plans can be replaced and freed afterwards. +INSERT INTO fk_self_ref VALUES (1, NULL), (2, 1), (3, 2), (4, 3), (5, 4); +SET fk_self_ref.fail = on; +DO $$ +DECLARE + failures int := 0; +BEGIN + FOR i IN 1..10 LOOP + BEGIN + DELETE FROM fk_self_ref WHERE id = 1; + EXCEPTION WHEN raise_exception THEN + failures := failures + 1; + END; + END LOOP; + RAISE NOTICE 'failures: %', failures; +END$$; +NOTICE: failures: 10 +SELECT * FROM fk_self_ref ORDER BY id; + id | parent_id +----+----------- + 1 | + 2 | 1 + 3 | 2 + 4 | 3 + 5 | 4 + 10 | + 11 | 10 +(7 rows) + +RESET fk_self_ref.fail; +GRANT SELECT ON fk_self_ref TO PUBLIC; +REVOKE SELECT ON fk_self_ref FROM PUBLIC; +DELETE FROM fk_self_ref WHERE id = 1; +SELECT * FROM fk_self_ref ORDER BY id; + id | parent_id +----+----------- + 10 | + 11 | 10 +(2 rows) + +DROP TABLE fk_self_ref; +DROP FUNCTION fk_self_ref_before_del(); diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index da62f601b46..fa3564c907e 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -3065,3 +3065,58 @@ DROP TABLE pktable_inval; DROP CAST (fkint AS int4); DROP FUNCTION fkint_to_int4(fkint); DROP TYPE fkint CASCADE; + +-- ON DELETE CASCADE on a self-referencing table, with a BEFORE DELETE trigger +-- that deletes from the same table. The cascade's plan is invalidated (by +-- the GRANT) while the cascade is executing it, and the nested DELETE reaches +-- the same RI query again; the outer execution must still be able to finish. +CREATE TABLE fk_self_ref ( + id int PRIMARY KEY, + parent_id int REFERENCES fk_self_ref (id) ON DELETE CASCADE +); +CREATE FUNCTION fk_self_ref_before_del() RETURNS trigger LANGUAGE plpgsql AS $$ +BEGIN + IF OLD.id = 2 THEN + GRANT SELECT ON fk_self_ref TO PUBLIC; + REVOKE SELECT ON fk_self_ref FROM PUBLIC; + DELETE FROM fk_self_ref WHERE parent_id = OLD.id; + ELSIF OLD.id = 4 AND current_setting('fk_self_ref.fail', true) = 'on' THEN + RAISE EXCEPTION 'fk_self_ref: deleting %', OLD.id; + END IF; + RETURN OLD; +END$$; +CREATE TRIGGER fk_self_ref_before_del BEFORE DELETE ON fk_self_ref + FOR EACH ROW EXECUTE PROCEDURE fk_self_ref_before_del(); + +INSERT INTO fk_self_ref VALUES (1, NULL), (2, 1), (3, 2), (4, 3), (5, 4), + (10, NULL), (11, 10); +DELETE FROM fk_self_ref WHERE id = 1; +SELECT * FROM fk_self_ref ORDER BY id; + +-- Same, but with the nested cascade failing, which aborts the executions +-- that are using the plans. Their pins must be released on error, so that +-- the plans can be replaced and freed afterwards. +INSERT INTO fk_self_ref VALUES (1, NULL), (2, 1), (3, 2), (4, 3), (5, 4); +SET fk_self_ref.fail = on; +DO $$ +DECLARE + failures int := 0; +BEGIN + FOR i IN 1..10 LOOP + BEGIN + DELETE FROM fk_self_ref WHERE id = 1; + EXCEPTION WHEN raise_exception THEN + failures := failures + 1; + END; + END LOOP; + RAISE NOTICE 'failures: %', failures; +END$$; +SELECT * FROM fk_self_ref ORDER BY id; +RESET fk_self_ref.fail; +GRANT SELECT ON fk_self_ref TO PUBLIC; +REVOKE SELECT ON fk_self_ref FROM PUBLIC; +DELETE FROM fk_self_ref WHERE id = 1; +SELECT * FROM fk_self_ref ORDER BY id; + +DROP TABLE fk_self_ref; +DROP FUNCTION fk_self_ref_before_del(); -- 2.34.1
From cd08091d011e5965ad8db82929848669399fae5f Mon Sep 17 00:00:00 2001 From: Lucas Jeffrey <[email protected]> Date: Mon, 5 Oct 2026 09:27:04 -0300 Subject: [PATCH v5 2/2] Fix use-after-free of RI query plans in reentrant RI checks RI checks can be reentered while running: for example, with an ON DELETE CASCADE foreign key on a self-referencing table and a BEFORE DELETE trigger that deletes from the same table, the cascade's DELETE fires the trigger, whose own DELETE fires the cascade again, which runs the same cached plan. If that plan has been invalidated in the meantime, the nested call's ri_FetchPreparedPlan() sees it as invalid and frees it with SPI_freeplan(), while the outer call is still executing it. The outer call then crashes in _SPI_execute_plan(). To fix, keep track of the plans being executed by ri_PerformCheck(), in a backend-local hash table keyed by the plan pointer, with the number of executions in progress. When ri_FetchPreparedPlan() finds an invalid plan that is being executed, it no longer frees it; it removes it from the query hash table as before, and marks it for deletion so that the last execution to finish frees it. An entry exists only while its plan is being executed: it is created by the first execution and removed when the last one finishes. A plan with an entry is never freed, so its address cannot be reused by a new plan while it is tracked, which is what makes it safe to use the plan pointer as the hash key. The count must also be decremented when the query fails. Rather than wrapping the execution in PG_TRY(), each execution is registered with the current resource owner, using a new ResourceOwnerDesc. That adds next to nothing on the success path, where ResourceOwnerRemember() and ResourceOwnerForget() just add and remove an array element, and it does the right thing with subtransactions: an execution aborted along with a subtransaction (e.g. by a PL/pgSQL exception block in a trigger) is released at that subtransaction's abort, while executions of the same plan in the outer levels keep their count. A leaked count at commit would also be reported by the resource owner. The resource owner callback only decrements the count; it doesn't free any plan, to avoid running SPI_freeplan() in the middle of releasing the aborted (sub)transaction's resources. A plan marked for deletion whose last execution was aborted is queued on a list instead, and freed by the next RI query execution, or at the latest by AtEOXact_RI(), which runs after the resource owners have been released. Such a plan is no longer in the query hash table, so the next execution cannot be using it. Hence the hash table is always empty at the end of the top transaction. Signed-off-by: Lucas Jeffrey <[email protected]> --- src/backend/utils/adt/ri_triggers.c | 242 +++++++++++++++++++++++++++- src/tools/pgindent/typedefs.list | 1 + 2 files changed, 241 insertions(+), 2 deletions(-) diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 0bcb0f9b3cf..7dcc7c78ce6 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -56,6 +56,7 @@ #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/rel.h" +#include "utils/resowner.h" #include "utils/rls.h" #include "utils/ruleutils.h" #include "utils/snapmgr.h" @@ -229,12 +230,46 @@ typedef struct RI_CompareHashEntry FmgrInfo cast_func_finfo; /* in case we must coerce input */ } RI_CompareHashEntry; +/* + * RI_QueryPlanCacheExecutingRefCountEntry + * + * Tracks a saved RI plan while ri_PerformCheck() is executing it. RI checks + * can nest: a trigger fired by an RI query may run another RI query using the + * same plan (e.g. ON DELETE CASCADE on a self-referencing table, with a BEFORE + * DELETE trigger that deletes from that table). If the inner call finds the + * plan invalid, it must not free it while an outer call is still executing + * it; instead it marks the plan for deletion, and the last execution frees it. + * + * An entry exists only while the plan is being executed, plus, for a plan + * marked for deletion whose last execution was aborted by an error, until the + * next RI query execution or the end of the transaction. A plan is never + * freed while it has an entry, so its address cannot be reused by another + * plan in the meantime. + * + * Each execution is also registered with the current resource owner, so that + * the count is decremented even if the query throws an error. + */ +typedef struct RI_QueryPlanCacheExecutingRefCountEntry +{ + SPIPlanPtr plan; /* hash key */ + bool markedForDeletion; /* free when refcount reaches 0? */ + uint32 refcount; /* # of executions in progress */ + dlist_node node; /* link in ri_plans_to_free */ +} RI_QueryPlanCacheExecutingRefCountEntry; + /* * Local data */ static HTAB *ri_constraint_cache = NULL; static HTAB *ri_query_cache = NULL; static HTAB *ri_compare_cache = NULL; +static HTAB *ri_query_plan_cache_executing_refcount = NULL; + +/* + * Entries of plans marked for deletion whose last execution was aborted, to + * be freed by ri_PreparedPlanFreeUnused(). + */ +static dlist_head ri_plans_to_free = DLIST_STATIC_INIT(ri_plans_to_free); static dclist_head ri_constraint_cache_valid_list; /* @@ -278,6 +313,22 @@ static SPIPlanPtr ri_FetchPreparedPlan(RI_QueryKey *key); static void ri_HashPreparedPlan(RI_QueryKey *key, SPIPlanPtr plan); static RI_CompareHashEntry *ri_HashCompareOp(Oid eq_opr, Oid typeid); +static void ri_PreparedPlanExecutionStarted(SPIPlanPtr plan); +static void ri_PreparedPlanExecutionFinished(SPIPlanPtr plan); +static void ri_PreparedPlanDecrementRefCount(SPIPlanPtr plan, bool canFree); +static void ri_PreparedPlanReleaseASAP(SPIPlanPtr plan); +static void ri_PreparedPlanFreeUnused(void); +static void ResOwnerReleaseRIPlanExecution(Datum res); + +static const ResourceOwnerDesc ri_plan_execution_resowner_desc = +{ + .name = "RI plan execution", + .release_phase = RESOURCE_RELEASE_AFTER_LOCKS, + .release_priority = RELEASE_PRIO_FIRST, + .ReleaseResource = ResOwnerReleaseRIPlanExecution, + .DebugPrint = NULL /* the default message is fine */ +}; + static void ri_CheckTrigger(FunctionCallInfo fcinfo, const char *funcname, int tgkind); static RI_ConstraintInfo *ri_FetchConstraintInfo(Trigger *trigger, @@ -2724,11 +2775,17 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, * Set fire_triggers to false to ensure that AFTER triggers are queued in * the outer query's after-trigger context and fire after all RI updates * on the same row are complete, rather than immediately. + * + * Triggers fired by the query may reach ri_FetchPreparedPlan() for the + * same query key and find the plan invalid, so tell it that the plan is + * in use while it runs. */ + ri_PreparedPlanExecutionStarted(qplan); spi_result = SPI_execute_snapshot(qplan, vals, nulls, test_snapshot, crosscheck_snapshot, false, false, limit); + ri_PreparedPlanExecutionFinished(qplan); /* Restore UID and security context */ SetUserIdAndSecContext(save_userid, save_sec_context); @@ -3625,6 +3682,13 @@ ri_InitHashTables(void) ri_compare_cache = hash_create("RI compare cache", RI_INIT_QUERYHASHSIZE, &ctl, HASH_ELEM | HASH_BLOBS); + + ctl.keysize = sizeof(SPIPlanPtr); + ctl.entrysize = sizeof(RI_QueryPlanCacheExecutingRefCountEntry); + ri_query_plan_cache_executing_refcount = + hash_create("RI plan execution refcount", + RI_INIT_QUERYHASHSIZE, + &ctl, HASH_ELEM | HASH_BLOBS); } @@ -3671,11 +3735,13 @@ ri_FetchPreparedPlan(RI_QueryKey *key) /* * Otherwise we might as well flush the cached plan now, to free a little - * memory space before we make a new one. + * memory space before we make a new one. An RI check further up the + * stack may still be executing it, though, in which case it's freed only + * once that execution finishes. */ entry->plan = NULL; if (plan) - SPI_freeplan(plan); + ri_PreparedPlanReleaseASAP(plan); return NULL; } @@ -3710,6 +3776,166 @@ ri_HashPreparedPlan(RI_QueryKey *key, SPIPlanPtr plan) } +/* + * ri_PreparedPlanExecutionStarted - + * + * Record that ri_PerformCheck() is about to execute a saved plan, so that it + * is not freed under us. The execution is also registered with the current + * resource owner, which undoes this if the execution is aborted by an error. + * + * This is also where plans left over by aborted executions are freed, if any. + * The plan about to be executed cannot be one of them: those are no longer in + * the query hashtable, and their addresses cannot have been reused yet. + */ +static void +ri_PreparedPlanExecutionStarted(SPIPlanPtr plan) +{ + RI_QueryPlanCacheExecutingRefCountEntry *entry; + bool found; + + /* The plan came from ri_FetchPreparedPlan() or ri_PlanCheck() */ + Assert(ri_query_plan_cache_executing_refcount != NULL); + + ri_PreparedPlanFreeUnused(); + + ResourceOwnerEnlarge(CurrentResourceOwner); + + entry = (RI_QueryPlanCacheExecutingRefCountEntry *) + hash_search(ri_query_plan_cache_executing_refcount, + &plan, HASH_ENTER, &found); + if (!found) + { + entry->markedForDeletion = false; + entry->refcount = 0; + } + entry->refcount++; + + ResourceOwnerRemember(CurrentResourceOwner, PointerGetDatum(plan), + &ri_plan_execution_resowner_desc); +} + +/* + * ri_PreparedPlanExecutionFinished - + * + * Record that an execution started by ri_PreparedPlanExecutionStarted() has + * finished normally. If it was the last execution of a plan marked for + * deletion in the meantime, the plan is freed. + */ +static void +ri_PreparedPlanExecutionFinished(SPIPlanPtr plan) +{ + ResourceOwnerForget(CurrentResourceOwner, PointerGetDatum(plan), + &ri_plan_execution_resowner_desc); + ri_PreparedPlanDecrementRefCount(plan, true); +} + +/* + * ResOwnerReleaseRIPlanExecution - + * + * ResourceOwner callback, for an execution aborted by an error. We only drop + * the count here: a plan marked for deletion whose last execution this was is + * freed later by ri_PreparedPlanFreeUnused(), called by the next RI query + * execution or at the end of the transaction, rather than while releasing + * the aborted (sub)transaction's resources. + */ +static void +ResOwnerReleaseRIPlanExecution(Datum res) +{ + ri_PreparedPlanDecrementRefCount((SPIPlanPtr) DatumGetPointer(res), false); +} + +/* + * ri_PreparedPlanDecrementRefCount - + * + * Drop one execution of a plan, removing its entry once no execution is left. + * If the plan was marked for deletion, it is freed at that point if canFree; + * otherwise its entry is queued for ri_PreparedPlanFreeUnused(). + */ +static void +ri_PreparedPlanDecrementRefCount(SPIPlanPtr plan, bool canFree) +{ + RI_QueryPlanCacheExecutingRefCountEntry *entry; + + entry = (RI_QueryPlanCacheExecutingRefCountEntry *) + hash_search(ri_query_plan_cache_executing_refcount, + &plan, HASH_FIND, NULL); + Assert(entry != NULL); + Assert(entry->refcount > 0); + + entry->refcount--; + if (entry->refcount > 0) + return; + + if (!entry->markedForDeletion) + hash_search(ri_query_plan_cache_executing_refcount, + &plan, HASH_REMOVE, NULL); + else if (canFree) + { + hash_search(ri_query_plan_cache_executing_refcount, + &plan, HASH_REMOVE, NULL); + SPI_freeplan(plan); + } + else + dlist_push_head(&ri_plans_to_free, &entry->node); +} + +/* + * ri_PreparedPlanReleaseASAP - + * + * Free a saved plan that has just been removed from the query hashtable, or, + * if it is being executed, mark it for deletion so that the last execution + * to finish frees it. + */ +static void +ri_PreparedPlanReleaseASAP(SPIPlanPtr plan) +{ + RI_QueryPlanCacheExecutingRefCountEntry *entry; + + /* Called from ri_FetchPreparedPlan(), so the hashtables exist */ + Assert(ri_query_plan_cache_executing_refcount != NULL); + + entry = (RI_QueryPlanCacheExecutingRefCountEntry *) + hash_search(ri_query_plan_cache_executing_refcount, + &plan, HASH_FIND, NULL); + if (entry == NULL) + { + SPI_freeplan(plan); + return; + } + + Assert(entry->refcount > 0); + Assert(!entry->markedForDeletion); + entry->markedForDeletion = true; +} + +/* + * ri_PreparedPlanFreeUnused - + * + * Free the plans marked for deletion whose last execution was aborted by an + * error, as queued by ri_PreparedPlanDecrementRefCount(). + */ +static void +ri_PreparedPlanFreeUnused(void) +{ + dlist_mutable_iter iter; + + dlist_foreach_modify(iter, &ri_plans_to_free) + { + RI_QueryPlanCacheExecutingRefCountEntry *entry; + SPIPlanPtr plan; + + entry = dlist_container(RI_QueryPlanCacheExecutingRefCountEntry, + node, iter.cur); + plan = entry->plan; + Assert(entry->markedForDeletion && entry->refcount == 0); + + dlist_delete(iter.cur); + hash_search(ri_query_plan_cache_executing_refcount, + &plan, HASH_REMOVE, NULL); + SPI_freeplan(plan); + } +} + /* * ri_KeysEqual - * @@ -3996,6 +4222,9 @@ RI_FKey_trigger_type(Oid tgfoid) * other AtEOXact_* routines but is not used: the release is the same on * the commit and the abort path. * + * It also frees the RI plans that ri_PreparedPlanFreeUnused() hasn't freed + * yet; see RI_QueryPlanCacheExecutingRefCountEntry. + * * There is no AtEOSubXact_RI() counterpart. Nothing here is scoped to a * subtransaction: a detached FastPathMeta stays reachable from the dead * list whichever subtransaction detached it, and a check holding a pointer @@ -4013,4 +4242,13 @@ AtEOXact_RI(bool isCommit) MemoryContextDelete(dead->scratch_cxt); pfree(dead); } + + /* + * Likewise, free plans marked for deletion whose last execution was + * aborted. All subtransactions and resource owners have been released by + * now, so the hash table must end up empty. + */ + ri_PreparedPlanFreeUnused(); + Assert(ri_query_plan_cache_executing_refcount == NULL || + hash_get_num_entries(ri_query_plan_cache_executing_refcount) == 0); } diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index d001f56efae..bea4c4d5848 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2502,6 +2502,7 @@ RI_ConstraintInfo RI_FastPathState RI_QueryHashEntry RI_QueryKey +RI_QueryPlanCacheExecutingRefCountEntry RTEKind RTEPermissionInfo RWConflict -- 2.34.1
