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.
