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

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
>>
>