Re: Revert RI fast-path batching from REL_19_STABLE

Hi,

On Tue, Sep 8, 2026 at 2:11 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> > On Sep 7, 2026, at 21:14, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> >
> > On Thu, Sep 3, 2026 at 10:36 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> >> Attached is a two-patch series to remove batching from the RI fast
> >> path in REL_19_STABLE, while retaining the per-row fast path. Batching
> >> accumulates rows from successive foreign-key trigger invocations and
> >> checks them together against the referenced index when the batch fills
> >> or the trigger-firing cycle ends, instead of checking each row during
> >> its trigger invocation. I plan to leave the batched implementation
> >> unchanged in master for v20 development.
> >>
> >> Given the concerns raised about shipping this code in v19 [1], I
> >> reviewed the fixes made since feature freeze. The batching fixes are
> >> considerably more concerning than those to the underlying per-row fast
> >> path, because they have repeatedly had to address how a live batch of
> >> pending checks interacts with trigger-firing cycles, subtransactions,
> >> deferred constraints, and SET CONSTRAINTS, rather than merely
> >> correcting isolated implementation errors. Missing one such
> >> interaction can leave a foreign key check buffered and never
> >> performed, allowing a violating row to be committed without an error.
> >> That fix history, together with the two still-open batching-related
> >> items, has persuaded me that the concerns about shipping the batching
> >> code in v19 are justified.
> >>
> >> 0001 is the same patch I posted separately at:
> >>
> >> https://postgr.es/m/CA+HiwqEhm+_=bs=2wavAJz-UqC+1KebD31++mapJQQGweE8iQQ@mail.gmail.com
> >>
> >> It fixes a snapshot-ordering bug in the per-row path that must be
> >> addressed before 0002 removes batching and makes ordinary DML use that
> >> path. In READ COMMITTED, ri_FastPathCheck() could take its scan
> >> snapshot before waiting to lock the referenced relation. A referenced
> >> row committed during that wait would not be visible to the old
> >> snapshot, causing a false foreign key violation. Batching currently
> >> masks this problem because it opens and caches the relations before
> >> taking the snapshot used for the batch. The SPI path likewise acquires
> >> the referenced-relation lock before selecting the snapshot used for
> >> the check.
> >
> > I asked GPT-6 whether I had missed anything else about snapshot
> > handling in the per-row fast path compared with SPI. It pointed out
> > that the fast path did not make the scan snapshot active. SQL executed
> > inside a STABLE user-defined cast could therefore use an older active
> > snapshot and miss changes visible to the index scan. I confirmed this
> > with a reproducer where the fast path rejected a valid foreign key but
> > SPI accepted it. Updated 0001 also pushes/pops the snapshot and adds a
> > regression test.
> >
> >> 0002 removes the batching layer, the per-batch RI cache, and the
> >> after-trigger callback and subtransaction machinery added to manage
> >> their lifetime. It restores the affected after-trigger code to its
> >> pre-batching form.
> >>
> >> 0002 removes tests that exercise only the batching implementation and
> >> its callback and cache lifetime machinery. It retains, and where
> >> necessary adapts, tests that continue to exercise the underlying RI
> >> cases through the per-row path, including ALTER TABLE validation,
> >> scan-key construction, deferred checks, re-entrancy, and metadata
> >> invalidation. This preserves useful regression coverage in v19 and
> >> minimizes unnecessary divergence in test coverage, simplifying future
> >> backpatching of test cases. When I first tested the batching removal,
> >> the only failure among the retained tests was the injection-point test
> >> that exposed the snapshot-ordering bug fixed by 0001.
> >
> > The cross-type recheck isolation test also exercises code retained in
> > the per-row fast path, so the updated 0002 restores it.
> >
> >> The per-row path has also required fixes since feature freeze,
> >> including the snapshot-ordering fix in 0001. So far, however, its fix
> >> history has not shown the same pattern of complexity as the fixes
> >> needed to manage the lifetime of batched checks. On that basis, I
> >> propose retaining the per-row path in v19 while removing the batching
> >> layer.
> >>
> >> Thoughts on removing batching while retaining the per-row fast path in
> >> v19, and on retaining batching in its current state in master for v20
> >> development, would be welcome.
> >
> > I'd like to push 0001 soon. I'll allow a few more days for comments
> > before committing 0002.
> >
> > --
> > Thanks, Amit Langote
> > <v2-0001-Take-RI-fast-path-snapshot-after-locking-referenc.patch><v2-0002-Remove-batching-from-RI-fast-path-checks.patch>
>
> I applied v2 to REL_19_STABLE and ran tests. The test failed because foreign_key.out contained 6 extra empty lines. After deleting them, the test passed.

Oops, thanks for catching that.

> Other than that, the patch looks good to me. For your convenience, I fixed the test in v3.
>
> * 0001 - just deleted 6 empty lines from foreign_key.out
> * 0002 - unchanged from v2.

Thanks.

In the attached v4, I've added an injection-point test to 0001 that
exercises the snapshot-ordering fix through per-row validation, even
with batching enabled. The existing REINDEX test exposed the bug after
batching was removed, but uses the batch path while batching remains
enabled. I've also updated stale comments in that test in 0002.

> By the way, I failed to apply v2 to master due to conflicts on release-19.sgml.

Under the current proposal, only 0001 would go into master; 0002 would
apply only to REL_19_STABLE. So the release-19.sgml conflict should
not matter for the proposed application.

--
Thanks, Amit Langote