Date: Sun, 5 Jul 2026 15:21:15 -0700
From: Noah Misch <noah@leadboat.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Junwang Zhao <zhjwpku@gmail.com>,
	Ayush Tiwari <ayushtiwari.slg01@gmail.com>,
	Nikolay Samokhvalov <nik@postgres.ai>,
	pgsql-hackers mailing list <pgsql-hackers@postgresql.org>,
	Andrey Borodin <amborodin@acm.org>, Kirk Wolak <wolakk@gmail.com>
Subject: Re: PG19 FK fast path: OOB write and missed FK checks during batched
Message-ID: <20260705222115.be.noahmisch@microsoft.com>
References: 
 <CA+HiwqHUz50YqJn4XiNsSLN2c+9eYBy1af=y_dfdJTsz5BmbJg@mail.gmail.com>
 <CAM527d_2OpJ3KCOT1QqGh4neCPpgZTgM+VUxTqVgOSweOzTDQw@mail.gmail.com>
 <CA+HiwqFBXTTy3KcfGVxqxkhX5zV99R7=s2EwkxMiiWnVbyTpyw@mail.gmail.com>
 <CAJTYsWVgHCNDZb2F62F+aELnKJO2BWtHaAXcN-AmgPPP+GAnUQ@mail.gmail.com>
 <CA+HiwqELE-eyOfBBEmpr_eGf-04PUvZg5BjypW2CMHbed5QGhA@mail.gmail.com>
 <CAEG8a3JhZwvdeedTvQCvvXgVkeb6ENW2haiHfxnXkXbv3NMmjg@mail.gmail.com>
 <CA+HiwqG4Psv+777t8FsSpOTtMeqgBYTYfD8EXHYQmCwVcprC_g@mail.gmail.com>
 <CAEG8a3L6avXJNnk1BBZZvDG8dR15FWZdK8RALuZ4SLpgXTw3xg@mail.gmail.com>
 <CA+HiwqHHLyjtk_4LoNkoFYfHQJy9LjDhydKDHn=UHCxu=Qo6Qg@mail.gmail.com>
 <CA+HiwqFwUwakVfcmC2-AQEjO2kgqfy8n0qqxcMkvDBsLGdMgjA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+HiwqFwUwakVfcmC2-AQEjO2kgqfy8n0qqxcMkvDBsLGdMgjA@mail.gmail.com>
User-Agent: Mutt/2.3.0 (2026-01-25)
Archived-At: 
 <https://www.postgresql.org/message-id/20260705222115.be.noahmisch%40microsoft.com>
Precedence: bulk

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.

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.

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

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

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?

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

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