ri_Fast* crash w/ nullable UNIQUE constraint

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.