MIME-Version: 1.0
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>
 <20260705222115.be.noahmisch@microsoft.com>
In-Reply-To: <20260705222115.be.noahmisch@microsoft.com>
From: Amit Langote <amitlangote09@gmail.com>
Date: Mon, 6 Jul 2026 23:29:18 +0900
Message-ID: 
 <CA+HiwqGRorZfmC1WJ+vanLjOawrwjMP1Jgf1XgVOuumiyKGg5g@mail.gmail.com>
Subject: Re: PG19 FK fast path: OOB write and missed FK checks during batched
To: Noah Misch <noah@leadboat.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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BHiwqGRorZfmC1WJ%2BvanLjOawrwjMP1Jgf1XgVOuumiyKGg5g%40mail.gmail.com>
Precedence: bulk

Hi Noah,

On Mon, Jul 6, 2026 at 7:21=E2=80=AFAM Noah Misch <noah@leadboat.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.6=
x
>    batching benefit, compared to omitting the SAVEPOINT.  That's a bad us=
er
>    experience not seen elsewhere.  Starting a high number of subtransacti=
ons
>    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 AbortSubTransacti=
on()
>    callees, e.g. AtEOSubXact_LargeObject().  This in turn makes the chang=
e
>    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 enclosin=
g
> >     transaction, not just the aborting subxact -- the cache is keyed by
> >     constraint, so a single entry can mix rows from multiple subxact le=
vels.
>
> That would imply having started a subtransaction and then added to the ba=
tch
> 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 subtransact=
ions.

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 intervenin=
g
> CommandCounterIncrement() and AfterTriggerBeginQuery(), reusing the outer
> batch is wrong even without subtransactions: it would check with the wron=
g
> snapshot.  Each level of reentrancy needs to finish its FK checks separat=
ely,
> even if the batch buffer were unbounded.)
>
> >     An internal subxact abort during after-trigger firing (e.g. a PL/pg=
SQL
> >     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_dep=
th 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_dep=
th>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 / VALI=
DATE CONSTRAINT per-row
>   validation. With firing_depth stranded, that validation is wrongly rout=
ed into ri_FastPathBatchAdd.
>   Repro (per-row forced via REFERENCES-only, no SELECT, so RI_Initial_Che=
ck bails):
>     CONTROL: per-row-validated ALTER ADD FK with violating row 99 -> erro=
rs AT the ALTER (correct).
>     BUG: run a caught FK violation first (DO/EXCEPTION), then the same AL=
TER in the same xact ->
>       the ALTER does NOT error, marks convalidated=3Dt, emits "WARNING: r=
esource was not closed:
>       relation pk2 / pk2_pkey / TupleDesc" (a resource-owner leak), and d=
efers the violation to
>       COMMIT. Consequences: resource leak + FK validation deferred past t=
he ALTER (documented
>       invariant broken). Fix: reset firing_depth in AfterTriggerEndSubXac=
t, 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) ab=
out
> 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 deferr=
ed
> >     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 i=
ts
> subxact level, leaving higher-subxact batches untouched.  A deferred trig=
ger
> 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 th=
e
> >     pre-19 SPI-based check.  A fuller fix that preserves batching acros=
s
> >     subtransactions -- whether by tracking the originating subxact of e=
ach
> >     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 f=
ix
> 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