PG19 FK fast path: OOB write and missed FK checks during batched

On Sat, Jun 6, 2026 at 17:31 Nikolay Samokhvalov <nik@postgres.ai> wrote:

Hi hackers,

The new FK existence-check fast path in ri_triggers.c (ri_FastPath*) runs
user-defined code in the middle of a deferred batch flush, which yields at
least three defects reachable by an unprivileged table owner. Present in
master and verified inREL_19_BETA1.

I identified these issues during recent security research with LLMs. While
they have clear security implications (OOB write, integrity bypass),
reporting them here because they are isolated to 19beta1, absent in PG18
and earlier; I don't have patches, only reproducibility.

Mechanism:

For an INSERT/UPDATE on the referencing side the fast path buffers rows
in a transaction-lived cache (ri_fastpath_cache, keyed by pg_constraint
OID) and probes the PK index in groups, flushing when a

per-constraint buffer reaches RI_FASTPATH_BATCH_SIZE (64) or when the

trigger-firing pass ends (ri_FastPathEndBatch, an
AfterTriggerBatchCallback). For a cross-type FK the flush calls the
column's cast function (ri_FastPathFlushArray, the FunctionCall3 at line
3069) and the equality operator -- arbitrary user code, mid-flush. Line
numbers below are from a REL_19_BETA1 build (commit 4b0bf07).

Unprivileged vehicle (defects 1 and 3). No superuser, no contrib: a role creates
a type it owns and an IMPLICIT cast from it to the PK type with a PL/pgSQL
function, which ri_HashCompareOp wires into the fast path's cast

slot. Below uses a composite type. Default btree opclass, ordinary single-column
FK, no GUC (fast path is unconditional for non-partitioned, non-temporal
FKs, per ri_fastpath_is_applicable).

1) ri_FastPathBatchAdd (line 2859): out-of-bounds write on re-entry

The write precedes the bound check, and batch_count is reset to 0 only at end
of flush (ri_FastPathBatchFlush, line 2971), so it is 64 throughout a full-batch
flush:

fpentry->batch[fpentry->batch_count] = ExecCopySlotHeapTuple(newslot);

fpentry->batch_count++;

if (fpentry->batch_count >= RI_FASTPATH_BATCH_SIZE)

ri_FastPathBatchFlush(fpentry, fk_rel, riinfo);

There is no re-entrancy guard and ri_FastPathGetEntry returns the same entry,
so user code that does DML on the same table during a full-batch flush
re-enters with batch_count == 64 and writes batch[64], one past the

array, overwriting the adjacent batch_count field (struct layout, lines
250-251). A single re-entrant row only stomps batch_count, which is then reset
to 0 before reuse; the crash manifests once the re-entrant insert is

itself large enough to fill and flush a batch, so the stomped batch_count
is used as an array index (batch[garbage]) and as nvals in memset(matched,
0, nvals * sizeof(bool)) (line 3054).

Reproduction (non-superuser; reliable SIGSEGV on --enable-cassert -O0;
under -O2 the out-of-bounds write is of undefined effect):

create table parent(id int primary key);

insert into parent select g from generate_series(1,2000) g;

create type vch as (v int);

create function vcast(vch) returns int language plpgsql as $$

begin

if $1.v = 64 then

insert into child select row(g)::vch from
generate_series(1001,1064) g;

end if;

return $1.v;

end$$;

create cast (vch as int) with function vcast(vch) as implicit;

create table child(a vch);

alter table child add constraint child_fkey

foreign key (a) references parent(id);

insert into child select row(g)::vch from generate_series(1,64) g; --
crash

-- gdb: crash at ri_FastPathBatchAdd line 2866 with batch_count
holding a

-- stomped HeapTuple pointer's low bits, i.e. batch[64] overwrote

-- batch_count; backend SIGSEGVs and the cluster restarts.

2) ri_FastPathSubXactCallback (line 4208): batch dropped on subxact abort

On SUBXACT_EVENT_ABORT_SUB the callback discards the whole cache:

ri_fastpath_cache = NULL;

ri_fastpath_callback_registered = false;

But batch[] holds outstanding rows of the enclosing transaction, not the aborting
subxact. An internal subxact abort during after-trigger firing (PL/pgSQL
BEGIN ... EXCEPTION) drops the buffered rows unflushed; their FK checks
never run and orphans commit behind a constraint that still reports itself
valid. No cast needed:

create table pk(id int primary key);

create table fk(a int, tag text);

insert into pk select g from generate_series(1,10) g;

alter table fk add constraint fk_a_fkey foreign key (a) references
pk(id);

create function abort_subxact() returns trigger language plpgsql as $$

begin

if NEW.tag = 'boom' then

begin perform 1/0; exception when others then null; end;

end if;

return NEW;

end$$;

create trigger fk_after after insert on fk

for each row execute function abort_subxact();

insert into fk values
(999,'bad'),(0,'boom'),(1,'ok'),(2,'ok'),(3,'ok');

-- INSERT 0 5, no error

select f.a from fk f left join pk p on f.a=p.id where p.id is null;

-- a

-- -----

-- 999

-- 0 (orphans)

-- the constraint still reports itself valid, and re-validation passes

-- while the orphans remain:

select convalidated from pg_constraint where conname = 'fk_a_fkey';

-- convalidated

-- --------------

-- t

alter table fk validate constraint fk_a_fkey;

-- ALTER TABLE (succeeds; does not re-scan committed rows)

select f.a from fk f left join pk p on f.a=p.id where p.id is null;

-- 999, 0 (orphans still present)

Controls (no EXCEPTION; between-statement SAVEPOINT; DEFERRABLE INITIALLY DEFERRED)
all behave correctly (FK violation raised, no orphans). The whole statement's
buffered batch is discarded, not just the aborting row's check. The abort
path also emits "WARNING: resource was not closed" (relation /

index / TupleDesc), a resource leak consistent with the missing flush.

3) ri_FastPathEndBatch (line 4133): cross-table re-entry drops a check

EndBatch flushes by iterating the cache with hash_seq_search (line 4143). If
flush-time user code INSERTs into a different fast-path FK table, ri_FastPathGetEntry
adds a new cache entry mid-scan; it can land in a bucket hash_seq_search
already passed and is never reached. ri_FastPathTeardown (line 4165) then
hash_destroys the cache (line 4188) without flushing entries that still
have batch_count > 0, so that buffered check is discarded. This survives a

per-entry guard for [1] (different entry, not a re-entry of the busy one):

create table parent(id int primary key);

insert into parent select g from generate_series(1,64) g;

create table child2(a int);

alter table child2 add constraint child2_fkey

foreign key (a) references parent(id);

create type vch as (v int);

create function vcast(vch) returns int language plpgsql as $$

begin

if $1.v = 1 then

insert into child2 values (999999); -- orphan into a
*different* FK

end if;

return $1.v;

end$$;

create cast (vch as int) with function vcast(vch) as implicit;

create table child(a vch);

alter table child add constraint child_fkey

foreign key (a) references parent(id);

insert into child values (row(1)::vch); -- flushed at
ri_FastPathEndBatch

select a from child2 where a not in (select id from parent); -- =>
999999

-- control: INSERT INTO child2 VALUES (999999); -- correctly raises
FK error

Root cause / thoughts:

All three stem from invoking user cast/operator code inside a deferred batch
flush: while a per-entry batch is half-updated [1], while a cache-wide hash_seq_search
is in progress and teardown drops non-empty entries [3], and against a
subxact-abort invalidation that cannot tell parent-xact rows from aborted-subxact
rows [2].

- [1] Bound-check before the write in ri_FastPathBatchAdd, and add a "flushing"
flag to RI_FastPathEntry, rejecting re-entrant modification of a busy
entry (a nested per-row probe is unsafe: the flush may hold PK-index buffer
locks).

- [3] Loop-flush in ri_FastPathEndBatch until no entry has batch_count >
0, and/or flush non-empty entries in ri_FastPathTeardown before
hash_destroy.

- [2] Do not discard outstanding parent-xact rows on
SUBXACT_EVENT_ABORT_SUB; track the buffering subxact, or flush
immediate-constraint batches subxact boundaries.

- Unifying: a global "in fast-path flush" guard routing any re-entrant FK check
to the immediate per-row path, and reconsidering running user code mid-flush
at all.

Nik

Thanks for the detailed report and reproducers. I’ve started looking into
this.

- thanks, Amit

On Sat, Jun 6, 2026 at 6:13 PM Amit Langote <amitlangote09@gmail.com> wrote:

Thanks for the detailed report and reproducers. I’ve started looking into this.

Continuing to look. Appended this to the open items list:

