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

Hi hackers,

We found a bug where executing a DELETE on a self-referential table that
fires triggers can cause a segmentation fault. This is due to a
*use-after-free* of a Postgres plan generated by the referential
integrity module (ri_triggers.c, RI_FKey_cascade_del). The crash occurs
if the Postgres plancache is invalidated (ResetPlanCache) during the
execution of a reentrant RI trigger.

A reentrant RI_FKey_cascade_del can occur if a table is self-referential
(i.e., it has a foreign key referencing its own primary key) and has BEFORE
DELETE triggers that delete rows from that same table.

-

*The first patch* adds a test case that reproduces the segmentation
fault. The crash itself happens in _SPI_execute_plan, but the root
cause is that the plan being executed was prematurely freed by the RI
module.
-

*The second patch* fixes ri_triggers.c by introducing reentrancy
guards, which maintain a reference count of plans in execution to prevent
them from being freed while active.

Feedback and reviews are welcome.

Best regards,

Lucas Jeffrey

Hi Luqui,

On 2026-May-29, Lucas Jeffrey wrote:

We found a bug where executing a DELETE on a self-referential table that
fires triggers can cause a segmentation fault. This is due to a
*use-after-free* of a Postgres plan generated by the referential

integrity

module (ri_triggers.c, RI_FKey_cascade_del). The crash occurs if the
Postgres plancache is invalidated (ResetPlanCache) during the execution

of

a reentrant RI trigger.

I confirm that this causes a crash. I'm really surprised that this kind
of behavior has survived this long.

However, while I'm not saying that we absolutely shouldn't fix this, I
think it's wrong usage to have BEFORE DELETE triggers delete other rows.
It's generally possible to have other BEFORE triggers after those ones,
that modify the row being deleted in a way that would make that other
trigger delete some _other_ row instead. As I recall, BEFORE triggers
are supposed to change row-local state only; any external state
(deletions or updates in other rows or other tables) should occur in
AFTER triggers, once the state from BEFORE triggers has quiesced.

The docs have this text:

Typically, row-level BEFORE triggers are used for checking or
modifying the data that will be inserted or updated. For example, a
BEFORE trigger might be used to insert the current time into a
timestamp column, or to check that two elements of the row are
consistent. Row-level AFTER triggers are most sensibly used to
propagate the updates to other tables, or make consistency checks
against other tables. The reason for this division of labor is that
an AFTER trigger can be certain it is seeing the final value of the
row, while a BEFORE trigger cannot; there might be other BEFORE
triggers firing after it. [...]
https://www.postgresql.org/docs/18/trigger-definition.html

Would changing you row-deleting BEFORE DELETE triggers to AFTER DELETE
triggers also fix your issue?

Thanks,

--
Álvaro Herrera 48°01'N 7°57'E —
https://www.EnterpriseDB.com/

With after triggers the crash does't reproduce because they don't create
reentrancy in RI_Fkey_cascade_del.
There are use cases in which a before delete trigger could want to delete
other rows, for example if you have a function that accesses the row by id
to free some external resource or mark something in other tables, you need
the row to be alive, that means it can't be an after trigger, and nothing
prevents you for using before triggers to do cleanup.
Also, you don't normally delete rows recursively from a self referential
table like the naive example in the test case, you normally want to do
something like table inheritance, where 2 tables inherit from a parent
table, and one references the other one, it is self-referential with
respect to the parent.

If BEFORE DELETE triggers should not delete rows, i think postgres should
explicitly throw a clean exception, something like "ERROR: Cannot delete
rows in before delete trigger".
Else, we should fix this bug.
Ultimately, I think the postgres team should decide which of the 2
approaches to take.

I can give time and work to fix the segfault.

Thanks,

El vie, 29 may 2026 a las 18:41, Álvaro Herrera (<alvherre@kurilemu.de>)
escribió:

Hi Luqui,

On 2026-May-29, Lucas Jeffrey wrote:

We found a bug where executing a DELETE on a self-referential table that
fires triggers can cause a segmentation fault. This is due to a
*use-after-free* of a Postgres plan generated by the referential

integrity

module (ri_triggers.c, RI_FKey_cascade_del). The crash occurs if the
Postgres plancache is invalidated (ResetPlanCache) during the execution

of

a reentrant RI trigger.

I confirm that this causes a crash. I'm really surprised that this kind
of behavior has survived this long.

However, while I'm not saying that we absolutely shouldn't fix this, I
think it's wrong usage to have BEFORE DELETE triggers delete other rows.
It's generally possible to have other BEFORE triggers after those ones,
that modify the row being deleted in a way that would make that other
trigger delete some _other_ row instead. As I recall, BEFORE triggers
are supposed to change row-local state only; any external state
(deletions or updates in other rows or other tables) should occur in
AFTER triggers, once the state from BEFORE triggers has quiesced.

The docs have this text:

Typically, row-level BEFORE triggers are used for checking or
modifying the data that will be inserted or updated. For example, a
BEFORE trigger might be used to insert the current time into a
timestamp column, or to check that two elements of the row are
consistent. Row-level AFTER triggers are most sensibly used to
propagate the updates to other tables, or make consistency checks
against other tables. The reason for this division of labor is that
an AFTER trigger can be certain it is seeing the final value of the
row, while a BEFORE trigger cannot; there might be other BEFORE
triggers firing after it. [...]
https://www.postgresql.org/docs/18/trigger-definition.html

Would changing you row-deleting BEFORE DELETE triggers to AFTER DELETE
triggers also fix your issue?

Thanks,

--
Álvaro Herrera 48°01'N 7°57'E —
https://www.EnterpriseDB.com/

Hi Alvaro,

On Fri, Aug 14, 2026 at 21:37 Álvaro Herrera <alvherre@kurilemu.de> wrote:

[offlist] Hi Amit,

Lucas reported this crash in the RI code two months ago. I have not had
a chance to review it and probably won't yet for another month, so if
you have a moment, I think you may have the code fresher in your head to
better assess whether his proposed fix is the most appropriate one. If
you don't, no problem, I will probably spend some time on this
eventually. We may not want to leave such a trivially invoked crasher
in the tree for much longer -- it's probably security-relevant.

/messages/by-id/CAGHzy7S0XfXdK3amd5c4QhpS1o9dySVzPr+-d9ohaE4iV_vNog@mail.gmail.com

Ok, I will take a look next week after I’ve finished dealing with all the
reported RI fastpath issues.

- Amit

On 2026-Jun-01, Lucas Jeffrey wrote:

Hi hackers
I detected some failing tests and I created a new version of the patch

that

fixes those errors in regression tests.

El vie, 29 may 2026 a las 12:32, Lucas Jeffrey (<
lucas.jeffrey@anachronics.com>) escribió:

Hi hackers,

We found a bug where executing a DELETE on a self-referential table

that

fires triggers can cause a segmentation fault. This is due to a
*use-after-free* of a Postgres plan generated by the referential
integrity module (ri_triggers.c, RI_FKey_cascade_del). The crash

occurs

if the Postgres plancache is invalidated (ResetPlanCache) during the
execution of a reentrant RI trigger.

A reentrant RI_FKey_cascade_del can occur if a table is

self-referential

(i.e., it has a foreign key referencing its own primary key) and has

BEFORE

DELETE triggers that delete rows from that same table.

-

*The first patch* adds a test case that reproduces the segmentation
fault. The crash itself happens in _SPI_execute_plan, but the root
cause is that the plan being executed was prematurely freed by the

RI

module.
-

*The second patch* fixes ri_triggers.c by introducing reentrancy
guards, which maintain a reference count of plans in execution to

prevent

them from being freed while active.

Feedback and reviews are welcome.

Best regards,

Lucas Jeffrey

From 534002b0999c6e620c055f3e52634937c4693849 Mon Sep 17 00:00:00 2001
From: luquijeffrey <lucas.jeffrey@anachronics.com>
Date: Fri, 29 May 2026 12:23:38 -0300
Subject: [PATCH 1/2]

=?UTF-8?q?Add=20test=20case=20that=20triggers=20self?=

=?UTF-8?q?=E2=80=91referencing=20table=20crash=20(patch1)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
.../isolation/expected/ri-cascade-del.out | 27 ++++++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/ri-cascade-del.spec | 83 +++++++++++++++++++
3 files changed, 111 insertions(+)
create mode 100644 src/test/isolation/expected/ri-cascade-del.out
create mode 100644 src/test/isolation/specs/ri-cascade-del.spec

diff --git a/src/test/isolation/expected/ri-cascade-del.out

