Re: PG19 FK fast path: OOB write and missed FK checks during batched

On Mon, Jul 6, 2026 at 11:29 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Mon, Jul 6, 2026 at 7:21 AM Noah Misch <noah(at)leadboat(dot)com> wrote:
> >
> > On Fri, Jun 12, 2026 at 11:46:07AM +0900, Amit Langote wrote:
> > > I've pushed these now. Thank you everyone.
> >
> > commit 4113873 wrote:
> > > Confine RI fast-path batching to the top transaction level
> >
> > I discourage this fix strategy, for three reasons:
> >
> > 1. It slows "BEGIN; SAVEPOINT s; COPY table_with_fk FROM ..." by the ~1.6x
> > batching benefit, compared to omitting the SAVEPOINT. That's a bad user
> > experience not seen elsewhere. Starting a high number of subtransactions
> > is expensive, but wrapping a long-running transaction body in one
> > subtransaction hasn't been a performance reducer.
>
> Ok, I agree it's a wart.
>
> > 2. It departs from the PostgreSQL norm of tracking resources by
> > subtransaction. You can see normal handling in many AbortSubTransaction()
> > callees, e.g. AtEOSubXact_LargeObject(). This in turn makes the change
> > harder to verify as correct.
> >
> > 3. It doesn't seem to have simplified code much, compared to our normal
> > subxact-based approach.
>
> Fair, I added a special case, which I can see now doesn't actually
> simplify things. I'll rework it to track batches per subtransaction
> the normal way.

0001 does this. It drops the top-level confinement and adds
AtEOSubXact_RI(), called from Commit/AbortSubTransaction() after the
subtransaction's ResourceOwnerRelease(). On abort it discards only the
entries opened by the ending subtransaction, identified by a subid
stamped on each entry at creation; it closes nothing itself, since the
ResourceOwner has already released those relations. Entries opened at
an outer level are left alone, so an inner subxact abort during
outer-level trigger firing no longer discards the outer statement's
batch. This follows the AtEOSubXact_* pattern you pointed at rather
than the special case I had.

> > > First, on subtransaction abort ri_FastPathSubXactCallback discarded the
> > > entire cache. An entry's batch holds rows buffered by the enclosing
> > > transaction, not just the aborting subxact -- the cache is keyed by
> > > constraint, so a single entry can mix rows from multiple subxact levels.
> >
> > That would imply having started a subtransaction and then added to the batch
> > without an intervening pair of CommandCounterIncrement() and
> > AfterTriggerBeginQuery(). That's an invalid thing for C code to do, so I'd
> > make it an error if a batch would contain rows from different subtransactions.
>
> Agreed. I confirmed a batch is flushed at AfterTriggerEndQuery and
> deferred checks don't populate a batch until they fire at top-level
> commit, so an entry is always at a single subxact level. The "mixes
> levels" justification in my commit message was a hypothesis I never
> verified, and it's wrong; a can't-happen assert is most likely the
> right thing.

Attached 0003 adds such an assert: Assert(fpentry->subid ==
GetCurrentSubTransactionId()) in ri_FastPathBatchAdd(), so every row
added to an entry comes from the subtransaction that created it. It
doesn't fire anywhere in the regression tests. AtEOSubXact_RI() relies
on this invariant to identify an aborting subtransaction's entries by
their stamped subid.