https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items#Open_Issues

--
Thanks, Amit Langote

On Mon, Jun 8, 2026 at 5:18 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Sat, Jun 6, 2026 at 6:13 PM Amit Langote <amitlangote09@gmail.com> wrote:

Thanks for the detailed report and reproducers. I’ve started looking into this.

Continuing to look. Appended this to the open items list:

https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items#Open_Issues

Thanks again, Nik, for the thorough analysis and the reproducers --
they made all three easy to confirm and pin down. Patches attached:
0001 for defect 1, 0002 for defects 2 and 3.

0001 (defect 1): check and flush before writing the row rather than
after, and add a per-entry "flushing" flag so a re-entrant add on the
same entry during a flush takes the per-row path instead of touching
the mid-flush batch. The flag is cleared in a PG_FINALLY, which also
resets batch_count, so the entry stays reusable if a flush error is
caught by a savepoint.

0002 (defects 2 and 3): rather than track subxact membership per row,
confine batching to the top transaction level -- in RI_FKey_check,
when GetCurrentTransactionNestLevel() > 1, use the per-row path. I
went this way because per-entry subxact tracking isn't enough (one
entry's batch can mix rows from several levels, since the cache is
keyed by constraint), and flushing at subxact boundaries doesn't work
for deferred constraints. Once the cache only ever holds top-level
rows, a subxact abort has nothing of its own to discard, so
ri_FastPathSubXactCallback goes away -- that's what fixes your defect
2 reproducer. For defect 3, which is still reachable at the top level,
the same patch adds a cache-wide flag set while ri_FastPathEndBatch
iterates, so a re-entrant check during the scan takes the per-row path
instead of inserting into the cache being scanned.

The per-row path still bypasses SPI, so these stay well ahead of the
pre-19 check in terms of performance. I'd like to recover batching
across subtransactions properly in v20 but didn't want to rush it now.

On defect 3, can you check whether your reproducer still commits the
orphan with 0002 applied, or whether (like on my build) it now raises
the violation? I'd like to be sure the bucket-placement variation you
hit is actually covered. And of course any review of the patches is
welcome.

--
Thanks, Amit Langote

On Tue, Jun 9, 2026 at 6:31 AM Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Jun 8, 2026 at 5:18 PM Amit Langote <amitlangote09@gmail.com>
wrote:

On Sat, Jun 6, 2026 at 6:13 PM Amit Langote <amitlangote09@gmail.com>

wrote:

Thanks for the detailed report and reproducers. I’ve started looking

into this.

Continuing to look. Appended this to the open items list:

https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items#Open_Issues

Thanks again, Nik, for the thorough analysis and the reproducers --
they made all three easy to confirm and pin down. Patches attached:
0001 for defect 1, 0002 for defects 2 and 3.

0001 (defect 1): check and flush before writing the row rather than
after, and add a per-entry "flushing" flag so a re-entrant add on the
same entry during a flush takes the per-row path instead of touching
the mid-flush batch. The flag is cleared in a PG_FINALLY, which also
resets batch_count, so the entry stays reusable if a flush error is
caught by a savepoint.

0002 (defects 2 and 3): rather than track subxact membership per row,
confine batching to the top transaction level -- in RI_FKey_check,
when GetCurrentTransactionNestLevel() > 1, use the per-row path. I
went this way because per-entry subxact tracking isn't enough (one
entry's batch can mix rows from several levels, since the cache is
keyed by constraint), and flushing at subxact boundaries doesn't work
for deferred constraints. Once the cache only ever holds top-level
rows, a subxact abort has nothing of its own to discard, so
ri_FastPathSubXactCallback goes away -- that's what fixes your defect
2 reproducer. For defect 3, which is still reachable at the top level,
the same patch adds a cache-wide flag set while ri_FastPathEndBatch
iterates, so a re-entrant check during the scan takes the per-row path
instead of inserting into the cache being scanned.

The per-row path still bypasses SPI, so these stay well ahead of the
pre-19 check in terms of performance. I'd like to recover batching
across subtransactions properly in v20 but didn't want to rush it now.

On defect 3, can you check whether your reproducer still commits the
orphan with 0002 applied, or whether (like on my build) it now raises
the violation? I'd like to be sure the bucket-placement variation you
hit is actually covered. And of course any review of the patches is
welcome.

--
Thanks, Amit Langote

Hi Amit,

Thanks for the quick fixes.

I checked v1-0001 + v1-0002 against current master (e18b0cb7) with an
assertion/debug build.

- Both apply cleanly to master (in sequence)
- Defect 1 same-FK re-entry no longer crashes; the original shape completes
and leaves the expected rows
- Defect 2 subtransaction-abort case now raises the FK violation instead of
committing orphans
- For your defect 3 question: with 0002 applied, my reproducer no longer
commits the child2 orphan. It raises:
ERROR: insert or update on table "child2" violates foreign key
constraint "child2_fkey"
DETAIL: Key (a)=(999999) is not present in table "parent".

After the error, child2_orphans = 0 and child2 is empty in my run.

I also ran the regression suite in that tree; foreign_key passed, and the
full run reported all 245 tests passed.

So v1 looks good to me for the three reported cases.

Thanks!

Nik

On Wed, Jun 10, 2026 at 5:16 PM Nikolay Samokhvalov <nik@postgres.ai> wrote:

On Tue, Jun 9, 2026 at 6:31 AM Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Jun 8, 2026 at 5:18 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Sat, Jun 6, 2026 at 6:13 PM Amit Langote <amitlangote09@gmail.com> wrote:

Thanks for the detailed report and reproducers. I’ve started looking into this.

Continuing to look. Appended this to the open items list:

https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items#Open_Issues

Thanks again, Nik, for the thorough analysis and the reproducers --
they made all three easy to confirm and pin down. Patches attached:
0001 for defect 1, 0002 for defects 2 and 3.

0001 (defect 1): check and flush before writing the row rather than
after, and add a per-entry "flushing" flag so a re-entrant add on the
same entry during a flush takes the per-row path instead of touching
the mid-flush batch. The flag is cleared in a PG_FINALLY, which also
resets batch_count, so the entry stays reusable if a flush error is
caught by a savepoint.

0002 (defects 2 and 3): rather than track subxact membership per row,
confine batching to the top transaction level -- in RI_FKey_check,
when GetCurrentTransactionNestLevel() > 1, use the per-row path. I
went this way because per-entry subxact tracking isn't enough (one
entry's batch can mix rows from several levels, since the cache is
keyed by constraint), and flushing at subxact boundaries doesn't work
for deferred constraints. Once the cache only ever holds top-level
rows, a subxact abort has nothing of its own to discard, so
ri_FastPathSubXactCallback goes away -- that's what fixes your defect
2 reproducer. For defect 3, which is still reachable at the top level,
the same patch adds a cache-wide flag set while ri_FastPathEndBatch
iterates, so a re-entrant check during the scan takes the per-row path
instead of inserting into the cache being scanned.

The per-row path still bypasses SPI, so these stay well ahead of the
pre-19 check in terms of performance. I'd like to recover batching
across subtransactions properly in v20 but didn't want to rush it now.

On defect 3, can you check whether your reproducer still commits the
orphan with 0002 applied, or whether (like on my build) it now raises
the violation? I'd like to be sure the bucket-placement variation you
hit is actually covered. And of course any review of the patches is
welcome.

Hi Amit,

Thanks for the quick fixes.

I checked v1-0001 + v1-0002 against current master (e18b0cb7) with an assertion/debug build.

- Both apply cleanly to master (in sequence)
- Defect 1 same-FK re-entry no longer crashes; the original shape completes and leaves the expected rows
- Defect 2 subtransaction-abort case now raises the FK violation instead of committing orphans
- For your defect 3 question: with 0002 applied, my reproducer no longer commits the child2 orphan. It raises:
ERROR: insert or update on table "child2" violates foreign key constraint "child2_fkey"
DETAIL: Key (a)=(999999) is not present in table "parent".

After the error, child2_orphans = 0 and child2 is empty in my run.

I also ran the regression suite in that tree; foreign_key passed, and the full run reported all 245 tests passed.

So v1 looks good to me for the three reported cases.

Thanks for checking. I will review them a bit more closely before
committing by Friday. Other reviews are welcome.

--
Thanks, Amit Langote

Hi,

On Wed, 10 Jun 2026 at 14:02, Amit Langote <amitlangote09@gmail.com> wrote

Thanks for checking. I will review them a bit more closely before
committing by Friday. Other reviews are welcome.

Thanks for the patch!

I read through v1-0001 and v1-0002 and tried them locally. I had a couple of
things I wanted to ask about.