b/src/test/isolation/expected/ri-cascade-del.out

new file mode 100644
index 00000000000..bd7ae0b0b5a
--- /dev/null
+++ b/src/test/isolation/expected/ri-cascade-del.out
@@ -0,0 +1,27 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s2_lock s1_delete s2_inval s2_unlock
+step s2_lock: SELECT pg_advisory_lock(0);
+pg_advisory_lock
+----------------
+
+(1 row)
+
+step s1_delete: DELETE FROM crash_reentrancia_tabla_autoreferencial

WHERE id = 1; <waiting ...>

+step s2_inval:
+  DO $$
+  BEGIN
+    FOR i IN 1..1000 LOOP
+      EXECUTE 'CREATE TEMPORARY TABLE t_temp_inval_(id INTEGER PRIMARY

KEY)';

+      EXECUTE 'DROP TABLE t_temp_inval_';
+    END LOOP;
+  END;
+  $$;
+
+step s2_unlock: SELECT pg_advisory_unlock(0);
+pg_advisory_unlock
+------------------
+t
+(1 row)
+
+step s1_delete: <... completed>
diff --git a/src/test/isolation/isolation_schedule

b/src/test/isolation/isolation_schedule

index 1578ba191c8..39a0a1ee792 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -12,6 +12,7 @@ test: project-manager
test: classroom-scheduling
test: total-cash
test: referential-integrity
+test: ri-cascade-del
test: ri-trigger
test: partial-index
test: two-ids
diff --git a/src/test/isolation/specs/ri-cascade-del.spec

b/src/test/isolation/specs/ri-cascade-del.spec

new file mode 100644
index 00000000000..c412ec2c772
--- /dev/null
+++ b/src/test/isolation/specs/ri-cascade-del.spec
@@ -0,0 +1,83 @@
+# Setup for referential integrity crash test
+setup
+{
+    CREATE TABLE crash_reentrancia_tabla_autoreferencial (
+        id int PRIMARY KEY,
+        nombre text,
+        padre_id int REFERENCES

crash_reentrancia_tabla_autoreferencial(id) ON DELETE CASCADE

+    );
+
+    CREATE TABLE crash_reentrancia_segunda_tabla (
+        id    int PRIMARY KEY,
+        valor text
+    );
+
+    CREATE OR REPLACE FUNCTION crash_reentrancia_before_delete()
+    RETURNS trigger AS $$
+    DECLARE
+        v_valor text;
+    BEGIN
+        IF OLD.id % 2 = 1 THEN
+            RETURN OLD;
+        END IF;
+
+        -- Wait for S2 to finish flooding the invalidation message

queue

+        IF OLD.id = 2 THEN
+            PERFORM pg_advisory_lock(0);
+            PERFORM pg_advisory_unlock(0);
+        END IF;
+
+        IF OLD.id > 4 THEN
+            -- This opens the table and forces processing of pending

inval messages

+ SELECT valor INTO v_valor FROM

crash_reentrancia_segunda_tabla WHERE id = OLD.id;

+        END IF;
+
+        DELETE FROM crash_reentrancia_tabla_autoreferencial WHERE

padre_id = OLD.id;

+        RETURN OLD;
+    END;
+    $$ LANGUAGE plpgsql;
+
+    CREATE TRIGGER trg_crash_reentrancia_before_delete
+        BEFORE DELETE ON crash_reentrancia_tabla_autoreferencial
+        FOR EACH ROW EXECUTE FUNCTION

crash_reentrancia_before_delete();

+
+    INSERT INTO crash_reentrancia_tabla_autoreferencial VALUES (1,

'A', NULL);

+ INSERT INTO crash_reentrancia_tabla_autoreferencial VALUES (2,

'B', 1);

+ INSERT INTO crash_reentrancia_tabla_autoreferencial VALUES (3,

'C', 2);

+ INSERT INTO crash_reentrancia_tabla_autoreferencial VALUES (4,

'D', 3);

+ INSERT INTO crash_reentrancia_tabla_autoreferencial VALUES (5,

'E', 4);

+ INSERT INTO crash_reentrancia_tabla_autoreferencial VALUES (6,

'F', 5);

+
+    INSERT INTO crash_reentrancia_segunda_tabla VALUES
+        (1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (5, 'e'), (6, 'f');
+}
+
+teardown
+{
+    DROP TRIGGER trg_crash_reentrancia_before_delete ON

crash_reentrancia_tabla_autoreferencial;

+    DROP FUNCTION crash_reentrancia_before_delete CASCADE;
+    DROP TABLE crash_reentrancia_tabla_autoreferencial CASCADE;
+    DROP TABLE crash_reentrancia_segunda_tabla CASCADE;
+}
+
+session s1
+step s1_delete { DELETE FROM crash_reentrancia_tabla_autoreferencial

WHERE id = 1; }

+
+session s2
+step s2_lock { SELECT pg_advisory_lock(0); }
+step s2_inval {
+  DO $$
+  BEGIN
+    FOR i IN 1..1000 LOOP
+      EXECUTE 'CREATE TEMPORARY TABLE t_temp_inval_(id INTEGER PRIMARY

KEY)';

+      EXECUTE 'DROP TABLE t_temp_inval_';
+    END LOOP;
+  END;
+  $$;
+}
+step s2_unlock { SELECT pg_advisory_unlock(0); }
+
+# Execution permutation
+# S2 locks -> S1 blocks on S2 -> S2 forces inval queue overflow -> S2

