Re: ri_Fast* crash w/ nullable UNIQUE constraint

Hi Noah,

On Mon, Jul 6, 2026 at 6:05 AM Noah Misch <noah(at)leadboat(dot)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