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

Hi Noah,

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.

> > 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.

> (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.

> 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.

> > 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.

> > 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.

Thanks for the deep review.

--
Thanks, Amit Langote