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

Hi,

On Thu, Jul 9, 2026 at 8:37 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> 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.

Would like to commit these tomorrow after review, barring objections.

--
Thanks, Amit Langote