public inbox for [email protected]
help / color / mirror / Atom feedPG19 FK fast path: OOB write and missed FK checks during batched
17+ messages / 5 participants
[nested] [flat]
* PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-06-06 08:30 Nikolay Samokhvalov <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Nikolay Samokhvalov @ 2026-06-06 08:30 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>; [email protected] <[email protected]>
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-06-06 09:13 Amit Langote <[email protected]>
parent: Nikolay Samokhvalov <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Amit Langote @ 2026-06-06 09:13 UTC (permalink / raw)
To: Nikolay Samokhvalov <[email protected]>; +Cc: pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
On Sat, Jun 6, 2026 at 17:31 Nikolay Samokhvalov <[email protected]> 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
>
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-06-08 08:18 Amit Langote <[email protected]>
parent: Amit Langote <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Amit Langote @ 2026-06-08 08:18 UTC (permalink / raw)
To: Nikolay Samokhvalov <[email protected]>; +Cc: pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
On Sat, Jun 6, 2026 at 6:13 PM Amit Langote <[email protected]> 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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-06-09 13:31 Amit Langote <[email protected]>
parent: Amit Langote <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Amit Langote @ 2026-06-09 13:31 UTC (permalink / raw)
To: Nikolay Samokhvalov <[email protected]>; +Cc: pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
On Mon, Jun 8, 2026 at 5:18 PM Amit Langote <[email protected]> wrote:
> On Sat, Jun 6, 2026 at 6:13 PM Amit Langote <[email protected]> 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
Attachments:
[application/octet-stream] v1-0001-Fix-out-of-bounds-write-in-RI-fast-path-batch-on-.patch (10.0K, ../../CA+HiwqHUz50YqJn4XiNsSLN2c+9eYBy1af=y_dfdJTsz5BmbJg@mail.gmail.com/2-v1-0001-Fix-out-of-bounds-write-in-RI-fast-path-batch-on-.patch)
download | inline diff:
From 00fa1c1137009cd64fb04b718c1de4c8c130f08e Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Tue, 9 Jun 2026 18:27:24 +0900
Subject: [PATCH v1 1/2] Fix out-of-bounds write in RI fast-path batch on
re-entry
The FK fast-path batching added in b7b27eb41a5 wrote the incoming row
into the batch array before checking whether the array was full:
fpentry->batch[fpentry->batch_count] = ExecCopySlotHeapTuple(newslot);
fpentry->batch_count++;
if (fpentry->batch_count >= RI_FASTPATH_BATCH_SIZE)
ri_FastPathBatchFlush(fpentry, fk_rel, riinfo);
batch_count is reset to zero only at the end of ri_FastPathBatchFlush(),
so it remains at RI_FASTPATH_BATCH_SIZE throughout a full-batch flush.
A flush runs user-defined cast functions and equality operators; if that
user code performs DML on the same FK table, ri_FastPathBatchAdd()
re-enters with batch_count == RI_FASTPATH_BATCH_SIZE and writes one past
the end of the array, corrupting the adjacent batch_count field. This
is reachable by an unprivileged table owner via an implicit cast with a
PL/pgSQL function and causes a SIGSEGV in assert-enabled builds.
Fix by checking whether the batch is full and flushing before writing
the new row, and by adding a "flushing" flag to RI_FastPathEntry that
routes re-entrant ri_FastPathBatchAdd() calls on a busy entry to the
per-row path (ri_FastPathCheck) instead of touching the mid-flush batch
array. The flag is set around the probe in ri_FastPathBatchFlush() and
cleared in a PG_FINALLY, which also resets batch_count, so the entry is
left empty and reusable if a flush error (including a reported FK
violation) is caught by a savepoint.
Add regression tests for both the re-entrant flush and reuse of an entry
after a flush error caught by a savepoint.
Reported-by: Nikolay Samokhvalov <[email protected]>
Discussion: https://postgr.es/m/CAM527d9exRCdWrhJOnAxk_vACg7sr_yPoaJp_+uCFY0qP8v=aw@mail.gmail.com
---
src/backend/utils/adt/ri_triggers.c | 58 ++++++++++++++++++-----
src/test/regress/expected/foreign_key.out | 56 ++++++++++++++++++++++
src/test/regress/sql/foreign_key.sql | 46 ++++++++++++++++++
3 files changed, 147 insertions(+), 13 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index dc89c686394..453b83ce85d 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -249,6 +249,12 @@ typedef struct RI_FastPathEntry
*/
HeapTuple batch[RI_FASTPATH_BATCH_SIZE];
int batch_count;
+
+ /*
+ * true while this entry's batch is being flushed; guards against
+ * re-entrant ri_FastPathBatchAdd from user code run during the flush.
+ */
+ bool flushing;
} RI_FastPathEntry;
/*
@@ -2862,14 +2868,26 @@ ri_FastPathBatchAdd(RI_ConstraintInfo *riinfo,
RI_FastPathEntry *fpentry = ri_FastPathGetEntry(riinfo, fk_rel);
MemoryContext oldcxt;
+ /*
+ * If this entry is already being flushed, a cast function or an operator
+ * invoked during the flush has re-entered with DML on the same FK. Fall
+ * back to the per-row path rather than touching the batch array, which is
+ * mid-flush.
+ */
+ if (fpentry->flushing)
+ {
+ ri_FastPathCheck(riinfo, fk_rel, newslot);
+ return;
+ }
+
+ if (fpentry->batch_count >= RI_FASTPATH_BATCH_SIZE)
+ ri_FastPathBatchFlush(fpentry, fk_rel, riinfo);
+
oldcxt = MemoryContextSwitchTo(fpentry->flush_cxt);
fpentry->batch[fpentry->batch_count] =
ExecCopySlotHeapTuple(newslot);
fpentry->batch_count++;
MemoryContextSwitchTo(oldcxt);
-
- if (fpentry->batch_count >= RI_FASTPATH_BATCH_SIZE)
- ri_FastPathBatchFlush(fpentry, fk_rel, riinfo);
}
/*
@@ -2944,13 +2962,30 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel,
}
Assert(riinfo->fpmeta);
- /* Skip array overhead for single-row batches. */
- if (riinfo->nkeys == 1 && fpentry->batch_count > 1)
- violation_index = ri_FastPathFlushArray(fpentry, fk_slot, riinfo,
- fk_rel, snapshot, scandesc);
- else
- violation_index = ri_FastPathFlushLoop(fpentry, fk_slot, riinfo,
- fk_rel, snapshot, scandesc);
+ /*
+ * The probe runs user-defined cast and equality functions. Set the
+ * flushing flag around it so a re-entrant ri_FastPathBatchAdd on this
+ * entry takes the per-row path, and clear it even on error so the entry
+ * is reusable if the error is caught by a savepoint.
+ */
+ Assert(!fpentry->flushing);
+ fpentry->flushing = true;
+ PG_TRY();
+ {
+ /* Skip array overhead for single-row batches. */
+ if (riinfo->nkeys == 1 && fpentry->batch_count > 1)
+ violation_index = ri_FastPathFlushArray(fpentry, fk_slot, riinfo,
+ fk_rel, snapshot, scandesc);
+ else
+ violation_index = ri_FastPathFlushLoop(fpentry, fk_slot, riinfo,
+ fk_rel, snapshot, scandesc);
+ }
+ PG_FINALLY();
+ {
+ fpentry->flushing = false;
+ fpentry->batch_count = 0;
+ }
+ PG_END_TRY();
SetUserIdAndSecContext(saved_userid, saved_sec_context);
UnregisterSnapshot(snapshot);
@@ -2966,9 +3001,6 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel,
MemoryContextReset(fpentry->flush_cxt);
MemoryContextSwitchTo(oldcxt);
-
- /* Reset. */
- fpentry->batch_count = 0;
}
/*
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 8b3b268de0f..fa80d9c915f 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -3712,3 +3712,59 @@ INSERT INTO fp_pk_dup VALUES (1);
CREATE TABLE fp_fk_dup (a int REFERENCES fp_pk_dup);
INSERT INTO fp_fk_dup SELECT 1 FROM generate_series(1, 100);
DROP TABLE fp_fk_dup, fp_pk_dup;
+-- Re-entrant FK fast-path: DML on the same FK table from a cast function
+-- during a full-batch flush must not corrupt the batch array.
+CREATE TABLE fp_reentry_pk (id int PRIMARY KEY);
+INSERT INTO fp_reentry_pk VALUES (1), (2);
+CREATE TYPE fp_vch AS (v int);
+CREATE FUNCTION fp_vcast(fp_vch) RETURNS int LANGUAGE plpgsql AS $$
+BEGIN
+ IF $1.v = 1 THEN
+ INSERT INTO fp_reentry_fk VALUES (row(2)::fp_vch);
+ END IF;
+ RETURN $1.v;
+END$$;
+CREATE CAST (fp_vch AS int) WITH FUNCTION fp_vcast(fp_vch) AS IMPLICIT;
+CREATE TABLE fp_reentry_fk (a fp_vch
+ REFERENCES fp_reentry_pk (id));
+-- Fill exactly one batch so the flush fires; the cast re-enters with DML
+-- on the same FK and must take the per-row path.
+INSERT INTO fp_reentry_fk SELECT row(1)::fp_vch FROM generate_series(1, 64);
+SELECT a, count(*) FROM fp_reentry_fk GROUP BY a ORDER BY a;
+ a | count
+-----+-------
+ (1) | 64
+ (2) | 64
+(2 rows)
+
+DROP TABLE fp_reentry_fk, fp_reentry_pk;
+DROP CAST (fp_vch AS int);
+DROP FUNCTION fp_vcast(fp_vch);
+DROP TYPE fp_vch;
+-- Flush error caught by a savepoint must leave the entry empty and reusable.
+CREATE TABLE fp_reentry_pk2 (id int PRIMARY KEY);
+INSERT INTO fp_reentry_pk2 VALUES (1);
+CREATE TABLE fp_reentry_fk2 (a int REFERENCES fp_reentry_pk2 (id));
+DO $$
+BEGIN
+ -- A batch containing a violating row; the flush reports the violation.
+ BEGIN
+ INSERT INTO fp_reentry_fk2 SELECT CASE WHEN g = 32 THEN 999 ELSE 1 END
+ FROM generate_series(1, 64) g;
+ EXCEPTION WHEN foreign_key_violation THEN
+ RAISE NOTICE 'caught fk violation';
+ END;
+
+ -- Reuse the same FK with a full batch in the same transaction. The
+ -- entry must be empty after the caught violation: no stale rows from the
+ -- rolled-back batch (in particular no 999), and no array overflow.
+ INSERT INTO fp_reentry_fk2 SELECT 1 FROM generate_series(1, 64);
+END$$;
+NOTICE: caught fk violation
+SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1
+ count | max
+-------+-----
+ 64 | 1
+(1 row)
+
+DROP TABLE fp_reentry_fk2, fp_reentry_pk2;
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 7eb86b188f0..0f2ce8f76f5 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -2680,3 +2680,49 @@ INSERT INTO fp_pk_dup VALUES (1);
CREATE TABLE fp_fk_dup (a int REFERENCES fp_pk_dup);
INSERT INTO fp_fk_dup SELECT 1 FROM generate_series(1, 100);
DROP TABLE fp_fk_dup, fp_pk_dup;
+
+-- Re-entrant FK fast-path: DML on the same FK table from a cast function
+-- during a full-batch flush must not corrupt the batch array.
+CREATE TABLE fp_reentry_pk (id int PRIMARY KEY);
+INSERT INTO fp_reentry_pk VALUES (1), (2);
+CREATE TYPE fp_vch AS (v int);
+CREATE FUNCTION fp_vcast(fp_vch) RETURNS int LANGUAGE plpgsql AS $$
+BEGIN
+ IF $1.v = 1 THEN
+ INSERT INTO fp_reentry_fk VALUES (row(2)::fp_vch);
+ END IF;
+ RETURN $1.v;
+END$$;
+CREATE CAST (fp_vch AS int) WITH FUNCTION fp_vcast(fp_vch) AS IMPLICIT;
+CREATE TABLE fp_reentry_fk (a fp_vch
+ REFERENCES fp_reentry_pk (id));
+-- Fill exactly one batch so the flush fires; the cast re-enters with DML
+-- on the same FK and must take the per-row path.
+INSERT INTO fp_reentry_fk SELECT row(1)::fp_vch FROM generate_series(1, 64);
+SELECT a, count(*) FROM fp_reentry_fk GROUP BY a ORDER BY a;
+DROP TABLE fp_reentry_fk, fp_reentry_pk;
+DROP CAST (fp_vch AS int);
+DROP FUNCTION fp_vcast(fp_vch);
+DROP TYPE fp_vch;
+
+-- Flush error caught by a savepoint must leave the entry empty and reusable.
+CREATE TABLE fp_reentry_pk2 (id int PRIMARY KEY);
+INSERT INTO fp_reentry_pk2 VALUES (1);
+CREATE TABLE fp_reentry_fk2 (a int REFERENCES fp_reentry_pk2 (id));
+DO $$
+BEGIN
+ -- A batch containing a violating row; the flush reports the violation.
+ BEGIN
+ INSERT INTO fp_reentry_fk2 SELECT CASE WHEN g = 32 THEN 999 ELSE 1 END
+ FROM generate_series(1, 64) g;
+ EXCEPTION WHEN foreign_key_violation THEN
+ RAISE NOTICE 'caught fk violation';
+ END;
+
+ -- Reuse the same FK with a full batch in the same transaction. The
+ -- entry must be empty after the caught violation: no stale rows from the
+ -- rolled-back batch (in particular no 999), and no array overflow.
+ INSERT INTO fp_reentry_fk2 SELECT 1 FROM generate_series(1, 64);
+END$$;
+SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1
+DROP TABLE fp_reentry_fk2, fp_reentry_pk2;
--
2.47.3
[application/octet-stream] v1-0002-Confine-RI-fast-path-batching-to-the-top-transact.patch (10.9K, ../../CA+HiwqHUz50YqJn4XiNsSLN2c+9eYBy1af=y_dfdJTsz5BmbJg@mail.gmail.com/3-v1-0002-Confine-RI-fast-path-batching-to-the-top-transact.patch)
download | inline diff:
From d136a8f5edc723899daafb59377930ee7fec3838 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Tue, 9 Jun 2026 22:11:54 +0900
Subject: [PATCH v1 2/2] Confine RI fast-path batching to the top transaction
level
The FK fast-path batching added in b7b27eb41a5 buffers rows in a
transaction-lived cache (ri_fastpath_cache) keyed by constraint OID.
Running user-defined cast and equality functions during a batch flush,
together with the cache's lifetime and iteration, exposed two defects
reachable by an unprivileged table owner.
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.
An internal subxact abort during after-trigger firing (e.g. a PL/pgSQL
BEGIN ... EXCEPTION block) therefore dropped buffered rows of the outer
transaction without running their FK checks, letting orphan rows commit
behind a constraint that still reported itself valid. The discard also
left relations opened by the batch unclosed, producing "resource was not
closed" warnings.
Second, ri_FastPathEndBatch flushes by iterating the cache with
hash_seq_search. If flush-time user code inserts into a different
fast-path FK table, a new entry is added to the cache mid-scan; it may
land in a bucket the scan has already passed and never be reached, and
ri_FastPathTeardown then destroys the cache without flushing it,
silently dropping that check.
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. Rather than
add that bookkeeping, confine batching to the top transaction level: in
RI_FKey_check, when GetCurrentTransactionNestLevel() > 1, use the
per-row fast path (ri_FastPathCheck) instead of buffering. Rows checked
inside a subtransaction are then verified immediately and roll back
cleanly with their subtransaction, and the cache only ever holds
top-level rows. With the cache confined to the top level, a
subtransaction abort has nothing of its own to discard, so
ri_FastPathSubXactCallback is removed along with its registration.
For the second defect, add a cache-wide flag (ri_fastpath_flushing) set
while ri_FastPathEndBatch iterates the cache. A re-entrant FK check
arriving while the flag is set takes the per-row path rather than adding
an entry to the cache being scanned, so no entry can be missed and torn
down unflushed. The flag is cleared in a PG_FINALLY so a flush that
throws (a reported violation or an error from user code) does not leave
it stuck.
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.
The subtransaction-abort case is covered by a new regression test. The
mid-scan cross-table case depends on hash bucket placement and so is not
reliably reproducible in a portable test, but the flag prevents it by
construction.
Reported-by: Nikolay Samokhvalov <[email protected]>
Discussion: https://postgr.es/m/CAM527d9exRCdWrhJOnAxk_vACg7sr_yPoaJp_+uCFY0qP8v=aw@mail.gmail.com
---
src/backend/utils/adt/ri_triggers.c | 77 +++++++++++++++--------
src/test/regress/expected/foreign_key.out | 24 +++++++
src/test/regress/sql/foreign_key.sql | 23 +++++++
3 files changed, 97 insertions(+), 27 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 453b83ce85d..9b49d87279f 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -267,6 +267,7 @@ static dclist_head ri_constraint_cache_valid_list;
static HTAB *ri_fastpath_cache = NULL;
static bool ri_fastpath_callback_registered = false;
+static bool ri_fastpath_flushing = false;
/*
* Local function prototypes
@@ -469,14 +470,31 @@ RI_FKey_check(TriggerData *trigdata)
*/
if (ri_fastpath_is_applicable(riinfo))
{
- if (AfterTriggerIsActive())
+ if (AfterTriggerIsActive() &&
+ GetCurrentTransactionNestLevel() == 1 &&
+ !ri_fastpath_flushing)
{
/* Batched path: buffer and probe in groups */
ri_FastPathBatchAdd(riinfo, fk_rel, newslot);
}
else
{
- /* ALTER TABLE validation: per-row, no cache */
+ /*
+ * Per-row path, used when batching is not safe or not
+ * applicable:
+ *
+ * - ALTER TABLE validation, where no after-trigger firing is
+ * active;
+ *
+ * - any FK check inside a subtransaction, since the batch cache
+ * is confined to the top transaction level (it cannot be
+ * cleanly unwound on subxact abort);
+ *
+ * - a re-entrant check from user cast/operator code running
+ * during a batch flush, since adding a cache entry while
+ * ri_FastPathEndBatch is iterating the cache could leave it
+ * unflushed.
+ */
ri_FastPathCheck(riinfo, fk_rel, newslot);
}
return PointerGetDatum(NULL);
@@ -4170,19 +4188,41 @@ ri_FastPathEndBatch(void *arg)
if (ri_fastpath_cache == NULL)
return;
- /* Flush any partial batches -- can throw ERROR */
- hash_seq_init(&status, ri_fastpath_cache);
- while ((entry = hash_seq_search(&status)) != NULL)
+ /*
+ * Set a flag for the duration of the scan so that any FK check triggered
+ * by user cast or operator code during a flush takes the per-row path
+ * instead of adding a new entry to the cache we are iterating. A new
+ * entry could land in an already-scanned bucket and then be torn down
+ * unflushed below.
+ *
+ * The flush can throw ERROR (a reported constraint violation, or an error
+ * from the user code it runs). In that case ri_FastPathTeardown below is
+ * skipped; the ResourceOwner and the transaction-end callback handle
+ * resource cleanup on the abort path. The PG_FINALLY only resets the
+ * flag and deliberately does not attempt teardown.
+ */
+ Assert(!ri_fastpath_flushing);
+ ri_fastpath_flushing = true;
+ PG_TRY();
{
- if (entry->batch_count > 0)
+ hash_seq_init(&status, ri_fastpath_cache);
+ while ((entry = hash_seq_search(&status)) != NULL)
{
- Relation fk_rel = table_open(entry->fk_relid, AccessShareLock);
- RI_ConstraintInfo *riinfo = ri_LoadConstraintInfo(entry->conoid);
+ if (entry->batch_count > 0)
+ {
+ Relation fk_rel = table_open(entry->fk_relid, AccessShareLock);
+ RI_ConstraintInfo *riinfo = ri_LoadConstraintInfo(entry->conoid);
- ri_FastPathBatchFlush(entry, fk_rel, riinfo);
- table_close(fk_rel, NoLock);
+ ri_FastPathBatchFlush(entry, fk_rel, riinfo);
+ table_close(fk_rel, NoLock);
+ }
}
}
+ PG_FINALLY();
+ {
+ ri_fastpath_flushing = false;
+ }
+ PG_END_TRY();
ri_FastPathTeardown();
}
@@ -4236,22 +4276,6 @@ ri_FastPathXactCallback(XactEvent event, void *arg)
ri_fastpath_callback_registered = false;
}
-static void
-ri_FastPathSubXactCallback(SubXactEvent event, SubTransactionId mySubid,
- SubTransactionId parentSubid, void *arg)
-{
- if (event == SUBXACT_EVENT_ABORT_SUB)
- {
- /*
- * ResourceOwner already released relations. NULL the static pointers
- * so the still-registered batch callback becomes a no-op for the rest
- * of this transaction.
- */
- ri_fastpath_cache = NULL;
- ri_fastpath_callback_registered = false;
- }
-}
-
/*
* ri_FastPathGetEntry
* Look up or create a per-batch cache entry for the given constraint.
@@ -4276,7 +4300,6 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel)
if (!ri_fastpath_xact_callback_registered)
{
RegisterXactCallback(ri_FastPathXactCallback, NULL);
- RegisterSubXactCallback(ri_FastPathSubXactCallback, NULL);
ri_fastpath_xact_callback_registered = true;
}
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index fa80d9c915f..3f99a4a59cc 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -3768,3 +3768,27 @@ SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1
(1 row)
DROP TABLE fp_reentry_fk2, fp_reentry_pk2;
+-- Subtransaction abort during after-trigger firing must not drop FK checks
+-- for rows buffered earlier in the same statement. Batching is confined to
+-- the top transaction level and the buffered batch is no longer discarded on
+-- subxact abort, so the violating rows are detected.
+CREATE TABLE fp_subxact_pk (id int PRIMARY KEY);
+INSERT INTO fp_subxact_pk SELECT g FROM generate_series(1, 10) g;
+CREATE TABLE fp_subxact_fk (a int, tag text);
+ALTER TABLE fp_subxact_fk ADD CONSTRAINT fp_subxact_fk_fkey
+ FOREIGN KEY (a) REFERENCES fp_subxact_pk (id);
+CREATE FUNCTION fp_abort_subxact() RETURNS trigger LANGUAGE plpgsql AS $$
+BEGIN
+ IF NEW.tag = 'boom' THEN
+ BEGIN PERFORM 1/0; EXCEPTION WHEN division_by_zero THEN NULL; END;
+ END IF;
+ RETURN NEW;
+END$$;
+CREATE TRIGGER fp_subxact_trg AFTER INSERT ON fp_subxact_fk
+ FOR EACH ROW EXECUTE FUNCTION fp_abort_subxact();
+INSERT INTO fp_subxact_fk VALUES (999, 'bad'), (0, 'boom'), (1, 'ok');
+ERROR: insert or update on table "fp_subxact_fk" violates foreign key constraint "fp_subxact_fk_fkey"
+DETAIL: Key (a)=(999) is not present in table "fp_subxact_pk".
+DROP TRIGGER fp_subxact_trg ON fp_subxact_fk;
+DROP FUNCTION fp_abort_subxact();
+DROP TABLE fp_subxact_fk, fp_subxact_pk;
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 0f2ce8f76f5..18c3e166f02 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -2726,3 +2726,26 @@ BEGIN
END$$;
SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1
DROP TABLE fp_reentry_fk2, fp_reentry_pk2;
+
+-- Subtransaction abort during after-trigger firing must not drop FK checks
+-- for rows buffered earlier in the same statement. Batching is confined to
+-- the top transaction level and the buffered batch is no longer discarded on
+-- subxact abort, so the violating rows are detected.
+CREATE TABLE fp_subxact_pk (id int PRIMARY KEY);
+INSERT INTO fp_subxact_pk SELECT g FROM generate_series(1, 10) g;
+CREATE TABLE fp_subxact_fk (a int, tag text);
+ALTER TABLE fp_subxact_fk ADD CONSTRAINT fp_subxact_fk_fkey
+ FOREIGN KEY (a) REFERENCES fp_subxact_pk (id);
+CREATE FUNCTION fp_abort_subxact() RETURNS trigger LANGUAGE plpgsql AS $$
+BEGIN
+ IF NEW.tag = 'boom' THEN
+ BEGIN PERFORM 1/0; EXCEPTION WHEN division_by_zero THEN NULL; END;
+ END IF;
+ RETURN NEW;
+END$$;
+CREATE TRIGGER fp_subxact_trg AFTER INSERT ON fp_subxact_fk
+ FOR EACH ROW EXECUTE FUNCTION fp_abort_subxact();
+INSERT INTO fp_subxact_fk VALUES (999, 'bad'), (0, 'boom'), (1, 'ok');
+DROP TRIGGER fp_subxact_trg ON fp_subxact_fk;
+DROP FUNCTION fp_abort_subxact();
+DROP TABLE fp_subxact_fk, fp_subxact_pk;
--
2.47.3
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-06-10 08:16 Nikolay Samokhvalov <[email protected]>
parent: Amit Langote <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Nikolay Samokhvalov @ 2026-06-10 08:16 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
On Tue, Jun 9, 2026 at 6:31 AM Amit Langote <[email protected]> wrote:
> On Mon, Jun 8, 2026 at 5:18 PM Amit Langote <[email protected]>
> wrote:
> > On Sat, Jun 6, 2026 at 6:13 PM Amit Langote <[email protected]>
> 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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-06-10 08:32 Amit Langote <[email protected]>
parent: Nikolay Samokhvalov <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Amit Langote @ 2026-06-10 08:32 UTC (permalink / raw)
To: Nikolay Samokhvalov <[email protected]>; +Cc: pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
On Wed, Jun 10, 2026 at 5:16 PM Nikolay Samokhvalov <[email protected]> wrote:
> On Tue, Jun 9, 2026 at 6:31 AM Amit Langote <[email protected]> wrote:
>> On Mon, Jun 8, 2026 at 5:18 PM Amit Langote <[email protected]> wrote:
>> > On Sat, Jun 6, 2026 at 6:13 PM Amit Langote <[email protected]> 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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-06-10 10:08 Ayush Tiwari <[email protected]>
parent: Amit Langote <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Ayush Tiwari @ 2026-06-10 10:08 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: Nikolay Samokhvalov <[email protected]>; pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
Hi,
On Wed, 10 Jun 2026 at 14:02, Amit Langote <[email protected]> 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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-06-10 12:17 Amit Langote <[email protected]>
parent: Ayush Tiwari <[email protected]>
0 siblings, 2 replies; 17+ messages in thread
From: Amit Langote @ 2026-06-10 12:17 UTC (permalink / raw)
To: Ayush Tiwari <[email protected]>; +Cc: Nikolay Samokhvalov <[email protected]>; pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
Hi Ayush,
Thanks for the review.
On Wed, Jun 10, 2026 at 7:09 PM Ayush Tiwari
<[email protected]> wrote:
> On Wed, 10 Jun 2026 at 14:02, Amit Langote <[email protected]> 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
Attachments:
[application/octet-stream] v2-0001-Fix-out-of-bounds-write-in-RI-fast-path-batch-on-.patch (10.5K, ../../CA+HiwqELE-eyOfBBEmpr_eGf-04PUvZg5BjypW2CMHbed5QGhA@mail.gmail.com/2-v2-0001-Fix-out-of-bounds-write-in-RI-fast-path-batch-on-.patch)
download | inline diff:
From f2979edb939d37e81d8144d18328aafcceb501c5 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Wed, 10 Jun 2026 21:07:09 +0900
Subject: [PATCH v2 1/2] Fix out-of-bounds write in RI fast-path batch on
re-entry
The FK fast-path batching added in b7b27eb41a5 wrote the incoming row
into the batch array before checking whether the array was full:
fpentry->batch[fpentry->batch_count] = ExecCopySlotHeapTuple(newslot);
fpentry->batch_count++;
if (fpentry->batch_count >= RI_FASTPATH_BATCH_SIZE)
ri_FastPathBatchFlush(fpentry, fk_rel, riinfo);
batch_count is reset to zero only at the end of ri_FastPathBatchFlush(),
so it remains at RI_FASTPATH_BATCH_SIZE throughout a full-batch flush.
A flush runs user-defined cast functions and equality operators; if that
user code performs DML on the same FK table, ri_FastPathBatchAdd()
re-enters with batch_count == RI_FASTPATH_BATCH_SIZE and writes one past
the end of the array, corrupting the adjacent batch_count field. This
is reachable by an unprivileged table owner via an implicit cast with a
PL/pgSQL function and causes a SIGSEGV in assert-enabled builds.
Fix by bounds-checking the write into the batch array so a re-entrant
add can never write past the end, and by adding a "flushing" flag to
RI_FastPathEntry that routes re-entrant ri_FastPathBatchAdd() calls on
a busy entry to the per-row path (ri_FastPathCheck) instead of touching
the mid-flush batch array. The flag is set around the probe in
ri_FastPathBatchFlush() and cleared in a PG_FINALLY, which also resets
batch_count, so the entry is left empty and reusable if a flush error
(including a reported FK violation) is caught by a savepoint.
Add regression tests for both the re-entrant flush and reuse of an entry
after a flush error caught by a savepoint.
Reported-by: Nikolay Samokhvalov <[email protected]>
Reviewed-by: Nikolay Samokhvalov <[email protected]>
Reviewed-by: Ayush Tiwari <[email protected]>
Discussion: https://postgr.es/m/CAM527d9exRCdWrhJOnAxk_vACg7sr_yPoaJp_+uCFY0qP8v=aw@mail.gmail.com
---
src/backend/utils/adt/ri_triggers.c | 70 +++++++++++++++++------
src/test/regress/expected/foreign_key.out | 56 ++++++++++++++++++
src/test/regress/sql/foreign_key.sql | 46 +++++++++++++++
3 files changed, 155 insertions(+), 17 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index dc89c686394..6d0d4204886 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -249,6 +249,12 @@ typedef struct RI_FastPathEntry
*/
HeapTuple batch[RI_FASTPATH_BATCH_SIZE];
int batch_count;
+
+ /*
+ * true while this entry's batch is being flushed; guards against
+ * re-entrant ri_FastPathBatchAdd from user code run during the flush.
+ */
+ bool flushing;
} RI_FastPathEntry;
/*
@@ -2860,15 +2866,31 @@ ri_FastPathBatchAdd(RI_ConstraintInfo *riinfo,
Relation fk_rel, TupleTableSlot *newslot)
{
RI_FastPathEntry *fpentry = ri_FastPathGetEntry(riinfo, fk_rel);
- MemoryContext oldcxt;
- oldcxt = MemoryContextSwitchTo(fpentry->flush_cxt);
- fpentry->batch[fpentry->batch_count] =
- ExecCopySlotHeapTuple(newslot);
- fpentry->batch_count++;
- MemoryContextSwitchTo(oldcxt);
+ /*
+ * If this entry is already being flushed, a cast function or an operator
+ * invoked during the flush has re-entered with DML on the same FK. Fall
+ * back to the per-row path rather than touching the batch array, which is
+ * mid-flush.
+ */
+ if (fpentry->flushing)
+ {
+ ri_FastPathCheck(riinfo, fk_rel, newslot);
+ return;
+ }
+
+ /* Buffer the row if there is room; otherwise it is flushed below. */
+ if (fpentry->batch_count < RI_FASTPATH_BATCH_SIZE)
+ {
+ MemoryContext oldcxt = MemoryContextSwitchTo(fpentry->flush_cxt);
+ fpentry->batch[fpentry->batch_count] =
+ ExecCopySlotHeapTuple(newslot);
+ fpentry->batch_count++;
+ MemoryContextSwitchTo(oldcxt);
+ }
- if (fpentry->batch_count >= RI_FASTPATH_BATCH_SIZE)
+ /* Flush as soon as the batch is full. */
+ if (fpentry->batch_count == RI_FASTPATH_BATCH_SIZE)
ri_FastPathBatchFlush(fpentry, fk_rel, riinfo);
}
@@ -2944,13 +2966,30 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel,
}
Assert(riinfo->fpmeta);
- /* Skip array overhead for single-row batches. */
- if (riinfo->nkeys == 1 && fpentry->batch_count > 1)
- violation_index = ri_FastPathFlushArray(fpentry, fk_slot, riinfo,
- fk_rel, snapshot, scandesc);
- else
- violation_index = ri_FastPathFlushLoop(fpentry, fk_slot, riinfo,
- fk_rel, snapshot, scandesc);
+ /*
+ * The probe runs user-defined cast and equality functions. Set the
+ * flushing flag around it so a re-entrant ri_FastPathBatchAdd on this
+ * entry takes the per-row path, and clear it even on error so the entry
+ * is reusable if the error is caught by a savepoint.
+ */
+ Assert(!fpentry->flushing);
+ fpentry->flushing = true;
+ PG_TRY();
+ {
+ /* Skip array overhead for single-row batches. */
+ if (riinfo->nkeys == 1 && fpentry->batch_count > 1)
+ violation_index = ri_FastPathFlushArray(fpentry, fk_slot, riinfo,
+ fk_rel, snapshot, scandesc);
+ else
+ violation_index = ri_FastPathFlushLoop(fpentry, fk_slot, riinfo,
+ fk_rel, snapshot, scandesc);
+ }
+ PG_FINALLY();
+ {
+ fpentry->flushing = false;
+ fpentry->batch_count = 0;
+ }
+ PG_END_TRY();
SetUserIdAndSecContext(saved_userid, saved_sec_context);
UnregisterSnapshot(snapshot);
@@ -2966,9 +3005,6 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel,
MemoryContextReset(fpentry->flush_cxt);
MemoryContextSwitchTo(oldcxt);
-
- /* Reset. */
- fpentry->batch_count = 0;
}
/*
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 8b3b268de0f..e08dff99f03 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -3712,3 +3712,59 @@ INSERT INTO fp_pk_dup VALUES (1);
CREATE TABLE fp_fk_dup (a int REFERENCES fp_pk_dup);
INSERT INTO fp_fk_dup SELECT 1 FROM generate_series(1, 100);
DROP TABLE fp_fk_dup, fp_pk_dup;
+-- Re-entrant FK fast-path: DML on the same FK table from a cast function
+-- during a full-batch flush must not corrupt the batch array.
+CREATE TABLE fp_reentry_pk (id int PRIMARY KEY);
+INSERT INTO fp_reentry_pk VALUES (1), (2);
+CREATE TYPE fp_vch AS (v int);
+CREATE FUNCTION fp_vcast(fp_vch) RETURNS int LANGUAGE plpgsql AS $$
+BEGIN
+ IF $1.v = 1 THEN
+ INSERT INTO fp_reentry_fk VALUES (row(2)::fp_vch);
+ END IF;
+ RETURN $1.v;
+END$$;
+CREATE CAST (fp_vch AS int) WITH FUNCTION fp_vcast(fp_vch) AS IMPLICIT;
+CREATE TABLE fp_reentry_fk (a fp_vch
+ REFERENCES fp_reentry_pk (id));
+-- Fill exactly one batch so the flush fires; the cast re-enters with DML
+-- on the same FK and must take the per-row path.
+INSERT INTO fp_reentry_fk SELECT row(1)::fp_vch FROM generate_series(1, 64);
+SELECT a, count(*) FROM fp_reentry_fk GROUP BY a ORDER BY a;
+ a | count
+-----+-------
+ (1) | 64
+ (2) | 64
+(2 rows)
+
+DROP TABLE fp_reentry_fk, fp_reentry_pk;
+DROP CAST (fp_vch AS int);
+DROP FUNCTION fp_vcast(fp_vch);
+DROP TYPE fp_vch;
+-- Flush error caught by a savepoint must leave the entry empty and reusable.
+CREATE TABLE fp_reentry_pk2 (id int PRIMARY KEY);
+INSERT INTO fp_reentry_pk2 VALUES (1);
+CREATE TABLE fp_reentry_fk2 (a int REFERENCES fp_reentry_pk2 (id));
+DO $$
+BEGIN
+ -- A batch containing a violating row; the flush reports the violation.
+ BEGIN
+ INSERT INTO fp_reentry_fk2 SELECT CASE WHEN g = 32 THEN 999 ELSE 1 END
+ FROM generate_series(1, 64) g;
+ EXCEPTION WHEN foreign_key_violation THEN
+ RAISE NOTICE 'caught fk violation';
+ END;
+
+ -- Reuse the same FK with a full batch in the same transaction. The
+ -- entry must be empty after the caught violation: no stale rows from the
+ -- rolled-back batch (in particular no 999), and no array overflow.
+ INSERT INTO fp_reentry_fk2 SELECT 1 FROM generate_series(1, 64);
+END$$;
+NOTICE: caught fk violation
+SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1
+ count | max
+-------+-----
+ 64 | 1
+(1 row)
+
+DROP TABLE fp_reentry_fk2, fp_reentry_pk2;
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 7eb86b188f0..87381194f41 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -2680,3 +2680,49 @@ INSERT INTO fp_pk_dup VALUES (1);
CREATE TABLE fp_fk_dup (a int REFERENCES fp_pk_dup);
INSERT INTO fp_fk_dup SELECT 1 FROM generate_series(1, 100);
DROP TABLE fp_fk_dup, fp_pk_dup;
+
+-- Re-entrant FK fast-path: DML on the same FK table from a cast function
+-- during a full-batch flush must not corrupt the batch array.
+CREATE TABLE fp_reentry_pk (id int PRIMARY KEY);
+INSERT INTO fp_reentry_pk VALUES (1), (2);
+CREATE TYPE fp_vch AS (v int);
+CREATE FUNCTION fp_vcast(fp_vch) RETURNS int LANGUAGE plpgsql AS $$
+BEGIN
+ IF $1.v = 1 THEN
+ INSERT INTO fp_reentry_fk VALUES (row(2)::fp_vch);
+ END IF;
+ RETURN $1.v;
+END$$;
+CREATE CAST (fp_vch AS int) WITH FUNCTION fp_vcast(fp_vch) AS IMPLICIT;
+CREATE TABLE fp_reentry_fk (a fp_vch
+ REFERENCES fp_reentry_pk (id));
+-- Fill exactly one batch so the flush fires; the cast re-enters with DML
+-- on the same FK and must take the per-row path.
+INSERT INTO fp_reentry_fk SELECT row(1)::fp_vch FROM generate_series(1, 64);
+SELECT a, count(*) FROM fp_reentry_fk GROUP BY a ORDER BY a;
+DROP TABLE fp_reentry_fk, fp_reentry_pk;
+DROP CAST (fp_vch AS int);
+DROP FUNCTION fp_vcast(fp_vch);
+DROP TYPE fp_vch;
+
+-- Flush error caught by a savepoint must leave the entry empty and reusable.
+CREATE TABLE fp_reentry_pk2 (id int PRIMARY KEY);
+INSERT INTO fp_reentry_pk2 VALUES (1);
+CREATE TABLE fp_reentry_fk2 (a int REFERENCES fp_reentry_pk2 (id));
+DO $$
+BEGIN
+ -- A batch containing a violating row; the flush reports the violation.
+ BEGIN
+ INSERT INTO fp_reentry_fk2 SELECT CASE WHEN g = 32 THEN 999 ELSE 1 END
+ FROM generate_series(1, 64) g;
+ EXCEPTION WHEN foreign_key_violation THEN
+ RAISE NOTICE 'caught fk violation';
+ END;
+
+ -- Reuse the same FK with a full batch in the same transaction. The
+ -- entry must be empty after the caught violation: no stale rows from the
+ -- rolled-back batch (in particular no 999), and no array overflow.
+ INSERT INTO fp_reentry_fk2 SELECT 1 FROM generate_series(1, 64);
+END$$;
+SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1
+DROP TABLE fp_reentry_fk2, fp_reentry_pk2;
--
2.47.3
[application/octet-stream] v2-0002-Confine-RI-fast-path-batching-to-the-top-transact.patch (11.3K, ../../CA+HiwqELE-eyOfBBEmpr_eGf-04PUvZg5BjypW2CMHbed5QGhA@mail.gmail.com/3-v2-0002-Confine-RI-fast-path-batching-to-the-top-transact.patch)
download | inline diff:
From dd87a2e45f2f2dbdccd4d65cbd90256b7f72e277 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Wed, 10 Jun 2026 21:11:10 +0900
Subject: [PATCH v2 2/2] Confine RI fast-path batching to the top transaction
level
The FK fast-path batching added in b7b27eb41a5 buffers rows in a
transaction-lived cache (ri_fastpath_cache) keyed by constraint OID.
Running user-defined cast and equality functions during a batch flush,
together with the cache's lifetime and iteration, exposed two defects
reachable by an unprivileged table owner.
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.
An internal subxact abort during after-trigger firing (e.g. a PL/pgSQL
BEGIN ... EXCEPTION block) therefore dropped buffered rows of the outer
transaction without running their FK checks, letting orphan rows commit
behind a constraint that still reported itself valid. The discard also
left relations opened by the batch unclosed, producing "resource was not
closed" warnings.
Second, ri_FastPathEndBatch flushes by iterating the cache with
hash_seq_search. If flush-time user code inserts into a different
fast-path FK table, a new entry is added to the cache mid-scan; it may
land in a bucket the scan has already passed and never be reached, and
ri_FastPathTeardown then destroys the cache without flushing it,
silently dropping that check.
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. Rather than
add that bookkeeping, confine batching to the top transaction level: in
RI_FKey_check, when GetCurrentTransactionNestLevel() > 1, use the
per-row fast path (ri_FastPathCheck) instead of buffering. Rows checked
inside a subtransaction are then verified immediately and roll back
cleanly with their subtransaction, and the cache only ever holds
top-level rows. With the cache confined to the top level, a
subtransaction abort has nothing of its own to discard, so
ri_FastPathSubXactCallback is removed along with its registration.
For the second defect, add a cache-wide flag (ri_fastpath_flushing) set
while ri_FastPathEndBatch iterates the cache. A re-entrant FK check
arriving while the flag is set takes the per-row path rather than adding
an entry to the cache being scanned, so no entry can be missed and torn
down unflushed. The flag is cleared in a PG_FINALLY so a flush that
throws (a reported violation or an error from user code) does not leave
it stuck. As defensive insurance it is also cleared in
ri_FastPathXactCallback() at transaction end.
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.
The subtransaction-abort case is covered by a new regression test. The
mid-scan cross-table case depends on hash bucket placement and so is not
reliably reproducible in a portable test, but the flag prevents it by
construction.
Reported-by: Nikolay Samokhvalov <[email protected]>
Reviewed-by: Nikolay Samokhvalov <[email protected]>
Reviewed-by: Ayush Tiwari <[email protected]>
Discussion: https://postgr.es/m/CAM527d9exRCdWrhJOnAxk_vACg7sr_yPoaJp_+uCFY0qP8v=aw@mail.gmail.com
---
src/backend/utils/adt/ri_triggers.c | 82 ++++++++++++++++-------
src/test/regress/expected/foreign_key.out | 24 +++++++
src/test/regress/sql/foreign_key.sql | 23 +++++++
3 files changed, 103 insertions(+), 26 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 6d0d4204886..6ad521fba34 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -267,6 +267,7 @@ static dclist_head ri_constraint_cache_valid_list;
static HTAB *ri_fastpath_cache = NULL;
static bool ri_fastpath_callback_registered = false;
+static bool ri_fastpath_flushing = false;
/*
* Local function prototypes
@@ -469,14 +470,31 @@ RI_FKey_check(TriggerData *trigdata)
*/
if (ri_fastpath_is_applicable(riinfo))
{
- if (AfterTriggerIsActive())
+ if (AfterTriggerIsActive() &&
+ GetCurrentTransactionNestLevel() == 1 &&
+ !ri_fastpath_flushing)
{
/* Batched path: buffer and probe in groups */
ri_FastPathBatchAdd(riinfo, fk_rel, newslot);
}
else
{
- /* ALTER TABLE validation: per-row, no cache */
+ /*
+ * Per-row path, used when batching is not safe or not
+ * applicable:
+ *
+ * - ALTER TABLE validation, where no after-trigger firing is
+ * active;
+ *
+ * - any FK check inside a subtransaction, since the batch cache
+ * is confined to the top transaction level (it cannot be
+ * cleanly unwound on subxact abort);
+ *
+ * - a re-entrant check from user cast/operator code running
+ * during a batch flush, since adding a cache entry while
+ * ri_FastPathEndBatch is iterating the cache could leave it
+ * unflushed.
+ */
ri_FastPathCheck(riinfo, fk_rel, newslot);
}
return PointerGetDatum(NULL);
@@ -4174,19 +4192,41 @@ ri_FastPathEndBatch(void *arg)
if (ri_fastpath_cache == NULL)
return;
- /* Flush any partial batches -- can throw ERROR */
- hash_seq_init(&status, ri_fastpath_cache);
- while ((entry = hash_seq_search(&status)) != NULL)
+ /*
+ * Set a flag for the duration of the scan so that any FK check triggered
+ * by user cast or operator code during a flush takes the per-row path
+ * instead of adding a new entry to the cache we are iterating. A new
+ * entry could land in an already-scanned bucket and then be torn down
+ * unflushed below.
+ *
+ * The flush can throw ERROR (a reported constraint violation, or an error
+ * from the user code it runs). In that case ri_FastPathTeardown below is
+ * skipped; the ResourceOwner and the transaction-end callback handle
+ * resource cleanup on the abort path. The PG_FINALLY only resets the
+ * flag and deliberately does not attempt teardown.
+ */
+ Assert(!ri_fastpath_flushing);
+ ri_fastpath_flushing = true;
+ PG_TRY();
{
- if (entry->batch_count > 0)
+ hash_seq_init(&status, ri_fastpath_cache);
+ while ((entry = hash_seq_search(&status)) != NULL)
{
- Relation fk_rel = table_open(entry->fk_relid, AccessShareLock);
- RI_ConstraintInfo *riinfo = ri_LoadConstraintInfo(entry->conoid);
+ if (entry->batch_count > 0)
+ {
+ Relation fk_rel = table_open(entry->fk_relid, AccessShareLock);
+ RI_ConstraintInfo *riinfo = ri_LoadConstraintInfo(entry->conoid);
- ri_FastPathBatchFlush(entry, fk_rel, riinfo);
- table_close(fk_rel, NoLock);
+ ri_FastPathBatchFlush(entry, fk_rel, riinfo);
+ table_close(fk_rel, NoLock);
+ }
}
}
+ PG_FINALLY();
+ {
+ ri_fastpath_flushing = false;
+ }
+ PG_END_TRY();
ri_FastPathTeardown();
}
@@ -4238,22 +4278,13 @@ ri_FastPathXactCallback(XactEvent event, void *arg)
*/
ri_fastpath_cache = NULL;
ri_fastpath_callback_registered = false;
-}
-static void
-ri_FastPathSubXactCallback(SubXactEvent event, SubTransactionId mySubid,
- SubTransactionId parentSubid, void *arg)
-{
- if (event == SUBXACT_EVENT_ABORT_SUB)
- {
- /*
- * ResourceOwner already released relations. NULL the static pointers
- * so the still-registered batch callback becomes a no-op for the rest
- * of this transaction.
- */
- ri_fastpath_cache = NULL;
- ri_fastpath_callback_registered = false;
- }
+ /*
+ * Also clear the in-flush flag. ri_FastPathEndBatch() already clears it
+ * via PG_FINALLY, so this is just defensive: it keeps a stale flag from
+ * surviving into the next transaction should any future path leave it set.
+ */
+ ri_fastpath_flushing = false;
}
/*
@@ -4280,7 +4311,6 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel)
if (!ri_fastpath_xact_callback_registered)
{
RegisterXactCallback(ri_FastPathXactCallback, NULL);
- RegisterSubXactCallback(ri_FastPathSubXactCallback, NULL);
ri_fastpath_xact_callback_registered = true;
}
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index e08dff99f03..e1563144d4c 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -3768,3 +3768,27 @@ SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1
(1 row)
DROP TABLE fp_reentry_fk2, fp_reentry_pk2;
+-- Subtransaction abort during after-trigger firing must not drop FK checks
+-- for rows buffered earlier in the same statement. Batching is confined to
+-- the top transaction level and the buffered batch is no longer discarded on
+-- subxact abort, so the violating rows are detected.
+CREATE TABLE fp_subxact_pk (id int PRIMARY KEY);
+INSERT INTO fp_subxact_pk SELECT g FROM generate_series(1, 10) g;
+CREATE TABLE fp_subxact_fk (a int, tag text);
+ALTER TABLE fp_subxact_fk ADD CONSTRAINT fp_subxact_fk_fkey
+ FOREIGN KEY (a) REFERENCES fp_subxact_pk (id);
+CREATE FUNCTION fp_abort_subxact() RETURNS trigger LANGUAGE plpgsql AS $$
+BEGIN
+ IF NEW.tag = 'boom' THEN
+ BEGIN PERFORM 1/0; EXCEPTION WHEN division_by_zero THEN NULL; END;
+ END IF;
+ RETURN NEW;
+END$$;
+CREATE TRIGGER fp_subxact_trg AFTER INSERT ON fp_subxact_fk
+ FOR EACH ROW EXECUTE FUNCTION fp_abort_subxact();
+INSERT INTO fp_subxact_fk VALUES (999, 'bad'), (0, 'boom'), (1, 'ok');
+ERROR: insert or update on table "fp_subxact_fk" violates foreign key constraint "fp_subxact_fk_fkey"
+DETAIL: Key (a)=(999) is not present in table "fp_subxact_pk".
+DROP TRIGGER fp_subxact_trg ON fp_subxact_fk;
+DROP FUNCTION fp_abort_subxact();
+DROP TABLE fp_subxact_fk, fp_subxact_pk;
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 87381194f41..abeb85965b9 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -2726,3 +2726,26 @@ BEGIN
END$$;
SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1
DROP TABLE fp_reentry_fk2, fp_reentry_pk2;
+
+-- Subtransaction abort during after-trigger firing must not drop FK checks
+-- for rows buffered earlier in the same statement. Batching is confined to
+-- the top transaction level and the buffered batch is no longer discarded on
+-- subxact abort, so the violating rows are detected.
+CREATE TABLE fp_subxact_pk (id int PRIMARY KEY);
+INSERT INTO fp_subxact_pk SELECT g FROM generate_series(1, 10) g;
+CREATE TABLE fp_subxact_fk (a int, tag text);
+ALTER TABLE fp_subxact_fk ADD CONSTRAINT fp_subxact_fk_fkey
+ FOREIGN KEY (a) REFERENCES fp_subxact_pk (id);
+CREATE FUNCTION fp_abort_subxact() RETURNS trigger LANGUAGE plpgsql AS $$
+BEGIN
+ IF NEW.tag = 'boom' THEN
+ BEGIN PERFORM 1/0; EXCEPTION WHEN division_by_zero THEN NULL; END;
+ END IF;
+ RETURN NEW;
+END$$;
+CREATE TRIGGER fp_subxact_trg AFTER INSERT ON fp_subxact_fk
+ FOR EACH ROW EXECUTE FUNCTION fp_abort_subxact();
+INSERT INTO fp_subxact_fk VALUES (999, 'bad'), (0, 'boom'), (1, 'ok');
+DROP TRIGGER fp_subxact_trg ON fp_subxact_fk;
+DROP FUNCTION fp_abort_subxact();
+DROP TABLE fp_subxact_fk, fp_subxact_pk;
--
2.47.3
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-06-10 12:48 Ayush Tiwari <[email protected]>
parent: Amit Langote <[email protected]>
1 sibling, 0 replies; 17+ messages in thread
From: Ayush Tiwari @ 2026-06-10 12:48 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: Nikolay Samokhvalov <[email protected]>; pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
Hi,
On Wed, 10 Jun 2026 at 17:47, Amit Langote <[email protected]> wrote:
> Hi Ayush,
>
> Thanks for the review.
>
> On Wed, Jun 10, 2026 at 7:09 PM Ayush Tiwari
> <[email protected]> wrote:
> > On Wed, 10 Jun 2026 at 14:02, Amit Langote <[email protected]>
> 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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-06-11 08:18 Junwang Zhao <[email protected]>
parent: Amit Langote <[email protected]>
1 sibling, 1 reply; 17+ messages in thread
From: Junwang Zhao @ 2026-06-11 08:18 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: Ayush Tiwari <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
Hi Amit,
On Wed, Jun 10, 2026 at 8:17 PM Amit Langote <[email protected]> wrote:
>
> Hi Ayush,
>
> Thanks for the review.
>
> On Wed, Jun 10, 2026 at 7:09 PM Ayush Tiwari
> <[email protected]> wrote:
> > On Wed, 10 Jun 2026 at 14:02, Amit Langote <[email protected]> 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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-06-11 09:05 Amit Langote <[email protected]>
parent: Junwang Zhao <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Amit Langote @ 2026-06-11 09:05 UTC (permalink / raw)
To: Junwang Zhao <[email protected]>; +Cc: Ayush Tiwari <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
On Thu, Jun 11, 2026 at 5:18 PM Junwang Zhao <[email protected]> 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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-06-11 09:50 Junwang Zhao <[email protected]>
parent: Amit Langote <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Junwang Zhao @ 2026-06-11 09:50 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: Ayush Tiwari <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
Hi Amit,
On Thu, Jun 11, 2026 at 5:05 PM Amit Langote <[email protected]> wrote:
>
> On Thu, Jun 11, 2026 at 5:18 PM Junwang Zhao <[email protected]> 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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-06-11 10:47 Amit Langote <[email protected]>
parent: Junwang Zhao <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Amit Langote @ 2026-06-11 10:47 UTC (permalink / raw)
To: Junwang Zhao <[email protected]>; +Cc: Ayush Tiwari <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
On Thu, Jun 11, 2026 at 6:51 PM Junwang Zhao <[email protected]> wrote:
> On Thu, Jun 11, 2026 at 5:05 PM Amit Langote <[email protected]> wrote:
> >
> > On Thu, Jun 11, 2026 at 5:18 PM Junwang Zhao <[email protected]> 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
Attachments:
[application/x-patch] v3-0002-Confine-RI-fast-path-batching-to-the-top-transact.patch (11.9K, ../../CA+HiwqHHLyjtk_4LoNkoFYfHQJy9LjDhydKDHn=UHCxu=Qo6Qg@mail.gmail.com/2-v3-0002-Confine-RI-fast-path-batching-to-the-top-transact.patch)
download | inline diff:
From 00e3f4ff89b3c533899eb996be4e98e3ee82c63c Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Wed, 10 Jun 2026 21:11:10 +0900
Subject: [PATCH v3 2/2] Confine RI fast-path batching to the top transaction
level
The FK fast-path batching added in b7b27eb41a5 buffers rows in a
transaction-lived cache (ri_fastpath_cache) keyed by constraint OID.
Running user-defined cast and equality functions during a batch flush,
together with the cache's lifetime and iteration, exposed two defects
reachable by an unprivileged table owner.
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.
An internal subxact abort during after-trigger firing (e.g. a PL/pgSQL
BEGIN ... EXCEPTION block) therefore dropped buffered rows of the outer
transaction without running their FK checks, letting orphan rows commit
behind a constraint that still reported itself valid. The discard also
left relations opened by the batch unclosed, producing "resource was not
closed" warnings.
Second, ri_FastPathEndBatch flushes by iterating the cache with
hash_seq_search. If flush-time user code inserts into a different
fast-path FK table, a new entry is added to the cache mid-scan; it may
land in a bucket the scan has already passed and never be reached, and
ri_FastPathTeardown then destroys the cache without flushing it,
silently dropping that check.
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. Rather than
add that bookkeeping, confine batching to the top transaction level: in
RI_FKey_check, when GetCurrentTransactionNestLevel() > 1, use the
per-row fast path (ri_FastPathCheck) instead of buffering. Rows checked
inside a subtransaction are then verified immediately and roll back
cleanly with their subtransaction, and the cache only ever holds
top-level rows. With the cache confined to the top level, a
subtransaction abort has nothing of its own to discard, so
ri_FastPathSubXactCallback is removed along with its registration.
For the second defect, add a cache-wide flag (ri_fastpath_flushing) set
while ri_FastPathEndBatch iterates the cache. A re-entrant FK check
arriving while the flag is set takes the per-row path rather than adding
an entry to the cache being scanned, so no entry can be missed and torn
down unflushed. The flag is cleared in a PG_FINALLY so a flush that
throws (a reported violation or an error from user code) does not leave
it stuck. As defensive insurance it is also cleared in
ri_FastPathXactCallback() at transaction end.
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.
The subtransaction-abort case is covered by a new regression test. The
mid-scan cross-table case depends on hash bucket placement and so is not
reliably reproducible in a portable test, but the flag prevents it by
construction.
Reported-by: Nikolay Samokhvalov <[email protected]>
Reviewed-by: Nikolay Samokhvalov <[email protected]>
Reviewed-by: Ayush Tiwari <[email protected]>
Discussion: https://postgr.es/m/CAM527d9exRCdWrhJOnAxk_vACg7sr_yPoaJp_+uCFY0qP8v=aw@mail.gmail.com
---
src/backend/utils/adt/ri_triggers.c | 86 +++++++++++++++--------
src/test/regress/expected/foreign_key.out | 24 +++++++
src/test/regress/sql/foreign_key.sql | 23 ++++++
3 files changed, 105 insertions(+), 28 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 917a544c32b..06e7728c45d 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -228,8 +228,8 @@ typedef struct RI_CompareHashEntry
* relations are held open with locks for the transaction duration, preventing
* relcache invalidation. The entry itself is torn down at batch end by
* ri_FastPathEndBatch(); on abort, ResourceOwner releases the cached
- * relations and the XactCallback/SubXactCallback NULL the static cache pointer
- * to prevent any subsequent access.
+ * relations and the XactCallback NULLs the static cache pointer to prevent
+ * any subsequent access.
*/
typedef struct RI_FastPathEntry
{
@@ -267,6 +267,7 @@ static dclist_head ri_constraint_cache_valid_list;
static HTAB *ri_fastpath_cache = NULL;
static bool ri_fastpath_callback_registered = false;
+static bool ri_fastpath_flushing = false;
/*
* Local function prototypes
@@ -469,14 +470,30 @@ RI_FKey_check(TriggerData *trigdata)
*/
if (ri_fastpath_is_applicable(riinfo))
{
- if (AfterTriggerIsActive())
+ if (AfterTriggerIsActive() &&
+ GetCurrentTransactionNestLevel() == 1 &&
+ !ri_fastpath_flushing)
{
/* Batched path: buffer and probe in groups */
ri_FastPathBatchAdd(riinfo, fk_rel, newslot);
}
else
{
- /* ALTER TABLE validation: per-row, no cache */
+ /*
+ * Per-row path, used when batching is not safe or not applicable:
+ *
+ * - ALTER TABLE validation, where no after-trigger firing is
+ * active;
+ *
+ * - any FK check inside a subtransaction, since the batch cache
+ * is confined to the top transaction level (it cannot be cleanly
+ * unwound on subxact abort);
+ *
+ * - a re-entrant check from user cast/operator code running
+ * during a batch flush, since adding a cache entry while
+ * ri_FastPathEndBatch is iterating the cache could leave it
+ * unflushed.
+ */
ri_FastPathCheck(riinfo, fk_rel, newslot);
}
return PointerGetDatum(NULL);
@@ -4181,19 +4198,41 @@ ri_FastPathEndBatch(void *arg)
if (ri_fastpath_cache == NULL)
return;
- /* Flush any partial batches -- can throw ERROR */
- hash_seq_init(&status, ri_fastpath_cache);
- while ((entry = hash_seq_search(&status)) != NULL)
+ /*
+ * Set a flag for the duration of the scan so that any FK check triggered
+ * by user cast or operator code during a flush takes the per-row path
+ * instead of adding a new entry to the cache we are iterating. A new
+ * entry could land in an already-scanned bucket and then be torn down
+ * unflushed below.
+ *
+ * The flush can throw ERROR (a reported constraint violation, or an error
+ * from the user code it runs). In that case ri_FastPathTeardown below is
+ * skipped; the ResourceOwner and the transaction-end callback handle
+ * resource cleanup on the abort path. The PG_FINALLY only resets the
+ * flag and deliberately does not attempt teardown.
+ */
+ Assert(!ri_fastpath_flushing);
+ ri_fastpath_flushing = true;
+ PG_TRY();
{
- if (entry->batch_count > 0)
+ hash_seq_init(&status, ri_fastpath_cache);
+ while ((entry = hash_seq_search(&status)) != NULL)
{
- Relation fk_rel = table_open(entry->fk_relid, AccessShareLock);
- RI_ConstraintInfo *riinfo = ri_LoadConstraintInfo(entry->conoid);
+ if (entry->batch_count > 0)
+ {
+ Relation fk_rel = table_open(entry->fk_relid, AccessShareLock);
+ RI_ConstraintInfo *riinfo = ri_LoadConstraintInfo(entry->conoid);
- ri_FastPathBatchFlush(entry, fk_rel, riinfo);
- table_close(fk_rel, NoLock);
+ ri_FastPathBatchFlush(entry, fk_rel, riinfo);
+ table_close(fk_rel, NoLock);
+ }
}
}
+ PG_FINALLY();
+ {
+ ri_fastpath_flushing = false;
+ }
+ PG_END_TRY();
ri_FastPathTeardown();
}
@@ -4245,22 +4284,14 @@ ri_FastPathXactCallback(XactEvent event, void *arg)
*/
ri_fastpath_cache = NULL;
ri_fastpath_callback_registered = false;
-}
-static void
-ri_FastPathSubXactCallback(SubXactEvent event, SubTransactionId mySubid,
- SubTransactionId parentSubid, void *arg)
-{
- if (event == SUBXACT_EVENT_ABORT_SUB)
- {
- /*
- * ResourceOwner already released relations. NULL the static pointers
- * so the still-registered batch callback becomes a no-op for the rest
- * of this transaction.
- */
- ri_fastpath_cache = NULL;
- ri_fastpath_callback_registered = false;
- }
+ /*
+ * Also clear the in-flush flag. ri_FastPathEndBatch() already clears it
+ * via PG_FINALLY, so this is just defensive: it keeps a stale flag from
+ * surviving into the next transaction should any future path leave it
+ * set.
+ */
+ ri_fastpath_flushing = false;
}
/*
@@ -4287,7 +4318,6 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel)
if (!ri_fastpath_xact_callback_registered)
{
RegisterXactCallback(ri_FastPathXactCallback, NULL);
- RegisterSubXactCallback(ri_FastPathSubXactCallback, NULL);
ri_fastpath_xact_callback_registered = true;
}
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index e08dff99f03..e1563144d4c 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -3768,3 +3768,27 @@ SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1
(1 row)
DROP TABLE fp_reentry_fk2, fp_reentry_pk2;
+-- Subtransaction abort during after-trigger firing must not drop FK checks
+-- for rows buffered earlier in the same statement. Batching is confined to
+-- the top transaction level and the buffered batch is no longer discarded on
+-- subxact abort, so the violating rows are detected.
+CREATE TABLE fp_subxact_pk (id int PRIMARY KEY);
+INSERT INTO fp_subxact_pk SELECT g FROM generate_series(1, 10) g;
+CREATE TABLE fp_subxact_fk (a int, tag text);
+ALTER TABLE fp_subxact_fk ADD CONSTRAINT fp_subxact_fk_fkey
+ FOREIGN KEY (a) REFERENCES fp_subxact_pk (id);
+CREATE FUNCTION fp_abort_subxact() RETURNS trigger LANGUAGE plpgsql AS $$
+BEGIN
+ IF NEW.tag = 'boom' THEN
+ BEGIN PERFORM 1/0; EXCEPTION WHEN division_by_zero THEN NULL; END;
+ END IF;
+ RETURN NEW;
+END$$;
+CREATE TRIGGER fp_subxact_trg AFTER INSERT ON fp_subxact_fk
+ FOR EACH ROW EXECUTE FUNCTION fp_abort_subxact();
+INSERT INTO fp_subxact_fk VALUES (999, 'bad'), (0, 'boom'), (1, 'ok');
+ERROR: insert or update on table "fp_subxact_fk" violates foreign key constraint "fp_subxact_fk_fkey"
+DETAIL: Key (a)=(999) is not present in table "fp_subxact_pk".
+DROP TRIGGER fp_subxact_trg ON fp_subxact_fk;
+DROP FUNCTION fp_abort_subxact();
+DROP TABLE fp_subxact_fk, fp_subxact_pk;
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 87381194f41..abeb85965b9 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -2726,3 +2726,26 @@ BEGIN
END$$;
SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1
DROP TABLE fp_reentry_fk2, fp_reentry_pk2;
+
+-- Subtransaction abort during after-trigger firing must not drop FK checks
+-- for rows buffered earlier in the same statement. Batching is confined to
+-- the top transaction level and the buffered batch is no longer discarded on
+-- subxact abort, so the violating rows are detected.
+CREATE TABLE fp_subxact_pk (id int PRIMARY KEY);
+INSERT INTO fp_subxact_pk SELECT g FROM generate_series(1, 10) g;
+CREATE TABLE fp_subxact_fk (a int, tag text);
+ALTER TABLE fp_subxact_fk ADD CONSTRAINT fp_subxact_fk_fkey
+ FOREIGN KEY (a) REFERENCES fp_subxact_pk (id);
+CREATE FUNCTION fp_abort_subxact() RETURNS trigger LANGUAGE plpgsql AS $$
+BEGIN
+ IF NEW.tag = 'boom' THEN
+ BEGIN PERFORM 1/0; EXCEPTION WHEN division_by_zero THEN NULL; END;
+ END IF;
+ RETURN NEW;
+END$$;
+CREATE TRIGGER fp_subxact_trg AFTER INSERT ON fp_subxact_fk
+ FOR EACH ROW EXECUTE FUNCTION fp_abort_subxact();
+INSERT INTO fp_subxact_fk VALUES (999, 'bad'), (0, 'boom'), (1, 'ok');
+DROP TRIGGER fp_subxact_trg ON fp_subxact_fk;
+DROP FUNCTION fp_abort_subxact();
+DROP TABLE fp_subxact_fk, fp_subxact_pk;
--
2.47.3
[application/x-patch] v3-0001-Fix-out-of-bounds-write-in-RI-fast-path-batch-on-.patch (10.9K, ../../CA+HiwqHHLyjtk_4LoNkoFYfHQJy9LjDhydKDHn=UHCxu=Qo6Qg@mail.gmail.com/3-v3-0001-Fix-out-of-bounds-write-in-RI-fast-path-batch-on-.patch)
download | inline diff:
From 9a078d0f332f96d8e16040558ddb22daaf199778 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Wed, 10 Jun 2026 21:07:09 +0900
Subject: [PATCH v3 1/2] Fix out-of-bounds write in RI fast-path batch on
re-entry
The FK fast-path batching added in b7b27eb41a5 wrote the incoming row
into the batch array before checking whether the array was full:
fpentry->batch[fpentry->batch_count] = ExecCopySlotHeapTuple(newslot);
fpentry->batch_count++;
if (fpentry->batch_count >= RI_FASTPATH_BATCH_SIZE)
ri_FastPathBatchFlush(fpentry, fk_rel, riinfo);
batch_count is reset to zero only at the end of ri_FastPathBatchFlush(),
so it remains at RI_FASTPATH_BATCH_SIZE throughout a full-batch flush.
A flush runs user-defined cast functions and equality operators; if that
user code performs DML on the same FK table, ri_FastPathBatchAdd()
re-enters with batch_count == RI_FASTPATH_BATCH_SIZE and writes one past
the end of the array, corrupting the adjacent batch_count field. This
is reachable by an unprivileged table owner via an implicit cast with a
PL/pgSQL function and causes a SIGSEGV in assert-enabled builds.
Fix by bounds-checking the write into the batch array so a re-entrant
add can never write past the end, and by adding a "flushing" flag to
RI_FastPathEntry that routes re-entrant ri_FastPathBatchAdd() calls on
a busy entry to the per-row path (ri_FastPathCheck) instead of touching
the mid-flush batch array. The flag is set around the probe in
ri_FastPathBatchFlush() and cleared in a PG_FINALLY, which also resets
batch_count, so the entry is left empty and reusable if a flush error
(including a reported FK violation) is caught by a savepoint.
Add regression tests for both the re-entrant flush and reuse of an entry
after a flush error caught by a savepoint.
Reported-by: Nikolay Samokhvalov <[email protected]>
Reviewed-by: Nikolay Samokhvalov <[email protected]>
Reviewed-by: Ayush Tiwari <[email protected]>
Discussion: https://postgr.es/m/CAM527d9exRCdWrhJOnAxk_vACg7sr_yPoaJp_+uCFY0qP8v=aw@mail.gmail.com
---
src/backend/utils/adt/ri_triggers.c | 80 ++++++++++++++++++-----
src/test/regress/expected/foreign_key.out | 56 ++++++++++++++++
src/test/regress/sql/foreign_key.sql | 46 +++++++++++++
3 files changed, 165 insertions(+), 17 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index dc89c686394..917a544c32b 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -249,6 +249,12 @@ typedef struct RI_FastPathEntry
*/
HeapTuple batch[RI_FASTPATH_BATCH_SIZE];
int batch_count;
+
+ /*
+ * true while this entry's batch is being flushed; guards against
+ * re-entrant ri_FastPathBatchAdd from user code run during the flush.
+ */
+ bool flushing;
} RI_FastPathEntry;
/*
@@ -2860,15 +2866,38 @@ ri_FastPathBatchAdd(RI_ConstraintInfo *riinfo,
Relation fk_rel, TupleTableSlot *newslot)
{
RI_FastPathEntry *fpentry = ri_FastPathGetEntry(riinfo, fk_rel);
- MemoryContext oldcxt;
- oldcxt = MemoryContextSwitchTo(fpentry->flush_cxt);
- fpentry->batch[fpentry->batch_count] =
- ExecCopySlotHeapTuple(newslot);
- fpentry->batch_count++;
- MemoryContextSwitchTo(oldcxt);
+ /*
+ * If this entry is already being flushed, a cast function or an operator
+ * invoked during the flush has re-entered with DML on the same FK. Fall
+ * back to the per-row path rather than touching the batch array, which is
+ * mid-flush.
+ */
+ if (unlikely(fpentry->flushing))
+ {
+ ri_FastPathCheck(riinfo, fk_rel, newslot);
+ return;
+ }
+
+ /*
+ * Buffer the row. A full batch is flushed below and re-entry is handled
+ * above, so there is always room here; the bounds check just guards the
+ * array write.
+ */
+ if (fpentry->batch_count < RI_FASTPATH_BATCH_SIZE)
+ {
+ MemoryContext oldcxt = MemoryContextSwitchTo(fpentry->flush_cxt);
- if (fpentry->batch_count >= RI_FASTPATH_BATCH_SIZE)
+ fpentry->batch[fpentry->batch_count] =
+ ExecCopySlotHeapTuple(newslot);
+ fpentry->batch_count++;
+ MemoryContextSwitchTo(oldcxt);
+ }
+ else
+ elog(ERROR, "RI fast-path batch unexpectedly full");
+
+ /* Flush as soon as the batch is full. */
+ if (fpentry->batch_count == RI_FASTPATH_BATCH_SIZE)
ri_FastPathBatchFlush(fpentry, fk_rel, riinfo);
}
@@ -2944,13 +2973,30 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel,
}
Assert(riinfo->fpmeta);
- /* Skip array overhead for single-row batches. */
- if (riinfo->nkeys == 1 && fpentry->batch_count > 1)
- violation_index = ri_FastPathFlushArray(fpentry, fk_slot, riinfo,
- fk_rel, snapshot, scandesc);
- else
- violation_index = ri_FastPathFlushLoop(fpentry, fk_slot, riinfo,
- fk_rel, snapshot, scandesc);
+ /*
+ * The probe runs user-defined cast and equality functions. Set the
+ * flushing flag around it so a re-entrant ri_FastPathBatchAdd on this
+ * entry takes the per-row path, and clear it even on error so the entry
+ * is reusable if the error is caught by a savepoint.
+ */
+ Assert(!fpentry->flushing);
+ fpentry->flushing = true;
+ PG_TRY();
+ {
+ /* Skip array overhead for single-row batches. */
+ if (riinfo->nkeys == 1 && fpentry->batch_count > 1)
+ violation_index = ri_FastPathFlushArray(fpentry, fk_slot, riinfo,
+ fk_rel, snapshot, scandesc);
+ else
+ violation_index = ri_FastPathFlushLoop(fpentry, fk_slot, riinfo,
+ fk_rel, snapshot, scandesc);
+ }
+ PG_FINALLY();
+ {
+ fpentry->flushing = false;
+ fpentry->batch_count = 0;
+ }
+ PG_END_TRY();
SetUserIdAndSecContext(saved_userid, saved_sec_context);
UnregisterSnapshot(snapshot);
@@ -2966,9 +3012,6 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel,
MemoryContextReset(fpentry->flush_cxt);
MemoryContextSwitchTo(oldcxt);
-
- /* Reset. */
- fpentry->batch_count = 0;
}
/*
@@ -4307,6 +4350,9 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel)
RegisterAfterTriggerBatchCallback(ri_FastPathEndBatch, NULL);
ri_fastpath_callback_registered = true;
}
+
+ entry->flushing = false;
+ entry->batch_count = 0;
}
return entry;
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 8b3b268de0f..e08dff99f03 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -3712,3 +3712,59 @@ INSERT INTO fp_pk_dup VALUES (1);
CREATE TABLE fp_fk_dup (a int REFERENCES fp_pk_dup);
INSERT INTO fp_fk_dup SELECT 1 FROM generate_series(1, 100);
DROP TABLE fp_fk_dup, fp_pk_dup;
+-- Re-entrant FK fast-path: DML on the same FK table from a cast function
+-- during a full-batch flush must not corrupt the batch array.
+CREATE TABLE fp_reentry_pk (id int PRIMARY KEY);
+INSERT INTO fp_reentry_pk VALUES (1), (2);
+CREATE TYPE fp_vch AS (v int);
+CREATE FUNCTION fp_vcast(fp_vch) RETURNS int LANGUAGE plpgsql AS $$
+BEGIN
+ IF $1.v = 1 THEN
+ INSERT INTO fp_reentry_fk VALUES (row(2)::fp_vch);
+ END IF;
+ RETURN $1.v;
+END$$;
+CREATE CAST (fp_vch AS int) WITH FUNCTION fp_vcast(fp_vch) AS IMPLICIT;
+CREATE TABLE fp_reentry_fk (a fp_vch
+ REFERENCES fp_reentry_pk (id));
+-- Fill exactly one batch so the flush fires; the cast re-enters with DML
+-- on the same FK and must take the per-row path.
+INSERT INTO fp_reentry_fk SELECT row(1)::fp_vch FROM generate_series(1, 64);
+SELECT a, count(*) FROM fp_reentry_fk GROUP BY a ORDER BY a;
+ a | count
+-----+-------
+ (1) | 64
+ (2) | 64
+(2 rows)
+
+DROP TABLE fp_reentry_fk, fp_reentry_pk;
+DROP CAST (fp_vch AS int);
+DROP FUNCTION fp_vcast(fp_vch);
+DROP TYPE fp_vch;
+-- Flush error caught by a savepoint must leave the entry empty and reusable.
+CREATE TABLE fp_reentry_pk2 (id int PRIMARY KEY);
+INSERT INTO fp_reentry_pk2 VALUES (1);
+CREATE TABLE fp_reentry_fk2 (a int REFERENCES fp_reentry_pk2 (id));
+DO $$
+BEGIN
+ -- A batch containing a violating row; the flush reports the violation.
+ BEGIN
+ INSERT INTO fp_reentry_fk2 SELECT CASE WHEN g = 32 THEN 999 ELSE 1 END
+ FROM generate_series(1, 64) g;
+ EXCEPTION WHEN foreign_key_violation THEN
+ RAISE NOTICE 'caught fk violation';
+ END;
+
+ -- Reuse the same FK with a full batch in the same transaction. The
+ -- entry must be empty after the caught violation: no stale rows from the
+ -- rolled-back batch (in particular no 999), and no array overflow.
+ INSERT INTO fp_reentry_fk2 SELECT 1 FROM generate_series(1, 64);
+END$$;
+NOTICE: caught fk violation
+SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1
+ count | max
+-------+-----
+ 64 | 1
+(1 row)
+
+DROP TABLE fp_reentry_fk2, fp_reentry_pk2;
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 7eb86b188f0..87381194f41 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -2680,3 +2680,49 @@ INSERT INTO fp_pk_dup VALUES (1);
CREATE TABLE fp_fk_dup (a int REFERENCES fp_pk_dup);
INSERT INTO fp_fk_dup SELECT 1 FROM generate_series(1, 100);
DROP TABLE fp_fk_dup, fp_pk_dup;
+
+-- Re-entrant FK fast-path: DML on the same FK table from a cast function
+-- during a full-batch flush must not corrupt the batch array.
+CREATE TABLE fp_reentry_pk (id int PRIMARY KEY);
+INSERT INTO fp_reentry_pk VALUES (1), (2);
+CREATE TYPE fp_vch AS (v int);
+CREATE FUNCTION fp_vcast(fp_vch) RETURNS int LANGUAGE plpgsql AS $$
+BEGIN
+ IF $1.v = 1 THEN
+ INSERT INTO fp_reentry_fk VALUES (row(2)::fp_vch);
+ END IF;
+ RETURN $1.v;
+END$$;
+CREATE CAST (fp_vch AS int) WITH FUNCTION fp_vcast(fp_vch) AS IMPLICIT;
+CREATE TABLE fp_reentry_fk (a fp_vch
+ REFERENCES fp_reentry_pk (id));
+-- Fill exactly one batch so the flush fires; the cast re-enters with DML
+-- on the same FK and must take the per-row path.
+INSERT INTO fp_reentry_fk SELECT row(1)::fp_vch FROM generate_series(1, 64);
+SELECT a, count(*) FROM fp_reentry_fk GROUP BY a ORDER BY a;
+DROP TABLE fp_reentry_fk, fp_reentry_pk;
+DROP CAST (fp_vch AS int);
+DROP FUNCTION fp_vcast(fp_vch);
+DROP TYPE fp_vch;
+
+-- Flush error caught by a savepoint must leave the entry empty and reusable.
+CREATE TABLE fp_reentry_pk2 (id int PRIMARY KEY);
+INSERT INTO fp_reentry_pk2 VALUES (1);
+CREATE TABLE fp_reentry_fk2 (a int REFERENCES fp_reentry_pk2 (id));
+DO $$
+BEGIN
+ -- A batch containing a violating row; the flush reports the violation.
+ BEGIN
+ INSERT INTO fp_reentry_fk2 SELECT CASE WHEN g = 32 THEN 999 ELSE 1 END
+ FROM generate_series(1, 64) g;
+ EXCEPTION WHEN foreign_key_violation THEN
+ RAISE NOTICE 'caught fk violation';
+ END;
+
+ -- Reuse the same FK with a full batch in the same transaction. The
+ -- entry must be empty after the caught violation: no stale rows from the
+ -- rolled-back batch (in particular no 999), and no array overflow.
+ INSERT INTO fp_reentry_fk2 SELECT 1 FROM generate_series(1, 64);
+END$$;
+SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1
+DROP TABLE fp_reentry_fk2, fp_reentry_pk2;
--
2.47.3
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-06-12 02:46 Amit Langote <[email protected]>
parent: Amit Langote <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Amit Langote @ 2026-06-12 02:46 UTC (permalink / raw)
To: Junwang Zhao <[email protected]>; +Cc: Ayush Tiwari <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
On Thu, Jun 11, 2026 at 7:47 PM Amit Langote <[email protected]> wrote:
>
> On Thu, Jun 11, 2026 at 6:51 PM Junwang Zhao <[email protected]> wrote:
> > On Thu, Jun 11, 2026 at 5:05 PM Amit Langote <[email protected]> wrote:
> > >
> > > On Thu, Jun 11, 2026 at 5:18 PM Junwang Zhao <[email protected]> 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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-07-05 22:21 Noah Misch <[email protected]>
parent: Amit Langote <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Noah Misch @ 2026-07-05 22:21 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: Junwang Zhao <[email protected]>; Ayush Tiwari <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
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.
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-07-06 14:29 Amit Langote <[email protected]>
parent: Noah Misch <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Amit Langote @ 2026-07-06 14:29 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Junwang Zhao <[email protected]>; Ayush Tiwari <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
Hi Noah,
On Mon, Jul 6, 2026 at 7:21 AM Noah Misch <[email protected]> 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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: PG19 FK fast path: OOB write and missed FK checks during batched
@ 2026-07-09 11:37 Amit Langote <[email protected]>
parent: Amit Langote <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Amit Langote @ 2026-07-09 11:37 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Junwang Zhao <[email protected]>; Ayush Tiwari <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; Andrey Borodin <[email protected]>; Kirk Wolak <[email protected]>
On Mon, Jul 6, 2026 at 11:29 PM Amit Langote <[email protected]> wrote:
> On Mon, Jul 6, 2026 at 7:21 AM Noah Misch <[email protected]> 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
Attachments:
[application/octet-stream] v1-0002-Restore-firing-state-at-subtransaction-end.patch (9.3K, ../../CA+HiwqGHa3tc6MZFSLyLrvwySdrmpkb1TqkH2jGd3HtKGGZ6cQ@mail.gmail.com/2-v1-0002-Restore-firing-state-at-subtransaction-end.patch)
download | inline diff:
From 970565ad88dd2e4befbb6c0cf7b64e64f5bbb529 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Thu, 9 Jul 2026 18:22:05 +0900
Subject: [PATCH v1 2/3] Restore firing state at subtransaction end
AfterTriggerEndQuery(), AfterTriggerFireDeferred(), and
AfterTriggerSetState() bracket their firing loops with
firing_depth++/--, and FireAfterTriggerBatchCallbacks() brackets its
loop with firing_batch_callbacks. The closing step runs after the loop
and is not protected by PG_FINALLY, so if a trigger or batch callback
throws and the error is caught by a subtransaction (e.g. a PL/pgSQL
EXCEPTION block), firing_depth is left too high and/or
firing_batch_callbacks is left set for the rest of the transaction.
firing_depth feeds AfterTriggerIsActive(), which the RI fast path uses
to decide whether an FK check is running inside trigger firing (and
may batch). firing_depth left too high makes AfterTriggerIsActive()
wrongly report firing as active afterwards. This is reachable and
results in silent data corruption: after a caught FK-check error, an
ALTER TABLE ... ADD FOREIGN KEY whose validation runs per-row
(RI_Initial_Check() having bailed, e.g. because RLS is enabled on the
referenced table) calls RI_FKey_check() with AfterTriggerIsActive()
wrongly true, so the check is routed into the batched fast path. 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 validated, and the cached PK
relation and index leak.
Fix by restoring firing_depth and firing_batch_callbacks in
AfterTriggerEndSubXact() to the values saved at subtransaction start,
next to the existing query_depth handling. Restoring (rather than
zeroing/clearing) is required because a subtransaction can begin and
end while an outer query is firing, where firing_depth is legitimately
positive and firing_batch_callbacks legitimately set; forcing them to
0/false there breaks the outer firing (FireAfterTriggerBatchCallbacks()
asserts firing_depth > 0).
Reported-by: Noah Misch <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Backpatch-through: 19
---
src/backend/commands/trigger.c | 29 ++++++++++++++--
src/test/regress/expected/foreign_key.out | 41 +++++++++++++++++++++++
src/test/regress/sql/foreign_key.sql | 39 +++++++++++++++++++++
3 files changed, 107 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 8a10f69c315..739dccda11b 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3946,6 +3946,8 @@ struct AfterTriggersTransData
SetConstraintState state; /* saved S C state, or NULL if not yet saved */
AfterTriggerEventList events; /* saved list pointer */
int query_depth; /* saved query_depth */
+ int firing_depth; /* saved firing_depth */
+ bool firing_batch_callbacks; /* saved firing_batch_callbacks */
CommandId firing_counter; /* saved firing_counter */
};
@@ -5507,6 +5509,9 @@ AfterTriggerBeginSubXact(void)
afterTriggers.trans_stack[my_level].state = NULL;
afterTriggers.trans_stack[my_level].events = afterTriggers.events;
afterTriggers.trans_stack[my_level].query_depth = afterTriggers.query_depth;
+ afterTriggers.trans_stack[my_level].firing_depth = afterTriggers.firing_depth;
+ afterTriggers.trans_stack[my_level].firing_batch_callbacks =
+ afterTriggers.firing_batch_callbacks;
afterTriggers.trans_stack[my_level].firing_counter = afterTriggers.firing_counter;
}
@@ -5607,8 +5612,28 @@ AfterTriggerEndSubXact(bool isCommit)
}
}
- /* Reset in case a callback threw an error while firing. */
- afterTriggers.firing_batch_callbacks = false;
+ /*
+ * Restore firing_depth and firing_batch_callbacks to their values at
+ * subtransaction start. The matching decrement of firing_depth in
+ * AfterTriggerEndQuery()/AfterTriggerFireDeferred(), and the clearing of
+ * firing_batch_callbacks in FireAfterTriggerBatchCallbacks(), run after
+ * their loops and are not protected by PG_FINALLY. A trigger or batch
+ * callback error caught by this subtransaction can therefore leave either
+ * one set; restoring the saved values unwinds only this subtransaction's
+ * firing.
+ *
+ * Restoring (rather than zeroing/clearing) matters because a
+ * subtransaction can begin and end while an outer query's triggers are
+ * firing -- for instance a batch callback whose user-supplied cast or
+ * equality function runs DML in a BEGIN ... EXCEPTION block. There
+ * firing_depth is positive and firing_batch_callbacks is true; forcing
+ * them to 0/false would corrupt the outer firing
+ * (FireAfterTriggerBatchCallbacks() asserts firing_depth > 0, and
+ * clearing the guard would defeat its re-entrancy check).
+ */
+ afterTriggers.firing_depth = afterTriggers.trans_stack[my_level].firing_depth;
+ afterTriggers.firing_batch_callbacks =
+ afterTriggers.trans_stack[my_level].firing_batch_callbacks;
}
/*
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 2dad1539cda..ac566f5826c 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -3864,3 +3864,44 @@ SELECT count(*) AS deferred_rows FROM fp_deferred_fk; -- 1, check passed at com
(1 row)
DROP TABLE fp_deferred_fk, fp_deferred_pk;
+-- Stranded firing state must not misroute ALTER TABLE ... ADD FOREIGN KEY
+-- validation into the batched fast path. A caught FK-check error inside a
+-- subtransaction leaves firing_depth set (its decrement is skipped); a
+-- following ALTER whose validation runs per-row (forced here by RLS on the
+-- referenced table, so RI_Initial_Check() bails) would then be wrongly treated
+-- as running inside trigger firing, batched, and never flushed (a utility
+-- command has no AfterTriggerEndQuery), silently validating a violating row.
+CREATE ROLE regress_fpav_role;
+CREATE TABLE fpav_pk (id int PRIMARY KEY);
+INSERT INTO fpav_pk VALUES (1);
+ALTER TABLE fpav_pk ENABLE ROW LEVEL SECURITY;
+CREATE POLICY fpav_pk_all ON fpav_pk FOR ALL USING (true) WITH CHECK (true);
+GRANT REFERENCES, SELECT ON fpav_pk TO regress_fpav_role;
+CREATE TABLE fpav_fk (a int);
+INSERT INTO fpav_fk VALUES (1), (99);
+ALTER TABLE fpav_fk OWNER TO regress_fpav_role;
+CREATE TABLE fpav_cv_pk (id int PRIMARY KEY);
+INSERT INTO fpav_cv_pk VALUES (1);
+CREATE TABLE fpav_cv_fk (a int REFERENCES fpav_cv_pk(id));
+GRANT INSERT ON fpav_cv_fk TO regress_fpav_role;
+GRANT SELECT, INSERT ON fpav_cv_pk TO regress_fpav_role;
+SET ROLE regress_fpav_role;
+BEGIN;
+-- Caught FK violation: leaves firing_depth set if it is not restored.
+DO $$
+BEGIN
+ BEGIN
+ INSERT INTO fpav_cv_fk VALUES (999);
+ EXCEPTION WHEN foreign_key_violation THEN
+ NULL;
+ END;
+END$$;
+-- Must ERROR on the violating row (99), not silently validate it.
+ALTER TABLE fpav_fk ADD CONSTRAINT fpav_fk_fkey
+ FOREIGN KEY (a) REFERENCES fpav_pk (id);
+ERROR: insert or update on table "fpav_fk" violates foreign key constraint "fpav_fk_fkey"
+DETAIL: Key (a)=(99) is not present in table "fpav_pk".
+ROLLBACK;
+RESET ROLE;
+DROP TABLE fpav_fk, fpav_pk, fpav_cv_fk, fpav_cv_pk;
+DROP ROLE regress_fpav_role;
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index bae83f2010d..d4e6e0559cf 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -2807,3 +2807,42 @@ INSERT INTO fp_deferred_pk VALUES (1);
COMMIT;
SELECT count(*) AS deferred_rows FROM fp_deferred_fk; -- 1, check passed at commit
DROP TABLE fp_deferred_fk, fp_deferred_pk;
+-- Stranded firing state must not misroute ALTER TABLE ... ADD FOREIGN KEY
+-- validation into the batched fast path. A caught FK-check error inside a
+-- subtransaction leaves firing_depth set (its decrement is skipped); a
+-- following ALTER whose validation runs per-row (forced here by RLS on the
+-- referenced table, so RI_Initial_Check() bails) would then be wrongly treated
+-- as running inside trigger firing, batched, and never flushed (a utility
+-- command has no AfterTriggerEndQuery), silently validating a violating row.
+CREATE ROLE regress_fpav_role;
+CREATE TABLE fpav_pk (id int PRIMARY KEY);
+INSERT INTO fpav_pk VALUES (1);
+ALTER TABLE fpav_pk ENABLE ROW LEVEL SECURITY;
+CREATE POLICY fpav_pk_all ON fpav_pk FOR ALL USING (true) WITH CHECK (true);
+GRANT REFERENCES, SELECT ON fpav_pk TO regress_fpav_role;
+CREATE TABLE fpav_fk (a int);
+INSERT INTO fpav_fk VALUES (1), (99);
+ALTER TABLE fpav_fk OWNER TO regress_fpav_role;
+CREATE TABLE fpav_cv_pk (id int PRIMARY KEY);
+INSERT INTO fpav_cv_pk VALUES (1);
+CREATE TABLE fpav_cv_fk (a int REFERENCES fpav_cv_pk(id));
+GRANT INSERT ON fpav_cv_fk TO regress_fpav_role;
+GRANT SELECT, INSERT ON fpav_cv_pk TO regress_fpav_role;
+SET ROLE regress_fpav_role;
+BEGIN;
+-- Caught FK violation: leaves firing_depth set if it is not restored.
+DO $$
+BEGIN
+ BEGIN
+ INSERT INTO fpav_cv_fk VALUES (999);
+ EXCEPTION WHEN foreign_key_violation THEN
+ NULL;
+ END;
+END$$;
+-- Must ERROR on the violating row (99), not silently validate it.
+ALTER TABLE fpav_fk ADD CONSTRAINT fpav_fk_fkey
+ FOREIGN KEY (a) REFERENCES fpav_pk (id);
+ROLLBACK;
+RESET ROLE;
+DROP TABLE fpav_fk, fpav_pk, fpav_cv_fk, fpav_cv_pk;
+DROP ROLE regress_fpav_role;
--
2.47.3
[application/octet-stream] v1-0003-Assert-RI-fast-path-batches-don-t-span-subtransac.patch (1.6K, ../../CA+HiwqGHa3tc6MZFSLyLrvwySdrmpkb1TqkH2jGd3HtKGGZ6cQ@mail.gmail.com/3-v1-0003-Assert-RI-fast-path-batches-don-t-span-subtransac.patch)
download | inline diff:
From 19f9054ccd1b66b7c9d0a34fa3a5bf72d9088e40 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Thu, 9 Jul 2026 18:21:57 +0900
Subject: [PATCH v1 3/3] Assert RI fast-path batches don't span subtransactions
A fast-path batch is filled and flushed within a single trigger-firing
cycle, so all rows in a batch come from the subtransaction that created
the cache entry. AtEOSubXact_RI() relies on this: it identifies an
aborting subtransaction's entries by the subid stamped at entry
creation. Assert the invariant in ri_FastPathBatchAdd() to catch any
future code that would violate it.
Suggested-by: Noah Misch <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Backpatch-through: 19
---
src/backend/utils/adt/ri_triggers.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index a5d0fa4721a..a8c5277bfdc 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -2907,6 +2907,14 @@ ri_FastPathBatchAdd(RI_ConstraintInfo *riinfo,
return;
}
+ /*
+ * A batch is filled and flushed within a single trigger-firing cycle, so
+ * every row added to an entry comes from the subtransaction that created
+ * it. AtEOSubXact_RI() relies on this to identify an aborting
+ * subtransaction's entries by the subid stamped at entry creation.
+ */
+ Assert(fpentry->subid == GetCurrentSubTransactionId());
+
/*
* Buffer the row. A full batch is flushed below and re-entry is handled
* above, so there is always room here; the bounds check just guards the
--
2.47.3
[application/octet-stream] v1-0001-Track-RI-fast-path-FK-check-batches-per-subtransa.patch (20.9K, ../../CA+HiwqGHa3tc6MZFSLyLrvwySdrmpkb1TqkH2jGd3HtKGGZ6cQ@mail.gmail.com/4-v1-0001-Track-RI-fast-path-FK-check-batches-per-subtransa.patch)
download | inline diff:
From 424891b41801044af1da32cdba889cd90bf002ce Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Thu, 9 Jul 2026 18:20:58 +0900
Subject: [PATCH v1 1/3] Track RI fast-path FK-check batches per subtransaction
Commit 4113873 confined RI fast-path batching to the top transaction
level to avoid mishandling the batch cache on subtransaction abort.
That disabled batching for a foreign-key load wrapped in a savepoint
(e.g. "BEGIN; SAVEPOINT s; COPY fk_table FROM ..."), a surprising
performance cliff, and departed from the usual per-subtransaction
resource handling.
Handle the cache per subtransaction instead. Add AtEOSubXact_RI(),
called from Commit/AbortSubTransaction() after the subtransaction's
ResourceOwnerRelease(): it drops the entries opened by the ending
subtransaction, whose relations that ResourceOwner has just released,
and leaves entries opened by an outer level intact (so an inner
subxact abort during outer-level trigger firing does not discard the
outer statement's batch). Each entry records the subtransaction that
opened it for this.
Removing the top-transaction-level gate also lets checks batch inside
a nested trigger-firing cycle -- an AFTER trigger doing FK DML on
another table. That batch must be flushed at its own cycle's end.
Registration of the ri_FastPathEndBatch() callback was gated behind a
single global flag; but RegisterAfterTriggerBatchCallback() appends to
the current after-trigger query depth's callback list, which
AfterTriggerEndQuery() fires for that depth (and a separate list at
depth -1, fired by AfterTriggerFireDeferred() for deferred
constraints). So the callback must be registered once per firing
cycle. The global flag, set by the first cycle, suppressed
registration for every cycle after the first, whose callback list
stayed empty; the entry was never flushed, silently skipping its FK
check and leaking its relations. Register once per query depth that
has an entry (checking the cache for an existing entry at that depth),
tag each entry with its depth, and have ri_FastPathEndBatch() flush
and release only the ending cycle's entries, destroying the cache once
empty. Add AfterTriggerCurrentQueryDepth() so ri_triggers.c can read
the current depth; depth -1 covers the deferred firing cycle.
Reported-by: Noah Misch <[email protected]>
Reported-by: Nikolay Samokhvalov <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Backpatch-through: 19
---
src/backend/access/transam/xact.c | 2 +
src/backend/commands/trigger.c | 14 ++
src/backend/utils/adt/ri_triggers.c | 192 +++++++++++++++++++---
src/include/commands/trigger.h | 3 +
src/test/regress/expected/foreign_key.out | 57 +++++++
src/test/regress/sql/foreign_key.sql | 44 +++++
6 files changed, 289 insertions(+), 23 deletions(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 3a89149016f..9e2d507c8a9 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5245,6 +5245,7 @@ CommitSubTransaction(void)
s->parent->subTransactionId);
AtEOSubXact_HashTables(true, s->nestingLevel);
AtEOSubXact_PgStat(true, s->nestingLevel);
+ AtEOSubXact_RI(true, s->subTransactionId, s->parent->subTransactionId);
AtSubCommit_Snapshot(s->nestingLevel);
/*
@@ -5419,6 +5420,7 @@ AbortSubTransaction(void)
s->parent->subTransactionId);
AtEOSubXact_HashTables(false, s->nestingLevel);
AtEOSubXact_PgStat(false, s->nestingLevel);
+ AtEOSubXact_RI(false, s->subTransactionId, s->parent->subTransactionId);
AtSubAbort_Snapshot(s->nestingLevel);
}
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 401baddbfc6..8a10f69c315 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -6929,3 +6929,17 @@ AfterTriggerIsActive(void)
{
return afterTriggers.firing_depth > 0;
}
+
+/*
+ * AfterTriggerCurrentQueryDepth
+ * Return the current after-trigger query nesting depth.
+ *
+ * Lets a batch-callback registrant (e.g. the RI fast path) associate cached
+ * state with the firing cycle that created it, so a nested cycle's callback
+ * acts only on its own entries. Returns -1 outside any query level.
+ */
+int
+AfterTriggerCurrentQueryDepth(void)
+{
+ return afterTriggers.query_depth;
+}
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 627a9fb38ea..a5d0fa4721a 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -255,6 +255,24 @@ typedef struct RI_FastPathEntry
* re-entrant ri_FastPathBatchAdd from user code run during the flush.
*/
bool flushing;
+
+ /*
+ * Subtransaction whose resource owner opened this entry's relations.
+ * AtEOSubXact_RI() drops only entries matching an aborting subxact, so a
+ * subxact abort during outer-level trigger firing leaves the outer batch
+ * intact.
+ */
+ SubTransactionId subid;
+
+ /*
+ * After-trigger query depth (firing cycle) that created this entry. Its
+ * ri_FastPathEndBatch() callback is registered in that cycle's callback
+ * list and fires at that cycle's AfterTriggerEndQuery(), so the callback
+ * acts only on entries matching its own depth -- a nested cycle must not
+ * flush an outer cycle's still-accumulating batch (wrong snapshot; cf.
+ * commit 0e47bb5).
+ */
+ int query_depth;
} RI_FastPathEntry;
/*
@@ -266,7 +284,6 @@ static HTAB *ri_compare_cache = NULL;
static dclist_head ri_constraint_cache_valid_list;
static HTAB *ri_fastpath_cache = NULL;
-static bool ri_fastpath_callback_registered = false;
static bool ri_fastpath_flushing = false;
/*
@@ -353,7 +370,7 @@ pg_noreturn static void ri_ReportViolation(const RI_ConstraintInfo *riinfo,
static RI_FastPathEntry *ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo,
Relation fk_rel);
static void ri_FastPathEndBatch(void *arg);
-static void ri_FastPathTeardown(void);
+static void ri_FastPathTeardown(int depth);
/*
@@ -470,9 +487,7 @@ RI_FKey_check(TriggerData *trigdata)
*/
if (ri_fastpath_is_applicable(riinfo))
{
- if (AfterTriggerIsActive() &&
- GetCurrentTransactionNestLevel() == 1 &&
- !ri_fastpath_flushing)
+ if (AfterTriggerIsActive() && !ri_fastpath_flushing)
{
/* Batched path: buffer and probe in groups */
ri_FastPathBatchAdd(riinfo, fk_rel, newslot);
@@ -480,15 +495,11 @@ RI_FKey_check(TriggerData *trigdata)
else
{
/*
- * Per-row path, used when batching is not safe or not applicable:
+ * Per-row path, used when batching is not applicable:
*
* - ALTER TABLE validation, where no after-trigger firing is
* active;
*
- * - any FK check inside a subtransaction, since the batch cache
- * is confined to the top transaction level (it cannot be cleanly
- * unwound on subxact abort);
- *
* - a re-entrant check from user cast/operator code running
* during a batch flush, since adding a cache entry while
* ri_FastPathEndBatch is iterating the cache could leave it
@@ -4195,6 +4206,7 @@ ri_FastPathEndBatch(void *arg)
{
HASH_SEQ_STATUS status;
RI_FastPathEntry *entry;
+ int my_depth = (int) (intptr_t) arg;
if (ri_fastpath_cache == NULL)
return;
@@ -4219,7 +4231,8 @@ ri_FastPathEndBatch(void *arg)
hash_seq_init(&status, ri_fastpath_cache);
while ((entry = hash_seq_search(&status)) != NULL)
{
- if (entry->batch_count > 0)
+ /* Flush only entries created in the cycle now ending. */
+ if (entry->query_depth == my_depth && entry->batch_count > 0)
{
Relation fk_rel = table_open(entry->fk_relid, AccessShareLock);
RI_ConstraintInfo *riinfo = ri_LoadConstraintInfo(entry->conoid);
@@ -4235,17 +4248,26 @@ ri_FastPathEndBatch(void *arg)
}
PG_END_TRY();
- ri_FastPathTeardown();
+ /*
+ * Release this cycle's entries and remove them from the cache; leave
+ * outer cycles' entries for their own callbacks. Destroy the cache once
+ * empty.
+ */
+ ri_FastPathTeardown(my_depth);
}
/*
* ri_FastPathTeardown
- * Tear down all cached fast-path state.
+ * Release and remove the cached entries of one firing cycle, and drop
+ * the cache once it holds no more entries.
*
- * Called from ri_FastPathEndBatch() after flushing any remaining rows.
+ * Called from ri_FastPathEndBatch() with the depth of the cycle that is
+ * ending: it releases only that cycle's entries, leaving an outer cycle's
+ * still-live entries for their own callbacks. The cache (and its static
+ * pointer) go away once the last entry is removed.
*/
static void
-ri_FastPathTeardown(void)
+ri_FastPathTeardown(int depth)
{
HASH_SEQ_STATUS status;
RI_FastPathEntry *entry;
@@ -4256,6 +4278,8 @@ ri_FastPathTeardown(void)
hash_seq_init(&status, ri_fastpath_cache);
while ((entry = hash_seq_search(&status)) != NULL)
{
+ if (entry->query_depth != depth)
+ continue;
if (entry->idx_rel)
index_close(entry->idx_rel, NoLock);
if (entry->pk_rel)
@@ -4266,11 +4290,15 @@ ri_FastPathTeardown(void)
ExecDropSingleTupleTableSlot(entry->fk_slot);
if (entry->flush_cxt)
MemoryContextDelete(entry->flush_cxt);
+ hash_search(ri_fastpath_cache, &entry->conoid, HASH_REMOVE, NULL);
}
- hash_destroy(ri_fastpath_cache);
- ri_fastpath_cache = NULL;
- ri_fastpath_callback_registered = false;
+ if (hash_get_num_entries(ri_fastpath_cache) == 0)
+ {
+ hash_destroy(ri_fastpath_cache);
+ ri_fastpath_cache = NULL;
+ ri_fastpath_flushing = false;
+ }
}
/*
@@ -4316,7 +4344,6 @@ AtEOXact_RI(bool isCommit)
* memory-context reset; here we only drop the references to it.
*/
ri_fastpath_cache = NULL;
- ri_fastpath_callback_registered = false;
/*
* Also clear the in-flush flag. ri_FastPathEndBatch() already clears it
@@ -4327,6 +4354,90 @@ AtEOXact_RI(bool isCommit)
ri_fastpath_flushing = false;
}
+/*
+ * AtEOSubXact_RI
+ * Reset fast-path batching state at subtransaction end.
+ *
+ * Called from CommitSubTransaction() with isCommit true and from
+ * AbortSubTransaction() with isCommit false, in both cases after the
+ * subtransaction's ResourceOwnerRelease().
+ *
+ * The fast-path cache is created and torn down within a single trigger-firing
+ * batch (ri_FastPathEndBatch(), an AfterTriggerBatchCallback), so at a normal
+ * subtransaction boundary it is already empty and this is a no-op.
+ *
+ * The exception is a batch flush that errors out partway and is caught by this
+ * subtransaction (e.g. a PL/pgSQL EXCEPTION block): ri_FastPathEndBatch()'s
+ * teardown was skipped, so the cache still points at entries whose relations
+ * were opened under this subtransaction's resource owner. That owner has just
+ * released those relations (this runs after ResourceOwnerRelease()), so the
+ * entries are now stale. Drop the cache so a later statement in the parent
+ * doesn't reuse it. Like AtEOXact_RI(), this touches only backend-local state
+ * and the hash table's own memory -- no relations, locks or buffers, which the
+ * ResourceOwner already handled. The entries' slots and flush contexts live in
+ * TopTransactionContext and are freed by its end-of-transaction reset.
+ */
+void
+AtEOSubXact_RI(bool isCommit, SubTransactionId mySubid,
+ SubTransactionId parentSubid)
+{
+ HASH_SEQ_STATUS status;
+ RI_FastPathEntry *entry;
+ long remaining;
+
+ if (ri_fastpath_cache == NULL)
+ return;
+
+ /*
+ * Drop only entries whose relations were opened under the ending
+ * subtransaction's resource owner. On abort that owner has just released
+ * those relations (this runs after ResourceOwnerRelease()), so the entry
+ * is stale and must go, but entries opened by an outer level -- e.g. an
+ * outer statement's batch, mid-build when an inner subxact fired and
+ * aborted -- must be left untouched.
+ *
+ * On commit the entry, if any, would have been flushed and torn down at
+ * the end of its statement (ri_FastPathEndBatch()); reaching here with a
+ * matching entry is not expected, but reassign it to the parent so it is
+ * still cleaned up, rather than leaving it under a vanished subxact id.
+ *
+ * We touch no relations, locks or buffers -- the ResourceOwner handled
+ * those. The entry's slots and flush context are memory in
+ * TopTransactionContext, freed at end-of-transaction reset; we only
+ * remove the hash entry so it is not reused or torn down again.
+ */
+ hash_seq_init(&status, ri_fastpath_cache);
+ while ((entry = hash_seq_search(&status)) != NULL)
+ {
+ if (entry->subid != mySubid)
+ continue;
+
+ if (isCommit)
+ {
+ /*
+ * A committing subxact's entry should already have been flushed
+ * and torn down at its statement's end (ri_FastPathEndBatch()),
+ * so we don't expect to find one here. If we do, reassign it to
+ * the parent so it's still cleaned up rather than left under a
+ * subxact id that no longer exists.
+ */
+ Assert(false);
+ entry->subid = parentSubid;
+ }
+ else
+ hash_search(ri_fastpath_cache, &entry->conoid, HASH_REMOVE, NULL);
+ }
+
+ /* If that emptied the cache, drop it so the next batch starts clean. */
+ remaining = hash_get_num_entries(ri_fastpath_cache);
+ if (remaining == 0)
+ {
+ hash_destroy(ri_fastpath_cache);
+ ri_fastpath_cache = NULL;
+ ri_fastpath_flushing = false;
+ }
+}
+
/*
* ri_FastPathGetEntry
* Look up or create a per-batch cache entry for the given constraint.
@@ -4401,15 +4512,50 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel)
ALLOCSET_SMALL_SIZES);
MemoryContextSwitchTo(oldcxt);
- /* Ensure cleanup at end of this trigger-firing batch */
- if (!ri_fastpath_callback_registered)
+ /*
+ * Ensure ri_FastPathEndBatch() is registered for THIS firing cycle.
+ * RegisterAfterTriggerBatchCallback() appends to the current
+ * after-trigger query depth's callback list and fires at that depth's
+ * AfterTriggerEndQuery(), so a nested cycle needs its own
+ * registration; a single global latch would leave the nested cycle's
+ * list empty and its batch unflushed. Pass the depth as the callback
+ * arg so the callback flushes only its own cycle's entries.
+ */
{
- RegisterAfterTriggerBatchCallback(ri_FastPathEndBatch, NULL);
- ri_fastpath_callback_registered = true;
+ int cur_depth = AfterTriggerCurrentQueryDepth();
+ bool depth_registered = false;
+ HASH_SEQ_STATUS reg_status;
+ RI_FastPathEntry *other;
+
+ /*
+ * Register the callback once per firing cycle (query depth),
+ * including the deferred cycle at depth -1. Rather than track
+ * registered depths separately, check whether any other cache
+ * entry was already created at this depth: if so, its creation
+ * already registered the callback for this cycle. (The
+ * just-created entry is already in the hash, so skip it by
+ * conoid.)
+ */
+ hash_seq_init(®_status, ri_fastpath_cache);
+ while ((other = hash_seq_search(®_status)) != NULL)
+ {
+ if (other != entry && other->query_depth == cur_depth)
+ {
+ depth_registered = true;
+ hash_seq_term(®_status);
+ break;
+ }
+ }
+
+ if (!depth_registered)
+ RegisterAfterTriggerBatchCallback(ri_FastPathEndBatch,
+ (void *) (intptr_t) cur_depth);
+ entry->query_depth = cur_depth;
}
entry->flushing = false;
entry->batch_count = 0;
+ entry->subid = GetCurrentSubTransactionId();
}
return entry;
diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 0c3d485abf4..fecdb785f35 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -309,7 +309,10 @@ typedef void (*AfterTriggerBatchCallback) (void *arg);
extern void RegisterAfterTriggerBatchCallback(AfterTriggerBatchCallback callback,
void *arg);
extern bool AfterTriggerIsActive(void);
+extern int AfterTriggerCurrentQueryDepth(void);
extern void AtEOXact_RI(bool isCommit);
+extern void AtEOSubXact_RI(bool isCommit, SubTransactionId mySubid,
+ SubTransactionId parentSubid);
#endif /* TRIGGER_H */
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index c334cce752c..2dad1539cda 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -3807,3 +3807,60 @@ DETAIL: Key (a)=(999) is not present in table "fp_subxact_pk".
DROP TRIGGER fp_subxact_trg ON fp_subxact_fk;
DROP FUNCTION fp_abort_subxact();
DROP TABLE fp_subxact_fk, fp_subxact_pk;
+-- Re-entrant fast-path check inside a committing subtransaction. An AFTER
+-- trigger on one FK table runs FK DML on a second FK table inside a PL/pgSQL
+-- BEGIN ... EXCEPTION block, so the inner check batches in its own
+-- trigger-firing cycle nested in the outer check's. The inner cycle must
+-- register its own end-of-batch callback and flush -- otherwise its FK check
+-- is skipped (an orphan commits) and its relations leak.
+CREATE TABLE fp_inner_pk (id int PRIMARY KEY);
+INSERT INTO fp_inner_pk VALUES (1);
+CREATE TABLE fp_inner_fk (a int REFERENCES fp_inner_pk (id));
+CREATE TABLE fp_outer_pk (id int PRIMARY KEY);
+INSERT INTO fp_outer_pk SELECT g FROM generate_series(1, 64) g;
+CREATE FUNCTION fp_reentry_subxact() RETURNS trigger LANGUAGE plpgsql AS $$
+BEGIN
+ IF NEW.a = 32 THEN
+ BEGIN
+ INSERT INTO fp_inner_fk VALUES (999); -- violates; must be caught
+ EXCEPTION WHEN foreign_key_violation THEN
+ NULL;
+ END;
+ END IF;
+ RETURN NEW;
+END$$;
+CREATE TABLE fp_outer_fk (a int REFERENCES fp_outer_pk (id));
+CREATE TRIGGER fp_reentry_subxact_trg AFTER INSERT ON fp_outer_fk
+ FOR EACH ROW EXECUTE FUNCTION fp_reentry_subxact();
+INSERT INTO fp_outer_fk SELECT g FROM generate_series(1, 64) g;
+SELECT count(*) AS outer_rows FROM fp_outer_fk; -- 64, outer batch intact
+ outer_rows
+------------
+ 64
+(1 row)
+
+SELECT count(*) AS inner_rows FROM fp_inner_fk; -- 0, inner check caught
+ inner_rows
+------------
+ 0
+(1 row)
+
+DROP TRIGGER fp_reentry_subxact_trg ON fp_outer_fk;
+DROP FUNCTION fp_reentry_subxact();
+DROP TABLE fp_outer_fk, fp_outer_pk, fp_inner_fk, fp_inner_pk;
+-- Deferred FK check fires at commit (query depth -1); its batch must still get
+-- a callback registered and flushed.
+CREATE TABLE fp_deferred_pk (id int PRIMARY KEY);
+CREATE TABLE fp_deferred_fk (a int REFERENCES fp_deferred_pk (id)
+ DEFERRABLE INITIALLY DEFERRED);
+BEGIN;
+INSERT INTO fp_deferred_fk VALUES (1);
+INSERT INTO fp_deferred_pk VALUES (1);
+COMMIT;
+SELECT count(*) AS deferred_rows FROM fp_deferred_fk; -- 1, check passed at commit
+ deferred_rows
+---------------
+ 1
+(1 row)
+
+DROP TABLE fp_deferred_fk, fp_deferred_pk;
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 17eadc4bb5a..bae83f2010d 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -2763,3 +2763,47 @@ INSERT INTO fp_subxact_fk VALUES (999, 'bad'), (0, 'boom'), (1, 'ok');
DROP TRIGGER fp_subxact_trg ON fp_subxact_fk;
DROP FUNCTION fp_abort_subxact();
DROP TABLE fp_subxact_fk, fp_subxact_pk;
+
+-- Re-entrant fast-path check inside a committing subtransaction. An AFTER
+-- trigger on one FK table runs FK DML on a second FK table inside a PL/pgSQL
+-- BEGIN ... EXCEPTION block, so the inner check batches in its own
+-- trigger-firing cycle nested in the outer check's. The inner cycle must
+-- register its own end-of-batch callback and flush -- otherwise its FK check
+-- is skipped (an orphan commits) and its relations leak.
+CREATE TABLE fp_inner_pk (id int PRIMARY KEY);
+INSERT INTO fp_inner_pk VALUES (1);
+CREATE TABLE fp_inner_fk (a int REFERENCES fp_inner_pk (id));
+CREATE TABLE fp_outer_pk (id int PRIMARY KEY);
+INSERT INTO fp_outer_pk SELECT g FROM generate_series(1, 64) g;
+CREATE FUNCTION fp_reentry_subxact() RETURNS trigger LANGUAGE plpgsql AS $$
+BEGIN
+ IF NEW.a = 32 THEN
+ BEGIN
+ INSERT INTO fp_inner_fk VALUES (999); -- violates; must be caught
+ EXCEPTION WHEN foreign_key_violation THEN
+ NULL;
+ END;
+ END IF;
+ RETURN NEW;
+END$$;
+CREATE TABLE fp_outer_fk (a int REFERENCES fp_outer_pk (id));
+CREATE TRIGGER fp_reentry_subxact_trg AFTER INSERT ON fp_outer_fk
+ FOR EACH ROW EXECUTE FUNCTION fp_reentry_subxact();
+INSERT INTO fp_outer_fk SELECT g FROM generate_series(1, 64) g;
+SELECT count(*) AS outer_rows FROM fp_outer_fk; -- 64, outer batch intact
+SELECT count(*) AS inner_rows FROM fp_inner_fk; -- 0, inner check caught
+DROP TRIGGER fp_reentry_subxact_trg ON fp_outer_fk;
+DROP FUNCTION fp_reentry_subxact();
+DROP TABLE fp_outer_fk, fp_outer_pk, fp_inner_fk, fp_inner_pk;
+
+-- Deferred FK check fires at commit (query depth -1); its batch must still get
+-- a callback registered and flushed.
+CREATE TABLE fp_deferred_pk (id int PRIMARY KEY);
+CREATE TABLE fp_deferred_fk (a int REFERENCES fp_deferred_pk (id)
+ DEFERRABLE INITIALLY DEFERRED);
+BEGIN;
+INSERT INTO fp_deferred_fk VALUES (1);
+INSERT INTO fp_deferred_pk VALUES (1);
+COMMIT;
+SELECT count(*) AS deferred_rows FROM fp_deferred_fk; -- 1, check passed at commit
+DROP TABLE fp_deferred_fk, fp_deferred_pk;
--
2.47.3
^ permalink raw reply [nested|flat] 17+ messages in thread
end of thread, other threads:[~2026-07-09 11:37 UTC | newest]
Thread overview: 17+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-06-06 08:30 PG19 FK fast path: OOB write and missed FK checks during batched Nikolay Samokhvalov <[email protected]>
2026-06-06 09:13 ` Amit Langote <[email protected]>
2026-06-08 08:18 ` Amit Langote <[email protected]>
2026-06-09 13:31 ` Amit Langote <[email protected]>
2026-06-10 08:16 ` Nikolay Samokhvalov <[email protected]>
2026-06-10 08:32 ` Amit Langote <[email protected]>
2026-06-10 10:08 ` Ayush Tiwari <[email protected]>
2026-06-10 12:17 ` Amit Langote <[email protected]>
2026-06-10 12:48 ` Ayush Tiwari <[email protected]>
2026-06-11 08:18 ` Junwang Zhao <[email protected]>
2026-06-11 09:05 ` Amit Langote <[email protected]>
2026-06-11 09:50 ` Junwang Zhao <[email protected]>
2026-06-11 10:47 ` Amit Langote <[email protected]>
2026-06-12 02:46 ` Amit Langote <[email protected]>
2026-07-05 22:21 ` Noah Misch <[email protected]>
2026-07-06 14:29 ` Amit Langote <[email protected]>
2026-07-09 11:37 ` Amit Langote <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox