pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedri_Fast* crash w/ nullable UNIQUE constraint
15+ messages / 3 participants
[nested] [flat]
* ri_Fast* crash w/ nullable UNIQUE constraint
@ 2026-07-05 21:05 Noah Misch <noah@leadboat.com>
2026-07-05 21:47 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-16 11:32 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
0 siblings, 2 replies; 15+ messages in thread
From: Noah Misch @ 2026-07-05 21:05 UTC (permalink / raw)
To: amitlangote09@gmail.com; zhjwpku@gmail.com; +Cc: pgsql-hackers
I reviewed the ri_Fast* family of commits. This thread covers $SUBJECT and
some other findings. Feel free to fork more threads as needed.
==== ri_Fast* crash w/ nullable UNIQUE constraint
Commit 2da86c1 wrote:
> + /* Form the index values and isnull flags given the table tuple. */
> + FormIndexDatum(indexInfo, new_slot, NULL, values, isnull);
> + for (int i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++)
> + {
> + ScanKeyData *skey = &skeys[i];
> +
> + /* A PK column can never be set to NULL. */
> + Assert(!isnull[i]);
It's true that CONSTRAINT_PRIMARY implies NOT NULL, but the PK side of a FK
constraint accepts indexes that aren't part of a CONSTRAINT_PRIMARY. The
attached demo patch shows a crash from this. I had Opus 4.8 write the demo,
and it included a fix that I've not vetted. But I've vetted that reverting
the src/backend changes and running "make -C src/test/isolation check" does
see the crash:
TRAP: failed Assert("!isnull[i]"), File: "ri_triggers.c", Line: 3431, PID: 297407
==== fn_mcxt=TopMemoryContext, so record_eq() has session-lifespan leak
> ri_populate_fastpath_metadata(RI_ConstraintInfo *riinfo,
> Relation fk_rel, Relation idx_rel)
> {
> FastPathMeta *fpmeta;
> MemoryContext oldcxt = MemoryContextSwitchTo(TopMemoryContext);
...
> fmgr_info_copy(&fpmeta->cast_func_finfo[i], &entry->cast_func_finfo,
> CurrentMemoryContext);
> fmgr_info_copy(&fpmeta->eq_opr_finfo[i], &entry->eq_opr_finfo,
> CurrentMemoryContext);
This sets fn_mcxt=TopMemoryContext. fn_mcxt is the designated scratch space
for function authors, and record_eq() uses it that way. Need to use a
shorter-lived context, probably a cxt reset once per FK check batch or more.
==== Triggers queued during deferred trigger firing: lost?
Commit b7b27eb wrote:
> @@ -5317,6 +5337,9 @@ AfterTriggerFireDeferred(void)
> break; /* all fired */
> }
>
> + /* Flush any fast-path batches accumulated by the triggers just fired. */
> + FireAfterTriggerBatchCallbacks();
The comment anticipates trigger firing queueing more triggers. Can you expand
it to discuss what happens if the late-breaking triggers queue yet more
triggers? I asked Opus 4.8 if things will work right. It thought not, but I
don't fully grok its explanation. I regret its hyperbolic language:
CLAUDE [CONFIRMED -- SEVERE, committed integrity hole]: NO, it does not.
AfterTriggerFireDeferred runs its internal while(afterTriggerMarkEvents(...)) loop to
completion, THEN calls FireAfterTriggerBatchCallbacks (the fast-path flush). If that flush runs
a user cast/equality function whose DML queues a NEW deferred trigger event, the event lands in
afterTriggers.events AFTER the loop already drained. xact.c's commit loop (xact.c:2299-2313)
re-runs AfterTriggerFireDeferred only when PreCommit_Portals() reports open portals -- NOT when
a batch callback queued events -- so AfterTriggerEndXact silently discards it. A deferred FK
check is SKIPPED and a dangling row commits.
Repro (master a8c2547): fk_main has a vch-typed FK (DEFERRABLE INITIALLY DEFERRED) -> int PK;
the vch->int cast vcast() does INSERT INTO t2 VALUES(999) where t2 has its own deferred FK and
999 is absent. Deferred fk_main insert; at COMMIT the fast-path flush runs vcast which queues
t2's deferred check -> skipped.
- Fast path (plain PK): COMMIT SUCCEEDS, t2 keeps committed dangling row 999.
- SPI oracle (partitioned PK): COMMIT FAILS "violates foreign key constraint t2_a_fkey". Correct.
Not FK-specific: ANY deferred trigger queued during a commit-time fast-path flush is dropped.
Needs a cast/operator with a DML side-effect (unusual but allowed; the 0e47bb5 regress test uses
one). Fix: fire batch callbacks inside the deferred retry structure / re-loop while callbacks
queue events.
Stepping back, the batch callback mechanism is quite tailored to the specifics
of ri_Fast*. That's somewhat okay.
==== ri_CheckPermissions() does not cover hooks / sepgsql
> The ri_CheckPermissions() function performs schema USAGE and table
> SELECT checks, matching what the SPI path gets implicitly through
> the executor's permission checks.
It doesn't call ExecutorCheckPerms_hook or object_access_hook (via
e.g. InvokeFunctionExecuteHook), so sepgsql doesn't get control. That might
be okay if called out in the sepgsql documentation.
==== Assumption of btree
> + * PK indexes are always btree, which supports SK_SEARCHARRAY.
Foreign key constraints don't need CONSTRAINT_PRIMARY on the "PK" side. If an
extension adds an amcanunique access method, it can make indexes acceptable to
FK constraints:
transformFkeyCheckAttrs(Relation pkrel,
...
/*
* Must have the right number of columns; must be unique (or if
* temporal then exclusion instead) and not a partial index; forget it
* if there are any expressions, too. Invalid indexes are out as well.
*/
if (indexStruct->indnkeyatts == numattrs &&
(with_period ? indexStruct->indisexclusion : indexStruct->indisunique) &&
indexStruct->indisvalid &&
heap_attisnull(indexTuple, Anum_pg_index_indpred, NULL) &&
heap_attisnull(indexTuple, Anum_pg_index_indexprs, NULL))
{
==== Stale comment
> @@ -2690,10 +2766,14 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
>
> /*
> * ri_FastPathCheck
> - * Perform FK existence check via direct index probe, bypassing SPI.
> + * Perform per row FK existence check via direct index probe,
> + * bypassing SPI.
> *
> * If no matching PK row exists, report the violation via ri_ReportViolation(),
> * otherwise, the function returns normally.
> + *
> + * Note: This is only used by the ALTER TABLE validation path. Other paths use
> + * ri_FastPathBatchAdd().
The last paragraph is no longer accurate; see block comment above
RI_FKey_check()'s call to this function.
==== Timing of index_beginscan() vs. user switch
> + scandesc = index_beginscan(pk_rel, idx_rel, snapshot, NULL,
> + riinfo->nkeys, 0, SO_NONE);
> +
> + GetUserIdAndSecContext(&saved_userid, &saved_sec_context);
> + SetUserIdAndSecContext(RelationGetForm(pk_rel)->relowner,
> + saved_sec_context |
> + SECURITY_LOCAL_USERID_CHANGE |
> + SECURITY_NOFORCE_RLS);
For future-proofing, index_beginscan() should be inside the userid switch. I
don't think btree does anything to make us regret beginscan-first functional
consequences, but beginscan-first sets a bad example for code that deals with
arbitrary out-of-tree access methods.
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: ri_Fast* crash w/ nullable UNIQUE constraint
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
@ 2026-07-05 21:47 ` Noah Misch <noah@leadboat.com>
2026-07-07 13:44 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
1 sibling, 1 reply; 15+ messages in thread
From: Noah Misch @ 2026-07-05 21:47 UTC (permalink / raw)
To: amitlangote09@gmail.com; zhjwpku@gmail.com; +Cc: pgsql-hackers
On Sun, Jul 05, 2026 at 02:05:33PM -0700, Noah Misch wrote:
> attached demo patch shows a crash from this. I had Opus 4.8 write the demo,
> and it included a fix that I've not vetted. But I've vetted that reverting
> the src/backend changes and running "make -C src/test/isolation check" does
> see the crash:
>
> TRAP: failed Assert("!isnull[i]"), File: "ri_triggers.c", Line: 3431, PID: 297407
Here's the attachment I forgot.
commit 31376ce (HEAD -> fk-fastpath-null-key)
Author: Noah Misch <nm@cqla.c.codeql-202606a.internal>
AuthorDate: Wed Jul 1 18:56:36 2026 +0000
Commit: Noah Misch <nm@cqla.c.codeql-202606a.internal>
CommitDate: Wed Jul 1 18:56:36 2026 +0000
Fix RI fast-path crash when referenced key is concurrently set to NULL
A foreign key may reference a nullable UNIQUE column, not only a NOT NULL
primary key. The RI fast-path check, however, assumed that a referenced
key column could never be NULL ("A PK column can never be set to NULL").
Under READ COMMITTED, ri_LockPKTuple() locks with
TUPLE_LOCK_FLAG_FIND_LAST_VERSION, so it can chase an update chain to a
newer version of the referenced row. If a concurrent transaction set the
referenced key to NULL and committed, the fast path follows the chain to
that NULL version. recheck_matched_pk_tuple() (per-row loop flush) and
ri_FastPathFlushArray() (SK_SEARCHARRAY flush) then read a NULL key where
they asserted one could not occur, tripping an assertion in cassert
builds and, in production builds, dereferencing a NULL datum for
by-reference key types or reporting a spurious violation for by-value
types.
Treat a NULL referenced key as "not a match", which is what the SPI path
does implicitly: its requalifying "pkatt = $n" evaluates to NULL and the
row is not returned, yielding an ordinary foreign-key violation.
Add an isolation test covering both the per-row and array flush paths.
The fast path and the NULL assumption were introduced by commit
2da86c1ef9; the array-path variant came with the batching in b7b27eb41a5.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uoxu6AaEfa1135tvCA2dHJ
---
src/backend/utils/adt/ri_triggers.c | 25 +++++++++++---
.../isolation/expected/fk-fastpath-null-key.out | 17 +++++++++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/fk-fastpath-null-key.spec | 40 ++++++++++++++++++++++
4 files changed, 78 insertions(+), 5 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 627a9fb..d2b969e 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -3184,9 +3184,19 @@ ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
if (!ri_LockPKTuple(pk_rel, pk_slot, snapshot, &concurrently_updated))
continue;
- /* Extract the PK value from the matched and locked tuple */
+ /*
+ * Extract the PK value from the matched and locked tuple.
+ *
+ * A foreign key may reference a nullable unique column, not just a NOT
+ * NULL primary key. If ri_LockPKTuple() chased an update chain to a
+ * version whose referenced key is now NULL, that version cannot equal
+ * any buffered (non-null) FK value, so skip it. This mirrors the SPI
+ * path, where the requalifying "pkatt = $n" yields NULL and the row is
+ * not returned.
+ */
found_val = slot_getattr(pk_slot, riinfo->pk_attnums[0], &found_null);
- Assert(!found_null);
+ if (found_null)
+ continue;
if (concurrently_updated)
{
@@ -3427,9 +3437,14 @@ recheck_matched_pk_tuple(Relation idxrel, ScanKeyData *skeys, int nkeys,
{
ScanKeyData *skey = &skeys[i];
- /* A PK column can never be set to NULL. */
- Assert(!isnull[i]);
- if (!DatumGetBool(FunctionCall2Coll(&skey->sk_func,
+ /*
+ * A foreign key may reference a nullable unique column, so the version
+ * we chased the update chain to may have a NULL in a key column. A
+ * NULL never equals the value we searched for, so treat it as no
+ * match, as the SPI path's requalification would.
+ */
+ if (isnull[i] ||
+ !DatumGetBool(FunctionCall2Coll(&skey->sk_func,
skey->sk_collation,
values[i],
skey->sk_argument)))
diff --git a/src/test/isolation/expected/fk-fastpath-null-key.out b/src/test/isolation/expected/fk-fastpath-null-key.out
new file mode 100644
index 0000000..3efe1e7
--- /dev/null
+++ b/src/test/isolation/expected/fk-fastpath-null-key.out
@@ -0,0 +1,17 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1b s1upd_null s2ins s1c
+step s1b: BEGIN;
+step s1upd_null: UPDATE pktable SET u = NULL WHERE u = 5;
+step s2ins: INSERT INTO fktable VALUES (5); <waiting ...>
+step s1c: COMMIT;
+step s2ins: <... completed>
+ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_a_fkey"
+
+starting permutation: s1b s1upd_null s2ins_arr s1c
+step s1b: BEGIN;
+step s1upd_null: UPDATE pktable SET u = NULL WHERE u = 5;
+step s2ins_arr: INSERT INTO fktable VALUES (5), (6); <waiting ...>
+step s1c: COMMIT;
+step s2ins_arr: <... completed>
+ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_a_fkey"
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index b8ebe92..cbc40f2 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -38,6 +38,7 @@ test: fk-snapshot
test: fk-snapshot-2
test: fk-snapshot-3
test: fk-concurrent-pk-upd
+test: fk-fastpath-null-key
test: subxid-overflow
test: eval-plan-qual
test: eval-plan-qual-trigger
diff --git a/src/test/isolation/specs/fk-fastpath-null-key.spec b/src/test/isolation/specs/fk-fastpath-null-key.spec
new file mode 100644
index 0000000..51e11b1
--- /dev/null
+++ b/src/test/isolation/specs/fk-fastpath-null-key.spec
@@ -0,0 +1,40 @@
+# A foreign key may reference a nullable UNIQUE column, not only a NOT NULL
+# primary key. Test that the RI fast-path check copes when a concurrent
+# transaction sets the referenced key to NULL.
+#
+# s2's INSERT probes the PK index, finds the (u=5) row, and blocks on s1's
+# in-progress key-changing UPDATE. After s1 commits, the tuple lock follows
+# the update chain (table_tuple_lock with TUPLE_LOCK_FLAG_FIND_LAST_VERSION)
+# to the now-NULL version. The check must treat that as "referenced row not
+# found" and raise an ordinary foreign-key violation -- not assume that a
+# referenced key can never be NULL.
+#
+# Two permutations exercise the two fast-path flush routines: a single-row
+# INSERT goes through ri_FastPathFlushLoop()/recheck_matched_pk_tuple(), while
+# a multi-row single-column INSERT goes through ri_FastPathFlushArray().
+
+setup
+{
+ CREATE TABLE pktable (u int UNIQUE, c int);
+ CREATE TABLE fktable (a int REFERENCES pktable (u));
+ INSERT INTO pktable VALUES (5, 1), (6, 2);
+}
+
+teardown
+{
+ DROP TABLE fktable, pktable;
+}
+
+session s1
+step s1b { BEGIN; }
+step s1upd_null { UPDATE pktable SET u = NULL WHERE u = 5; }
+step s1c { COMMIT; }
+
+session s2
+# single-row batch -> per-row loop flush path
+step s2ins { INSERT INTO fktable VALUES (5); }
+# multi-row single-column batch -> SK_SEARCHARRAY flush path
+step s2ins_arr { INSERT INTO fktable VALUES (5), (6); }
+
+permutation s1b s1upd_null s2ins s1c
+permutation s1b s1upd_null s2ins_arr s1c
Attachments:
[text/plain] test-fk-unique-nullable-v0.patch (6.9K, ../../20260705214720.33@rfd.leadboat.com/2-test-fk-unique-nullable-v0.patch)
download | inline diff:
commit 31376ce (HEAD -> fk-fastpath-null-key)
Author: Noah Misch <nm@cqla.c.codeql-202606a.internal>
AuthorDate: Wed Jul 1 18:56:36 2026 +0000
Commit: Noah Misch <nm@cqla.c.codeql-202606a.internal>
CommitDate: Wed Jul 1 18:56:36 2026 +0000
Fix RI fast-path crash when referenced key is concurrently set to NULL
A foreign key may reference a nullable UNIQUE column, not only a NOT NULL
primary key. The RI fast-path check, however, assumed that a referenced
key column could never be NULL ("A PK column can never be set to NULL").
Under READ COMMITTED, ri_LockPKTuple() locks with
TUPLE_LOCK_FLAG_FIND_LAST_VERSION, so it can chase an update chain to a
newer version of the referenced row. If a concurrent transaction set the
referenced key to NULL and committed, the fast path follows the chain to
that NULL version. recheck_matched_pk_tuple() (per-row loop flush) and
ri_FastPathFlushArray() (SK_SEARCHARRAY flush) then read a NULL key where
they asserted one could not occur, tripping an assertion in cassert
builds and, in production builds, dereferencing a NULL datum for
by-reference key types or reporting a spurious violation for by-value
types.
Treat a NULL referenced key as "not a match", which is what the SPI path
does implicitly: its requalifying "pkatt = $n" evaluates to NULL and the
row is not returned, yielding an ordinary foreign-key violation.
Add an isolation test covering both the per-row and array flush paths.
The fast path and the NULL assumption were introduced by commit
2da86c1ef9; the array-path variant came with the batching in b7b27eb41a5.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uoxu6AaEfa1135tvCA2dHJ
---
src/backend/utils/adt/ri_triggers.c | 25 +++++++++++---
.../isolation/expected/fk-fastpath-null-key.out | 17 +++++++++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/fk-fastpath-null-key.spec | 40 ++++++++++++++++++++++
4 files changed, 78 insertions(+), 5 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 627a9fb..d2b969e 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -3184,9 +3184,19 @@ ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
if (!ri_LockPKTuple(pk_rel, pk_slot, snapshot, &concurrently_updated))
continue;
- /* Extract the PK value from the matched and locked tuple */
+ /*
+ * Extract the PK value from the matched and locked tuple.
+ *
+ * A foreign key may reference a nullable unique column, not just a NOT
+ * NULL primary key. If ri_LockPKTuple() chased an update chain to a
+ * version whose referenced key is now NULL, that version cannot equal
+ * any buffered (non-null) FK value, so skip it. This mirrors the SPI
+ * path, where the requalifying "pkatt = $n" yields NULL and the row is
+ * not returned.
+ */
found_val = slot_getattr(pk_slot, riinfo->pk_attnums[0], &found_null);
- Assert(!found_null);
+ if (found_null)
+ continue;
if (concurrently_updated)
{
@@ -3427,9 +3437,14 @@ recheck_matched_pk_tuple(Relation idxrel, ScanKeyData *skeys, int nkeys,
{
ScanKeyData *skey = &skeys[i];
- /* A PK column can never be set to NULL. */
- Assert(!isnull[i]);
- if (!DatumGetBool(FunctionCall2Coll(&skey->sk_func,
+ /*
+ * A foreign key may reference a nullable unique column, so the version
+ * we chased the update chain to may have a NULL in a key column. A
+ * NULL never equals the value we searched for, so treat it as no
+ * match, as the SPI path's requalification would.
+ */
+ if (isnull[i] ||
+ !DatumGetBool(FunctionCall2Coll(&skey->sk_func,
skey->sk_collation,
values[i],
skey->sk_argument)))
diff --git a/src/test/isolation/expected/fk-fastpath-null-key.out b/src/test/isolation/expected/fk-fastpath-null-key.out
new file mode 100644
index 0000000..3efe1e7
--- /dev/null
+++ b/src/test/isolation/expected/fk-fastpath-null-key.out
@@ -0,0 +1,17 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1b s1upd_null s2ins s1c
+step s1b: BEGIN;
+step s1upd_null: UPDATE pktable SET u = NULL WHERE u = 5;
+step s2ins: INSERT INTO fktable VALUES (5); <waiting ...>
+step s1c: COMMIT;
+step s2ins: <... completed>
+ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_a_fkey"
+
+starting permutation: s1b s1upd_null s2ins_arr s1c
+step s1b: BEGIN;
+step s1upd_null: UPDATE pktable SET u = NULL WHERE u = 5;
+step s2ins_arr: INSERT INTO fktable VALUES (5), (6); <waiting ...>
+step s1c: COMMIT;
+step s2ins_arr: <... completed>
+ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_a_fkey"
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index b8ebe92..cbc40f2 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -38,6 +38,7 @@ test: fk-snapshot
test: fk-snapshot-2
test: fk-snapshot-3
test: fk-concurrent-pk-upd
+test: fk-fastpath-null-key
test: subxid-overflow
test: eval-plan-qual
test: eval-plan-qual-trigger
diff --git a/src/test/isolation/specs/fk-fastpath-null-key.spec b/src/test/isolation/specs/fk-fastpath-null-key.spec
new file mode 100644
index 0000000..51e11b1
--- /dev/null
+++ b/src/test/isolation/specs/fk-fastpath-null-key.spec
@@ -0,0 +1,40 @@
+# A foreign key may reference a nullable UNIQUE column, not only a NOT NULL
+# primary key. Test that the RI fast-path check copes when a concurrent
+# transaction sets the referenced key to NULL.
+#
+# s2's INSERT probes the PK index, finds the (u=5) row, and blocks on s1's
+# in-progress key-changing UPDATE. After s1 commits, the tuple lock follows
+# the update chain (table_tuple_lock with TUPLE_LOCK_FLAG_FIND_LAST_VERSION)
+# to the now-NULL version. The check must treat that as "referenced row not
+# found" and raise an ordinary foreign-key violation -- not assume that a
+# referenced key can never be NULL.
+#
+# Two permutations exercise the two fast-path flush routines: a single-row
+# INSERT goes through ri_FastPathFlushLoop()/recheck_matched_pk_tuple(), while
+# a multi-row single-column INSERT goes through ri_FastPathFlushArray().
+
+setup
+{
+ CREATE TABLE pktable (u int UNIQUE, c int);
+ CREATE TABLE fktable (a int REFERENCES pktable (u));
+ INSERT INTO pktable VALUES (5, 1), (6, 2);
+}
+
+teardown
+{
+ DROP TABLE fktable, pktable;
+}
+
+session s1
+step s1b { BEGIN; }
+step s1upd_null { UPDATE pktable SET u = NULL WHERE u = 5; }
+step s1c { COMMIT; }
+
+session s2
+# single-row batch -> per-row loop flush path
+step s2ins { INSERT INTO fktable VALUES (5); }
+# multi-row single-column batch -> SK_SEARCHARRAY flush path
+step s2ins_arr { INSERT INTO fktable VALUES (5), (6); }
+
+permutation s1b s1upd_null s2ins s1c
+permutation s1b s1upd_null s2ins_arr s1c
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: ri_Fast* crash w/ nullable UNIQUE constraint
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-05 21:47 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
@ 2026-07-07 13:44 ` Amit Langote <amitlangote09@gmail.com>
0 siblings, 0 replies; 15+ messages in thread
From: Amit Langote @ 2026-07-07 13:44 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: zhjwpku@gmail.com; pgsql-hackers
Hi Noah,
On Mon, Jul 6, 2026 at 6:47 AM Noah Misch <noah@leadboat.com> wrote:
>
> On Sun, Jul 05, 2026 at 02:05:33PM -0700, Noah Misch wrote:
> > attached demo patch shows a crash from this. I had Opus 4.8 write the demo,
> > and it included a fix that I've not vetted. But I've vetted that reverting
> > the src/backend changes and running "make -C src/test/isolation check" does
> > see the crash:
> >
> > TRAP: failed Assert("!isnull[i]"), File: "ri_triggers.c", Line: 3431, PID: 297407
>
> Here's the attachment I forgot.
Thanks for the review.
I'll take a look as soon as I'm done addressing the subxact
fastpath-buffering issue in the other thread [1].
--
Thanks, Amit Langote
[1] https://www.postgresql.org/message-id/20260705222115.be.noahmisch%40microsoft.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: ri_Fast* crash w/ nullable UNIQUE constraint
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
@ 2026-07-16 11:32 ` Amit Langote <amitlangote09@gmail.com>
2026-07-18 03:25 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-09-22 11:26 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
1 sibling, 2 replies; 15+ messages in thread
From: Amit Langote @ 2026-07-16 11:32 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: zhjwpku@gmail.com; pgsql-hackers
Hi Noah,
On Mon, Jul 6, 2026 at 6:05 AM Noah Misch <noah@leadboat.com> wrote:
> I reviewed the ri_Fast* family of commits. This thread covers $SUBJECT and
> some other findings. Feel free to fork more threads as needed.
>
> ==== ri_Fast* crash w/ nullable UNIQUE constraint
>
> Commit 2da86c1 wrote:
> > + /* Form the index values and isnull flags given the table tuple. */
> > + FormIndexDatum(indexInfo, new_slot, NULL, values, isnull);
> > + for (int i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++)
> > + {
> > + ScanKeyData *skey = &skeys[i];
> > +
> > + /* A PK column can never be set to NULL. */
> > + Assert(!isnull[i]);
>
> It's true that CONSTRAINT_PRIMARY implies NOT NULL, but the PK side of a FK
> constraint accepts indexes that aren't part of a CONSTRAINT_PRIMARY. The
> attached demo patch shows a crash from this. I had Opus 4.8 write the demo,
> and it included a fix that I've not vetted. But I've vetted that reverting
> the src/backend changes and running "make -C src/test/isolation check" does
> see the crash:
>
> TRAP: failed Assert("!isnull[i]"), File: "ri_triggers.c", Line: 3431, PID: 297407
Your test case proves the bug and the fix looks right: a FK can
reference a nullable UNIQUE column, and under READ COMMITTED
ri_LockPKTuple() can follow the update chain to a now-NULL version.
Attached v2 keeps your isolation test and treats a NULL referenced key
as no-match in both flush routines, matching SPI. I confirmed the
crash needs the concurrent update-chain path: with a plain
committed-NULL key the index probe simply finds no matching row and an
ordinary violation is raised, so the assert is only reached when
ri_LockPKTuple() follows the chain to a version that became NULL.
Running the same spec against REL_18_STABLE, where every check goes
through SPI, produces the same output, so the fast path matches SPI
here.
> ==== fn_mcxt=TopMemoryContext, so record_eq() has session-lifespan leak
>
> > ri_populate_fastpath_metadata(RI_ConstraintInfo *riinfo,
> > Relation fk_rel, Relation idx_rel)
> > {
> > FastPathMeta *fpmeta;
> > MemoryContext oldcxt = MemoryContextSwitchTo(TopMemoryContext);
> ...
> > fmgr_info_copy(&fpmeta->cast_func_finfo[i], &entry->cast_func_finfo,
> > CurrentMemoryContext);
> > fmgr_info_copy(&fpmeta->eq_opr_finfo[i], &entry->eq_opr_finfo,
> > CurrentMemoryContext);
>
> This sets fn_mcxt=TopMemoryContext. fn_mcxt is the designated scratch space
> for function authors, and record_eq() uses it that way. Need to use a
> shorter-lived context, probably a cxt reset once per FK check batch or more.
Agreed. Attached sets fn_mcxt to a dedicated scratch context, reset
once per flush and deleted along with the metadata.
> ==== Triggers queued during deferred trigger firing: lost?
>
> Commit b7b27eb wrote:
> > @@ -5317,6 +5337,9 @@ AfterTriggerFireDeferred(void)
> > break; /* all fired */
> > }
> >
> > + /* Flush any fast-path batches accumulated by the triggers just fired. */
> > + FireAfterTriggerBatchCallbacks();
>
> The comment anticipates trigger firing queueing more triggers. Can you expand
> it to discuss what happens if the late-breaking triggers queue yet more
> triggers? I asked Opus 4.8 if things will work right. It thought not, but I
> don't fully grok its explanation. I regret its hyperbolic language:
>
> CLAUDE [CONFIRMED -- SEVERE, committed integrity hole]: NO, it does not.
> AfterTriggerFireDeferred runs its internal while(afterTriggerMarkEvents(...)) loop to
> completion, THEN calls FireAfterTriggerBatchCallbacks (the fast-path flush). If that flush runs
> a user cast/equality function whose DML queues a NEW deferred trigger event, the event lands in
> afterTriggers.events AFTER the loop already drained. xact.c's commit loop (xact.c:2299-2313)
> re-runs AfterTriggerFireDeferred only when PreCommit_Portals() reports open portals -- NOT when
> a batch callback queued events -- so AfterTriggerEndXact silently discards it. A deferred FK
> check is SKIPPED and a dangling row commits.
> Repro (master a8c2547): fk_main has a vch-typed FK (DEFERRABLE INITIALLY DEFERRED) -> int PK;
> the vch->int cast vcast() does INSERT INTO t2 VALUES(999) where t2 has its own deferred FK and
> 999 is absent. Deferred fk_main insert; at COMMIT the fast-path flush runs vcast which queues
> t2's deferred check -> skipped.
> - Fast path (plain PK): COMMIT SUCCEEDS, t2 keeps committed dangling row 999.
> - SPI oracle (partitioned PK): COMMIT FAILS "violates foreign key constraint t2_a_fkey". Correct.
> Not FK-specific: ANY deferred trigger queued during a commit-time fast-path flush is dropped.
> Needs a cast/operator with a DML side-effect (unusual but allowed; the 0e47bb5 regress test uses
> one). Fix: fire batch callbacks inside the deferred retry structure / re-loop while callbacks
> queue events.
Confirmed. The queued event is a deferred FK check that never fires:
it lands in afterTriggers.events after the drain loop has exited, and
AfterTriggerFireDeferred() is the last drainer of the queued events at
commit, so the check is skipped and a row violating the foreign key
commits.
The fix moves the flush inside the drain loop and drops the "all
fired" break, so afterTriggerMarkEvents() picks up whatever a flush
queued and fires it at transaction end. The other two callers of
FireAfterTriggerBatchCallbacks() don't need this change. An event
queued there is still drained by the commit-time
AfterTriggerFireDeferred() (I checked the SET CONSTRAINTS path, which
still errors at commit), so only the last drainer,
AfterTriggerFireDeferred(), loses events.
> Stepping back, the batch callback mechanism is quite tailored to the specifics
> of ri_Fast*. That's somewhat okay.
Yeah, I invented the callback mechanism expressly to let trigger.c
flush the RI fast-path batches at the after-trigger firing boundaries,
which are query end and commit-time deferred firing, since those are
the points ri_triggers.c can't see on its own. So it is tailored to
that one caller. It could be generalized if another batching consumer
ever appears, but I didn't want to design a general facility around a
single use case, so I left it specific.
> ==== ri_CheckPermissions() does not cover hooks / sepgsql
>
> > The ri_CheckPermissions() function performs schema USAGE and table
> > SELECT checks, matching what the SPI path gets implicitly through
> > the executor's permission checks.
>
> It doesn't call ExecutorCheckPerms_hook or object_access_hook (via
> e.g. InvokeFunctionExecuteHook), so sepgsql doesn't get control. That might
> be okay if called out in the sepgsql documentation.
You're right that the fast path doesn't reach ExecutorCheckPerms_hook
or the object access hooks, so sepgsql doesn't get control where it
would on the SPI path. Let me think through what restoring that would
mean, because I'm not sure it's the right goal.
On the SPI path these hooks fired as a consequence of the check
running through the executor. For the per-row validation path in
particular, that meant a hook invocation per row checked, so a foreign
key validation over a large table would have produced an audit record
per row. I don't think that was ever an intended sepgsql behavior; it
seems more like a side effect of the execution path. Reproducing it
deliberately on the fast path doesn't seem necessary IMHO.
Invoking the hooks would also mean synthesizing an RTEPermissionInfo
list outside any planned query, which is the kind of executor
scaffolding the fast path is trying to avoid.
Given that, my inclination is to leave it as is rather than wire up
the hooks. I'm also unsure a sepgsql doc note is the right place,
since it might read as a limitation we intend to close rather than an
implementation detail. But I'd rather get your read before deciding.
If you or anyone else thinks the bypass is worth addressing or noting
somewhere, I'm happy to work out how.
> ==== Assumption of btree
>
> > + * PK indexes are always btree, which supports SK_SEARCHARRAY.
>
> Foreign key constraints don't need CONSTRAINT_PRIMARY on the "PK" side. If an
> extension adds an amcanunique access method, it can make indexes acceptable to
> FK constraints:
>
> transformFkeyCheckAttrs(Relation pkrel,
> ...
> /*
> * Must have the right number of columns; must be unique (or if
> * temporal then exclusion instead) and not a partial index; forget it
> * if there are any expressions, too. Invalid indexes are out as well.
> */
> if (indexStruct->indnkeyatts == numattrs &&
> (with_period ? indexStruct->indisexclusion : indexStruct->indisunique) &&
> indexStruct->indisvalid &&
> heap_attisnull(indexTuple, Anum_pg_index_indpred, NULL) &&
> heap_attisnull(indexTuple, Anum_pg_index_indexprs, NULL))
> {
Agreed. transformFkeyCheckAttrs() accepts any unique index, so an
out-of-tree amcanunique access method could supply a non-btree index
that reaches the fast path, which assumes btree for both the direct
probe and SK_SEARCHARRAY. Attached records whether the referenced
index is btree when the constraint is loaded and makes
ri_fastpath_is_applicable() fall back to SPI otherwise; the "always
btree" comment now points at that gate.
> ==== Stale comment
>
> > @@ -2690,10 +2766,14 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
> >
> > /*
> > * ri_FastPathCheck
> > - * Perform FK existence check via direct index probe, bypassing SPI.
> > + * Perform per row FK existence check via direct index probe,
> > + * bypassing SPI.
> > *
> > * If no matching PK row exists, report the violation via ri_ReportViolation(),
> > * otherwise, the function returns normally.
> > + *
> > + * Note: This is only used by the ALTER TABLE validation path. Other paths use
> > + * ri_FastPathBatchAdd().
>
> The last paragraph is no longer accurate; see block comment above
> RI_FKey_check()'s call to this function.
Removed rather than reworded. The block comment above
RI_FKey_check()'s call already covers it.
> ==== Timing of index_beginscan() vs. user switch
>
> > + scandesc = index_beginscan(pk_rel, idx_rel, snapshot, NULL,
> > + riinfo->nkeys, 0, SO_NONE);
> > +
> > + GetUserIdAndSecContext(&saved_userid, &saved_sec_context);
> > + SetUserIdAndSecContext(RelationGetForm(pk_rel)->relowner,
> > + saved_sec_context |
> > + SECURITY_LOCAL_USERID_CHANGE |
> > + SECURITY_NOFORCE_RLS);
>
> For future-proofing, index_beginscan() should be inside the userid switch. I
> don't think btree does anything to make us regret beginscan-first functional
> consequences, but beginscan-first sets a bad example for code that deals with
> arbitrary out-of-tree access methods.
Agreed. Attached moves index_beginscan() after
SetUserIdAndSecContext(). No functional consequence for btree, but as
you say it sets a bad example for out-of-tree access methods.
Patches attached:
0001 - Handle nullable referenced key in RI fast-path check (fix +
your isolation test)
0002 - Fire fast-path FK batches inside the deferred trigger loop (the
dropped deferred-check bug above)
0003 - Give RI fast-path cached FmgrInfos a resettable fn_mcxt
0004 - Restrict RI fast-path FK check to btree referenced indexes
0005 - Begin RI fast-path index scan under the switched user id
0006 - Remove stale comment on ri_FastPathCheck()
No patch for the ri_CheckPermissions()/sepgsql item; see above.
Btw, do we want to tag Opus as the reporter and the author in the
commit message as you did in your version of 0001? I'd rather not as
that's not something the project has started doing unless I have
missed. Since you found and reported the bug, I decided to tag you as
the reporter in all of the patches for now.
--
Thanks, Amit Langote
Attachments:
[application/octet-stream] v1-0004-Give-RI-fast-path-cached-FmgrInfos-a-resettable-f.patch (3.8K, ../../CA+HiwqFYjOXwxKEjkTz6HB5O80J1k_XhdYcifGJ0cuG-ZFhY0A@mail.gmail.com/2-v1-0004-Give-RI-fast-path-cached-FmgrInfos-a-resettable-f.patch)
download | inline diff:
From 67704fede6a84a512db904c9b7cf6d48fe659b5e Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Thu, 16 Jul 2026 18:17:04 +0900
Subject: [PATCH v1 4/6] Give RI fast-path cached FmgrInfos a resettable
fn_mcxt
ri_populate_fastpath_metadata() copies the cast and equality FmgrInfos
into the cached FastPathMeta with fn_mcxt set to TopMemoryContext (the
context active at the copy). fn_mcxt is scratch space for the called
function; record_eq() and other type-specific equality and cast
functions allocate there. With fn_mcxt = TopMemoryContext, that scratch
lives for the session, so repeated fast-path checks on composite or
record key types leak memory.
The cached metadata itself must persist, so give it a dedicated child
context for the FmgrInfos' fn_mcxt and reset that context once per
flush, alongside the existing per-flush reset of the entry's flush_cxt.
Delete the context when the metadata is freed on cache invalidation.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260705210533.ee.noahmisch@microsoft.com
Backpatch-through: 19
---
src/backend/utils/adt/ri_triggers.c | 32 +++++++++++++++++++++++++++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index f95f72348e1..8805b283995 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -159,6 +159,15 @@ typedef struct FastPathMeta
Oid subtypes[RI_MAX_NUMKEYS];
int strats[RI_MAX_NUMKEYS];
AttrNumber index_attnos[RI_MAX_NUMKEYS]; /* index column positions */
+
+ /*
+ * fn_mcxt for the cached FmgrInfos above. Cast and equality functions
+ * (e.g. record_eq()) use fn_mcxt as per-call scratch space; pointing it at
+ * a dedicated context that ri_populate_fastpath_metadata()'s caller resets
+ * keeps that scratch from accumulating for the session, as it would if
+ * fn_mcxt were TopMemoryContext.
+ */
+ MemoryContext scratch_cxt;
} FastPathMeta;
/*
@@ -2595,6 +2604,7 @@ InvalidateConstraintCacheCallBack(Datum arg, SysCacheIdentifier cacheid,
riinfo->valid = false;
if (riinfo->fpmeta)
{
+ MemoryContextDelete(riinfo->fpmeta->scratch_cxt);
pfree(riinfo->fpmeta);
riinfo->fpmeta = NULL;
}
@@ -3032,6 +3042,13 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel,
}
MemoryContextReset(fpentry->flush_cxt);
+
+ /*
+ * Reset the fn_mcxt scratch used by the cached cast/equality FmgrInfos, so
+ * anything they left behind this flush does not accumulate.
+ */
+ MemoryContextReset(riinfo->fpmeta->scratch_cxt);
+
MemoryContextSwitchTo(oldcxt);
}
@@ -3543,6 +3560,17 @@ ri_populate_fastpath_metadata(RI_ConstraintInfo *riinfo,
Assert(riinfo != NULL && riinfo->valid);
fpmeta = palloc_object(FastPathMeta);
+
+ /*
+ * Dedicated scratch context for the cached FmgrInfos' fn_mcxt. It lives
+ * as long as fpmeta (a child of TopMemoryContext) but is reset once per
+ * flush by the caller, so scratch left behind by cast/equality functions
+ * does not accumulate for the session.
+ */
+ fpmeta->scratch_cxt = AllocSetContextCreate(TopMemoryContext,
+ "RI fast-path finfo scratch",
+ ALLOCSET_SMALL_SIZES);
+
for (int i = 0; i < riinfo->nkeys; i++)
{
Oid eq_opr = riinfo->pf_eq_oprs[i];
@@ -3568,9 +3596,9 @@ ri_populate_fastpath_metadata(RI_ConstraintInfo *riinfo,
fpmeta->index_attnos[i] = idx_col + 1;
fmgr_info_copy(&fpmeta->cast_func_finfo[i], &entry->cast_func_finfo,
- CurrentMemoryContext);
+ fpmeta->scratch_cxt);
fmgr_info_copy(&fpmeta->eq_opr_finfo[i], &entry->eq_opr_finfo,
- CurrentMemoryContext);
+ fpmeta->scratch_cxt);
fpmeta->regops[i] = get_opcode(eq_opr);
get_op_opfamily_properties(eq_opr,
--
2.47.3
[application/octet-stream] v1-0005-Begin-RI-fast-path-index-scan-under-the-switched-.patch (2.1K, ../../CA+HiwqFYjOXwxKEjkTz6HB5O80J1k_XhdYcifGJ0cuG-ZFhY0A@mail.gmail.com/3-v1-0005-Begin-RI-fast-path-index-scan-under-the-switched-.patch)
download | inline diff:
From c9272847c466d4cd9d4c7d13437e3b99137cf4aa Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Thu, 16 Jul 2026 18:17:09 +0900
Subject: [PATCH v1 5/6] Begin RI fast-path index scan under the switched user
id
ri_FastPathCheck() called index_beginscan() before switching to the
referenced relation's owner for the permission check and probe. For
btree this has no consequence, but starting the scan before the user id
switch sets a poor example for code dealing with out-of-tree access
methods, whose beginscan could observe the wrong user id. Move
index_beginscan() after the SetUserIdAndSecContext() call.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260705210533.ee.noahmisch@microsoft.com
Backpatch-through: 19
---
src/backend/utils/adt/ri_triggers.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 8805b283995..53b834a42de 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -2841,10 +2841,6 @@ ri_FastPathCheck(RI_ConstraintInfo *riinfo,
idx_rel = index_open(riinfo->conindid, AccessShareLock);
slot = table_slot_create(pk_rel, NULL);
- scandesc = index_beginscan(pk_rel, idx_rel,
- snapshot, NULL,
- riinfo->nkeys, 0,
- SO_NONE);
GetUserIdAndSecContext(&saved_userid, &saved_sec_context);
SetUserIdAndSecContext(RelationGetForm(pk_rel)->relowner,
@@ -2853,6 +2849,17 @@ ri_FastPathCheck(RI_ConstraintInfo *riinfo,
SECURITY_NOFORCE_RLS);
ri_CheckPermissions(pk_rel);
+ /*
+ * Begin the scan under the switched user id, so that any access method
+ * code invoked by index_beginscan() runs as the PK relation's owner. For
+ * btree this has no functional consequence, but it keeps the ordering
+ * correct for out-of-tree access methods.
+ */
+ scandesc = index_beginscan(pk_rel, idx_rel,
+ snapshot, NULL,
+ riinfo->nkeys, 0,
+ SO_NONE);
+
if (riinfo->fpmeta == NULL)
{
/* Reload to ensure it's valid. */
--
2.47.3
[application/octet-stream] v1-0002-Fire-fast-path-FK-batches-inside-the-deferred-tri.patch (8.7K, ../../CA+HiwqFYjOXwxKEjkTz6HB5O80J1k_XhdYcifGJ0cuG-ZFhY0A@mail.gmail.com/4-v1-0002-Fire-fast-path-FK-batches-inside-the-deferred-tri.patch)
download | inline diff:
From 0f83990fafb1940f5248e61920321542ea79565f Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Thu, 16 Jul 2026 18:16:57 +0900
Subject: [PATCH v1 2/6] Fire fast-path FK batches inside the deferred trigger
loop
AfterTriggerFireDeferred() drains queued after-trigger events in a loop,
then called FireAfterTriggerBatchCallbacks() once after the loop to
flush any RI fast-path FK-check batches the fired triggers accumulated.
A batch callback runs user-supplied cast or equality functions during
the flush, and their DML can queue further deferred trigger events --
for example a cast that inserts into another table with its own deferred
foreign key. Because the flush ran after the drain loop had already
exited, such an event was left in afterTriggers.events with nothing left
to fire it: AfterTriggerFireDeferred() is the last drainer at commit, so
the event was silently dropped. The queued deferred FK check was
skipped and a row violating the constraint committed.
Move the flush inside the drain loop, after afterTriggerInvokeEvents(),
and drop the "all fired" break. The loop condition,
afterTriggerMarkEvents(), now re-checks after each flush, so events
queued by a flush are fired on the next iteration, at the correct time
(transaction end), and the loop terminates only when nothing remains.
The other FireAfterTriggerBatchCallbacks() callers do not have this
problem: AfterTriggerEndQuery() fires immediate triggers and any
deferred event a flush queues there is drained later by
AfterTriggerFireDeferred(); AfterTriggerSetState() (SET CONSTRAINTS)
likewise leaves a queued event for the eventual commit-time
AfterTriggerFireDeferred(). Only the commit-time deferred firing has no
later drainer, so only it lost events.
The fast-path FK check and its batch flush were added by commits
2da86c1ef9 and b7b27eb41a5. Add a regression test to foreign_key.sql
for the dropped commit-time check.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260705210533.ee.noahmisch@microsoft.com
Backpatch-through: 19
---
src/backend/commands/trigger.c | 20 +++++++++---
src/test/regress/expected/foreign_key.out | 39 +++++++++++++++++++++++
src/test/regress/sql/foreign_key.sql | 38 ++++++++++++++++++++++
3 files changed, 92 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 401baddbfc6..c41157ce50c 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -5380,12 +5380,22 @@ AfterTriggerFireDeferred(void)
{
CommandId firing_id = afterTriggers.firing_counter++;
- if (afterTriggerInvokeEvents(events, firing_id, NULL, true))
- break; /* all fired */
- }
+ afterTriggerInvokeEvents(events, firing_id, NULL, true);
- /* Flush any fast-path batches accumulated by the triggers just fired. */
- FireAfterTriggerBatchCallbacks(afterTriggers.batch_callbacks);
+ /*
+ * Flush any fast-path FK-check batches accumulated by the triggers
+ * just fired. A batch callback runs user-supplied cast or equality
+ * functions, whose DML can queue further deferred trigger events.
+ * Flush inside the loop so afterTriggerMarkEvents() sees any such
+ * events on the next iteration and fires them; flushing after the
+ * loop would leave them unfired, silently skipping e.g. a deferred FK
+ * check and letting a violating row commit. (The former "all fired"
+ * break is therefore gone: the loop now terminates only when
+ * afterTriggerMarkEvents() finds nothing left, including events queued
+ * by the flush.)
+ */
+ FireAfterTriggerBatchCallbacks(afterTriggers.batch_callbacks);
+ }
afterTriggers.firing_depth--;
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index c334cce752c..120d3319451 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -3604,6 +3604,45 @@ ERROR: insert or update on table "fp_fk_defer" violates foreign key constraint
DETAIL: Key (a)=(3) is not present in table "fp_pk_defer".
COMMIT;
DROP TABLE fp_pk_defer, fp_fk_defer;
+-- A deferred FK check queued while firing deferred triggers at commit must
+-- not be lost. The RI fast-path flush runs during the deferred firing loop
+-- and can run user cast/equality code whose DML queues a further deferred
+-- check; that check must still fire. fk_defer_main's deferred fast-path check
+-- runs a cast whose function inserts a violating row into fk_defer_t2,
+-- queueing fk_defer_t2's own deferred check, which must still be reported.
+CREATE TABLE fk_defer_t2_pk (id int PRIMARY KEY);
+CREATE TABLE fk_defer_t2 (a int REFERENCES fk_defer_t2_pk(id)
+ DEFERRABLE INITIALLY DEFERRED);
+CREATE TYPE fk_defer_vch AS (v int);
+CREATE FUNCTION fk_defer_cast(fk_defer_vch) RETURNS int
+ LANGUAGE plpgsql AS $$
+BEGIN
+ INSERT INTO fk_defer_t2 VALUES (999); -- 999 absent, queues deferred check
+ RETURN $1.v;
+END$$;
+CREATE CAST (fk_defer_vch AS int) WITH FUNCTION fk_defer_cast(fk_defer_vch)
+ AS IMPLICIT;
+CREATE TABLE fk_defer_main_pk (id int PRIMARY KEY);
+INSERT INTO fk_defer_main_pk VALUES (1);
+CREATE TABLE fk_defer_main (a fk_defer_vch REFERENCES fk_defer_main_pk(id)
+ DEFERRABLE INITIALLY DEFERRED);
+-- At COMMIT the queued fk_defer_t2 check must fire and report the violation,
+-- rather than being dropped (which would let the dangling row commit).
+BEGIN;
+INSERT INTO fk_defer_main VALUES (row(1)::fk_defer_vch);
+COMMIT; -- expected: ERROR on fk_defer_t2_a_fkey
+ERROR: insert or update on table "fk_defer_t2" violates foreign key constraint "fk_defer_t2_a_fkey"
+DETAIL: Key (a)=(999) is not present in table "fk_defer_t2_pk".
+-- The queued check must fire at transaction end, not early: making the
+-- referenced row present in the same transaction lets the commit succeed.
+INSERT INTO fk_defer_t2_pk VALUES (999);
+BEGIN;
+INSERT INTO fk_defer_main VALUES (row(1)::fk_defer_vch);
+COMMIT; -- expected: success
+DROP TABLE fk_defer_main, fk_defer_main_pk, fk_defer_t2, fk_defer_t2_pk;
+DROP CAST (fk_defer_vch AS int);
+DROP FUNCTION fk_defer_cast(fk_defer_vch);
+DROP TYPE fk_defer_vch;
-- Subtransaction abort: cached state must be invalidated on ROLLBACK TO
CREATE TABLE fp_pk_subxact (a int PRIMARY KEY);
CREATE TABLE fp_fk_subxact (a int REFERENCES fp_pk_subxact);
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 17eadc4bb5a..b9b88064ea5 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -2580,6 +2580,44 @@ INSERT INTO fp_fk_defer VALUES (3); -- should fail, also tests that cache was c
COMMIT;
DROP TABLE fp_pk_defer, fp_fk_defer;
+-- A deferred FK check queued while firing deferred triggers at commit must
+-- not be lost. The RI fast-path flush runs during the deferred firing loop
+-- and can run user cast/equality code whose DML queues a further deferred
+-- check; that check must still fire. fk_defer_main's deferred fast-path check
+-- runs a cast whose function inserts a violating row into fk_defer_t2,
+-- queueing fk_defer_t2's own deferred check, which must still be reported.
+CREATE TABLE fk_defer_t2_pk (id int PRIMARY KEY);
+CREATE TABLE fk_defer_t2 (a int REFERENCES fk_defer_t2_pk(id)
+ DEFERRABLE INITIALLY DEFERRED);
+CREATE TYPE fk_defer_vch AS (v int);
+CREATE FUNCTION fk_defer_cast(fk_defer_vch) RETURNS int
+ LANGUAGE plpgsql AS $$
+BEGIN
+ INSERT INTO fk_defer_t2 VALUES (999); -- 999 absent, queues deferred check
+ RETURN $1.v;
+END$$;
+CREATE CAST (fk_defer_vch AS int) WITH FUNCTION fk_defer_cast(fk_defer_vch)
+ AS IMPLICIT;
+CREATE TABLE fk_defer_main_pk (id int PRIMARY KEY);
+INSERT INTO fk_defer_main_pk VALUES (1);
+CREATE TABLE fk_defer_main (a fk_defer_vch REFERENCES fk_defer_main_pk(id)
+ DEFERRABLE INITIALLY DEFERRED);
+-- At COMMIT the queued fk_defer_t2 check must fire and report the violation,
+-- rather than being dropped (which would let the dangling row commit).
+BEGIN;
+INSERT INTO fk_defer_main VALUES (row(1)::fk_defer_vch);
+COMMIT; -- expected: ERROR on fk_defer_t2_a_fkey
+-- The queued check must fire at transaction end, not early: making the
+-- referenced row present in the same transaction lets the commit succeed.
+INSERT INTO fk_defer_t2_pk VALUES (999);
+BEGIN;
+INSERT INTO fk_defer_main VALUES (row(1)::fk_defer_vch);
+COMMIT; -- expected: success
+DROP TABLE fk_defer_main, fk_defer_main_pk, fk_defer_t2, fk_defer_t2_pk;
+DROP CAST (fk_defer_vch AS int);
+DROP FUNCTION fk_defer_cast(fk_defer_vch);
+DROP TYPE fk_defer_vch;
+
-- Subtransaction abort: cached state must be invalidated on ROLLBACK TO
CREATE TABLE fp_pk_subxact (a int PRIMARY KEY);
CREATE TABLE fp_fk_subxact (a int REFERENCES fp_pk_subxact);
--
2.47.3
[application/octet-stream] v1-0003-Restrict-RI-fast-path-FK-check-to-btree-reference.patch (3.2K, ../../CA+HiwqFYjOXwxKEjkTz6HB5O80J1k_XhdYcifGJ0cuG-ZFhY0A@mail.gmail.com/5-v1-0003-Restrict-RI-fast-path-FK-check-to-btree-reference.patch)
download | inline diff:
From 6b2ba82493599c4ca1084226cbafbba7c127b546 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Thu, 16 Jul 2026 18:17:01 +0900
Subject: [PATCH v1 3/6] Restrict RI fast-path FK check to btree referenced
indexes
The RI fast-path check probes the referenced index directly and, for
single-column keys, uses SK_SEARCHARRAY. Both assume the index is a
btree. A comment claimed "PK indexes are always btree", but a foreign
key's referenced index need not be a primary key:
transformFkeyCheckAttrs() accepts any unique (or, for temporal keys,
exclusion) index, so an out-of-tree access method advertising
amcanunique could supply a non-btree index reachable by the fast path.
Add pk_index_is_btree to RI_ConstraintInfo, set from the referenced
index's access method when the constraint is loaded, and make
ri_fastpath_is_applicable() return false for non-btree indexes so such
constraints fall back to the SPI path.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260705210533.ee.noahmisch@microsoft.com
Backpatch-through: 19
---
src/backend/utils/adt/ri_triggers.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index d2b969e31ae..f95f72348e1 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -32,6 +32,7 @@
#include "access/tableam.h"
#include "access/xact.h"
#include "catalog/index.h"
+#include "catalog/pg_am_d.h"
#include "catalog/pg_collation.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_namespace.h"
@@ -142,6 +143,7 @@ typedef struct RI_ConstraintInfo
Oid conindid;
bool pk_is_partitioned;
+ bool pk_index_is_btree; /* is conindid a btree index? */
FastPathMeta *fpmeta;
} RI_ConstraintInfo;
@@ -2503,6 +2505,8 @@ ri_LoadConstraintInfo(Oid constraintOid)
riinfo->conindid = conForm->conindid;
riinfo->pk_is_partitioned =
(get_rel_relkind(riinfo->pk_relid) == RELKIND_PARTITIONED_TABLE);
+ riinfo->pk_index_is_btree =
+ (get_rel_relam(riinfo->conindid) == BTREE_AM_OID);
ReleaseSysCache(tup);
@@ -3153,7 +3157,8 @@ ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
* Build scan key with SK_SEARCHARRAY. The index AM code will internally
* sort and deduplicate, then walk leaf pages in order.
*
- * PK indexes are always btree, which supports SK_SEARCHARRAY.
+ * ri_fastpath_is_applicable() restricts the fast path to btree indexes,
+ * which support SK_SEARCHARRAY.
*
* This path handles single-column FKs only, so index_attnos[0] == 1.
*/
@@ -3372,6 +3377,17 @@ ri_fastpath_is_applicable(const RI_ConstraintInfo *riinfo)
if (riinfo->hasperiod)
return false;
+ /*
+ * The fast path probes the referenced index directly and, for
+ * single-column keys, uses SK_SEARCHARRAY. A foreign key's referenced
+ * index need not be a primary key; transformFkeyCheckAttrs() accepts any
+ * unique index, so an out-of-tree amcanunique access method could reach
+ * here. Restrict the fast path to btree, which is what the direct probe
+ * and SK_SEARCHARRAY assume; other access methods fall back to SPI.
+ */
+ if (!riinfo->pk_index_is_btree)
+ return false;
+
return true;
}
--
2.47.3
[application/octet-stream] v1-0006-Remove-stale-comment-on-ri_FastPathCheck.patch (1.3K, ../../CA+HiwqFYjOXwxKEjkTz6HB5O80J1k_XhdYcifGJ0cuG-ZFhY0A@mail.gmail.com/6-v1-0006-Remove-stale-comment-on-ri_FastPathCheck.patch)
download | inline diff:
From 159fcafb0e03ca68b2be92ec78e5b37bc7048b86 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Thu, 16 Jul 2026 18:17:12 +0900
Subject: [PATCH v1 6/6] Remove stale comment on ri_FastPathCheck()
The note claiming ri_FastPathCheck() is only used by the ALTER TABLE
validation path is no longer accurate. Rather than reword it, remove
it; the block comment above RI_FKey_check()'s call to the function
already documents when each path is taken, and a duplicated
cross-reference here would only drift out of date again.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260705210533.ee.noahmisch@microsoft.com
Backpatch-through: 19
---
src/backend/utils/adt/ri_triggers.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 53b834a42de..5513fa3b710 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -2809,9 +2809,6 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
*
* If no matching PK row exists, report the violation via ri_ReportViolation(),
* otherwise, the function returns normally.
- *
- * Note: This is only used by the ALTER TABLE validation path. Other paths use
- * ri_FastPathBatchAdd().
*/
static void
ri_FastPathCheck(RI_ConstraintInfo *riinfo,
--
2.47.3
[application/octet-stream] v1-0001-Handle-nullable-referenced-key-in-RI-fast-path-ch.patch (7.2K, ../../CA+HiwqFYjOXwxKEjkTz6HB5O80J1k_XhdYcifGJ0cuG-ZFhY0A@mail.gmail.com/7-v1-0001-Handle-nullable-referenced-key-in-RI-fast-path-ch.patch)
download | inline diff:
From 0311881433859e138271e36aec7ce99653c162cd Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Thu, 16 Jul 2026 18:16:51 +0900
Subject: [PATCH v1 1/6] Handle nullable referenced key in RI fast-path check
The RI fast-path FK check asserted that the referenced key is never
NULL, in ri_FastPathFlushArray() and in recheck_matched_pk_tuple().
That holds for a primary key, but a foreign key may reference any unique
column, and a UNIQUE column is nullable.
The assertion is reachable under READ COMMITTED. ri_LockPKTuple() locks
the matched PK tuple with TUPLE_LOCK_FLAG_FIND_LAST_VERSION, so when a
concurrent transaction commits a key-changing UPDATE while the check
waits, the lock follows the update chain to the latest version. If that
version now has NULL in the referenced column, the fast path reaches the
assert; in a non-assert build it would compare against the NULL and
treat it as a match.
A NULL referenced key cannot equal any (non-null) FK value, so treat it
as "referenced row not found" and let the ordinary foreign-key violation
be raised. This matches the SPI path, whose requalifying "pkatt = $n"
evaluates to NULL for such a row, so the row is not returned and the
check reports a violation.
Both fix sites are on the update-chain recheck path: the value read back
from the chased-to tuple in ri_FastPathFlushArray() (the single-column
SK_SEARCHARRAY flush), and the scan key built from that tuple in
recheck_matched_pk_tuple(), which both the single-column and
multi-column (ri_FastPathFlushLoop()) flush routines use to requalify
a concurrently updated tuple.
Add an isolation test exercising both flush routines: a concurrent
UPDATE sets the referenced key to NULL while an FK INSERT waits on the
row, and the INSERT must report a foreign-key violation rather than trip
the assert.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260705210533.ee.noahmisch@microsoft.com
Backpatch-through: 19
---
src/backend/utils/adt/ri_triggers.c | 25 +++++++++---
.../expected/fk-fastpath-null-key.out | 17 ++++++++
src/test/isolation/isolation_schedule | 1 +
.../isolation/specs/fk-fastpath-null-key.spec | 40 +++++++++++++++++++
4 files changed, 78 insertions(+), 5 deletions(-)
create mode 100644 src/test/isolation/expected/fk-fastpath-null-key.out
create mode 100644 src/test/isolation/specs/fk-fastpath-null-key.spec
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 627a9fb38ea..d2b969e31ae 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -3184,9 +3184,19 @@ ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
if (!ri_LockPKTuple(pk_rel, pk_slot, snapshot, &concurrently_updated))
continue;
- /* Extract the PK value from the matched and locked tuple */
+ /*
+ * Extract the PK value from the matched and locked tuple.
+ *
+ * A foreign key may reference a nullable unique column, not just a NOT
+ * NULL primary key. If ri_LockPKTuple() chased an update chain to a
+ * version whose referenced key is now NULL, that version cannot equal
+ * any buffered (non-null) FK value, so skip it. This mirrors the SPI
+ * path, where the requalifying "pkatt = $n" yields NULL and the row is
+ * not returned.
+ */
found_val = slot_getattr(pk_slot, riinfo->pk_attnums[0], &found_null);
- Assert(!found_null);
+ if (found_null)
+ continue;
if (concurrently_updated)
{
@@ -3427,9 +3437,14 @@ recheck_matched_pk_tuple(Relation idxrel, ScanKeyData *skeys, int nkeys,
{
ScanKeyData *skey = &skeys[i];
- /* A PK column can never be set to NULL. */
- Assert(!isnull[i]);
- if (!DatumGetBool(FunctionCall2Coll(&skey->sk_func,
+ /*
+ * A foreign key may reference a nullable unique column, so the version
+ * we chased the update chain to may have a NULL in a key column. A
+ * NULL never equals the value we searched for, so treat it as no
+ * match, as the SPI path's requalification would.
+ */
+ if (isnull[i] ||
+ !DatumGetBool(FunctionCall2Coll(&skey->sk_func,
skey->sk_collation,
values[i],
skey->sk_argument)))
diff --git a/src/test/isolation/expected/fk-fastpath-null-key.out b/src/test/isolation/expected/fk-fastpath-null-key.out
new file mode 100644
index 00000000000..3efe1e781b2
--- /dev/null
+++ b/src/test/isolation/expected/fk-fastpath-null-key.out
@@ -0,0 +1,17 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1b s1upd_null s2ins s1c
+step s1b: BEGIN;
+step s1upd_null: UPDATE pktable SET u = NULL WHERE u = 5;
+step s2ins: INSERT INTO fktable VALUES (5); <waiting ...>
+step s1c: COMMIT;
+step s2ins: <... completed>
+ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_a_fkey"
+
+starting permutation: s1b s1upd_null s2ins_arr s1c
+step s1b: BEGIN;
+step s1upd_null: UPDATE pktable SET u = NULL WHERE u = 5;
+step s2ins_arr: INSERT INTO fktable VALUES (5), (6); <waiting ...>
+step s1c: COMMIT;
+step s2ins_arr: <... completed>
+ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_a_fkey"
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index b8ebe92553c..cbc40f20713 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -38,6 +38,7 @@ test: fk-snapshot
test: fk-snapshot-2
test: fk-snapshot-3
test: fk-concurrent-pk-upd
+test: fk-fastpath-null-key
test: subxid-overflow
test: eval-plan-qual
test: eval-plan-qual-trigger
diff --git a/src/test/isolation/specs/fk-fastpath-null-key.spec b/src/test/isolation/specs/fk-fastpath-null-key.spec
new file mode 100644
index 00000000000..51e11b101dc
--- /dev/null
+++ b/src/test/isolation/specs/fk-fastpath-null-key.spec
@@ -0,0 +1,40 @@
+# A foreign key may reference a nullable UNIQUE column, not only a NOT NULL
+# primary key. Test that the RI fast-path check copes when a concurrent
+# transaction sets the referenced key to NULL.
+#
+# s2's INSERT probes the PK index, finds the (u=5) row, and blocks on s1's
+# in-progress key-changing UPDATE. After s1 commits, the tuple lock follows
+# the update chain (table_tuple_lock with TUPLE_LOCK_FLAG_FIND_LAST_VERSION)
+# to the now-NULL version. The check must treat that as "referenced row not
+# found" and raise an ordinary foreign-key violation -- not assume that a
+# referenced key can never be NULL.
+#
+# Two permutations exercise the two fast-path flush routines: a single-row
+# INSERT goes through ri_FastPathFlushLoop()/recheck_matched_pk_tuple(), while
+# a multi-row single-column INSERT goes through ri_FastPathFlushArray().
+
+setup
+{
+ CREATE TABLE pktable (u int UNIQUE, c int);
+ CREATE TABLE fktable (a int REFERENCES pktable (u));
+ INSERT INTO pktable VALUES (5, 1), (6, 2);
+}
+
+teardown
+{
+ DROP TABLE fktable, pktable;
+}
+
+session s1
+step s1b { BEGIN; }
+step s1upd_null { UPDATE pktable SET u = NULL WHERE u = 5; }
+step s1c { COMMIT; }
+
+session s2
+# single-row batch -> per-row loop flush path
+step s2ins { INSERT INTO fktable VALUES (5); }
+# multi-row single-column batch -> SK_SEARCHARRAY flush path
+step s2ins_arr { INSERT INTO fktable VALUES (5), (6); }
+
+permutation s1b s1upd_null s2ins s1c
+permutation s1b s1upd_null s2ins_arr s1c
--
2.47.3
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: ri_Fast* crash w/ nullable UNIQUE constraint
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-16 11:32 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
@ 2026-07-18 03:25 ` Noah Misch <noah@leadboat.com>
2026-07-21 05:56 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
1 sibling, 1 reply; 15+ messages in thread
From: Noah Misch @ 2026-07-18 03:25 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: zhjwpku@gmail.com; pgsql-hackers
On Thu, Jul 16, 2026 at 08:32:11PM +0900, Amit Langote wrote:
> Btw, do we want to tag Opus as the reporter and the author in the
> commit message as you did in your version of 0001?
I don't have an opinion on that. It generated the commit message. I'm
neither endorsing nor opposing inclusion of those lines.
> I'd rather not as
> that's not something the project has started doing unless I have
> missed. Since you found and reported the bug, I decided to tag you as
> the reporter in all of the patches for now.
That decision works for me.
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: ri_Fast* crash w/ nullable UNIQUE constraint
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-16 11:32 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-07-18 03:25 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
@ 2026-07-21 05:56 ` Amit Langote <amitlangote09@gmail.com>
2026-07-21 10:59 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Ayush Tiwari <ayushtiwari.slg01@gmail.com>
2026-07-21 12:05 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
0 siblings, 2 replies; 15+ messages in thread
From: Amit Langote @ 2026-07-21 05:56 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: zhjwpku@gmail.com; pgsql-hackers
Hi Noah,
On Sat, Jul 18, 2026 at 12:25 PM Noah Misch <noah@leadboat.com> wrote:
>
> On Thu, Jul 16, 2026 at 08:32:11PM +0900, Amit Langote wrote:
> > Btw, do we want to tag Opus as the reporter and the author in the
> > commit message as you did in your version of 0001?
>
> I don't have an opinion on that. It generated the commit message. I'm
> neither endorsing nor opposing inclusion of those lines.
>
> > I'd rather not as
> > that's not something the project has started doing unless I have
> > missed. Since you found and reported the bug, I decided to tag you as
> > the reporter in all of the patches for now.
>
> That decision works for me.
Thanks for confirming.
I plan to go on vacation starting Sunday for a couple of weeks, so I
was hoping to push these patches this week after taking another look.
If you or anyone else has comments, please let me know.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: ri_Fast* crash w/ nullable UNIQUE constraint
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-16 11:32 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-07-18 03:25 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-21 05:56 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
@ 2026-07-21 10:59 ` Ayush Tiwari <ayushtiwari.slg01@gmail.com>
2026-07-24 10:06 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
1 sibling, 1 reply; 15+ messages in thread
From: Ayush Tiwari @ 2026-07-21 10:59 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Noah Misch <noah@leadboat.com>; zhjwpku@gmail.com; pgsql-hackers
Hi,
On Tue, 21 Jul 2026 at 11:27, Amit Langote <amitlangote09@gmail.com> wrote:
> Hi Noah,
>
> On Sat, Jul 18, 2026 at 12:25 PM Noah Misch <noah@leadboat.com> wrote:
> >
> > On Thu, Jul 16, 2026 at 08:32:11PM +0900, Amit Langote wrote:
> > > Btw, do we want to tag Opus as the reporter and the author in the
> > > commit message as you did in your version of 0001?
> >
> > I don't have an opinion on that. It generated the commit message. I'm
> > neither endorsing nor opposing inclusion of those lines.
> >
> > > I'd rather not as
> > > that's not something the project has started doing unless I have
> > > missed. Since you found and reported the bug, I decided to tag you as
> > > the reporter in all of the patches for now.
> >
> > That decision works for me.
>
> Thanks for confirming.
>
> I plan to go on vacation starting Sunday for a couple of weeks, so I
> was hoping to push these patches this week after taking another look.
> If you or anyone else has comments, please let me know.
>
Thanks for the patches!
I was reviewing it yesterday and got stalled on 0004, went through
rest today. 0001 to 0003 looked good to me.
One thing I got stuck on in 0004 (resettable fn_mcxt): each flush does
MemoryContextReset(scratch_cxt), but I couldn't find where the cached
FmgrInfos' fn_extra is cleared. record_eq() (and some cast/eq
functions) cache state via fn_extra allocated in fn_mcxt, so after the
reset fn_extra seems to dangle, and the next flush reusing the same
FmgrInfo reads it back as valid; fmgr_info_copy() zeroes fn_extra for
what looks like this reason. With a composite key over two batches,
and an assert after the reset, fn_extra was non-NULL on the second
batch. Would clearing fn_extra (or re-copying the FmgrInfos) on reset
make sense, maybe with a record-typed two-batch test?
Also, the reset is only in ri_FastPathBatchFlush(); ri_FastPathCheck()
(ALTER TABLE validate / sub-transaction path) uses the same cached
FmgrInfos and scratch_cxt but doesn't reset. I wonder if the growth this
patch targets still applies there.
Doubt: 0005 moves index_beginscan() after the user-id switch in
ri_FastPathCheck(), but ri_FastPathBatchFlush() still begins the scan
before the switch. Is that intentional, or worth the same reordering?
Regards,
Ayush
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: ri_Fast* crash w/ nullable UNIQUE constraint
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-16 11:32 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-07-18 03:25 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-21 05:56 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-07-21 10:59 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Ayush Tiwari <ayushtiwari.slg01@gmail.com>
@ 2026-07-24 10:06 ` Amit Langote <amitlangote09@gmail.com>
2026-08-05 20:30 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Ayush Tiwari <ayushtiwari.slg01@gmail.com>
0 siblings, 1 reply; 15+ messages in thread
From: Amit Langote @ 2026-07-24 10:06 UTC (permalink / raw)
To: Ayush Tiwari <ayushtiwari.slg01@gmail.com>; +Cc: Noah Misch <noah@leadboat.com>; zhjwpku@gmail.com; pgsql-hackers
Hi Ayush,
On Tue, Jul 21, 2026 at 7:59 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:
> On Tue, 21 Jul 2026 at 11:27, Amit Langote <amitlangote09@gmail.com> wrote:
>> I plan to go on vacation starting Sunday for a couple of weeks, so I
>> was hoping to push these patches this week after taking another look.
>> If you or anyone else has comments, please let me know.
>
> Thanks for the patches!
>
> I was reviewing it yesterday and got stalled on 0004, went through
> rest today. 0001 to 0003 looked good to me.
Thanks for the review.
> One thing I got stuck on in 0004 (resettable fn_mcxt): each flush does
> MemoryContextReset(scratch_cxt), but I couldn't find where the cached
> FmgrInfos' fn_extra is cleared. record_eq() (and some cast/eq
> functions) cache state via fn_extra allocated in fn_mcxt, so after the
> reset fn_extra seems to dangle, and the next flush reusing the same
> FmgrInfo reads it back as valid; fmgr_info_copy() zeroes fn_extra for
> what looks like this reason. With a composite key over two batches,
> and an assert after the reset, fn_extra was non-NULL on the second
> batch. Would clearing fn_extra (or re-copying the FmgrInfos) on reset
> make sense, maybe with a record-typed two-batch test?
Good catch. I don't think scratch_cxt should be reset per flush at all
-- fn_extra points into it, so the reset frees state fn_extra still
refers to. Clearing fn_extra too would fix the dangling pointer but
discard the cache every flush, and nothing accumulates during use
anyway: record_eq() allocates only when fn_extra is NULL.
The actual leak is at invalidation, not during use: with fn_mcxt =
TopMemoryContext the cached state outlives fpmeta, so each
repopulation orphans the previous one. v2 keeps the context but never
resets it, deleting it with fpmeta in
InvalidateConstraintCacheCallBack().
I don't think we need a regression test here, since the fix only
prevents a leak.
> Also, the reset is only in ri_FastPathBatchFlush(); ri_FastPathCheck()
> (ALTER TABLE validate / sub-transaction path) uses the same cached
> FmgrInfos and scratch_cxt but doesn't reset. I wonder if the growth this
> patch targets still applies there.
Yes, that applies there too. The problem isn't anything piling up
across flushes, it's that the cached state gets left behind when
fpmeta goes away, so the calling path doesn't come into it at all.
> Doubt: 0005 moves index_beginscan() after the user-id switch in
> ri_FastPathCheck(), but ri_FastPathBatchFlush() still begins the scan
> before the switch. Is that intentional, or worth the same reordering?
Fixed, both sites are consistent now.
Updated patches attached. I don't think much time is left between now
and when I board a plane, so I will commit these when I'm back from
vacation; Aug 5 or 6.
--
Thanks, Amit Langote
Attachments:
[application/octet-stream] v2-0006-Remove-stale-comment-on-ri_FastPathCheck.patch (1.4K, ../../CA+HiwqHHK2D69+QqAom+th1kjZGK-0dYnrZujkEGFWpX_ZtxqQ@mail.gmail.com/2-v2-0006-Remove-stale-comment-on-ri_FastPathCheck.patch)
download | inline diff:
From d844822058874e1a4a8bccfa8428eed67b672f38 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Fri, 24 Jul 2026 18:49:12 +0900
Subject: [PATCH v2 6/6] Remove stale comment on ri_FastPathCheck()
The note claiming ri_FastPathCheck() is only used by the ALTER TABLE
validation path is no longer accurate. Rather than reword it, remove
it; the block comment above RI_FKey_check()'s call to the function
already documents when each path is taken, and a duplicated
cross-reference here would only drift out of date again.
Reported-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/20260705210533.ee.noahmisch@microsoft.com
Backpatch-through: 19
---
src/backend/utils/adt/ri_triggers.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 31ecb2d6dca..02ba20fff2c 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -2813,9 +2813,6 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
*
* If no matching PK row exists, report the violation via ri_ReportViolation(),
* otherwise, the function returns normally.
- *
- * Note: This is only used by the ALTER TABLE validation path. Other paths use
- * ri_FastPathBatchAdd().
*/
static void
ri_FastPathCheck(RI_ConstraintInfo *riinfo,
--
2.47.3
[application/octet-stream] v2-0005-Begin-RI-fast-path-index-scan-under-the-switched-.patch (3.1K, ../../CA+HiwqHHK2D69+QqAom+th1kjZGK-0dYnrZujkEGFWpX_ZtxqQ@mail.gmail.com/3-v2-0005-Begin-RI-fast-path-index-scan-under-the-switched-.patch)
download | inline diff:
From 35a0aa0000eac9ec7b5a209b38048d591b73ce3a Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Fri, 24 Jul 2026 18:49:04 +0900
Subject: [PATCH v2 5/6] Begin RI fast-path index scan under the switched user
id
ri_FastPathCheck() called index_beginscan() before switching to the
referenced relation's owner for the permission check and probe. For
btree this has no consequence, but starting the scan before the user id
switch sets a poor example for code dealing with out-of-tree access
methods, whose beginscan could observe the wrong user id. Move
index_beginscan() after the SetUserIdAndSecContext() call.
Reported-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/20260705210533.ee.noahmisch@microsoft.com
Backpatch-through: 19
---
src/backend/utils/adt/ri_triggers.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index dc2119cf475..31ecb2d6dca 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -2845,10 +2845,6 @@ ri_FastPathCheck(RI_ConstraintInfo *riinfo,
idx_rel = index_open(riinfo->conindid, AccessShareLock);
slot = table_slot_create(pk_rel, NULL);
- scandesc = index_beginscan(pk_rel, idx_rel,
- snapshot, NULL,
- riinfo->nkeys, 0,
- SO_NONE);
GetUserIdAndSecContext(&saved_userid, &saved_sec_context);
SetUserIdAndSecContext(RelationGetForm(pk_rel)->relowner,
@@ -2857,6 +2853,17 @@ ri_FastPathCheck(RI_ConstraintInfo *riinfo,
SECURITY_NOFORCE_RLS);
ri_CheckPermissions(pk_rel);
+ /*
+ * Begin the scan under the switched user id, so that any access method
+ * code invoked by index_beginscan() runs as the PK relation's owner. For
+ * btree this has no functional consequence, but it keeps the ordering
+ * correct for out-of-tree access methods.
+ */
+ scandesc = index_beginscan(pk_rel, idx_rel,
+ snapshot, NULL,
+ riinfo->nkeys, 0,
+ SO_NONE);
+
if (riinfo->fpmeta == NULL)
{
/* Reload to ensure it's valid. */
@@ -2982,9 +2989,6 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel,
*/
oldcxt = MemoryContextSwitchTo(fpentry->flush_cxt);
- scandesc = index_beginscan(pk_rel, idx_rel, snapshot, NULL,
- riinfo->nkeys, 0, SO_NONE);
-
GetUserIdAndSecContext(&saved_userid, &saved_sec_context);
SetUserIdAndSecContext(RelationGetForm(pk_rel)->relowner,
saved_sec_context |
@@ -3000,6 +3004,15 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel,
*/
ri_CheckPermissions(pk_rel);
+ /*
+ * Begin the scan under the switched user id, so that any access method
+ * code invoked by index_beginscan() runs as the PK relation's owner. For
+ * btree this has no functional consequence, but it keeps the ordering
+ * correct for out-of-tree access methods.
+ */
+ scandesc = index_beginscan(pk_rel, idx_rel, snapshot, NULL,
+ riinfo->nkeys, 0, SO_NONE);
+
if (riinfo->fpmeta == NULL)
{
/* Reload to ensure it's valid. */
--
2.47.3
[application/octet-stream] v2-0004-Give-RI-fast-path-cached-FmgrInfos-their-own-memo.patch (4.2K, ../../CA+HiwqHHK2D69+QqAom+th1kjZGK-0dYnrZujkEGFWpX_ZtxqQ@mail.gmail.com/4-v2-0004-Give-RI-fast-path-cached-FmgrInfos-their-own-memo.patch)
download | inline diff:
From c283389c9ea5767eb6bd52a7a5b5ef6d7186c5a7 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Fri, 24 Jul 2026 18:48:43 +0900
Subject: [PATCH v2 4/6] Give RI fast-path cached FmgrInfos their own memory
context
ri_populate_fastpath_metadata() copies the cast and equality FmgrInfos
into the cached FastPathMeta with fn_mcxt set to TopMemoryContext, the
context active at the copy. fn_mcxt is scratch space for the called
function: record_eq(), and the record I/O functions generally, allocate
their per-call cache there and keep a pointer to it in fn_extra.
Because that scratch is in TopMemoryContext, it outlives the metadata.
When the constraint is invalidated the FastPathMeta is pfree'd, but
whatever the cast and equality functions cached is left behind, with
nothing pointing at it. Each subsequent repopulation allocates afresh,
so a session that repeatedly invalidates a foreign key constraint grows
TopMemoryContext without bound: 1000 invalidations of a constraint on a
20-column composite key leaked about 450kB.
Give the metadata its own context for the FmgrInfos' fn_mcxt and delete
it along with the metadata, so the cached scratch is freed with the
FmgrInfos that point at it.
The context deliberately is not reset while the metadata is in use.
fn_extra points into fn_mcxt, so resetting it would leave those pointers
dangling; the next call would find fn_extra non-NULL and read freed
memory. fmgr_info_copy() zeroing fn_extra in the copy is the same
invariant seen from the other side. Nothing accumulates in the context
during use in any case: record_eq() and friends allocate only when
fn_extra is NULL and reuse the cache afterwards.
Reported-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/20260705210533.ee.noahmisch@microsoft.com
Backpatch-through: 19
---
src/backend/utils/adt/ri_triggers.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index f95f72348e1..dc2119cf475 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -159,6 +159,19 @@ typedef struct FastPathMeta
Oid subtypes[RI_MAX_NUMKEYS];
int strats[RI_MAX_NUMKEYS];
AttrNumber index_attnos[RI_MAX_NUMKEYS]; /* index column positions */
+
+ /*
+ * fn_mcxt for the cached FmgrInfos above. Cast and equality functions
+ * (e.g. record_eq()) use fn_mcxt as scratch space, caching state there and
+ * keeping a pointer to it in FmgrInfo.fn_extra. Give them a context of
+ * their own, created and destroyed with this struct, rather than
+ * TopMemoryContext: otherwise whatever they cached is orphaned when this
+ * metadata is invalidated and freed.
+ *
+ * Note this context must not be reset while the FmgrInfos remain in use,
+ * since that would free the state fn_extra still points at.
+ */
+ MemoryContext scratch_cxt;
} FastPathMeta;
/*
@@ -2595,6 +2608,7 @@ InvalidateConstraintCacheCallBack(Datum arg, SysCacheIdentifier cacheid,
riinfo->valid = false;
if (riinfo->fpmeta)
{
+ MemoryContextDelete(riinfo->fpmeta->scratch_cxt);
pfree(riinfo->fpmeta);
riinfo->fpmeta = NULL;
}
@@ -3543,6 +3557,12 @@ ri_populate_fastpath_metadata(RI_ConstraintInfo *riinfo,
Assert(riinfo != NULL && riinfo->valid);
fpmeta = palloc_object(FastPathMeta);
+
+ /* Scratch context for the cached FmgrInfos' fn_mcxt; see FastPathMeta. */
+ fpmeta->scratch_cxt = AllocSetContextCreate(TopMemoryContext,
+ "RI fast-path finfo scratch",
+ ALLOCSET_SMALL_SIZES);
+
for (int i = 0; i < riinfo->nkeys; i++)
{
Oid eq_opr = riinfo->pf_eq_oprs[i];
@@ -3568,9 +3588,9 @@ ri_populate_fastpath_metadata(RI_ConstraintInfo *riinfo,
fpmeta->index_attnos[i] = idx_col + 1;
fmgr_info_copy(&fpmeta->cast_func_finfo[i], &entry->cast_func_finfo,
- CurrentMemoryContext);
+ fpmeta->scratch_cxt);
fmgr_info_copy(&fpmeta->eq_opr_finfo[i], &entry->eq_opr_finfo,
- CurrentMemoryContext);
+ fpmeta->scratch_cxt);
fpmeta->regops[i] = get_opcode(eq_opr);
get_op_opfamily_properties(eq_opr,
--
2.47.3
[application/octet-stream] v2-0002-Fire-fast-path-FK-batches-inside-the-deferred-tri.patch (8.7K, ../../CA+HiwqHHK2D69+QqAom+th1kjZGK-0dYnrZujkEGFWpX_ZtxqQ@mail.gmail.com/5-v2-0002-Fire-fast-path-FK-batches-inside-the-deferred-tri.patch)
download | inline diff:
From a9bf94876a4cf59993cd9a302fdefe6939f4951d Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Fri, 24 Jul 2026 18:47:53 +0900
Subject: [PATCH v2 2/6] Fire fast-path FK batches inside the deferred trigger
loop
AfterTriggerFireDeferred() drains queued after-trigger events in a loop,
then called FireAfterTriggerBatchCallbacks() once after the loop to
flush any RI fast-path FK-check batches the fired triggers accumulated.
A batch callback runs user-supplied cast or equality functions during
the flush, and their DML can queue further deferred trigger events --
for example a cast that inserts into another table with its own deferred
foreign key. Because the flush ran after the drain loop had already
exited, such an event was left in afterTriggers.events with nothing left
to fire it: AfterTriggerFireDeferred() is the last drainer at commit, so
the event was silently dropped. The queued deferred FK check was
skipped and a row violating the constraint committed.
Move the flush inside the drain loop, after afterTriggerInvokeEvents(),
and drop the "all fired" break. The loop condition,
afterTriggerMarkEvents(), now re-checks after each flush, so events
queued by a flush are fired on the next iteration, at the correct time
(transaction end), and the loop terminates only when nothing remains.
The other FireAfterTriggerBatchCallbacks() callers do not have this
problem: AfterTriggerEndQuery() fires immediate triggers and any
deferred event a flush queues there is drained later by
AfterTriggerFireDeferred(); AfterTriggerSetState() (SET CONSTRAINTS)
likewise leaves a queued event for the eventual commit-time
AfterTriggerFireDeferred(). Only the commit-time deferred firing has no
later drainer, so only it lost events.
The fast-path FK check and its batch flush were added by commits
2da86c1ef9 and b7b27eb41a5. Add a regression test to foreign_key.sql
for the dropped commit-time check.
Reported-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/20260705210533.ee.noahmisch@microsoft.com
Backpatch-through: 19
---
src/backend/commands/trigger.c | 20 +++++++++---
src/test/regress/expected/foreign_key.out | 39 +++++++++++++++++++++++
src/test/regress/sql/foreign_key.sql | 38 ++++++++++++++++++++++
3 files changed, 92 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 401baddbfc6..c41157ce50c 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -5380,12 +5380,22 @@ AfterTriggerFireDeferred(void)
{
CommandId firing_id = afterTriggers.firing_counter++;
- if (afterTriggerInvokeEvents(events, firing_id, NULL, true))
- break; /* all fired */
- }
+ afterTriggerInvokeEvents(events, firing_id, NULL, true);
- /* Flush any fast-path batches accumulated by the triggers just fired. */
- FireAfterTriggerBatchCallbacks(afterTriggers.batch_callbacks);
+ /*
+ * Flush any fast-path FK-check batches accumulated by the triggers
+ * just fired. A batch callback runs user-supplied cast or equality
+ * functions, whose DML can queue further deferred trigger events.
+ * Flush inside the loop so afterTriggerMarkEvents() sees any such
+ * events on the next iteration and fires them; flushing after the
+ * loop would leave them unfired, silently skipping e.g. a deferred FK
+ * check and letting a violating row commit. (The former "all fired"
+ * break is therefore gone: the loop now terminates only when
+ * afterTriggerMarkEvents() finds nothing left, including events queued
+ * by the flush.)
+ */
+ FireAfterTriggerBatchCallbacks(afterTriggers.batch_callbacks);
+ }
afterTriggers.firing_depth--;
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index c334cce752c..120d3319451 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -3604,6 +3604,45 @@ ERROR: insert or update on table "fp_fk_defer" violates foreign key constraint
DETAIL: Key (a)=(3) is not present in table "fp_pk_defer".
COMMIT;
DROP TABLE fp_pk_defer, fp_fk_defer;
+-- A deferred FK check queued while firing deferred triggers at commit must
+-- not be lost. The RI fast-path flush runs during the deferred firing loop
+-- and can run user cast/equality code whose DML queues a further deferred
+-- check; that check must still fire. fk_defer_main's deferred fast-path check
+-- runs a cast whose function inserts a violating row into fk_defer_t2,
+-- queueing fk_defer_t2's own deferred check, which must still be reported.
+CREATE TABLE fk_defer_t2_pk (id int PRIMARY KEY);
+CREATE TABLE fk_defer_t2 (a int REFERENCES fk_defer_t2_pk(id)
+ DEFERRABLE INITIALLY DEFERRED);
+CREATE TYPE fk_defer_vch AS (v int);
+CREATE FUNCTION fk_defer_cast(fk_defer_vch) RETURNS int
+ LANGUAGE plpgsql AS $$
+BEGIN
+ INSERT INTO fk_defer_t2 VALUES (999); -- 999 absent, queues deferred check
+ RETURN $1.v;
+END$$;
+CREATE CAST (fk_defer_vch AS int) WITH FUNCTION fk_defer_cast(fk_defer_vch)
+ AS IMPLICIT;
+CREATE TABLE fk_defer_main_pk (id int PRIMARY KEY);
+INSERT INTO fk_defer_main_pk VALUES (1);
+CREATE TABLE fk_defer_main (a fk_defer_vch REFERENCES fk_defer_main_pk(id)
+ DEFERRABLE INITIALLY DEFERRED);
+-- At COMMIT the queued fk_defer_t2 check must fire and report the violation,
+-- rather than being dropped (which would let the dangling row commit).
+BEGIN;
+INSERT INTO fk_defer_main VALUES (row(1)::fk_defer_vch);
+COMMIT; -- expected: ERROR on fk_defer_t2_a_fkey
+ERROR: insert or update on table "fk_defer_t2" violates foreign key constraint "fk_defer_t2_a_fkey"
+DETAIL: Key (a)=(999) is not present in table "fk_defer_t2_pk".
+-- The queued check must fire at transaction end, not early: making the
+-- referenced row present in the same transaction lets the commit succeed.
+INSERT INTO fk_defer_t2_pk VALUES (999);
+BEGIN;
+INSERT INTO fk_defer_main VALUES (row(1)::fk_defer_vch);
+COMMIT; -- expected: success
+DROP TABLE fk_defer_main, fk_defer_main_pk, fk_defer_t2, fk_defer_t2_pk;
+DROP CAST (fk_defer_vch AS int);
+DROP FUNCTION fk_defer_cast(fk_defer_vch);
+DROP TYPE fk_defer_vch;
-- Subtransaction abort: cached state must be invalidated on ROLLBACK TO
CREATE TABLE fp_pk_subxact (a int PRIMARY KEY);
CREATE TABLE fp_fk_subxact (a int REFERENCES fp_pk_subxact);
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 17eadc4bb5a..b9b88064ea5 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -2580,6 +2580,44 @@ INSERT INTO fp_fk_defer VALUES (3); -- should fail, also tests that cache was c
COMMIT;
DROP TABLE fp_pk_defer, fp_fk_defer;
+-- A deferred FK check queued while firing deferred triggers at commit must
+-- not be lost. The RI fast-path flush runs during the deferred firing loop
+-- and can run user cast/equality code whose DML queues a further deferred
+-- check; that check must still fire. fk_defer_main's deferred fast-path check
+-- runs a cast whose function inserts a violating row into fk_defer_t2,
+-- queueing fk_defer_t2's own deferred check, which must still be reported.
+CREATE TABLE fk_defer_t2_pk (id int PRIMARY KEY);
+CREATE TABLE fk_defer_t2 (a int REFERENCES fk_defer_t2_pk(id)
+ DEFERRABLE INITIALLY DEFERRED);
+CREATE TYPE fk_defer_vch AS (v int);
+CREATE FUNCTION fk_defer_cast(fk_defer_vch) RETURNS int
+ LANGUAGE plpgsql AS $$
+BEGIN
+ INSERT INTO fk_defer_t2 VALUES (999); -- 999 absent, queues deferred check
+ RETURN $1.v;
+END$$;
+CREATE CAST (fk_defer_vch AS int) WITH FUNCTION fk_defer_cast(fk_defer_vch)
+ AS IMPLICIT;
+CREATE TABLE fk_defer_main_pk (id int PRIMARY KEY);
+INSERT INTO fk_defer_main_pk VALUES (1);
+CREATE TABLE fk_defer_main (a fk_defer_vch REFERENCES fk_defer_main_pk(id)
+ DEFERRABLE INITIALLY DEFERRED);
+-- At COMMIT the queued fk_defer_t2 check must fire and report the violation,
+-- rather than being dropped (which would let the dangling row commit).
+BEGIN;
+INSERT INTO fk_defer_main VALUES (row(1)::fk_defer_vch);
+COMMIT; -- expected: ERROR on fk_defer_t2_a_fkey
+-- The queued check must fire at transaction end, not early: making the
+-- referenced row present in the same transaction lets the commit succeed.
+INSERT INTO fk_defer_t2_pk VALUES (999);
+BEGIN;
+INSERT INTO fk_defer_main VALUES (row(1)::fk_defer_vch);
+COMMIT; -- expected: success
+DROP TABLE fk_defer_main, fk_defer_main_pk, fk_defer_t2, fk_defer_t2_pk;
+DROP CAST (fk_defer_vch AS int);
+DROP FUNCTION fk_defer_cast(fk_defer_vch);
+DROP TYPE fk_defer_vch;
+
-- Subtransaction abort: cached state must be invalidated on ROLLBACK TO
CREATE TABLE fp_pk_subxact (a int PRIMARY KEY);
CREATE TABLE fp_fk_subxact (a int REFERENCES fp_pk_subxact);
--
2.47.3
[application/octet-stream] v2-0003-Restrict-RI-fast-path-FK-check-to-btree-reference.patch (3.3K, ../../CA+HiwqHHK2D69+QqAom+th1kjZGK-0dYnrZujkEGFWpX_ZtxqQ@mail.gmail.com/6-v2-0003-Restrict-RI-fast-path-FK-check-to-btree-reference.patch)
download | inline diff:
From 40631234b3372e0dcffdb726a710907e657dfcde Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Fri, 24 Jul 2026 18:48:10 +0900
Subject: [PATCH v2 3/6] Restrict RI fast-path FK check to btree referenced
indexes
The RI fast-path check probes the referenced index directly and, for
single-column keys, uses SK_SEARCHARRAY. Both assume the index is a
btree. A comment claimed "PK indexes are always btree", but a foreign
key's referenced index need not be a primary key:
transformFkeyCheckAttrs() accepts any unique (or, for temporal keys,
exclusion) index, so an out-of-tree access method advertising
amcanunique could supply a non-btree index reachable by the fast path.
Add pk_index_is_btree to RI_ConstraintInfo, set from the referenced
index's access method when the constraint is loaded, and make
ri_fastpath_is_applicable() return false for non-btree indexes so such
constraints fall back to the SPI path.
Reported-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/20260705210533.ee.noahmisch@microsoft.com
Backpatch-through: 19
---
src/backend/utils/adt/ri_triggers.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index d2b969e31ae..f95f72348e1 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -32,6 +32,7 @@
#include "access/tableam.h"
#include "access/xact.h"
#include "catalog/index.h"
+#include "catalog/pg_am_d.h"
#include "catalog/pg_collation.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_namespace.h"
@@ -142,6 +143,7 @@ typedef struct RI_ConstraintInfo
Oid conindid;
bool pk_is_partitioned;
+ bool pk_index_is_btree; /* is conindid a btree index? */
FastPathMeta *fpmeta;
} RI_ConstraintInfo;
@@ -2503,6 +2505,8 @@ ri_LoadConstraintInfo(Oid constraintOid)
riinfo->conindid = conForm->conindid;
riinfo->pk_is_partitioned =
(get_rel_relkind(riinfo->pk_relid) == RELKIND_PARTITIONED_TABLE);
+ riinfo->pk_index_is_btree =
+ (get_rel_relam(riinfo->conindid) == BTREE_AM_OID);
ReleaseSysCache(tup);
@@ -3153,7 +3157,8 @@ ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
* Build scan key with SK_SEARCHARRAY. The index AM code will internally
* sort and deduplicate, then walk leaf pages in order.
*
- * PK indexes are always btree, which supports SK_SEARCHARRAY.
+ * ri_fastpath_is_applicable() restricts the fast path to btree indexes,
+ * which support SK_SEARCHARRAY.
*
* This path handles single-column FKs only, so index_attnos[0] == 1.
*/
@@ -3372,6 +3377,17 @@ ri_fastpath_is_applicable(const RI_ConstraintInfo *riinfo)
if (riinfo->hasperiod)
return false;
+ /*
+ * The fast path probes the referenced index directly and, for
+ * single-column keys, uses SK_SEARCHARRAY. A foreign key's referenced
+ * index need not be a primary key; transformFkeyCheckAttrs() accepts any
+ * unique index, so an out-of-tree amcanunique access method could reach
+ * here. Restrict the fast path to btree, which is what the direct probe
+ * and SK_SEARCHARRAY assume; other access methods fall back to SPI.
+ */
+ if (!riinfo->pk_index_is_btree)
+ return false;
+
return true;
}
--
2.47.3
[application/octet-stream] v2-0001-Handle-nullable-referenced-key-in-RI-fast-path-ch.patch (7.3K, ../../CA+HiwqHHK2D69+QqAom+th1kjZGK-0dYnrZujkEGFWpX_ZtxqQ@mail.gmail.com/7-v2-0001-Handle-nullable-referenced-key-in-RI-fast-path-ch.patch)
download | inline diff:
From abd774d85f431d7d64648d427b2e2be891cea76d Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Fri, 24 Jul 2026 18:47:33 +0900
Subject: [PATCH v2 1/6] Handle nullable referenced key in RI fast-path check
The RI fast-path FK check asserted that the referenced key is never
NULL, in ri_FastPathFlushArray() and in recheck_matched_pk_tuple().
That holds for a primary key, but a foreign key may reference any unique
column, and a UNIQUE column is nullable.
The assertion is reachable under READ COMMITTED. ri_LockPKTuple() locks
the matched PK tuple with TUPLE_LOCK_FLAG_FIND_LAST_VERSION, so when a
concurrent transaction commits a key-changing UPDATE while the check
waits, the lock follows the update chain to the latest version. If that
version now has NULL in the referenced column, the fast path reaches the
assert; in a non-assert build it would compare against the NULL and
treat it as a match.
A NULL referenced key cannot equal any (non-null) FK value, so treat it
as "referenced row not found" and let the ordinary foreign-key violation
be raised. This matches the SPI path, whose requalifying "pkatt = $n"
evaluates to NULL for such a row, so the row is not returned and the
check reports a violation.
Both fix sites are on the update-chain recheck path: the value read back
from the chased-to tuple in ri_FastPathFlushArray() (the single-column
SK_SEARCHARRAY flush), and the scan key built from that tuple in
recheck_matched_pk_tuple(), which both the single-column and
multi-column (ri_FastPathFlushLoop()) flush routines use to requalify
a concurrently updated tuple.
Add an isolation test exercising both flush routines: a concurrent
UPDATE sets the referenced key to NULL while an FK INSERT waits on the
row, and the INSERT must report a foreign-key violation rather than trip
the assert.
Reported-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/20260705210533.ee.noahmisch@microsoft.com
Backpatch-through: 19
---
src/backend/utils/adt/ri_triggers.c | 25 +++++++++---
.../expected/fk-fastpath-null-key.out | 17 ++++++++
src/test/isolation/isolation_schedule | 1 +
.../isolation/specs/fk-fastpath-null-key.spec | 40 +++++++++++++++++++
4 files changed, 78 insertions(+), 5 deletions(-)
create mode 100644 src/test/isolation/expected/fk-fastpath-null-key.out
create mode 100644 src/test/isolation/specs/fk-fastpath-null-key.spec
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 627a9fb38ea..d2b969e31ae 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -3184,9 +3184,19 @@ ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
if (!ri_LockPKTuple(pk_rel, pk_slot, snapshot, &concurrently_updated))
continue;
- /* Extract the PK value from the matched and locked tuple */
+ /*
+ * Extract the PK value from the matched and locked tuple.
+ *
+ * A foreign key may reference a nullable unique column, not just a NOT
+ * NULL primary key. If ri_LockPKTuple() chased an update chain to a
+ * version whose referenced key is now NULL, that version cannot equal
+ * any buffered (non-null) FK value, so skip it. This mirrors the SPI
+ * path, where the requalifying "pkatt = $n" yields NULL and the row is
+ * not returned.
+ */
found_val = slot_getattr(pk_slot, riinfo->pk_attnums[0], &found_null);
- Assert(!found_null);
+ if (found_null)
+ continue;
if (concurrently_updated)
{
@@ -3427,9 +3437,14 @@ recheck_matched_pk_tuple(Relation idxrel, ScanKeyData *skeys, int nkeys,
{
ScanKeyData *skey = &skeys[i];
- /* A PK column can never be set to NULL. */
- Assert(!isnull[i]);
- if (!DatumGetBool(FunctionCall2Coll(&skey->sk_func,
+ /*
+ * A foreign key may reference a nullable unique column, so the version
+ * we chased the update chain to may have a NULL in a key column. A
+ * NULL never equals the value we searched for, so treat it as no
+ * match, as the SPI path's requalification would.
+ */
+ if (isnull[i] ||
+ !DatumGetBool(FunctionCall2Coll(&skey->sk_func,
skey->sk_collation,
values[i],
skey->sk_argument)))
diff --git a/src/test/isolation/expected/fk-fastpath-null-key.out b/src/test/isolation/expected/fk-fastpath-null-key.out
new file mode 100644
index 00000000000..3efe1e781b2
--- /dev/null
+++ b/src/test/isolation/expected/fk-fastpath-null-key.out
@@ -0,0 +1,17 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1b s1upd_null s2ins s1c
+step s1b: BEGIN;
+step s1upd_null: UPDATE pktable SET u = NULL WHERE u = 5;
+step s2ins: INSERT INTO fktable VALUES (5); <waiting ...>
+step s1c: COMMIT;
+step s2ins: <... completed>
+ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_a_fkey"
+
+starting permutation: s1b s1upd_null s2ins_arr s1c
+step s1b: BEGIN;
+step s1upd_null: UPDATE pktable SET u = NULL WHERE u = 5;
+step s2ins_arr: INSERT INTO fktable VALUES (5), (6); <waiting ...>
+step s1c: COMMIT;
+step s2ins_arr: <... completed>
+ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_a_fkey"
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index b8ebe92553c..cbc40f20713 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -38,6 +38,7 @@ test: fk-snapshot
test: fk-snapshot-2
test: fk-snapshot-3
test: fk-concurrent-pk-upd
+test: fk-fastpath-null-key
test: subxid-overflow
test: eval-plan-qual
test: eval-plan-qual-trigger
diff --git a/src/test/isolation/specs/fk-fastpath-null-key.spec b/src/test/isolation/specs/fk-fastpath-null-key.spec
new file mode 100644
index 00000000000..51e11b101dc
--- /dev/null
+++ b/src/test/isolation/specs/fk-fastpath-null-key.spec
@@ -0,0 +1,40 @@
+# A foreign key may reference a nullable UNIQUE column, not only a NOT NULL
+# primary key. Test that the RI fast-path check copes when a concurrent
+# transaction sets the referenced key to NULL.
+#
+# s2's INSERT probes the PK index, finds the (u=5) row, and blocks on s1's
+# in-progress key-changing UPDATE. After s1 commits, the tuple lock follows
+# the update chain (table_tuple_lock with TUPLE_LOCK_FLAG_FIND_LAST_VERSION)
+# to the now-NULL version. The check must treat that as "referenced row not
+# found" and raise an ordinary foreign-key violation -- not assume that a
+# referenced key can never be NULL.
+#
+# Two permutations exercise the two fast-path flush routines: a single-row
+# INSERT goes through ri_FastPathFlushLoop()/recheck_matched_pk_tuple(), while
+# a multi-row single-column INSERT goes through ri_FastPathFlushArray().
+
+setup
+{
+ CREATE TABLE pktable (u int UNIQUE, c int);
+ CREATE TABLE fktable (a int REFERENCES pktable (u));
+ INSERT INTO pktable VALUES (5, 1), (6, 2);
+}
+
+teardown
+{
+ DROP TABLE fktable, pktable;
+}
+
+session s1
+step s1b { BEGIN; }
+step s1upd_null { UPDATE pktable SET u = NULL WHERE u = 5; }
+step s1c { COMMIT; }
+
+session s2
+# single-row batch -> per-row loop flush path
+step s2ins { INSERT INTO fktable VALUES (5); }
+# multi-row single-column batch -> SK_SEARCHARRAY flush path
+step s2ins_arr { INSERT INTO fktable VALUES (5), (6); }
+
+permutation s1b s1upd_null s2ins s1c
+permutation s1b s1upd_null s2ins_arr s1c
--
2.47.3
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: ri_Fast* crash w/ nullable UNIQUE constraint
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-16 11:32 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-07-18 03:25 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-21 05:56 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-07-21 10:59 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Ayush Tiwari <ayushtiwari.slg01@gmail.com>
2026-07-24 10:06 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
@ 2026-08-05 20:30 ` Ayush Tiwari <ayushtiwari.slg01@gmail.com>
2026-08-06 14:59 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 15+ messages in thread
From: Ayush Tiwari @ 2026-08-05 20:30 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Noah Misch <noah@leadboat.com>; zhjwpku@gmail.com; pgsql-hackers
Hi,
On Fri, 24 Jul 2026 at 15:37, Amit Langote <amitlangote09@gmail.com> wrote:
> Hi Ayush,
>
> On Tue, Jul 21, 2026 at 7:59 PM Ayush Tiwari
> <ayushtiwari.slg01@gmail.com> wrote:
> > On Tue, 21 Jul 2026 at 11:27, Amit Langote <amitlangote09@gmail.com>
> wrote:
> >> I plan to go on vacation starting Sunday for a couple of weeks, so I
> >> was hoping to push these patches this week after taking another look.
> >> If you or anyone else has comments, please let me know.
> >
> > Thanks for the patches!
> >
> > I was reviewing it yesterday and got stalled on 0004, went through
> > rest today. 0001 to 0003 looked good to me.
>
> Thanks for the review.
>
> > One thing I got stuck on in 0004 (resettable fn_mcxt): each flush does
> > MemoryContextReset(scratch_cxt), but I couldn't find where the cached
> > FmgrInfos' fn_extra is cleared. record_eq() (and some cast/eq
> > functions) cache state via fn_extra allocated in fn_mcxt, so after the
> > reset fn_extra seems to dangle, and the next flush reusing the same
> > FmgrInfo reads it back as valid; fmgr_info_copy() zeroes fn_extra for
> > what looks like this reason. With a composite key over two batches,
> > and an assert after the reset, fn_extra was non-NULL on the second
> > batch. Would clearing fn_extra (or re-copying the FmgrInfos) on reset
> > make sense, maybe with a record-typed two-batch test?
>
> Good catch. I don't think scratch_cxt should be reset per flush at all
> -- fn_extra points into it, so the reset frees state fn_extra still
> refers to. Clearing fn_extra too would fix the dangling pointer but
> discard the cache every flush, and nothing accumulates during use
> anyway: record_eq() allocates only when fn_extra is NULL.
>
> The actual leak is at invalidation, not during use: with fn_mcxt =
> TopMemoryContext the cached state outlives fpmeta, so each
> repopulation orphans the previous one. v2 keeps the context but never
> resets it, deleting it with fpmeta in
> InvalidateConstraintCacheCallBack().
>
> I don't think we need a regression test here, since the fix only
> prevents a leak.
>
> > Also, the reset is only in ri_FastPathBatchFlush(); ri_FastPathCheck()
> > (ALTER TABLE validate / sub-transaction path) uses the same cached
> > FmgrInfos and scratch_cxt but doesn't reset. I wonder if the growth this
> > patch targets still applies there.
>
> Yes, that applies there too. The problem isn't anything piling up
> across flushes, it's that the cached state gets left behind when
> fpmeta goes away, so the calling path doesn't come into it at all.
>
> > Doubt: 0005 moves index_beginscan() after the user-id switch in
> > ri_FastPathCheck(), but ri_FastPathBatchFlush() still begins the scan
> > before the switch. Is that intentional, or worth the same reordering?
>
> Fixed, both sites are consistent now.
>
> Updated patches attached. I don't think much time is left between now
> and when I board a plane, so I will commit these when I'm back from
> vacation; Aug 5 or 6.
>
Thanks for the updated patches!
I went through v2. 0001, 0002, 0003, 0005 and 0006 look good to me, and
I agree with not resetting the context in 0004.
One small question on 0004. It adds a MemoryContextDelete to
InvalidateConstraintCacheCallBack(), next to the existing pfree(fpmeta).
The comment above that function says entries are never removed, only
marked invalid, because there may be active references at that point.
ri_FastPathFlushArray() and build_index_scankeys() do hold fpmeta, and
pointers into it, across the user supplied cast and equality functions.
ISTM something must keep an invalidation from arriving there, but I
could not work out what. Is that guaranteed somewhere, and would a
comment help?
Regards,
Ayush
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: ri_Fast* crash w/ nullable UNIQUE constraint
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-16 11:32 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-07-18 03:25 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-21 05:56 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-07-21 10:59 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Ayush Tiwari <ayushtiwari.slg01@gmail.com>
2026-07-24 10:06 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-08-05 20:30 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Ayush Tiwari <ayushtiwari.slg01@gmail.com>
@ 2026-08-06 14:59 ` Amit Langote <amitlangote09@gmail.com>
2026-08-06 15:07 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-08-07 08:55 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
0 siblings, 2 replies; 15+ messages in thread
From: Amit Langote @ 2026-08-06 14:59 UTC (permalink / raw)
To: Ayush Tiwari <ayushtiwari.slg01@gmail.com>; +Cc: Noah Misch <noah@leadboat.com>; zhjwpku@gmail.com; pgsql-hackers
HI Ayush,
On Thu, Aug 6, 2026 at 5:30 AM Ayush Tiwari <ayushtiwari.slg01@gmail.com> wrote:
> On Fri, 24 Jul 2026 at 15:37, Amit Langote <amitlangote09@gmail.com> wrote:
>> On Tue, Jul 21, 2026 at 7:59 PM Ayush Tiwari
>> <ayushtiwari.slg01@gmail.com> wrote:
>> > One thing I got stuck on in 0004 (resettable fn_mcxt): each flush does
>> > MemoryContextReset(scratch_cxt), but I couldn't find where the cached
>> > FmgrInfos' fn_extra is cleared. record_eq() (and some cast/eq
>> > functions) cache state via fn_extra allocated in fn_mcxt, so after the
>> > reset fn_extra seems to dangle, and the next flush reusing the same
>> > FmgrInfo reads it back as valid; fmgr_info_copy() zeroes fn_extra for
>> > what looks like this reason. With a composite key over two batches,
>> > and an assert after the reset, fn_extra was non-NULL on the second
>> > batch. Would clearing fn_extra (or re-copying the FmgrInfos) on reset
>> > make sense, maybe with a record-typed two-batch test?
>>
>> Good catch. I don't think scratch_cxt should be reset per flush at all
>> -- fn_extra points into it, so the reset frees state fn_extra still
>> refers to. Clearing fn_extra too would fix the dangling pointer but
>> discard the cache every flush, and nothing accumulates during use
>> anyway: record_eq() allocates only when fn_extra is NULL.
>>
>> The actual leak is at invalidation, not during use: with fn_mcxt =
>> TopMemoryContext the cached state outlives fpmeta, so each
>> repopulation orphans the previous one. v2 keeps the context but never
>> resets it, deleting it with fpmeta in
>> InvalidateConstraintCacheCallBack().
>>
>> I don't think we need a regression test here, since the fix only
>> prevents a leak.
>>
>> > Also, the reset is only in ri_FastPathBatchFlush(); ri_FastPathCheck()
>> > (ALTER TABLE validate / sub-transaction path) uses the same cached
>> > FmgrInfos and scratch_cxt but doesn't reset. I wonder if the growth this
>> > patch targets still applies there.
>>
>> Yes, that applies there too. The problem isn't anything piling up
>> across flushes, it's that the cached state gets left behind when
>> fpmeta goes away, so the calling path doesn't come into it at all.
>>
>
> Thanks for the updated patches!
>
> I went through v2. 0001, 0002, 0003, 0005 and 0006 look good to me, and
> I agree with not resetting the context in 0004.
Thanks for checking. I'll push 0001, 0002, 0003, 0005 and 0006
shortly. 0004 needs more thought first -- see below.
> One small question on 0004. It adds a MemoryContextDelete to
> InvalidateConstraintCacheCallBack(), next to the existing pfree(fpmeta).
> The comment above that function says entries are never removed, only
> marked invalid, because there may be active references at that point.
> ri_FastPathFlushArray() and build_index_scankeys() do hold fpmeta, and
> pointers into it, across the user supplied cast and equality functions.
> ISTM something must keep an invalidation from arriving there, but I
> could not work out what. Is that guaranteed somewhere, and would a
> comment help?
Good question, and the answer is that nothing guarantees it -- you've
found a live bug. An invalidation really can arrive inside those
user-supplied functions, and the pfree() that's there today then frees
fpmeta while ri_FastPathFlushArray() is still reading it and calling
through FmgrInfos in it. 0004 makes that worse rather than better,
since MemoryContextDelete() next to the pfree() has the same problem.
So I'll hold 0004 back for now.
I'd started looking at this from a report off-list and posted it
separately just before your mail arrived:
https://www.postgresql.org/message-id/CA%2BHiwqFFB6vzx8v3t2%3DrbNYyxMistLf5kkJfqzJ81nadFyLrxA%40mail...
Your analysis there is the same as mine, arrived at independently.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: ri_Fast* crash w/ nullable UNIQUE constraint
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-16 11:32 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-07-18 03:25 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-21 05:56 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-07-21 10:59 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Ayush Tiwari <ayushtiwari.slg01@gmail.com>
2026-07-24 10:06 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-08-05 20:30 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Ayush Tiwari <ayushtiwari.slg01@gmail.com>
2026-08-06 14:59 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
@ 2026-08-06 15:07 ` Amit Langote <amitlangote09@gmail.com>
1 sibling, 0 replies; 15+ messages in thread
From: Amit Langote @ 2026-08-06 15:07 UTC (permalink / raw)
To: Ayush Tiwari <ayushtiwari.slg01@gmail.com>; +Cc: Noah Misch <noah@leadboat.com>; zhjwpku@gmail.com; pgsql-hackers
On Thu, Aug 6, 2026 at 23:59 Amit Langote <amitlangote09@gmail.com> wrote:
> HI Ayush,
>
> On Thu, Aug 6, 2026 at 5:30 AM Ayush Tiwari <ayushtiwari.slg01@gmail.com>
> wrote:
> > On Fri, 24 Jul 2026 at 15:37, Amit Langote <amitlangote09@gmail.com>
> wrote:
> >> On Tue, Jul 21, 2026 at 7:59 PM Ayush Tiwari
> >> <ayushtiwari.slg01@gmail.com> wrote:
> >> > One thing I got stuck on in 0004 (resettable fn_mcxt): each flush does
> >> > MemoryContextReset(scratch_cxt), but I couldn't find where the cached
> >> > FmgrInfos' fn_extra is cleared. record_eq() (and some cast/eq
> >> > functions) cache state via fn_extra allocated in fn_mcxt, so after the
> >> > reset fn_extra seems to dangle, and the next flush reusing the same
> >> > FmgrInfo reads it back as valid; fmgr_info_copy() zeroes fn_extra for
> >> > what looks like this reason. With a composite key over two batches,
> >> > and an assert after the reset, fn_extra was non-NULL on the second
> >> > batch. Would clearing fn_extra (or re-copying the FmgrInfos) on reset
> >> > make sense, maybe with a record-typed two-batch test?
> >>
> >> Good catch. I don't think scratch_cxt should be reset per flush at all
> >> -- fn_extra points into it, so the reset frees state fn_extra still
> >> refers to. Clearing fn_extra too would fix the dangling pointer but
> >> discard the cache every flush, and nothing accumulates during use
> >> anyway: record_eq() allocates only when fn_extra is NULL.
> >>
> >> The actual leak is at invalidation, not during use: with fn_mcxt =
> >> TopMemoryContext the cached state outlives fpmeta, so each
> >> repopulation orphans the previous one. v2 keeps the context but never
> >> resets it, deleting it with fpmeta in
> >> InvalidateConstraintCacheCallBack().
> >>
> >> I don't think we need a regression test here, since the fix only
> >> prevents a leak.
> >>
> >> > Also, the reset is only in ri_FastPathBatchFlush(); ri_FastPathCheck()
> >> > (ALTER TABLE validate / sub-transaction path) uses the same cached
> >> > FmgrInfos and scratch_cxt but doesn't reset. I wonder if the growth
> this
> >> > patch targets still applies there.
> >>
> >> Yes, that applies there too. The problem isn't anything piling up
> >> across flushes, it's that the cached state gets left behind when
> >> fpmeta goes away, so the calling path doesn't come into it at all.
> >>
> >
> > Thanks for the updated patches!
> >
> > I went through v2. 0001, 0002, 0003, 0005 and 0006 look good to me, and
> > I agree with not resetting the context in 0004.
>
> Thanks for checking. I'll push 0001, 0002, 0003, 0005 and 0006
> shortly. 0004 needs more thought first -- see below.
>
> > One small question on 0004. It adds a MemoryContextDelete to
> > InvalidateConstraintCacheCallBack(), next to the existing pfree(fpmeta).
> > The comment above that function says entries are never removed, only
> > marked invalid, because there may be active references at that point.
> > ri_FastPathFlushArray() and build_index_scankeys() do hold fpmeta, and
> > pointers into it, across the user supplied cast and equality functions.
> > ISTM something must keep an invalidation from arriving there, but I
> > could not work out what. Is that guaranteed somewhere, and would a
> > comment help?
>
> Good question, and the answer is that nothing guarantees it -- you've
> found a live bug. An invalidation really can arrive inside those
> user-supplied functions, and the pfree() that's there today then frees
> fpmeta while ri_FastPathFlushArray() is still reading it and calling
> through FmgrInfos in it. 0004 makes that worse rather than better,
> since MemoryContextDelete() next to the pfree() has the same problem.
> So I'll hold 0004 back for now.
>
> I'd started looking at this from a report off-list and posted it
> separately just before your mail arrived:
I meant to write “before I had the chance to read your email properly” ;-).
- Amit
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: ri_Fast* crash w/ nullable UNIQUE constraint
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-16 11:32 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-07-18 03:25 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-21 05:56 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-07-21 10:59 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Ayush Tiwari <ayushtiwari.slg01@gmail.com>
2026-07-24 10:06 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-08-05 20:30 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Ayush Tiwari <ayushtiwari.slg01@gmail.com>
2026-08-06 14:59 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
@ 2026-08-07 08:55 ` Amit Langote <amitlangote09@gmail.com>
1 sibling, 0 replies; 15+ messages in thread
From: Amit Langote @ 2026-08-07 08:55 UTC (permalink / raw)
To: Ayush Tiwari <ayushtiwari.slg01@gmail.com>; +Cc: Noah Misch <noah@leadboat.com>; zhjwpku@gmail.com; pgsql-hackers
Hi,
On Thu, Aug 6, 2026 at 11:59 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Aug 6, 2026 at 5:30 AM Ayush Tiwari <ayushtiwari.slg01@gmail.com> wrote:
> > I went through v2. 0001, 0002, 0003, 0005 and 0006 look good to me, and
> > I agree with not resetting the context in 0004.
>
> Thanks for checking. I'll push 0001, 0002, 0003, 0005 and 0006
> shortly. 0004 needs more thought first -- see below.
I have now pushed 0001 through 0006 except 0004.
> > One small question on 0004. It adds a MemoryContextDelete to
> > InvalidateConstraintCacheCallBack(), next to the existing pfree(fpmeta).
> > The comment above that function says entries are never removed, only
> > marked invalid, because there may be active references at that point.
> > ri_FastPathFlushArray() and build_index_scankeys() do hold fpmeta, and
> > pointers into it, across the user supplied cast and equality functions.
> > ISTM something must keep an invalidation from arriving there, but I
> > could not work out what. Is that guaranteed somewhere, and would a
> > comment help?
>
> Good question, and the answer is that nothing guarantees it -- you've
> found a live bug. An invalidation really can arrive inside those
> user-supplied functions, and the pfree() that's there today then frees
> fpmeta while ri_FastPathFlushArray() is still reading it and calling
> through FmgrInfos in it. 0004 makes that worse rather than better,
> since MemoryContextDelete() next to the pfree() has the same problem.
> So I'll hold 0004 back for now.
>
> I'd started looking at this from a report off-list and posted it
> separately just before your mail arrived:
>
> https://www.postgresql.org/message-id/CA%2BHiwqFFB6vzx8v3t2%3DrbNYyxMistLf5kkJfqzJ81nadFyLrxA%40mail...
>
> Your analysis there is the same as mine, arrived at independently.
Rather than reposting 0004 here, I'll move it to that thread. It now
has to be committed after the fix for the lifetime problem, so I felt
it better to keep the two together than to split them across threads.
I'll note there that it began as 0004 in this thread and that your
review prompted the change.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: ri_Fast* crash w/ nullable UNIQUE constraint
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-16 11:32 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-07-18 03:25 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-21 05:56 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
@ 2026-07-21 12:05 ` Noah Misch <noah@leadboat.com>
1 sibling, 0 replies; 15+ messages in thread
From: Noah Misch @ 2026-07-21 12:05 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: zhjwpku@gmail.com; pgsql-hackers
On Tue, Jul 21, 2026 at 02:56:44PM +0900, Amit Langote wrote:
> On Sat, Jul 18, 2026 at 12:25 PM Noah Misch <noah@leadboat.com> wrote:
> > On Thu, Jul 16, 2026 at 08:32:11PM +0900, Amit Langote wrote:
> > > Btw, do we want to tag Opus as the reporter and the author in the
> > > commit message as you did in your version of 0001?
> >
> > I don't have an opinion on that. It generated the commit message. I'm
> > neither endorsing nor opposing inclusion of those lines.
> >
> > > I'd rather not as
> > > that's not something the project has started doing unless I have
> > > missed. Since you found and reported the bug, I decided to tag you as
> > > the reporter in all of the patches for now.
> >
> > That decision works for me.
>
> Thanks for confirming.
>
> I plan to go on vacation starting Sunday for a couple of weeks, so I
> was hoping to push these patches this week after taking another look.
> If you or anyone else has comments, please let me know.
Got it. I don't plan to review these before Sunday. No concerns with that
plan.
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: ri_Fast* crash w/ nullable UNIQUE constraint
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-16 11:32 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
@ 2026-09-22 11:26 ` Amit Langote <amitlangote09@gmail.com>
2026-09-22 11:31 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
1 sibling, 1 reply; 15+ messages in thread
From: Amit Langote @ 2026-09-22 11:26 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: zhjwpku@gmail.com; pgsql-hackers
Hi,
On Thu, Jul 16, 2026 at 8:32 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Mon, Jul 6, 2026 at 6:05 AM Noah Misch <noah@leadboat.com> wrote:
> > I reviewed the ri_Fast* family of commits. This thread covers $SUBJECT and
> > some other findings. Feel free to fork more threads as needed.
> > ==== ri_CheckPermissions() does not cover hooks / sepgsql
> >
> > > The ri_CheckPermissions() function performs schema USAGE and table
> > > SELECT checks, matching what the SPI path gets implicitly through
> > > the executor's permission checks.
> >
> > It doesn't call ExecutorCheckPerms_hook or object_access_hook (via
> > e.g. InvokeFunctionExecuteHook), so sepgsql doesn't get control. That might
> > be okay if called out in the sepgsql documentation.
>
> You're right that the fast path doesn't reach ExecutorCheckPerms_hook
> or the object access hooks, so sepgsql doesn't get control where it
> would on the SPI path. Let me think through what restoring that would
> mean, because I'm not sure it's the right goal.
>
> On the SPI path these hooks fired as a consequence of the check
> running through the executor. For the per-row validation path in
> particular, that meant a hook invocation per row checked, so a foreign
> key validation over a large table would have produced an audit record
> per row. I don't think that was ever an intended sepgsql behavior; it
> seems more like a side effect of the execution path. Reproducing it
> deliberately on the fast path doesn't seem necessary IMHO.
>
> Invoking the hooks would also mean synthesizing an RTEPermissionInfo
> list outside any planned query, which is the kind of executor
> scaffolding the fast path is trying to avoid.
>
> Given that, my inclination is to leave it as is rather than wire up
> the hooks. I'm also unsure a sepgsql doc note is the right place,
> since it might read as a limitation we intend to close rather than an
> implementation detail. But I'd rather get your read before deciding.
> If you or anyone else thinks the bypass is worth addressing or noting
> somewhere, I'm happy to work out how.
On thinking about this more, I think it was wrong to say that RI
checks going through ExecutorCheckPerms_hook is an accidental detail.
These checks do access relations, and anyone who relies on the hook to
track every relation access, for example, won't see these, because the
fast path doesn't call it and so diverges from the SPI path there. In
light of the various recent fixes whose point was to bring the fast
path's behavior in line with the SPI path's, I'd like to propose
changing it to call the hook as well. Patch attached. I'll add an open
item.
--
Thanks, Amit Langote
Attachments:
[application/octet-stream] 0001-Route-RI-fast-path-permission-check-through-ExecChec.patch (3.4K, ../../CA+HiwqE4oLLTJqRA=pzahLGcnXVPxgYtNsLvOm_-tW04rOqSXw@mail.gmail.com/2-0001-Route-RI-fast-path-permission-check-through-ExecChec.patch)
download | inline diff:
From 7b1ffadf79371ff4dbb4462d4bd21ac5a38e4c93 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Tue, 22 Sep 2026 20:16:12 +0900
Subject: [PATCH] Route RI fast-path permission check through
ExecCheckPermissions()
ri_CheckPermissions() called ExecCheckOneRelPerms() directly, so
ExecutorCheckPerms_hook never saw the referenced-table access that the
SPI path's SELECT ... FOR KEY SHARE exposed to it via ExecutorStart().
Modules such as sepgsql that rely on this hook therefore lost control
over FK checks against non-partitioned referenced tables.
So, build a one-entry range table and permission-info list and call
ExecCheckPermissions(). The hook now sees the same relation and
requiredPerms as before, once per check.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260705210533.ee.noahmisch%40microsoft.com
Backpatch-through: 19
---
src/backend/utils/adt/ri_triggers.c | 30 ++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 059dafce836..e9edc7cd5e9 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -3613,14 +3613,18 @@ ri_check_fastpath_index(RI_ConstraintInfo *riinfo,
* ri_CheckPermissions
* Check permissions for the SELECT ... FOR KEY SHARE used by the SPI
* path, as the referenced table's owner.
+ *
+ * Go through ExecCheckPermissions() with a manufactured range table so that
+ * ExecutorCheckPerms_hook gets control exactly as it did when the check ran
+ * through the executor.
*/
static void
ri_CheckPermissions(const RI_ConstraintInfo *riinfo, Relation query_rel)
{
AclResult aclresult;
AclMode requiredPerms = ACL_SELECT | ACL_SELECT_FOR_UPDATE;
+ RangeTblEntry *rte;
RTEPermissionInfo *perminfo;
- bool result;
/* USAGE on schema. */
aclresult = object_aclcheck(NamespaceRelationId,
@@ -3630,16 +3634,10 @@ ri_CheckPermissions(const RI_ConstraintInfo *riinfo, Relation query_rel)
aclcheck_error(aclresult, OBJECT_SCHEMA,
get_namespace_name(RelationGetNamespace(query_rel)));
- /* Avoid building the column bitmap when table privileges suffice. */
- if (pg_class_aclmask(RelationGetRelid(query_rel), GetUserId(),
- requiredPerms, ACLMASK_ALL) == requiredPerms)
- return;
-
/*
* SELECT is needed only on the referenced key columns. FOR KEY SHARE
- * also needs UPDATE privilege, which may be granted on any column. Use
- * the executor's checks for both, leaving updatedCols empty as the SPI
- * query does.
+ * also needs UPDATE privilege, which may be granted on any column; leave
+ * updatedCols empty as the SPI query does.
*/
perminfo = makeNode(RTEPermissionInfo);
perminfo->relid = RelationGetRelid(query_rel);
@@ -3651,12 +3649,14 @@ ri_CheckPermissions(const RI_ConstraintInfo *riinfo, Relation query_rel)
perminfo->selectedCols = bms_add_member(perminfo->selectedCols, attno);
}
- result = ExecCheckOneRelPerms(perminfo);
- bms_free(perminfo->selectedCols);
- pfree(perminfo);
- if (!result)
- aclcheck_error(ACLCHECK_NO_PRIV, OBJECT_TABLE,
- RelationGetRelationName(query_rel));
+ rte = makeNode(RangeTblEntry);
+ rte->rtekind = RTE_RELATION;
+ rte->relid = RelationGetRelid(query_rel);
+ rte->relkind = query_rel->rd_rel->relkind;
+ rte->rellockmode = RowShareLock;
+ rte->perminfoindex = 1;
+
+ (void) ExecCheckPermissions(list_make1(rte), list_make1(perminfo), true);
}
/*
--
2.47.3
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: ri_Fast* crash w/ nullable UNIQUE constraint
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-16 11:32 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
2026-09-22 11:26 ` Re: ri_Fast* crash w/ nullable UNIQUE constraint Amit Langote <amitlangote09@gmail.com>
@ 2026-09-22 11:31 ` Amit Langote <amitlangote09@gmail.com>
0 siblings, 0 replies; 15+ messages in thread
From: Amit Langote @ 2026-09-22 11:31 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: zhjwpku@gmail.com; pgsql-hackers
On Tue, Sep 22, 2026 at 8:26 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Jul 16, 2026 at 8:32 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Mon, Jul 6, 2026 at 6:05 AM Noah Misch <noah@leadboat.com> wrote:
> > > I reviewed the ri_Fast* family of commits. This thread covers $SUBJECT and
> > > some other findings. Feel free to fork more threads as needed.
> > > ==== ri_CheckPermissions() does not cover hooks / sepgsql
> > >
> > > > The ri_CheckPermissions() function performs schema USAGE and table
> > > > SELECT checks, matching what the SPI path gets implicitly through
> > > > the executor's permission checks.
> > >
> > > It doesn't call ExecutorCheckPerms_hook or object_access_hook (via
> > > e.g. InvokeFunctionExecuteHook), so sepgsql doesn't get control. That might
> > > be okay if called out in the sepgsql documentation.
> >
> > You're right that the fast path doesn't reach ExecutorCheckPerms_hook
> > or the object access hooks, so sepgsql doesn't get control where it
> > would on the SPI path. Let me think through what restoring that would
> > mean, because I'm not sure it's the right goal.
> >
> > On the SPI path these hooks fired as a consequence of the check
> > running through the executor. For the per-row validation path in
> > particular, that meant a hook invocation per row checked, so a foreign
> > key validation over a large table would have produced an audit record
> > per row. I don't think that was ever an intended sepgsql behavior; it
> > seems more like a side effect of the execution path. Reproducing it
> > deliberately on the fast path doesn't seem necessary IMHO.
> >
> > Invoking the hooks would also mean synthesizing an RTEPermissionInfo
> > list outside any planned query, which is the kind of executor
> > scaffolding the fast path is trying to avoid.
> >
> > Given that, my inclination is to leave it as is rather than wire up
> > the hooks. I'm also unsure a sepgsql doc note is the right place,
> > since it might read as a limitation we intend to close rather than an
> > implementation detail. But I'd rather get your read before deciding.
> > If you or anyone else thinks the bypass is worth addressing or noting
> > somewhere, I'm happy to work out how.
>
> On thinking about this more, I think it was wrong to say that RI
> checks going through ExecutorCheckPerms_hook is an accidental detail.
> These checks do access relations, and anyone who relies on the hook to
> track every relation access, for example, won't see these, because the
> fast path doesn't call it and so diverges from the SPI path there. In
> light of the various recent fixes whose point was to bring the fast
> path's behavior in line with the SPI path's, I'd like to propose
> changing it to call the hook as well. Patch attached. I'll add an open
> item.
Added:
RI fastpath doesn't call ExecutorCheckPerms_hook
Commit: 2da86c1ef9b
Owner: Amit Langote
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2026-09-22 11:31 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-07-05 21:05 ri_Fast* crash w/ nullable UNIQUE constraint Noah Misch <noah@leadboat.com>
2026-07-05 21:47 ` Noah Misch <noah@leadboat.com>
2026-07-07 13:44 ` Amit Langote <amitlangote09@gmail.com>
2026-07-16 11:32 ` Amit Langote <amitlangote09@gmail.com>
2026-07-18 03:25 ` Noah Misch <noah@leadboat.com>
2026-07-21 05:56 ` Amit Langote <amitlangote09@gmail.com>
2026-07-21 10:59 ` Ayush Tiwari <ayushtiwari.slg01@gmail.com>
2026-07-24 10:06 ` Amit Langote <amitlangote09@gmail.com>
2026-08-05 20:30 ` Ayush Tiwari <ayushtiwari.slg01@gmail.com>
2026-08-06 14:59 ` Amit Langote <amitlangote09@gmail.com>
2026-08-06 15:07 ` Amit Langote <amitlangote09@gmail.com>
2026-08-07 08:55 ` Amit Langote <amitlangote09@gmail.com>
2026-07-21 12:05 ` Noah Misch <noah@leadboat.com>
2026-09-22 11:26 ` Amit Langote <amitlangote09@gmail.com>
2026-09-22 11:31 ` Amit Langote <amitlangote09@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox