Date: Sun, 5 Jul 2026 14:05:33 -0700
From: Noah Misch <noah@leadboat.com>
To: amitlangote09@gmail.com, zhjwpku@gmail.com
Cc: pgsql-hackers@postgresql.org
Subject: ri_Fast* crash w/ nullable UNIQUE constraint
Message-ID: <20260705210533.ee.noahmisch@microsoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/2.3.0 (2026-01-25)
Archived-At: 
 <https://www.postgresql.org/message-id/20260705210533.ee.noahmisch%40microsoft.com>
Precedence: bulk

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.