ri_Fast* crash w/ nullable UNIQUE constraint

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.

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]/messages/by-id/20260705222115.be.noahmisch@microsoft.com.

--
Thanks, Amit Langote

[1]: /messages/by-id/20260705222115.be.noahmisch@microsoft.com

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

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.

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

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

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.

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

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

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:

/messages/by-id/CA+HiwqFFB6vzx8v3t2=rbNYyxMistLf5kkJfqzJ81nadFyLrxA@mail.gmail.com

Your analysis there is the same as mine, arrived at independently.

--
Thanks, Amit Langote

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

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:

/messages/by-id/CA+HiwqFFB6vzx8v3t2=rbNYyxMistLf5kkJfqzJ81nadFyLrxA@mail.gmail.com

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

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

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