1. The per-entry "flushing" flag and test coverage. If I'm reading the two
patches together correctly, with both applied the 64-row re-entry test in
0001
reaches the flush through ri_FastPathEndBatch(), where 0002's cache-wide
ri_fastpath_flushing guard already routes the re-entrant check to the
per-row
path before it gets back into ri_FastPathBatchAdd(). Does that mean the
per-entry flag from 0001 isn't really exercised by that test once 0002 is
in?
As far as I can tell you'd need the flush to fire from ri_FastPathBatchAdd()
itself (a 65th row) to reach it. I tried a 65-row variant (same FK,
re-entrant
DML from the cast during the full-batch flush), including a case where the
re-entrant row was an orphan, and it seemed to do the right thing; the
per-row fallback still raised the violation. Would it be worth switching
the
test to 65 rows, or adding that variant, so the per-entry guard is covered
too?
Or am I missing a path where the committed test already hits it?

2. Resetting ri_fastpath_flushing. I noticed it's cleared only in the
PG_FINALLY of ri_FastPathEndBatch(), which does seem to cover the cases I
could
think of. Since ri_FastPathXactCallback already NULLs ri_fastpath_cache and
clears ri_fastpath_callback_registered at transaction end, I wondered
whether
it might be worth clearing ri_fastpath_flushing there too, just as cheap
insurance against some future path that leaves it set across transactions
though maybe that's unnecessary given the PG_FINALLY.

Other than the above queries, the patch looks good to me.

Regards,
Ayush

Hi Ayush,

Thanks for the review.

On Wed, Jun 10, 2026 at 7:09 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

On Wed, 10 Jun 2026 at 14:02, Amit Langote <amitlangote09@gmail.com> wrote

Thanks for checking. I will review them a bit more closely before
committing by Friday. Other reviews are welcome.

Thanks for the patch!

I read through v1-0001 and v1-0002 and tried them locally. I had a couple of
things I wanted to ask about.

1. The per-entry "flushing" flag and test coverage. If I'm reading the two
patches together correctly, with both applied the 64-row re-entry test in 0001
reaches the flush through ri_FastPathEndBatch(), where 0002's cache-wide
ri_fastpath_flushing guard already routes the re-entrant check to the per-row
path before it gets back into ri_FastPathBatchAdd(). Does that mean the
per-entry flag from 0001 isn't really exercised by that test once 0002 is in?
As far as I can tell you'd need the flush to fire from ri_FastPathBatchAdd()
itself (a 65th row) to reach it. I tried a 65-row variant (same FK, re-entrant
DML from the cast during the full-batch flush), including a case where the
re-entrant row was an orphan, and it seemed to do the right thing; the
per-row fallback still raised the violation. Would it be worth switching the
test to 65 rows, or adding that variant, so the per-entry guard is covered too?
Or am I missing a path where the committed test already hits it?

You're right. With 0002 applied, the 64-row test reaches the flush
through ri_FastPathEndBatch(), where the cache-wide
ri_fastpath_flushing guard catches the re-entry before it returns to
ri_FastPathBatchAdd(), so the per-entry flag is no longer exercised by
that test. To hit the per-entry flag the flush has to fire from
ri_FastPathBatchAdd() itself, which the 64-row case no longer does
once the add and flush are reordered.

Rather than bump the test to 65 rows, I'd prefer to keep the flush
firing from ri_FastPathBatchAdd() at 64 by not reordering the add and
flush, and prevent the OOB write by bounds-checking the write instead,
as done in the attached updated 0001. A re-entrant add then can't
overrun the array regardless of the flag, the per-entry flushing guard
still routes the re-entry to the per-row path, and a 64-row statement
flushes from ri_FastPathBatchAdd() on the 64th row, so the existing
test exercises the per-entry guard.

2. Resetting ri_fastpath_flushing. I noticed it's cleared only in the
PG_FINALLY of ri_FastPathEndBatch(), which does seem to cover the cases I could
think of. Since ri_FastPathXactCallback already NULLs ri_fastpath_cache and
clears ri_fastpath_callback_registered at transaction end, I wondered whether
it might be worth clearing ri_fastpath_flushing there too, just as cheap
insurance against some future path that leaves it set across transactions
though maybe that's unnecessary given the PG_FINALLY.

Agreed, it's cheap and matches the existing resets there, so I've
added it to ri_FastPathXactCallback() in v2-0002.

Other than the above queries, the patch looks good to me.

Updated patches attached.

--
Thanks, Amit Langote

Hi,

On Wed, 10 Jun 2026 at 17:47, Amit Langote <amitlangote09@gmail.com> wrote:

Hi Ayush,

Thanks for the review.

On Wed, Jun 10, 2026 at 7:09 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

On Wed, 10 Jun 2026 at 14:02, Amit Langote <amitlangote09@gmail.com>

wrote

Thanks for checking. I will review them a bit more closely before
committing by Friday. Other reviews are welcome.

Thanks for the patch!

I read through v1-0001 and v1-0002 and tried them locally. I had a

couple of

things I wanted to ask about.

1. The per-entry "flushing" flag and test coverage. If I'm reading the

two

patches together correctly, with both applied the 64-row re-entry test

in 0001

reaches the flush through ri_FastPathEndBatch(), where 0002's cache-wide
ri_fastpath_flushing guard already routes the re-entrant check to the

per-row

path before it gets back into ri_FastPathBatchAdd(). Does that mean the
per-entry flag from 0001 isn't really exercised by that test once 0002

is in?

As far as I can tell you'd need the flush to fire from

ri_FastPathBatchAdd()

itself (a 65th row) to reach it. I tried a 65-row variant (same FK,

re-entrant

DML from the cast during the full-batch flush), including a case where

the

re-entrant row was an orphan, and it seemed to do the right thing; the
per-row fallback still raised the violation. Would it be worth

switching the

test to 65 rows, or adding that variant, so the per-entry guard is

covered too?

Or am I missing a path where the committed test already hits it?

You're right. With 0002 applied, the 64-row test reaches the flush
through ri_FastPathEndBatch(), where the cache-wide
ri_fastpath_flushing guard catches the re-entry before it returns to
ri_FastPathBatchAdd(), so the per-entry flag is no longer exercised by
that test. To hit the per-entry flag the flush has to fire from
ri_FastPathBatchAdd() itself, which the 64-row case no longer does
once the add and flush are reordered.

Rather than bump the test to 65 rows, I'd prefer to keep the flush
firing from ri_FastPathBatchAdd() at 64 by not reordering the add and
flush, and prevent the OOB write by bounds-checking the write instead,
as done in the attached updated 0001. A re-entrant add then can't
overrun the array regardless of the flag, the per-entry flushing guard
still routes the re-entry to the per-row path, and a 64-row statement
flushes from ri_FastPathBatchAdd() on the 64th row, so the existing
test exercises the per-entry guard.

Makes sense, it is better.

2. Resetting ri_fastpath_flushing. I noticed it's cleared only in the

PG_FINALLY of ri_FastPathEndBatch(), which does seem to cover the cases

I could

think of. Since ri_FastPathXactCallback already NULLs ri_fastpath_cache

and

clears ri_fastpath_callback_registered at transaction end, I wondered

whether

it might be worth clearing ri_fastpath_flushing there too, just as cheap
insurance against some future path that leaves it set across transactions
though maybe that's unnecessary given the PG_FINALLY.

Agreed, it's cheap and matches the existing resets there, so I've
added it to ri_FastPathXactCallback() in v2-0002.

Other than the above queries, the patch looks good to me.

Updated patches attached.

Thanks for the updated patches!

Both patches, lgtm.

Regards,
Ayush

Hi Amit,

On Wed, Jun 10, 2026 at 8:17 PM Amit Langote <amitlangote09@gmail.com> wrote:

Hi Ayush,

Thanks for the review.

On Wed, Jun 10, 2026 at 7:09 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

On Wed, 10 Jun 2026 at 14:02, Amit Langote <amitlangote09@gmail.com> wrote

Thanks for checking. I will review them a bit more closely before
committing by Friday. Other reviews are welcome.

Thanks for the patch!

I read through v1-0001 and v1-0002 and tried them locally. I had a couple of
things I wanted to ask about.

1. The per-entry "flushing" flag and test coverage. If I'm reading the two
patches together correctly, with both applied the 64-row re-entry test in 0001
reaches the flush through ri_FastPathEndBatch(), where 0002's cache-wide
ri_fastpath_flushing guard already routes the re-entrant check to the per-row
path before it gets back into ri_FastPathBatchAdd(). Does that mean the
per-entry flag from 0001 isn't really exercised by that test once 0002 is in?
As far as I can tell you'd need the flush to fire from ri_FastPathBatchAdd()
itself (a 65th row) to reach it. I tried a 65-row variant (same FK, re-entrant
DML from the cast during the full-batch flush), including a case where the
re-entrant row was an orphan, and it seemed to do the right thing; the
per-row fallback still raised the violation. Would it be worth switching the
test to 65 rows, or adding that variant, so the per-entry guard is covered too?
Or am I missing a path where the committed test already hits it?

You're right. With 0002 applied, the 64-row test reaches the flush
through ri_FastPathEndBatch(), where the cache-wide
ri_fastpath_flushing guard catches the re-entry before it returns to
ri_FastPathBatchAdd(), so the per-entry flag is no longer exercised by
that test. To hit the per-entry flag the flush has to fire from
ri_FastPathBatchAdd() itself, which the 64-row case no longer does
once the add and flush are reordered.

Rather than bump the test to 65 rows, I'd prefer to keep the flush
firing from ri_FastPathBatchAdd() at 64 by not reordering the add and
flush, and prevent the OOB write by bounds-checking the write instead,
as done in the attached updated 0001. A re-entrant add then can't
overrun the array regardless of the flag, the per-entry flushing guard
still routes the re-entry to the per-row path, and a 64-row statement
flushes from ri_FastPathBatchAdd() on the 64th row, so the existing
test exercises the per-entry guard.

2. Resetting ri_fastpath_flushing. I noticed it's cleared only in the
PG_FINALLY of ri_FastPathEndBatch(), which does seem to cover the cases I could
think of. Since ri_FastPathXactCallback already NULLs ri_fastpath_cache and
clears ri_fastpath_callback_registered at transaction end, I wondered whether
it might be worth clearing ri_fastpath_flushing there too, just as cheap
insurance against some future path that leaves it set across transactions
though maybe that's unnecessary given the PG_FINALLY.

Agreed, it's cheap and matches the existing resets there, so I've
added it to ri_FastPathXactCallback() in v2-0002.

Other than the above queries, the patch looks good to me.

Updated patches attached.

I only reviewed and applied patch 0001 on my local machine, and it
successfully fixed the crash.

One minor comment:

+ if (fpentry->flushing)
+ {
+ ri_FastPathCheck(riinfo, fk_rel, newslot);
+ return;
+ }

Would it be worth wrapping the condition with unlikely()? It seems
this branch is expected to be false in most cases, not a strong
opinion though.

--
Thanks, Amit Langote

--
Regards
Junwang Zhao

On Thu, Jun 11, 2026 at 5:18 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

I only reviewed and applied patch 0001 on my local machine, and it
successfully fixed the crash.

One minor comment:

+ if (fpentry->flushing)
+ {
+ ri_FastPathCheck(riinfo, fk_rel, newslot);
+ return;
+ }

Would it be worth wrapping the condition with unlikely()? It seems
this branch is expected to be false in most cases, not a strong
opinion though.

Good idea. Will do.

Are you planning to look at 0002?

--
Thanks, Amit Langote

Hi Amit,

On Thu, Jun 11, 2026 at 5:05 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Jun 11, 2026 at 5:18 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

I only reviewed and applied patch 0001 on my local machine, and it
successfully fixed the crash.

One minor comment:

+ if (fpentry->flushing)
+ {
+ ri_FastPathCheck(riinfo, fk_rel, newslot);
+ return;
+ }

Would it be worth wrapping the condition with unlikely()? It seems
this branch is expected to be false in most cases, not a strong
opinion though.

Good idea. Will do.

Are you planning to look at 0002?

I just applied 0002 and ran the regression successfully.

I have one trivial comment, subXact abort doesn't NULL the
ri_fastpath_cache, so I think the following comment of
RI_FastPathEntry should be polished accordingly by removing the
`SubXactCallback`.

* ri_FastPathEndBatch(); on abort, ResourceOwner releases the cached
* relations and the XactCallback/SubXactCallback NULL the static cache pointer
* to prevent any subsequent access.

--
Thanks, Amit Langote

--
Regards
Junwang Zhao

On Thu, Jun 11, 2026 at 6:51 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

On Thu, Jun 11, 2026 at 5:05 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Jun 11, 2026 at 5:18 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

I only reviewed and applied patch 0001 on my local machine, and it
successfully fixed the crash.

One minor comment:

+ if (fpentry->flushing)
+ {
+ ri_FastPathCheck(riinfo, fk_rel, newslot);
+ return;
+ }

Would it be worth wrapping the condition with unlikely()? It seems
this branch is expected to be false in most cases, not a strong
opinion though.

Good idea. Will do.

Are you planning to look at 0002?

I just applied 0002 and ran the regression successfully.

I have one trivial comment, subXact abort doesn't NULL the
ri_fastpath_cache, so I think the following comment of
RI_FastPathEntry should be polished accordingly by removing the
`SubXactCallback`.

* ri_FastPathEndBatch(); on abort, ResourceOwner releases the cached
* relations and the XactCallback/SubXactCallback NULL the static cache pointer
* to prevent any subsequent access.

Thanks for the review. Yes, I missed that.

I've updated the patches to address your comments and did some other polishing.

--
Thanks, Amit Langote

On Thu, Jun 11, 2026 at 7:47 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Jun 11, 2026 at 6:51 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

On Thu, Jun 11, 2026 at 5:05 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Jun 11, 2026 at 5:18 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

I only reviewed and applied patch 0001 on my local machine, and it
successfully fixed the crash.

One minor comment:

+ if (fpentry->flushing)
+ {
+ ri_FastPathCheck(riinfo, fk_rel, newslot);
+ return;
+ }

Would it be worth wrapping the condition with unlikely()? It seems
this branch is expected to be false in most cases, not a strong
opinion though.

Good idea. Will do.

Are you planning to look at 0002?

I just applied 0002 and ran the regression successfully.

I have one trivial comment, subXact abort doesn't NULL the
ri_fastpath_cache, so I think the following comment of
RI_FastPathEntry should be polished accordingly by removing the
`SubXactCallback`.

* ri_FastPathEndBatch(); on abort, ResourceOwner releases the cached
* relations and the XactCallback/SubXactCallback NULL the static cache pointer
* to prevent any subsequent access.

Thanks for the review. Yes, I missed that.

I've updated the patches to address your comments and did some other polishing.

I've pushed these now. Thank you everyone.

--
Thanks, Amit Langote

On Fri, Jun 12, 2026 at 11:46:07AM +0900, Amit Langote wrote:

I've pushed these now. Thank you everyone.

commit 4113873 wrote:

Confine RI fast-path batching to the top transaction level

I discourage this fix strategy, for three reasons:

1. It slows "BEGIN; SAVEPOINT s; COPY table_with_fk FROM ..." by the ~1.6x
batching benefit, compared to omitting the SAVEPOINT. That's a bad user
experience not seen elsewhere. Starting a high number of subtransactions
is expensive, but wrapping a long-running transaction body in one
subtransaction hasn't been a performance reducer.

2. It departs from the PostgreSQL norm of tracking resources by
subtransaction. You can see normal handling in many AbortSubTransaction()
callees, e.g. AtEOSubXact_LargeObject(). This in turn makes the change
harder to verify as correct.

3. It doesn't seem to have simplified code much, compared to our normal
subxact-based approach.

First, on subtransaction abort ri_FastPathSubXactCallback discarded the
entire cache. An entry's batch holds rows buffered by the enclosing
transaction, not just the aborting subxact -- the cache is keyed by
constraint, so a single entry can mix rows from multiple subxact levels.

That would imply having started a subtransaction and then added to the batch
without an intervening pair of CommandCounterIncrement() and
AfterTriggerBeginQuery(). That's an invalid thing for C code to do, so I'd
make it an error if a batch would contain rows from different subtransactions.

(This relates to the reentrancy bug fixed in 0e47bb5. With an intervening
CommandCounterIncrement() and AfterTriggerBeginQuery(), reusing the outer
batch is wrong even without subtransactions: it would check with the wrong
snapshot. Each level of reentrancy needs to finish its FK checks separately,
even if the batch buffer were unbounded.)

An internal subxact abort during after-trigger firing (e.g. a PL/pgSQL
BEGIN ... EXCEPTION block) therefore dropped buffered rows

I suspect a subxact abort during after-trigger firing can still cause a
different problem via AfterTriggerEndQuery():

