Hi Lucas, Thanks for the rebase. v5 is a bit more than a rebase, though, so I went through it again rather than just checking that it applies.
Test environment: master @ 85adc584e2b, plus v5-0001 and v5-0002 Linux x86_64, gcc 15.2.0 meson, --buildtype=debug -Dcassert=true == What changed == 425daf545d9 (Remove batching from RI fast-path checks) removed AtEOSubXact_RI(), which is where v4 freed the plans whose last execution was aborted with a subtransaction. In v5 those plans are now freed at the start of the next ri_PreparedPlanExecutionStarted(), with AtEOXact_RI() as the backstop. The rest of 0002 and all of 0001 are unchanged apart from context. I think the new placement is safe. A plan on ri_plans_to_free has refcount 0, so no frame up the stack is executing it. It has already been removed from ri_query_cache, so nothing can fetch it again. And it still has its entry in the refcount table until it is freed, so its address cannot be handed to a new plan before then. The cost on the normal path is one empty-list check per RI query. == Testing == Both patches apply cleanly with no whitespace errors, and the build has no warnings. Full "meson test" with v5: 360 ok, 0 failed, 50 skipped. With only v5-0001 applied, the new test still segfaults the backend on the cascade, and foreign_key plus the tests after it fail, as with v4. To see which path frees each plan, I added temporary LOG lines where a plan is marked for deletion, queued, and freed. Results: - The DO block in the test (10 subtransaction aborts in one transaction): 10 plans marked, 10 queued, 9 freed by the next iteration's RI query and the last one by AtEOXact_RI() at commit. Plan addresses do get reused across iterations, but only after the earlier plan with that address has been freed. - Top-level abort, 50 times, followed by an invalidation and a successful cascade: 50 queued, all 50 freed by AtEOXact_RI() on the abort path. No crash and no assertion failure. - SAVEPOINT / ROLLBACK TO in a transaction block, then COMMIT with no further RI query: the plan is freed at commit, and the Assert that the table is empty at transaction end holds. == Comments == 1. The comments and the commit message say the deferred plans are freed by "the next RI query execution". Strictly, it is the next RI query that goes through ri_PerformCheck(). Checks that take the fast path don't call ri_PreparedPlanExecutionStarted(), so after a savepoint rollback followed only by plain FK inserts, the plan waits for AtEOXact_RI(). That is harmless, but "the next ri_PerformCheck() call" would describe it more exactly. 2. The test still covers only the subtransaction case. The top-level abort path, where AtEOXact_RI() frees the plan after AtEOXact_SPI() has run, is still something I ran by hand rather than something the test checks. As before, I would not hold the patch for it. == Summary == The rebased fix looks right to me, and the nits from my last mail are gone. I am leaving the entry at Ready for Committer. Regards, Trakshan Mishra On Mon, Oct 05, 2026 08:01 PM, Lucas Jeffrey <[email protected]> wrote: > 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%2BNC-XX4NKn4K6h8NrwA@ >> mail.gmail.com&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%2BNC-XX4NKn4K6h8NrwA@ >>>> mail.gmail.com&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 >>>>> >>>>