> > (This relates to the reentrancy bug fixed in 0e47bb5. With an intervening
> > CommandCounterIncrement() and AfterTriggerBeginQuery(), reusing the outer
> > batch is wrong even without subtransactions: it would check with the wrong
> > snapshot. Each level of reentrancy needs to finish its FK checks separately,
> > even if the batch buffer were unbounded.)
> >
> > > An internal subxact abort during after-trigger firing (e.g. a PL/pgSQL
> > > BEGIN ... EXCEPTION block) therefore dropped buffered rows
> >
> > I suspect a subxact abort during after-trigger firing can still cause a
> > different problem via AfterTriggerEndQuery():
> >
> > AfterTriggerEndQuery(EState *estate)
> > {
> > ...
> > afterTriggers.firing_depth++;
> >
> > AfterTriggerEndSubXact() doesn't undo this increment. Having identified that
> > as suspect, I asked Opus 4.8 to try to confirm or refute bug reachability. I
> > have not personally verified its finding, but it said:
> >
> > CLAUDE [CONFIRMED -- reachable bug; your instinct is right]: firing_depth is ++/-- in matched
> > pairs inside AfterTriggerEndQuery/FireDeferred/SetState with NO PG_TRY, and AfterTriggerEndSubXact
> > resets firing_batch_callbacks but NOT firing_depth. So a trigger ERROR caught by an outer
> > subtransaction (PL/pgSQL EXCEPTION) skips the -- and strands firing_depth>0 for the rest of the
> > xact. Its sole consumer is AfterTriggerIsActive() -> the RI_FKey_check batching gate; the only
> > RI check that runs OUTSIDE genuine trigger firing is ALTER TABLE / VALIDATE CONSTRAINT per-row
> > validation. With firing_depth stranded, that validation is wrongly routed into ri_FastPathBatchAdd.
> > Repro (per-row forced via REFERENCES-only, no SELECT, so RI_Initial_Check bails):
> > CONTROL: per-row-validated ALTER ADD FK with violating row 99 -> errors AT the ALTER (correct).
> > BUG: run a caught FK violation first (DO/EXCEPTION), then the same ALTER in the same xact ->
> > the ALTER does NOT error, marks convalidated=t, emits "WARNING: resource was not closed:
> > relation pk2 / pk2_pkey / TupleDesc" (a resource-owner leak), and defers the violation to
> > COMMIT. Consequences: resource leak + FK validation deferred past the ALTER (documented
> > invariant broken). Fix: reset firing_depth in AfterTriggerEndSubXact, mirroring the
> > firing_batch_callbacks reset right below it.
> >
> > Even if that's a hallucination, it's an example of what I meant in (2) about
> > making the change harder to verify.
>
> That looks like a real bug, likely of the same class as an earlier
> error-path reset I fixed. I'll verify the reproducer; the fix is
> likely resetting firing_depth in AfterTriggerEndSubXact alongside the
> firing_batch_callbacks reset.

Confirmed, and it's worse than a stranded flag -- it corrupts data.
0002 to fix it. The reproducer:

A caught FK-check error inside a subtransaction (a PL/pgSQL EXCEPTION
block) skips the firing_depth-- , leaving firing_depth set. A later
ALTER TABLE ... ADD FOREIGN KEY in the same transaction, whose
validation runs per-row rather than via RI_Initial_Check()'s bulk join
(e.g. because RLS is enabled on the referenced table, so
RI_Initial_Check() bails), then calls RI_FKey_check() with
AfterTriggerIsActive() wrongly true. The check is routed into the
batched fast path -- but a utility command has no
AfterTriggerEndQuery() to fire the flush callback, so the batch is
never flushed. The violating row is not reported, the constraint is
marked convalidated = t, and the cached PK relation and index leak
("resource was not closed"). So a foreign key ends up validated with a
row that violates it. I've added this as a regression test in 0003.

On the fix: 0003 saves firing_depth at subtransaction start and
restores it at end, in AfterTriggerEndSubXact(), next to the existing
query_depth handling. Resetting to 0 instead is wrong -- a subxact can
begin and end while an outer query is firing, where firing_depth is
legitimately positive, and zeroing it there trips the firing_depth > 0
assert in FireAfterTriggerBatchCallbacks() (the fp_subxact test in
0001 and the transition-table tests in the PL suites both hit this).

> > Also, it's not clear to me why AfterTriggerEndSubXact() is right to reset
> > afterTriggers.firing_batch_callbacks. The outer xact may be firing. If
> > that's okay, can you expand the code comment to explain it?
>
> I'll go back and reconstruct why that reset is correct. If I can't,
> I'll treat it as suspect and address it in the rework, with a comment
> either way.

The unconditional clear is wrong for the same reason: a subtransaction
that begins and ends while an outer FireAfterTriggerBatchCallbacks()
loop is active leaves firing_batch_callbacks legitimately true at
AfterTriggerEndSubXact(), and clearing it drops a flag the outer
firing still needs. 0003 gives it the same save/restore treatment as
firing_depth. I could not construct an observable failure from the
firing_batch_callbacks case specifically -- a separate guard,
ri_fastpath_flushing, routes any FK check re-entered from user code
during a flush onto the per-row path, so a re-entrant check never
reaches RegisterAfterTriggerBatchCallback() while the flag might be
wrongly cleared -- but restoring it is correct regardless and matches
the firing_depth handling.

> > > Cleanly unwinding the cache on subxact abort would require tracking the
> > > originating subxact of each buffered row, since rows from different
> > > levels share an entry (the cache is keyed by constraint) and deferred
> > > constraints cannot be flushed early at a subxact boundary.
> >
> > It's true that they can't be flushed early, but I'm not seeing a need for
> > explicit code to avoid that. A subxact commit shall just confirm there's no
> > batch of its subxact level. A subxact abort shall discard any batch of its
> > subxact level, leaving higher-subxact batches untouched. A deferred trigger
> > doesn't start a batch until the end of the top-level transaction.
>
> Yes. I was confused about the relationship between deferred firing and
> subxacts, which is where that justification came from; deferred
> batches only exist at the top level, so there's nothing to unwind for
> them at a subxact boundary.

This is what 0001 does. Abort discards the ending level's entries;
commit leaves nothing to do at that level, since the batch was already
flushed at statement end. Deferred checks populate a batch only when
they fire at top-level commit (query depth -1), so no subxact boundary
ever has a deferred batch to unwind.

> > > The per-row fast path still bypasses SPI and stays well ahead of the
> > > pre-19 SPI-based check. A fuller fix that preserves batching across
> > > subtransactions -- whether by tracking the originating subxact of each
> > > buffered row or by per-subxact cache stacks merged into the parent on
> > > commit -- is left for a future release.
> >
> > If the above suspicion corresponds to a live bug, I'd bet on the fuller fix
> > being cleaner than a surgical fix. That may not pan out, but I recommend
> > trying it first.
>
> Trying it in the direction you describe: subxact abort discards that
> level's batch, and subxact commit just checks that there's no batch
> left at that level (it was already flushed at statement end). The part
> that needs care is the resource-owner handling of the cached PK
> relation and index when a batch flush errors partway through inside a
> subxact that then aborts. That's more involved than a subid tag. I'll
> work through the details and try to post a patch tomorrow.

Done; the three patches are attached. The resource-owner handling
turned out simpler than I expected: because AtEOSubXact_RI() runs
after the subtransaction's ResourceOwnerRelease(), it only forgets the
aborting level's cache entries and never closes their relations
itself, so a batch flush that errors partway through inside a subxact
is cleaned up by the ResourceOwner on the way out.

On performance: with the top-level-txn-only confinement gone, a
savepoint-wrapped FK load no longer drops to the per-row path. On a
check-dominated test the savepoint case now matches the top-level
case. Before (i.e. with 4113873a) it was slowed by roughly 10% in my
tests, because it didn't take the same buffered path.

--
Thanks, Amit Langote