Re: Re: Re: [PATCH] Fix segmentation fault caused by reentrancy in RI_Fkey_cascade_del (ri_triggers.c)

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 (<
trakshanmishra477(at)gmail(dot)com>) 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 <
> lucas(dot)jeffrey(at)anachronics(dot)com> 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 (<
>> trakshanmishra477(at)gmail(dot)com>) 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 <
>>> trakshanmishra477(at)gmail(dot)com> 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
>>>>
>>>