Hi, On Thu, Jul 9, 2026 at 8:37 PM Amit Langote <[email protected]> wrote: > > On Mon, Jul 6, 2026 at 11:29 PM Amit Langote <[email protected]> wrote: > > On Mon, Jul 6, 2026 at 7:21 AM Noah Misch <[email protected]> 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
