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

Hi,

On Fri, Aug 7, 2026 at 10:15 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> 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.
> >
<snip>
> >
> > > > > 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.

After considering Peter's report of another bug [1] that is fixed by
0001, I tested both his reproducer and nested firing involving the
same constraint.

The latter exposed a problem in v1: the cache was still keyed only by
constraint OID, so a nested check of the same constraint reused the
outer entry and did not register a callback at the nested query depth.
In v2, the key is now (constraint OID, query depth), giving each
firing level its own entry and callback.

0001 now includes regression tests for both cases. 0002 and 0003 are unchanged.

I would like to commit these sometime this week and would appreciate a review.

[1] https://postgr.es/m/CAH2-Wz%3DD533JbF_ak_Pc8kP0FKse-ju8DnMxtjvY%3D%3DyHsP4xgw%40mail.gmail.com

--
Thanks, Amit Langote