AfterTriggerEndQuery(EState *estate)
{
...
afterTriggers.firing_depth++;

AfterTriggerEndSubXact() doesn't undo this increment. Having identified that
as suspect, I asked Opus 4.8 to try to confirm or refute bug reachability. I
have not personally verified its finding, but it said:

CLAUDE [CONFIRMED -- reachable bug; your instinct is right]: firing_depth is ++/-- in matched
pairs inside AfterTriggerEndQuery/FireDeferred/SetState with NO PG_TRY, and AfterTriggerEndSubXact
resets firing_batch_callbacks but NOT firing_depth. So a trigger ERROR caught by an outer
subtransaction (PL/pgSQL EXCEPTION) skips the -- and strands firing_depth>0 for the rest of the
xact. Its sole consumer is AfterTriggerIsActive() -> the RI_FKey_check batching gate; the only
RI check that runs OUTSIDE genuine trigger firing is ALTER TABLE / VALIDATE CONSTRAINT per-row
validation. With firing_depth stranded, that validation is wrongly routed into ri_FastPathBatchAdd.
Repro (per-row forced via REFERENCES-only, no SELECT, so RI_Initial_Check bails):
CONTROL: per-row-validated ALTER ADD FK with violating row 99 -> errors AT the ALTER (correct).
BUG: run a caught FK violation first (DO/EXCEPTION), then the same ALTER in the same xact ->
the ALTER does NOT error, marks convalidated=t, emits "WARNING: resource was not closed:
relation pk2 / pk2_pkey / TupleDesc" (a resource-owner leak), and defers the violation to
COMMIT. Consequences: resource leak + FK validation deferred past the ALTER (documented
invariant broken). Fix: reset firing_depth in AfterTriggerEndSubXact, mirroring the
firing_batch_callbacks reset right below it.

Even if that's a hallucination, it's an example of what I meant in (2) about
making the change harder to verify.

Also, it's not clear to me why AfterTriggerEndSubXact() is right to reset
afterTriggers.firing_batch_callbacks. The outer xact may be firing. If
that's okay, can you expand the code comment to explain it?

Cleanly unwinding the cache on subxact abort would require tracking the
originating subxact of each buffered row, since rows from different
levels share an entry (the cache is keyed by constraint) and deferred
constraints cannot be flushed early at a subxact boundary.

It's true that they can't be flushed early, but I'm not seeing a need for
explicit code to avoid that. A subxact commit shall just confirm there's no
batch of its subxact level. A subxact abort shall discard any batch of its
subxact level, leaving higher-subxact batches untouched. A deferred trigger
doesn't start a batch until the end of the top-level transaction.

The per-row fast path still bypasses SPI and stays well ahead of the
pre-19 SPI-based check. A fuller fix that preserves batching across
subtransactions -- whether by tracking the originating subxact of each
buffered row or by per-subxact cache stacks merged into the parent on
commit -- is left for a future release.

If the above suspicion corresponds to a live bug, I'd bet on the fuller fix
being cleaner than a surgical fix. That may not pan out, but I recommend
trying it first.

Hi Noah,

On Mon, Jul 6, 2026 at 7:21 AM Noah Misch <noah@leadboat.com> wrote:

On Fri, Jun 12, 2026 at 11:46:07AM +0900, Amit Langote wrote:

I've pushed these now. Thank you everyone.

commit 4113873 wrote:

Confine RI fast-path batching to the top transaction level

I discourage this fix strategy, for three reasons:

1. It slows "BEGIN; SAVEPOINT s; COPY table_with_fk FROM ..." by the ~1.6x
batching benefit, compared to omitting the SAVEPOINT. That's a bad user
experience not seen elsewhere. Starting a high number of subtransactions
is expensive, but wrapping a long-running transaction body in one
subtransaction hasn't been a performance reducer.

Ok, I agree it's a wart.

2. It departs from the PostgreSQL norm of tracking resources by
subtransaction. You can see normal handling in many AbortSubTransaction()
callees, e.g. AtEOSubXact_LargeObject(). This in turn makes the change
harder to verify as correct.

3. It doesn't seem to have simplified code much, compared to our normal
subxact-based approach.

Fair, I added a special case, which I can see now doesn't actually
simplify things. I'll rework it to track batches per subtransaction
the normal way.

First, on subtransaction abort ri_FastPathSubXactCallback discarded the
entire cache. An entry's batch holds rows buffered by the enclosing
transaction, not just the aborting subxact -- the cache is keyed by
constraint, so a single entry can mix rows from multiple subxact levels.

That would imply having started a subtransaction and then added to the batch
without an intervening pair of CommandCounterIncrement() and
AfterTriggerBeginQuery(). That's an invalid thing for C code to do, so I'd
make it an error if a batch would contain rows from different subtransactions.

Agreed. I confirmed a batch is flushed at AfterTriggerEndQuery and
deferred checks don't populate a batch until they fire at top-level
commit, so an entry is always at a single subxact level. The "mixes
levels" justification in my commit message was a hypothesis I never
verified, and it's wrong; a can't-happen assert is most likely the
right thing.

(This relates to the reentrancy bug fixed in 0e47bb5. With an intervening
CommandCounterIncrement() and AfterTriggerBeginQuery(), reusing the outer
batch is wrong even without subtransactions: it would check with the wrong
snapshot. Each level of reentrancy needs to finish its FK checks separately,
even if the batch buffer were unbounded.)

An internal subxact abort during after-trigger firing (e.g. a PL/pgSQL
BEGIN ... EXCEPTION block) therefore dropped buffered rows

I suspect a subxact abort during after-trigger firing can still cause a
different problem via AfterTriggerEndQuery():

AfterTriggerEndQuery(EState *estate)
{
...
afterTriggers.firing_depth++;

AfterTriggerEndSubXact() doesn't undo this increment. Having identified that
as suspect, I asked Opus 4.8 to try to confirm or refute bug reachability. I
have not personally verified its finding, but it said:

CLAUDE [CONFIRMED -- reachable bug; your instinct is right]: firing_depth is ++/-- in matched
pairs inside AfterTriggerEndQuery/FireDeferred/SetState with NO PG_TRY, and AfterTriggerEndSubXact
resets firing_batch_callbacks but NOT firing_depth. So a trigger ERROR caught by an outer
subtransaction (PL/pgSQL EXCEPTION) skips the -- and strands firing_depth>0 for the rest of the
xact. Its sole consumer is AfterTriggerIsActive() -> the RI_FKey_check batching gate; the only
RI check that runs OUTSIDE genuine trigger firing is ALTER TABLE / VALIDATE CONSTRAINT per-row
validation. With firing_depth stranded, that validation is wrongly routed into ri_FastPathBatchAdd.
Repro (per-row forced via REFERENCES-only, no SELECT, so RI_Initial_Check bails):
CONTROL: per-row-validated ALTER ADD FK with violating row 99 -> errors AT the ALTER (correct).
BUG: run a caught FK violation first (DO/EXCEPTION), then the same ALTER in the same xact ->
the ALTER does NOT error, marks convalidated=t, emits "WARNING: resource was not closed:
relation pk2 / pk2_pkey / TupleDesc" (a resource-owner leak), and defers the violation to
COMMIT. Consequences: resource leak + FK validation deferred past the ALTER (documented
invariant broken). Fix: reset firing_depth in AfterTriggerEndSubXact, mirroring the
firing_batch_callbacks reset right below it.

Even if that's a hallucination, it's an example of what I meant in (2) about
making the change harder to verify.

That looks like a real bug, likely of the same class as an earlier
error-path reset I fixed. I'll verify the reproducer; the fix is
likely resetting firing_depth in AfterTriggerEndSubXact alongside the
firing_batch_callbacks reset.

Also, it's not clear to me why AfterTriggerEndSubXact() is right to reset
afterTriggers.firing_batch_callbacks. The outer xact may be firing. If
that's okay, can you expand the code comment to explain it?

I'll go back and reconstruct why that reset is correct. If I can't,
I'll treat it as suspect and address it in the rework, with a comment
either way.

Cleanly unwinding the cache on subxact abort would require tracking the
originating subxact of each buffered row, since rows from different
levels share an entry (the cache is keyed by constraint) and deferred
constraints cannot be flushed early at a subxact boundary.

It's true that they can't be flushed early, but I'm not seeing a need for
explicit code to avoid that. A subxact commit shall just confirm there's no
batch of its subxact level. A subxact abort shall discard any batch of its
subxact level, leaving higher-subxact batches untouched. A deferred trigger
doesn't start a batch until the end of the top-level transaction.

Yes. I was confused about the relationship between deferred firing and
subxacts, which is where that justification came from; deferred
batches only exist at the top level, so there's nothing to unwind for
them at a subxact boundary.

The per-row fast path still bypasses SPI and stays well ahead of the
pre-19 SPI-based check. A fuller fix that preserves batching across
subtransactions -- whether by tracking the originating subxact of each
buffered row or by per-subxact cache stacks merged into the parent on
commit -- is left for a future release.

If the above suspicion corresponds to a live bug, I'd bet on the fuller fix
being cleaner than a surgical fix. That may not pan out, but I recommend
trying it first.

Trying it in the direction you describe: subxact abort discards that
level's batch, and subxact commit just checks that there's no batch
left at that level (it was already flushed at statement end). The part
that needs care is the resource-owner handling of the cached PK
relation and index when a batch flush errors partway through inside a
subxact that then aborts. That's more involved than a subid tag. I'll
work through the details and try to post a patch tomorrow.

Thanks for the deep review.

--
Thanks, Amit Langote

On Mon, Jul 6, 2026 at 11:29 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Jul 6, 2026 at 7:21 AM Noah Misch <noah@leadboat.com> wrote:

On Fri, Jun 12, 2026 at 11:46:07AM +0900, Amit Langote wrote:

I've pushed these now. Thank you everyone.

commit 4113873 wrote:

Confine RI fast-path batching to the top transaction level

I discourage this fix strategy, for three reasons:

1. It slows "BEGIN; SAVEPOINT s; COPY table_with_fk FROM ..." by the ~1.6x
batching benefit, compared to omitting the SAVEPOINT. That's a bad user
experience not seen elsewhere. Starting a high number of subtransactions
is expensive, but wrapping a long-running transaction body in one
subtransaction hasn't been a performance reducer.

Ok, I agree it's a wart.

2. It departs from the PostgreSQL norm of tracking resources by
subtransaction. You can see normal handling in many AbortSubTransaction()
callees, e.g. AtEOSubXact_LargeObject(). This in turn makes the change
harder to verify as correct.

3. It doesn't seem to have simplified code much, compared to our normal
subxact-based approach.

Fair, I added a special case, which I can see now doesn't actually
simplify things. I'll rework it to track batches per subtransaction
the normal way.

0001 does this. It drops the top-level confinement and adds
AtEOSubXact_RI(), called from Commit/AbortSubTransaction() after the
subtransaction's ResourceOwnerRelease(). On abort it discards only the
entries opened by the ending subtransaction, identified by a subid
stamped on each entry at creation; it closes nothing itself, since the
ResourceOwner has already released those relations. Entries opened at
an outer level are left alone, so an inner subxact abort during
outer-level trigger firing no longer discards the outer statement's
batch. This follows the AtEOSubXact_* pattern you pointed at rather
than the special case I had.

First, on subtransaction abort ri_FastPathSubXactCallback discarded the
entire cache. An entry's batch holds rows buffered by the enclosing
transaction, not just the aborting subxact -- the cache is keyed by
constraint, so a single entry can mix rows from multiple subxact levels.

That would imply having started a subtransaction and then added to the batch
without an intervening pair of CommandCounterIncrement() and
AfterTriggerBeginQuery(). That's an invalid thing for C code to do, so I'd
make it an error if a batch would contain rows from different subtransactions.

Agreed. I confirmed a batch is flushed at AfterTriggerEndQuery and
deferred checks don't populate a batch until they fire at top-level
commit, so an entry is always at a single subxact level. The "mixes
levels" justification in my commit message was a hypothesis I never
verified, and it's wrong; a can't-happen assert is most likely the
right thing.

Attached 0003 adds such an assert: Assert(fpentry->subid ==
GetCurrentSubTransactionId()) in ri_FastPathBatchAdd(), so every row
added to an entry comes from the subtransaction that created it. It
doesn't fire anywhere in the regression tests. AtEOSubXact_RI() relies
on this invariant to identify an aborting subtransaction's entries by
their stamped subid.

(This relates to the reentrancy bug fixed in 0e47bb5. With an intervening
CommandCounterIncrement() and AfterTriggerBeginQuery(), reusing the outer
batch is wrong even without subtransactions: it would check with the wrong
snapshot. Each level of reentrancy needs to finish its FK checks separately,
even if the batch buffer were unbounded.)

An internal subxact abort during after-trigger firing (e.g. a PL/pgSQL
BEGIN ... EXCEPTION block) therefore dropped buffered rows

I suspect a subxact abort during after-trigger firing can still cause a
different problem via AfterTriggerEndQuery():

AfterTriggerEndQuery(EState *estate)
{
...
afterTriggers.firing_depth++;

AfterTriggerEndSubXact() doesn't undo this increment. Having identified that
as suspect, I asked Opus 4.8 to try to confirm or refute bug reachability. I
have not personally verified its finding, but it said:

CLAUDE [CONFIRMED -- reachable bug; your instinct is right]: firing_depth is ++/-- in matched
pairs inside AfterTriggerEndQuery/FireDeferred/SetState with NO PG_TRY, and AfterTriggerEndSubXact
resets firing_batch_callbacks but NOT firing_depth. So a trigger ERROR caught by an outer
subtransaction (PL/pgSQL EXCEPTION) skips the -- and strands firing_depth>0 for the rest of the
xact. Its sole consumer is AfterTriggerIsActive() -> the RI_FKey_check batching gate; the only
RI check that runs OUTSIDE genuine trigger firing is ALTER TABLE / VALIDATE CONSTRAINT per-row
validation. With firing_depth stranded, that validation is wrongly routed into ri_FastPathBatchAdd.
Repro (per-row forced via REFERENCES-only, no SELECT, so RI_Initial_Check bails):
CONTROL: per-row-validated ALTER ADD FK with violating row 99 -> errors AT the ALTER (correct).
BUG: run a caught FK violation first (DO/EXCEPTION), then the same ALTER in the same xact ->
the ALTER does NOT error, marks convalidated=t, emits "WARNING: resource was not closed:
relation pk2 / pk2_pkey / TupleDesc" (a resource-owner leak), and defers the violation to
COMMIT. Consequences: resource leak + FK validation deferred past the ALTER (documented
invariant broken). Fix: reset firing_depth in AfterTriggerEndSubXact, mirroring the
firing_batch_callbacks reset right below it.

Even if that's a hallucination, it's an example of what I meant in (2) about
making the change harder to verify.

That looks like a real bug, likely of the same class as an earlier
error-path reset I fixed. I'll verify the reproducer; the fix is
likely resetting firing_depth in AfterTriggerEndSubXact alongside the
firing_batch_callbacks reset.

Confirmed, and it's worse than a stranded flag -- it corrupts data.
0002 to fix it. The reproducer:

A caught FK-check error inside a subtransaction (a PL/pgSQL EXCEPTION
block) skips the firing_depth-- , leaving firing_depth set. A later
ALTER TABLE ... ADD FOREIGN KEY in the same transaction, whose
validation runs per-row rather than via RI_Initial_Check()'s bulk join
(e.g. because RLS is enabled on the referenced table, so
RI_Initial_Check() bails), then calls RI_FKey_check() with
AfterTriggerIsActive() wrongly true. The check is routed into the
batched fast path -- but a utility command has no
AfterTriggerEndQuery() to fire the flush callback, so the batch is
never flushed. The violating row is not reported, the constraint is
marked convalidated = t, and the cached PK relation and index leak
("resource was not closed"). So a foreign key ends up validated with a
row that violates it. I've added this as a regression test in 0003.

On the fix: 0003 saves firing_depth at subtransaction start and
restores it at end, in AfterTriggerEndSubXact(), next to the existing
query_depth handling. Resetting to 0 instead is wrong -- a subxact can
begin and end while an outer query is firing, where firing_depth is
legitimately positive, and zeroing it there trips the firing_depth > 0
assert in FireAfterTriggerBatchCallbacks() (the fp_subxact test in
0001 and the transition-table tests in the PL suites both hit this).

Also, it's not clear to me why AfterTriggerEndSubXact() is right to reset
afterTriggers.firing_batch_callbacks. The outer xact may be firing. If
that's okay, can you expand the code comment to explain it?

I'll go back and reconstruct why that reset is correct. If I can't,
I'll treat it as suspect and address it in the rework, with a comment
either way.

The unconditional clear is wrong for the same reason: a subtransaction
that begins and ends while an outer FireAfterTriggerBatchCallbacks()
loop is active leaves firing_batch_callbacks legitimately true at
AfterTriggerEndSubXact(), and clearing it drops a flag the outer
firing still needs. 0003 gives it the same save/restore treatment as
firing_depth. I could not construct an observable failure from the
firing_batch_callbacks case specifically -- a separate guard,
ri_fastpath_flushing, routes any FK check re-entered from user code
during a flush onto the per-row path, so a re-entrant check never
reaches RegisterAfterTriggerBatchCallback() while the flag might be
wrongly cleared -- but restoring it is correct regardless and matches
the firing_depth handling.

Cleanly unwinding the cache on subxact abort would require tracking the
originating subxact of each buffered row, since rows from different
levels share an entry (the cache is keyed by constraint) and deferred
constraints cannot be flushed early at a subxact boundary.

It's true that they can't be flushed early, but I'm not seeing a need for
explicit code to avoid that. A subxact commit shall just confirm there's no
batch of its subxact level. A subxact abort shall discard any batch of its
subxact level, leaving higher-subxact batches untouched. A deferred trigger
doesn't start a batch until the end of the top-level transaction.

Yes. I was confused about the relationship between deferred firing and
subxacts, which is where that justification came from; deferred
batches only exist at the top level, so there's nothing to unwind for
them at a subxact boundary.

This is what 0001 does. Abort discards the ending level's entries;
commit leaves nothing to do at that level, since the batch was already
flushed at statement end. Deferred checks populate a batch only when
they fire at top-level commit (query depth -1), so no subxact boundary
ever has a deferred batch to unwind.

The per-row fast path still bypasses SPI and stays well ahead of the
pre-19 SPI-based check. A fuller fix that preserves batching across
subtransactions -- whether by tracking the originating subxact of each
buffered row or by per-subxact cache stacks merged into the parent on
commit -- is left for a future release.

If the above suspicion corresponds to a live bug, I'd bet on the fuller fix
being cleaner than a surgical fix. That may not pan out, but I recommend
trying it first.

Trying it in the direction you describe: subxact abort discards that
level's batch, and subxact commit just checks that there's no batch
left at that level (it was already flushed at statement end). The part
that needs care is the resource-owner handling of the cached PK
relation and index when a batch flush errors partway through inside a
subxact that then aborts. That's more involved than a subid tag. I'll
work through the details and try to post a patch tomorrow.

Done; the three patches are attached. The resource-owner handling
turned out simpler than I expected: because AtEOSubXact_RI() runs
after the subtransaction's ResourceOwnerRelease(), it only forgets the
aborting level's cache entries and never closes their relations
itself, so a batch flush that errors partway through inside a subxact
is cleaned up by the ResourceOwner on the way out.

On performance: with the top-level-txn-only confinement gone, a
savepoint-wrapped FK load no longer drops to the per-row path. On a
check-dominated test the savepoint case now matches the top-level
case. Before (i.e. with 4113873a) it was slowed by roughly 10% in my
tests, because it didn't take the same buffered path.

--
Thanks, Amit Langote

Hi,

On Thu, Jul 9, 2026 at 8:37 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Jul 6, 2026 at 11:29 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Jul 6, 2026 at 7:21 AM Noah Misch <noah@leadboat.com> wrote:

On Fri, Jun 12, 2026 at 11:46:07AM +0900, Amit Langote wrote:

I've pushed these now. Thank you everyone.

commit 4113873 wrote:

Confine RI fast-path batching to the top transaction level

I discourage this fix strategy, for three reasons:

1. It slows "BEGIN; SAVEPOINT s; COPY table_with_fk FROM ..." by the ~1.6x
batching benefit, compared to omitting the SAVEPOINT. That's a bad user
experience not seen elsewhere. Starting a high number of subtransactions
is expensive, but wrapping a long-running transaction body in one
subtransaction hasn't been a performance reducer.

Ok, I agree it's a wart.

2. It departs from the PostgreSQL norm of tracking resources by
subtransaction. You can see normal handling in many AbortSubTransaction()
callees, e.g. AtEOSubXact_LargeObject(). This in turn makes the change
harder to verify as correct.

3. It doesn't seem to have simplified code much, compared to our normal
subxact-based approach.

Fair, I added a special case, which I can see now doesn't actually
simplify things. I'll rework it to track batches per subtransaction
the normal way.

0001 does this. It drops the top-level confinement and adds
AtEOSubXact_RI(), called from Commit/AbortSubTransaction() after the
subtransaction's ResourceOwnerRelease(). On abort it discards only the
entries opened by the ending subtransaction, identified by a subid
stamped on each entry at creation; it closes nothing itself, since the
ResourceOwner has already released those relations. Entries opened at
an outer level are left alone, so an inner subxact abort during
outer-level trigger firing no longer discards the outer statement's
batch. This follows the AtEOSubXact_* pattern you pointed at rather
than the special case I had.

First, on subtransaction abort ri_FastPathSubXactCallback discarded the
entire cache. An entry's batch holds rows buffered by the enclosing
transaction, not just the aborting subxact -- the cache is keyed by
constraint, so a single entry can mix rows from multiple subxact levels.

That would imply having started a subtransaction and then added to the batch
without an intervening pair of CommandCounterIncrement() and
AfterTriggerBeginQuery(). That's an invalid thing for C code to do, so I'd
make it an error if a batch would contain rows from different subtransactions.

Agreed. I confirmed a batch is flushed at AfterTriggerEndQuery and
deferred checks don't populate a batch until they fire at top-level
commit, so an entry is always at a single subxact level. The "mixes
levels" justification in my commit message was a hypothesis I never
verified, and it's wrong; a can't-happen assert is most likely the
right thing.

Attached 0003 adds such an assert: Assert(fpentry->subid ==
GetCurrentSubTransactionId()) in ri_FastPathBatchAdd(), so every row
added to an entry comes from the subtransaction that created it. It
doesn't fire anywhere in the regression tests. AtEOSubXact_RI() relies
on this invariant to identify an aborting subtransaction's entries by
their stamped subid.

(This relates to the reentrancy bug fixed in 0e47bb5. With an intervening
CommandCounterIncrement() and AfterTriggerBeginQuery(), reusing the outer
batch is wrong even without subtransactions: it would check with the wrong
snapshot. Each level of reentrancy needs to finish its FK checks separately,
even if the batch buffer were unbounded.)

An internal subxact abort during after-trigger firing (e.g. a PL/pgSQL
BEGIN ... EXCEPTION block) therefore dropped buffered rows

I suspect a subxact abort during after-trigger firing can still cause a
different problem via AfterTriggerEndQuery():

AfterTriggerEndQuery(EState *estate)
{
...
afterTriggers.firing_depth++;

AfterTriggerEndSubXact() doesn't undo this increment. Having identified that
as suspect, I asked Opus 4.8 to try to confirm or refute bug reachability. I
have not personally verified its finding, but it said:

CLAUDE [CONFIRMED -- reachable bug; your instinct is right]: firing_depth is ++/-- in matched
pairs inside AfterTriggerEndQuery/FireDeferred/SetState with NO PG_TRY, and AfterTriggerEndSubXact
resets firing_batch_callbacks but NOT firing_depth. So a trigger ERROR caught by an outer
subtransaction (PL/pgSQL EXCEPTION) skips the -- and strands firing_depth>0 for the rest of the
xact. Its sole consumer is AfterTriggerIsActive() -> the RI_FKey_check batching gate; the only
RI check that runs OUTSIDE genuine trigger firing is ALTER TABLE / VALIDATE CONSTRAINT per-row
validation. With firing_depth stranded, that validation is wrongly routed into ri_FastPathBatchAdd.
Repro (per-row forced via REFERENCES-only, no SELECT, so RI_Initial_Check bails):
CONTROL: per-row-validated ALTER ADD FK with violating row 99 -> errors AT the ALTER (correct).
BUG: run a caught FK violation first (DO/EXCEPTION), then the same ALTER in the same xact ->
the ALTER does NOT error, marks convalidated=t, emits "WARNING: resource was not closed:
relation pk2 / pk2_pkey / TupleDesc" (a resource-owner leak), and defers the violation to
COMMIT. Consequences: resource leak + FK validation deferred past the ALTER (documented
invariant broken). Fix: reset firing_depth in AfterTriggerEndSubXact, mirroring the
firing_batch_callbacks reset right below it.

Even if that's a hallucination, it's an example of what I meant in (2) about
making the change harder to verify.

That looks like a real bug, likely of the same class as an earlier
error-path reset I fixed. I'll verify the reproducer; the fix is
likely resetting firing_depth in AfterTriggerEndSubXact alongside the
firing_batch_callbacks reset.

Confirmed, and it's worse than a stranded flag -- it corrupts data.
0002 to fix it. The reproducer:

A caught FK-check error inside a subtransaction (a PL/pgSQL EXCEPTION
block) skips the firing_depth-- , leaving firing_depth set. A later
ALTER TABLE ... ADD FOREIGN KEY in the same transaction, whose
validation runs per-row rather than via RI_Initial_Check()'s bulk join
(e.g. because RLS is enabled on the referenced table, so
RI_Initial_Check() bails), then calls RI_FKey_check() with
AfterTriggerIsActive() wrongly true. The check is routed into the
batched fast path -- but a utility command has no
AfterTriggerEndQuery() to fire the flush callback, so the batch is
never flushed. The violating row is not reported, the constraint is
marked convalidated = t, and the cached PK relation and index leak
("resource was not closed"). So a foreign key ends up validated with a
row that violates it. I've added this as a regression test in 0003.

On the fix: 0003 saves firing_depth at subtransaction start and
restores it at end, in AfterTriggerEndSubXact(), next to the existing
query_depth handling. Resetting to 0 instead is wrong -- a subxact can
begin and end while an outer query is firing, where firing_depth is
legitimately positive, and zeroing it there trips the firing_depth > 0
assert in FireAfterTriggerBatchCallbacks() (the fp_subxact test in
0001 and the transition-table tests in the PL suites both hit this).

Also, it's not clear to me why AfterTriggerEndSubXact() is right to reset
afterTriggers.firing_batch_callbacks. The outer xact may be firing. If
that's okay, can you expand the code comment to explain it?

I'll go back and reconstruct why that reset is correct. If I can't,
I'll treat it as suspect and address it in the rework, with a comment
either way.

The unconditional clear is wrong for the same reason: a subtransaction
that begins and ends while an outer FireAfterTriggerBatchCallbacks()
loop is active leaves firing_batch_callbacks legitimately true at
AfterTriggerEndSubXact(), and clearing it drops a flag the outer
firing still needs. 0003 gives it the same save/restore treatment as
firing_depth. I could not construct an observable failure from the
firing_batch_callbacks case specifically -- a separate guard,
ri_fastpath_flushing, routes any FK check re-entered from user code
during a flush onto the per-row path, so a re-entrant check never
reaches RegisterAfterTriggerBatchCallback() while the flag might be
wrongly cleared -- but restoring it is correct regardless and matches
the firing_depth handling.

Cleanly unwinding the cache on subxact abort would require tracking the
originating subxact of each buffered row, since rows from different
levels share an entry (the cache is keyed by constraint) and deferred
constraints cannot be flushed early at a subxact boundary.

It's true that they can't be flushed early, but I'm not seeing a need for
explicit code to avoid that. A subxact commit shall just confirm there's no
batch of its subxact level. A subxact abort shall discard any batch of its
subxact level, leaving higher-subxact batches untouched. A deferred trigger
doesn't start a batch until the end of the top-level transaction.

Yes. I was confused about the relationship between deferred firing and
subxacts, which is where that justification came from; deferred
batches only exist at the top level, so there's nothing to unwind for
them at a subxact boundary.

This is what 0001 does. Abort discards the ending level's entries;
commit leaves nothing to do at that level, since the batch was already
flushed at statement end. Deferred checks populate a batch only when
they fire at top-level commit (query depth -1), so no subxact boundary
ever has a deferred batch to unwind.

The per-row fast path still bypasses SPI and stays well ahead of the
pre-19 SPI-based check. A fuller fix that preserves batching across
subtransactions -- whether by tracking the originating subxact of each
buffered row or by per-subxact cache stacks merged into the parent on
commit -- is left for a future release.

If the above suspicion corresponds to a live bug, I'd bet on the fuller fix
being cleaner than a surgical fix. That may not pan out, but I recommend
trying it first.

Trying it in the direction you describe: subxact abort discards that
level's batch, and subxact commit just checks that there's no batch
left at that level (it was already flushed at statement end). The part
that needs care is the resource-owner handling of the cached PK
relation and index when a batch flush errors partway through inside a
subxact that then aborts. That's more involved than a subid tag. I'll
work through the details and try to post a patch tomorrow.

Done; the three patches are attached. The resource-owner handling
turned out simpler than I expected: because AtEOSubXact_RI() runs
after the subtransaction's ResourceOwnerRelease(), it only forgets the
aborting level's cache entries and never closes their relations
itself, so a batch flush that errors partway through inside a subxact
is cleaned up by the ResourceOwner on the way out.

Would like to commit these tomorrow after review, barring objections.

--
Thanks, Amit Langote

Hi,

On Fri, Aug 7, 2026 at 10:15 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Jul 9, 2026 at 8:37 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Jul 6, 2026 at 11:29 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Jul 6, 2026 at 7:21 AM Noah Misch <noah@leadboat.com> wrote:

On Fri, Jun 12, 2026 at 11:46:07AM +0900, Amit Langote wrote:

I've pushed these now. Thank you everyone.

commit 4113873 wrote:

Confine RI fast-path batching to the top transaction level

I discourage this fix strategy, for three reasons:

1. It slows "BEGIN; SAVEPOINT s; COPY table_with_fk FROM ..." by the ~1.6x
batching benefit, compared to omitting the SAVEPOINT. That's a bad user
experience not seen elsewhere. Starting a high number of subtransactions
is expensive, but wrapping a long-running transaction body in one
subtransaction hasn't been a performance reducer.

Ok, I agree it's a wart.

2. It departs from the PostgreSQL norm of tracking resources by
subtransaction. You can see normal handling in many AbortSubTransaction()
callees, e.g. AtEOSubXact_LargeObject(). This in turn makes the change
harder to verify as correct.

3. It doesn't seem to have simplified code much, compared to our normal
subxact-based approach.

Fair, I added a special case, which I can see now doesn't actually
simplify things. I'll rework it to track batches per subtransaction
the normal way.

0001 does this. It drops the top-level confinement and adds
AtEOSubXact_RI(), called from Commit/AbortSubTransaction() after the
subtransaction's ResourceOwnerRelease(). On abort it discards only the
entries opened by the ending subtransaction, identified by a subid
stamped on each entry at creation; it closes nothing itself, since the
ResourceOwner has already released those relations. Entries opened at
an outer level are left alone, so an inner subxact abort during
outer-level trigger firing no longer discards the outer statement's
batch. This follows the AtEOSubXact_* pattern you pointed at rather
than the special case I had.

<snip>

The per-row fast path still bypasses SPI and stays well ahead of the
pre-19 SPI-based check. A fuller fix that preserves batching across
subtransactions -- whether by tracking the originating subxact of each
buffered row or by per-subxact cache stacks merged into the parent on
commit -- is left for a future release.

If the above suspicion corresponds to a live bug, I'd bet on the fuller fix
being cleaner than a surgical fix. That may not pan out, but I recommend
trying it first.

Trying it in the direction you describe: subxact abort discards that
level's batch, and subxact commit just checks that there's no batch
left at that level (it was already flushed at statement end). The part
that needs care is the resource-owner handling of the cached PK
relation and index when a batch flush errors partway through inside a
subxact that then aborts. That's more involved than a subid tag. I'll
work through the details and try to post a patch tomorrow.

Done; the three patches are attached. The resource-owner handling
turned out simpler than I expected: because AtEOSubXact_RI() runs
after the subtransaction's ResourceOwnerRelease(), it only forgets the
aborting level's cache entries and never closes their relations
itself, so a batch flush that errors partway through inside a subxact
is cleaned up by the ResourceOwner on the way out.

Would like to commit these tomorrow after review, barring objections.

After considering Peter's report of another bug [1]/messages/by-id/CAH2-Wz=D533JbF_ak_Pc8kP0FKse-ju8DnMxtjvY==yHsP4xgw@mail.gmail.com that is fixed by
0001, I tested both his reproducer and nested firing involving the
same constraint.

The latter exposed a problem in v1: the cache was still keyed only by
constraint OID, so a nested check of the same constraint reused the
outer entry and did not register a callback at the nested query depth.
In v2, the key is now (constraint OID, query depth), giving each
firing level its own entry and callback.

0001 now includes regression tests for both cases. 0002 and 0003 are unchanged.

I would like to commit these sometime this week and would appreciate a review.

[1]: /messages/by-id/CAH2-Wz=D533JbF_ak_Pc8kP0FKse-ju8DnMxtjvY==yHsP4xgw@mail.gmail.com

--
Thanks, Amit Langote

On Tue, Aug 18, 2026 at 10:42:14PM +0900, Amit Langote wrote:

After considering Peter's report of another bug [1] that is fixed by
0001, I tested both his reproducer and nested firing involving the
same constraint.

The latter exposed a problem in v1: the cache was still keyed only by
constraint OID, so a nested check of the same constraint reused the
outer entry and did not register a callback at the nested query depth.
In v2, the key is now (constraint OID, query depth), giving each
firing level its own entry and callback.

0001 now includes regression tests for both cases. 0002 and 0003 are unchanged.

I would like to commit these sometime this week and would appreciate a review.

I won't be able to review this. The list should consider it up for grabs.