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

Hi Lucas, Amit,

I ran Lucas's counterexample against Amit's v1, v1 plus the depth counter
from Amit's second mail, and Lucas's v8, all on master e75fcc9867f
(debugoptimized, cassert). To see the peak rather than the state after the
statement, dummy_bd reads pg_backend_memory_contexts right after its DELETE
FROM parent, while the outer cascade check is still running, and counts the
SPI Plan and CachedPlan* contexts.

rows v1 v1 + depth v8
1000 3024 / 7 MB 3024 / 7 MB 27 / 80 kB
5000 15024 / 34 MB 15024 / 34 MB 27 / 80 kB
20000 60024 / 137 MB 60024 / 137 MB

So Lucas is right about this case. The depth counter does drain once the
outer check returns (9 SPI plans left at the end of the statement), but the
peak is the same as v1.

I don't think that means we need the hash table though. What
ri_FetchPreparedPlan() needs to know is whether this particular plan is
being executed further up the stack, and the plans being executed are just
the ri_PerformCheck() frames, usually one or two. I tried that on top of
v1: ri_PerformCheck() pushes qplan on a small array before
SPI_execute_snapshot() and pops it after. ri_FetchPreparedPlan() frees the
stale plan right away unless it is on the array, and queues it otherwise.
After the pop, queued plans that are no longer on the array are freed. An
error can only skip a pop, so the depth can end up too high but never too
low, and every live frame's entry stays below it. The worst case is a plan
kept until AtEOXact_RI(), which resets the depth. No PG_TRY or resource
owner, so it would be the same on every branch.

With that:

- the counterexample stays at 27 contexts / 80 kB at 1000 and 5000 rows,
same as v8
- regress and isolation pass with Lucas's v8-0001 test added
- the fp_reentry test and the v8-0001 test under debug_discard_caches = 1
don't crash (neither do v1, v1 + depth or v8)
- Lucas's benchmark script, release builds, 6 rounds, 300k rows / 60k
roots, base is v1 (which has no per-row cost):

array v8
cascade_nochild -0.65% +2.36%
cascade_child +0.93% +4.41%
selfref_nested -0.11% +4.64%

An A/A run on the same machine gave -0.97% to -0.06%, so the array version
is within noise and v8 is a bit above it. My v8 numbers are somewhat higher
than Lucas's 1.3-2.8%, probably the laptop.

The diff against v1 is attached as a .txt, just to show the idea, not as a
patch for cfbot.

Regards,
Trakshan

On Tue, Oct 20, 2026 06:45 PM, Lucas Jeffrey <lucas(dot)jeffrey(at)anachronics(dot)com>
wrote:

