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

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