unlocks

+# S1 awakens -> S1 forces table_open -> invalidation processed ->

segfault!

+permutation s2_lock s1_delete s2_inval s2_unlock
--
2.34.1

From e8dfb066e7d49cc1550969871b43fac9ed01c04c Mon Sep 17 00:00:00 2001
From: luquijeffrey <lucas.jeffrey@anachronics.com>
Date: Fri, 29 May 2026 12:23:54 -0300
Subject: [PATCH 2/2]

=?UTF-8?q?Fix=20crash=20of=20self=E2=80=91referencing?=

=?UTF-8?q?=20tables=20with=20delete=20triggers?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
src/backend/utils/adt/ri_triggers.c | 130 +++++++++++++++++++++++++++-
1 file changed, 129 insertions(+), 1 deletion(-)

diff --git a/src/backend/utils/adt/ri_triggers.c

b/src/backend/utils/adt/ri_triggers.c

index dc89c686394..425e7f9e1eb 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -251,12 +251,25 @@ typedef struct RI_FastPathEntry
int                     batch_count;
} RI_FastPathEntry;

+/*
+ * RI_QueryPlanCacheExecutingRefCountEntry
+ *
+ * Entry to track the number of times a prepared plan is being

executed.

+ */
+typedef struct RI_QueryPlanCacheExecutingRefCountEntry
+{
+     SPIPlanPtr  plan;
+     bool        markedForDeletion; /* If true, it will be freed when

refcount reaches 0 */

+ uint32 refcount; /* number of times this plan is being

executed (can be more than 1 if reentrant) */

+} RI_QueryPlanCacheExecutingRefCountEntry;
+
/*
* Local data
*/
static HTAB *ri_constraint_cache = NULL;
static HTAB *ri_query_cache = NULL;
static HTAB *ri_compare_cache = NULL;
+static HTAB *ri_query_plan_cache_executing_refcount = NULL;
static dclist_head ri_constraint_cache_valid_list;

static HTAB *ri_fastpath_cache = NULL;
@@ -295,6 +308,11 @@ static SPIPlanPtr ri_FetchPreparedPlan(RI_QueryKey

*key);

static void ri_HashPreparedPlan(RI_QueryKey *key, SPIPlanPtr plan);
static RI_CompareHashEntry *ri_HashCompareOp(Oid eq_opr, Oid typeid);

+/* Reentrancy protection: prevent segfault on deleting a plan in

execution if invalidated during reentrant RI check. */

+static void ri_PreparedPlanExecutionStarted(SPIPlanPtr plan);
+static void ri_PreparedPlanExecutionFinished(SPIPlanPtr plan);
+static void ri_PreparedPlanReleaseASAP(SPIPlanPtr plan);
+
static void ri_CheckTrigger(FunctionCallInfo fcinfo, const char

*funcname,

int tgkind);
static RI_ConstraintInfo *ri_FetchConstraintInfo(Trigger *trigger,
@@ -2724,6 +2742,9 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
save_sec_context |

SECURITY_LOCAL_USERID_CHANGE |

SECURITY_NOFORCE_RLS);

