ri_Fast* crash w/ nullable UNIQUE constraint - Mailing list 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.