> Hi Amit,
> I checked this snippet you have proposed:
>
> ri_check_depth++;
> spi_result = SPI_execute_snapshot(...);
> if (--ri_check_depth == 0 && ri_plan_dead_list != NIL)
> ri_FreeDeadPlans(); /* same drain AtEOXact_RI() does */
>
> AFAIK the problem with this approach of keeping a "global RI check
> depth count" is that nested checks,
> for instance the recursive DROP case which started this email thread,
> and a plan having deferred invalidation
> until "ri_check_depth == 0" doesn't fully escape your regression you
> pointed out, in which memory usage
> could skyrocket if continuous invalidations occur in nested plans.
>
> for instance, think about this pathological case:
>
> CREATE TABLE parent (id int PRIMARY KEY);
> CREATE TABLE child (id int PRIMARY KEY,
> parent_id int REFERENCES parent(id) ON DELETE
> CASCADE);
> CREATE ROLE ri_dummy;
>
> INSERT INTO parent SELECT g FROM generate_series(1, 500000) g;
> INSERT INTO child SELECT g, g FROM generate_series(1, 500000) g;
>
> CREATE FUNCTION child_bd() RETURNS trigger LANGUAGE plpgsql AS $$
> BEGIN
> -- alternate to ensure real catalog change and real invalidation
> IF OLD.id % 2 = 0 THEN
> GRANT SELECT ON child TO ri_dummy;
> ELSE
> REVOKE SELECT ON child FROM ri_dummy;
> END IF;
> RETURN OLD;
> END $$;
>
> CREATE TRIGGER child_bd BEFORE DELETE ON child
> FOR EACH ROW EXECUTE FUNCTION child_bd();
>
> CREATE TABLE outer_parent (id int PRIMARY KEY);
> CREATE TABLE dummy (
> id int PRIMARY KEY,
> op_id int REFERENCES outer_parent(id) ON DELETE CASCADE
> );
>
> CREATE FUNCTION dummy_bd() RETURNS trigger LANGUAGE plpgsql AS $$
> BEGIN
> -- Reached from INSIDE the outer_parent->dummy cascade RI check,
> -- so ri_check_depth is already >= 1 here and never returns to 0
> -- until this outer check finishes. Every parent/child cascade below
> -- runs nested under it, so the depth-counter drain never fires and
> -- all ~500k invalidated plans pile up until commit.
> DELETE FROM parent;
> RETURN OLD;
> END $$;
>
> CREATE TRIGGER dummy_bd BEFORE DELETE ON dummy
> FOR EACH ROW EXECUTE FUNCTION dummy_bd();
>
> INSERT INTO outer_parent VALUES (1);
> INSERT INTO dummy VALUES (1, 1);
>
> DELETE FROM outer_parent; -- cascade -> dummy_bd -> DELETE FROM
> parent, all at depth >= 1
>
> so, given this counterexample I think we cannot escape from
> refcounting the plans being executed
> if the intent is to free the invalidated plans ASAP.
>
>
> El mar, 20 oct 2026 a las 9:36, Amit Langote
> (<amitlangote09(at)gmail(dot)com>) escribió:
> >
> > On Tue, Oct 20, 2026 at 8:30 PM Amit Langote <amitlangote09(at)gmail(dot)com>
> wrote:
> > > On Sat, Oct 17, 2026 at 2:57 AM Lucas Jeffrey
> > > <lucas(dot)jeffrey(at)anachronics(dot)com> wrote:
> > > > Hi Tomas,
> > > >
> > > > Thanks for your reply. I attached a v8 with patches for all the
> supported
> > > > branches:
> > > >
> > > > - v8-0001/0002: master, no changes from v7, just rebased on current
> master.
> > > > - nocfbot-v8-0001/0002-PG19: same as master.
> > > > - nocfbot-v8-0001/0002-PG14 to PG18: same test as master, and the
> same fix
> > > > on the five branches, with two differences:
> > > >
> > > > 1) ResourceOwnerDesc doesn't exist before 17, so ri_PerformCheck()
> wraps
> > > > the execution in PG_TRY(), and the catch block drops the count
> of that
> > > > execution and re-throws. I used PG_TRY() on 17 and 18 too, so
> that
> > > > the five branches get the same patch. On master I kept the
> resource
> > > > owner, since SPI already pins the CachedPlan it executes that
> way, and
> > > > it warns if a code path ever misses the decrement.
> > > >
> > > > 2) AtEOXact_RI() doesn't exist before 19, so a plan whose freeing
> was
> > > > deferred and whose last execution was aborted is freed in the
> next call to
> > > > ri_PerformCheck().
> > > >
> > > > Without the fix, the test crashes the backend on 14, 15, 18, 19 and
> master.
> > > > On 16 and 17 it doesn't crash in my builds, although the plan is
> freed while
> > > > in use (I checked it with some logging), since it depends on how the
> freed
> > > > memory gets reused. With the fix, regress and isolation pass on all
> > > > branches.
> > > >
> > > > For the benchmark I used release builds of master with and without
> the
> > > > patch, one client, 10 rounds alternating both builds, and took the
> average:
> > > >
> > > > test case base patched
> diff
> > > > cascade, no children (1M) 4642 ms 4772 ms +2.8%
> > > > cascade, one child (1M) 8046 ms 8251 ms +2.6%
> > > > self-ref, nested (200k) 7168 ms 7258 ms +1.3%
> > > > self-ref, nested + inval (20k) crash 115705 ms
> > > >
> > > > The first two are the common case, a cascade with no nesting. The
> last two
> > > > are the case of the test: a BEFORE DELETE trigger on a
> self-referencing
> > > > table reenters the cascade, so the refcount table is not empty and
> > > > ri_PreparedPlanFreeDeferred() scans it on every nested call; in the
> last one
> > > > the trigger also invalidates the plan, which is the crash without
> the fix.
> > > >
> > > > The setup minimizes I/O (unlogged tables, fsync and autovacuum off)
> to focus
> > > > on the cost of the patched code, so with real I/O the difference
> should be
> > > > much smaller. The scripts are attached.
> > > >
> > > > The scripts used for benchmarking are attached.
> > >
> > > Thanks for all your work on this, and for notifying me about this on
> > > the thread where I posted a patch for basically the same issue, found
> > > by the buildfarm [1].
> > >
> > > I’m attaching that patch here too, since it takes a different
> > > approach. It defers freeing an invalid SPI plan to end of transaction
> > > by collecting such plans in a list that AtEOXact_RI() frees, much like
> > > 170c9344def does for FastPathMeta objects whose immediate freeing
> > > caused the same kind of problem. Your 0001 test passes with it. It’s
> > > also the same patch across every branch. The only variation is that
> > > branches before 19 need AtEOXact_RI() added, with just this cleanup in
> > > it, and called from the same three places in xact.c.
> > >
> > > Reading your 0002, if I understand it right, the refcounting allows
> > > you to free a detached plan as soon as its last execution finishes
> > > rather than at commit as with my patch. The hash table, the resource
> > > owner callback (PG_TRY on 14-16) are there to keep that count correct
> > > across errors and subtransaction aborts. My hunch is that this
> > > approach requires more machinery than freeing a plan a bit earlier is
> > > worth. This only matters when one transaction invalidates and replans
> > > the same RI plan many times, which doesn’t seem like a common
> > > scenario. The cost of waiting until commit in that case is memory
> > > usage. There’s also some CPU cost as the patch stands, because the
> > > detached SPI plans’ CachedPlanSources stay on the plan cache’s
> > > saved-plan list until commit, so later invalidations in the
> > > transaction have to rescan them. The latter is easy to avoid by
> > > unlinking them from that list when the SPI plan is detached, which I’d
> > > do in a follow-up if we proceed this way.
> > >
> > > I’m also not keen on what this adds to every ri_PerformCheck():
> > >
> > > @@ -2719,11 +2764,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);
> > >
> > > That’s two hash lookups and a resource owner remember/forget per row
> > > (a PG_TRY per row on 14-16), paid by every FK check to handle a case
> > > that almost none of them are in. Your numbers put it at 2-3%, IIUC,
> > > which isn’t large, but the deferral approach doesn’t touch this path
> > > at all.
> >
> > One more thought on the trade-off, against my own patch.
> >
> > Deferring every free to the end of the transaction creates a cliff as
> > Tomas would call it. A transaction that invalidates and replans the
> > RI plan many times accumulates that many SPI plans and the
> > CachedPlanSources they point to, until commit. That’s not hard to
> > construct, and it hurts most where the plan is big, say a PK with
> > thousands of partitions where plancache went generic. Today’s code
> > frees those promptly, and so does Lucas's patch; mine would be a
> > regression there. I’ll also concede the per-row cost point:
> > GetCachedPlan() already does a fair amount of bookkeeping on that path
> > for every row, so the system accepts such costs for robustness.
> >
> > I think my patch can get the same bound without the refcounting,
> > though. As posted, ri_FetchPreparedPlan() queues every invalid plan
> > until commit because it can’t tell whether a check up the stack is
> > executing it. A depth counter around SPI_execute_snapshot() in
> > ri_PerformCheck() fixes both halves: when it’s zero at detach time,
> > free immediately as today; and when it drops back to zero after an
> > execution, free whatever was queued meanwhile, so nothing outlives the
> > outermost check that was running, like this:
> >
> > ri_check_depth++;
> > spi_result = SPI_execute_snapshot(...);
> > if (--ri_check_depth == 0 && ri_plan_dead_list != NIL)
> > ri_FreeDeadPlans(); /* same drain AtEOXact_RI() does */
> >
> > An error inside the query can only leave the counter too high, which
> > just defers to AtEOXact_RI() for the rest of that transaction, so it
> > needs no PG_TRY or resource owner. Maybe I should try that.
> >
> > --
> > Thanks, Amit Langote
>
>
>