+     /* Increase plan use count for reentrancy protection. */
+     ri_PreparedPlanExecutionStarted(qplan);
+
/*
* Finally we can run the query.
*
@@ -2735,6 +2756,9 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,

vals, nulls,

test_snapshot, crosscheck_snapshot,

false, false, limit);

+
+     /* Decrease plan use count. this call can free the plan if it was

invalidated and no longer in use. */

+ ri_PreparedPlanExecutionFinished(qplan);

/* Restore UID and security context */
SetUserIdAndSecContext(save_userid, save_sec_context);
@@ -3762,6 +3786,12 @@ ri_InitHashTables(void)
ri_compare_cache = hash_create("RI compare cache",

RI_INIT_QUERYHASHSIZE,

&ctl,

HASH_ELEM | HASH_BLOBS);

+
+     ctl.keysize = sizeof(SPIPlanPtr);
+     ctl.entrysize = sizeof(RI_QueryPlanCacheExecutingRefCountEntry);
+     ri_query_plan_cache_executing_refcount = hash_create("RI plan

cache execution refcount",

+

RI_INIT_QUERYHASHSIZE,

+ &ctl,

HASH_ELEM | HASH_BLOBS);

}

@@ -3812,7 +3842,7 @@ ri_FetchPreparedPlan(RI_QueryKey *key)
*/
entry->plan = NULL;
if (plan)
-             SPI_freeplan(plan);
+             ri_PreparedPlanReleaseASAP(plan);

return NULL;
}
@@ -3847,6 +3877,104 @@ ri_HashPreparedPlan(RI_QueryKey *key,

SPIPlanPtr plan)

}

+static void
+ri_PreparedPlanExecutionStarted(SPIPlanPtr plan)
+{
+     RI_QueryPlanCacheExecutingRefCountEntry* entry;
+     bool found;
+
+     if (!ri_query_plan_cache_executing_refcount)
+             ri_InitHashTables();
+
+     entry = (RI_QueryPlanCacheExecutingRefCountEntry*)

hash_search(ri_query_plan_cache_executing_refcount, &plan, HASH_ENTER,
&found);

+     if (found)
+             entry->refcount++;
+     else
+     {
+             entry->refcount = 1;
+             entry->markedForDeletion = false;
+     }
+}
+
+static void
+ri_PreparedPlanExecutionFinished(SPIPlanPtr plan)
+{
+     RI_QueryPlanCacheExecutingRefCountEntry* entry;
+     bool found;
+
+     if (!ri_query_plan_cache_executing_refcount)
+             return;
+
+     entry = (RI_QueryPlanCacheExecutingRefCountEntry*)

hash_search(ri_query_plan_cache_executing_refcount, &plan, HASH_FIND,
&found);

+     if (!entry)
+             return;
+
+     entry->refcount--;
+     if (entry->refcount == 0 && entry->markedForDeletion)
+     {
+             // Remove the entry
+             hash_search(ri_query_plan_cache_executing_refcount,

&plan, HASH_REMOVE, NULL);

+             SPI_freeplan(plan);
+     }
+}
+
+/*
+ * ri_PreparedPlanReleaseASAP
+ *
+ * Release a cached SPI plan, or mark it for deferred deletion if it
+ * is currently in use.
+ *
+ * If the plan has an active executing-refcount entry with refcount >

0,

+ * we cannot free it immediately.  Instead we mark it for deletion so
+ * that the last executor to finish will free it.
+ */
+static void
+ri_PreparedPlanReleaseASAP(SPIPlanPtr plan)
+{
+     RI_QueryPlanCacheExecutingRefCountEntry *entry;
+     bool            found;
+
+     /*
+      * If there is no executing-refcount hash table, it's not in use,
+      * so we can free immediately.
+      */
+     if (!ri_query_plan_cache_executing_refcount)
+     {
+             SPI_freeplan(plan);
+             return;
+     }
+
+     entry = (RI_QueryPlanCacheExecutingRefCountEntry *)
+             hash_search(ri_query_plan_cache_executing_refcount,
+                                     &plan, HASH_FIND, &found);
+
+     /*
+      * No refcount entry means the plan is not being executed; free

it now.

+      */
+     if (!found)
+     {
+             SPI_freeplan(plan);
+             return;
+     }
+
+     /*
+      * If the refcount has dropped to zero, remove the entry and free

the

+ * plan. Otherwise mark it for deletion once the last executor

finishes.

+      */
+     if (entry->refcount == 0)
+     {
+             hash_search(ri_query_plan_cache_executing_refcount,
+                                     &plan, HASH_REMOVE, NULL);
+             SPI_freeplan(plan);
+             return;
+     }
+
+     /*
+      * Mark for deletion once the last executor finishes.
+      */
+     entry->markedForDeletion = true;
+}
+
/*
* ri_KeysEqual -
*
--
2.34.1

--
Álvaro Herrera 48°01'N 7°57'E —
https://www.EnterpriseDB.com/
"La espina, desde que nace, ya pincha" (Proverbio africano)

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

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&amp;source=gmail&amp;ust=1790347576108000&amp;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@gmail.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

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@gmail.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@gmail.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

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&amp;source=gmail&amp;ust=1790398611321000&amp;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.jeffrey@anachronics.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@gmail.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&amp;source=gmail&amp;ust=1790347576108000&amp;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@gmail.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

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@gmail.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.jeffrey@anachronics.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@gmail.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@gmail.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

Hi,

On 10/5/26 16:31, Lucas Jeffrey 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

I accidentally ran into this issue a couple days ago, unaware of this
thread. I really like the approach of the proposed fix, refcounting
seems like a sound approach and we already useit elsewhere to track
similar objects. I've only done a quick revie, but it seems much cleaner
than my PoC fix.

A couple (very cosmetic) review comments:

1) The naming seems a bit inconsistent

static void ri_PreparedPlanReleaseASAP(SPIPlanPtr plan);
static void ri_PreparedPlanFreeUnused(void);

Why release vs. free? And the flag is called markedForDeletion, so
that's a third term. Also, I wouldn't use the ASAP in function name.

The SPI function is called SPI_freeplan, so what about using "free"
everywhere?

static void ri_PreparedPlanFree(SPIPlanPtr plan);
static void ri_PreparedPlanFreeUnused(void);

Although, I now realize we're not necessarily freeing the plan, so maybe
this is not an improvement ...

2) Is ri_plans_to_free necessary?

IIUC those plans are still in the hashtable, right? Why not to simply
scan the hashtable, and free the plans there? That'd mean we don't need
a separate list that can get out of sync. Maybe we expect the hash table
to be large, in which case this could be expensive. But is it? I don't
see any such argument in this thread.

If the list is necessary, maybe ri_PreparedPlanFreeUnused() should at
least have an assert that the hash_search() found a matching entry?

3) What about backpatching?

AFAIK this affects all backbranches, not just master, right? So we
should aim to fix the backbranches too. Do you think this refcounting
fix is backpatchable (it seems like it'd be OK), or do you have an idea
for a less invasive fix?

regards

--
Tomas Vondra

Hi Tomas,

Thanks for your review. I attached a v6, rebased on current master and
with some changes handling some items in your review.

1) Agreed on the naming, there were three different terms for the same
thing. I used "free" everywhere, but since the plan is not always freed
immediately, the names now say that explicitly:

ri_PreparedPlanReleaseASAP() -> ri_PreparedPlanFreeOrDefer()
markedForDeletion -> freeDeferred
ri_PreparedPlanFreeUnused() -> ri_PreparedPlanFreeDeferred()

The comments and the commit message were updated to use the same
terms. The only remaining "Release" is ResOwnerReleaseRIPlanExecution(),
which follows the naming convention of the other resource owner
callbacks (ResOwnerReleaseBuffer(), ResOwnerReleaseCachedPlan(), etc.),
so I kept it.

2) Agreed, the "free later list" was not necessary, and I removed it. To be
precise, those plans are no longer in ri_query_cache, since
ri_FetchPreparedPlan() removes them from it, but their entries remain in
the refcount hash table, ri_query_plan_cache_executing_refcount, with a
zero count. ri_PreparedPlanFreeDeferred() now scans that table instead.

That table only has entries while RI queries are being executed, so it
stays small. However, ri_PreparedPlanFreeDeferred() is called on every
ri_PerformCheck(), so it returns immediately when the table is empty,
which is the case unless an RI query is already being executed or a
deferred plan is pending. This way the common path does not pay for a
hash_seq_search() over all the buckets. I also reduced the initial size
of the table to 16, instead of RI_INIT_QUERYHASHSIZE (256).

3) Yes, all supported branches are affected. However, this fix cannot be
backpatched as is: the ResourceOwnerDesc API exists only since 17, and
AtEOXact_RI() is new in 19. I would propose to agree on the fix for
master first, and then submit a separate, less invasive patch for 14-18,
using only the APIs and functions available in those branches.

Regards,
Lucas Jeffrey

El mar, 6 oct 2026 a las 15:26, Tomas Vondra (<tomas@vondra.me>) escribió:

Hi,

On 10/5/26 16:31, Lucas Jeffrey 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

I accidentally ran into this issue a couple days ago, unaware of this
thread. I really like the approach of the proposed fix, refcounting
seems like a sound approach and we already useit elsewhere to track
similar objects. I've only done a quick revie, but it seems much cleaner
than my PoC fix.

A couple (very cosmetic) review comments:

1) The naming seems a bit inconsistent

static void ri_PreparedPlanReleaseASAP(SPIPlanPtr plan);
static void ri_PreparedPlanFreeUnused(void);

Why release vs. free? And the flag is called markedForDeletion, so
that's a third term. Also, I wouldn't use the ASAP in function name.

The SPI function is called SPI_freeplan, so what about using "free"
everywhere?

static void ri_PreparedPlanFree(SPIPlanPtr plan);
static void ri_PreparedPlanFreeUnused(void);

Although, I now realize we're not necessarily freeing the plan, so maybe
this is not an improvement ...

2) Is ri_plans_to_free necessary?

IIUC those plans are still in the hashtable, right? Why not to simply
scan the hashtable, and free the plans there? That'd mean we don't need
a separate list that can get out of sync. Maybe we expect the hash table
to be large, in which case this could be expensive. But is it? I don't
see any such argument in this thread.

If the list is necessary, maybe ri_PreparedPlanFreeUnused() should at
least have an assert that the hash_search() found a matching entry?

3) What about backpatching?

AFAIK this affects all backbranches, not just master, right? So we
should aim to fix the backbranches too. Do you think this refcounting
fix is backpatchable (it seems like it'd be OK), or do you have an idea
for a less invasive fix?

regards

--
Tomas Vondra

Hi Lucas,

Thanks for v6. I retested it, with most of the attention on the new
deferred-free path, since that is the part that changed.

Test environment:
master @ 1b5dd3a24a4, plus v6-0001 and v6-0002
Linux x86_64, gcc 15.2.0
meson, --buildtype=debug -Dcassert=true

== Testing ==

Both patches apply cleanly with no whitespace errors, and the build has
no warnings.

Full "meson test" with v6: 362 ok, 0 failed, 50 skipped.

With only v6-0001 applied, the new test still segfaults the backend on
the cascade, and foreign_key plus the tests after it fail (52 in
total), as with v5.

As last time, I added temporary LOG lines where a plan is deferred,
left pending after an abort, and freed, to see which path frees each
plan:

- The DO block in the test: 12 plans deferred, 10 left pending by
the aborted executions. 9 of those are freed by the next
iteration's RI query and the last one by AtEOXact_RI() at commit.
The other 2 are freed when their last execution finishes normally.

- Top-level abort, 50 times, then an invalidation and a successful
cascade: all 50 pending plans are freed by AtEOXact_RI() on the
abort path. No crash and no assertion failure.

- SAVEPOINT / ROLLBACK TO, then COMMIT with no further RI query: the
plan is freed at commit, and the Assert that the table is empty
holds.

- New for v6: a case where a pending plan is freed by the hash scan
while another RI execution is still running. An outer ON DELETE
CASCADE on a different FK fires a BEFORE DELETE trigger that runs
the failing self-referencing cascade three times, each inside an
exception block. The scan sees two entries, skips the outer plan
(refcount 1), and frees the pending one; the third pending plan
goes at AtEOXact_RI(). The outer delete completes and the inner
deletes are rolled back as expected. So the refcount > 0 branch
and removing the current entry during hash_seq_search() both get
exercised, and nothing complained.

== Comments ==

All three of Tomas's points look addressed to me. Scanning the
refcount table instead of keeping a separate list is simpler, and the
early return on an empty table keeps the common path cheap. Two small
things:

1. The comment above RI_QueryPlanCacheExecutingRefCountEntry has a line
that is 91 characters long ("error, until the next RI query execution
or the end of the transaction. A plan is never"); it probably just
needs rewrapping after the rename.

2. The comment on ri_PreparedPlanFreeDeferred() says the table is empty
outside of nested RI checks. It can also hold pending entries after an
aborted execution, until the next ri_PerformCheck() or the end of the
transaction, which your mail describes correctly. Maybe worth saying
the same in the comment.

Doing master first and a separate, smaller fix for 14-18 sounds
reasonable to me.

Regards,
Trakshan Mishra

On Fri, Oct 09, 2026 12:56 AM, Lucas Jeffrey <
lucas.jeffrey@anachronics.com> wrote:

Hi Tomas,

Thanks for your review. I attached a v6, rebased on current master and
with some changes handling some items in your review.

1) Agreed on the naming, there were three different terms for the same
thing. I used "free" everywhere, but since the plan is not always freed
immediately, the names now say that explicitly:

ri_PreparedPlanReleaseASAP() -> ri_PreparedPlanFreeOrDefer()
markedForDeletion -> freeDeferred
ri_PreparedPlanFreeUnused() -> ri_PreparedPlanFreeDeferred()

The comments and the commit message were updated to use the same
terms. The only remaining "Release" is ResOwnerReleaseRIPlanExecution(),
which follows the naming convention of the other resource owner
callbacks (ResOwnerReleaseBuffer(), ResOwnerReleaseCachedPlan(), etc.),
so I kept it.

2) Agreed, the "free later list" was not necessary, and I removed it. To
be
precise, those plans are no longer in ri_query_cache, since
ri_FetchPreparedPlan() removes them from it, but their entries remain in
the refcount hash table, ri_query_plan_cache_executing_refcount, with a
zero count. ri_PreparedPlanFreeDeferred() now scans that table instead.

That table only has entries while RI queries are being executed, so it
stays small. However, ri_PreparedPlanFreeDeferred() is called on every
ri_PerformCheck(), so it returns immediately when the table is empty,
which is the case unless an RI query is already being executed or a
deferred plan is pending. This way the common path does not pay for a
hash_seq_search() over all the buckets. I also reduced the initial size
of the table to 16, instead of RI_INIT_QUERYHASHSIZE (256).

3) Yes, all supported branches are affected. However, this fix cannot be
backpatched as is: the ResourceOwnerDesc API exists only since 17, and
AtEOXact_RI() is new in 19. I would propose to agree on the fix for
master first, and then submit a separate, less invasive patch for 14-18,
using only the APIs and functions available in those branches.

Regards,
Lucas Jeffrey

El mar, 6 oct 2026 a las 15:26, Tomas Vondra (<tomas@vondra.me>)
escribió:

Hi,

On 10/5/26 16:31, Lucas Jeffrey 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

I accidentally ran into this issue a couple days ago, unaware of this
thread. I really like the approach of the proposed fix, refcounting
seems like a sound approach and we already useit elsewhere to track
similar objects. I've only done a quick revie, but it seems much cleaner
than my PoC fix.

A couple (very cosmetic) review comments:

1) The naming seems a bit inconsistent

static void ri_PreparedPlanReleaseASAP(SPIPlanPtr plan);
static void ri_PreparedPlanFreeUnused(void);

Why release vs. free? And the flag is called markedForDeletion, so
that's a third term. Also, I wouldn't use the ASAP in function name.

The SPI function is called SPI_freeplan, so what about using "free"
everywhere?

static void ri_PreparedPlanFree(SPIPlanPtr plan);
static void ri_PreparedPlanFreeUnused(void);

Although, I now realize we're not necessarily freeing the plan, so maybe
this is not an improvement ...

2) Is ri_plans_to_free necessary?

IIUC those plans are still in the hashtable, right? Why not to simply
scan the hashtable, and free the plans there? That'd mean we don't need
a separate list that can get out of sync. Maybe we expect the hash table
to be large, in which case this could be expensive. But is it? I don't
see any such argument in this thread.

If the list is necessary, maybe ri_PreparedPlanFreeUnused() should at
least have an assert that the hash_search() found a matching entry?

3) What about backpatching?

AFAIK this affects all backbranches, not just master, right? So we
should aim to fix the backbranches too. Do you think this refcounting
fix is backpatchable (it seems like it'd be OK), or do you have an idea
for a less invasive fix?

regards

--
Tomas Vondra