Re: Revert RI fast-path batching from REL_19_STABLE

On Wed, Sep 9, 2026 at 2:59 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Sep 3, 2026 at 9:36 AM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > 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.
>
> Does this approach have any significant downsides that we should be
> thinking about? For example, are we relying on the batching to buy
> back slowdowns that the per-row fast path might otherwise introduce in
> some cases? Or is this just a case of the per-row fast path is an
> optimization and then the batching is a further optimization, so if
> the second one is buggy we can take it out without causing any
> problems for the first one?

Yes, it's the latter. Batching is a further optimization on top of the
per-row fast path. It wasn't introduced to compensate for a known
slowdown caused by the per-row path. The per-row path avoids SPI’s
plan-cache and executor overhead while performing each check
synchronously.

The proposed removal also gives up reuse of open relations and tuple
slots. The original series had a separate patch that cached open
relations and tuple slots while keeping checks synchronous. When
adding batching, I decided to use the same cache entry to track both
those resources and outstanding rows. The proposed v19 patch removes
both.

Note that the per-row path retained in v19 still caches lookup and
comparison metadata attached to RI_ConstraintInfo, with its own
invalidation and lifetime handling. The additional relation and slot
cache holds open relation references and reusable tuple slots. These
are normally released at the end of each trigger-firing cycle, with
transaction and subtransaction abort cleanup covering interrupted
teardown.

> I do think I generally agree that the batching stuff feels much
> riskier than the per-row fast path stuff. I think batching in this
> context intrinsically requires changing the timing of trigger firing,
> and that is risky because (1) important things may be different at the
> two timings, such as the choice of snapshot, and (2) the change in
> timing may be user-perceptible in some way. However, I'm not entirely
> sure whether (1) batching is in good enough shape that it makes sense
> to keep it in v20 or (2) the per-row fast-path is in good enough shape
> to stay in v19. In other words, I think we should do at least as much
> as what you're proposing here, but possibly more. However, I'm not
> very sure what the right answer is at this point.

Your concerns about delaying checks also make me reconsider retaining
batching in master. How about this:

* For v19, retain the per-row fast path and remove batching and the
relation and slot cache, as currently proposed. I don't think there is
time left to develop an intermediate resource-caching version. The
remaining work should be fixing bugs in what we retain.

* For master, remove batched probing while retaining resource caching
for synchronous checks. Initially, keep the current resource lifetime:
a multirow INSERT or UPDATE would reuse relations and slots across its
FK trigger calls, then release them after the statement's queued AFTER
triggers have finished firing. The same applies when deferred triggers
are fired by SET CONSTRAINTS ... IMMEDIATE or at commit.

Concretely, RI_FastPathEntry currently holds both open relations and
reusable slots, and the pending-row buffer and state used for batch
flushing. I would remove the pending-row storage and batch-flushing
machinery, and have each trigger invocation complete its check
synchronously using cached resources. Reusing slots would still
require protection against re-entrant checks.

The cleanup callback invoked after the queued AFTER triggers have
finished firing would remain. It would only drop the cached slots and
close the relation references, retaining the locks until transaction
end. On transaction or subtransaction abort, ResourceOwner would
continue to release tracked resources, and RI cleanup would discard
the affected cache entries. Keeping this cleanup structure would
preserve the current resource lifetime while removing the delayed
checks.

Any reintroduction of SK_SEARCHARRAY batching would require a separate
proposal addressing user-visible changes from delaying checks,
including their ordering relative to other AFTER ROW triggers. As
Tomas Vondra also pointed out off-list, those triggers can change data
read by an FK check or execute SQL that fires further triggers. Taking
the right snapshot and ensuring that every batch gets flushed do not
by themselves preserve those interactions.

This separation would also make it easier to measure the performance
benefit of resource reuse independently from batched probing, and
assess each against its own complexity.

That still leaves your concern about whether the per-row path is
sufficiently solid for v19. The snapshot fixes and Amit's recent
collation report show that there are differences from SPI that we (I)
missed. The proposed collation fix falls back to SPI when the
referenced index and column collations differ. I still propose
retaining the per-row path, but agree that its readiness needs to be
assessed separately from the decision to remove batching.

--
Thanks, Amit Langote