Revert RI fast-path batching from REL_19_STABLE

On Thu, Sep 3, 2026 at 10:36 PM Amit Langote <amitlangote09@gmail.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:

/messages/by-id/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

On Sep 7, 2026, at 21:14, Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Sep 3, 2026 at 10:36 PM Amit Langote <amitlangote09@gmail.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:

/messages/by-id/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>

Hi Amit,

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.

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.

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

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Hi,

On Tue, Sep 8, 2026 at 2:11 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Sep 7, 2026, at 21:14, Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Sep 3, 2026 at 10:36 PM Amit Langote <amitlangote09@gmail.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:

/messages/by-id/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

On Mon, Sep 7, 2026 at 6:45 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Sep 3, 2026 at 10:36 PM Amit Langote <amitlangote09@gmail.com> wrote:

Updated 0001 also pushes/pops the snapshot and adds a
regression test.

One more point about divergence in fast-patch versus SPI path:
ri_FastPathCheck()
{
...
/*
* Advance the command counter so the snapshot sees the effects of prior
* triggers in this statement. Mirrors what the SPI path does in
* ri_PerformCheck().
*/
CommandCounterIncrement();

Related to above, IIUC, ri_PerformCheck() doesn't seem to be calling
CommandCounterIncrement() when called via RI_FKey_check() as it passes
detectNewRows as false for non-partitioned tables. It seems to be
calling somewhere in the SPI code path which makes the above comment
misleading. The other related point is that, in SPI path we call CCI
after acquiring lock on pk_rel which means local invalidations are
processed (CCI->AtCCI_LocalCache) after acquiring the LOCK. I am not
able to see any problem with it but maybe a comment reflecting that
difference is worth it or we can change the order to keep both paths
consistent.

Few other comments on 0001:
========================
*
ri_LockPKTuple()
{
...
+           /*
+            * In READ COMMITTED, FIND_LAST_VERSION should have chased the
+            * chain and returned TM_Ok.  Getting here means something
+            * unexpected -- fall through to error.
+            */
+           elog(ERROR, "unexpected table_tuple_lock status: %u", result);
+           break;
+
+       case TM_SelfModified:
+
+           /*
+            * The current command or a later command in this transaction
+            * modified the PK row.  This shouldn't normally happen during an
+            * FK check (we're not modifying pk_rel), but handle it safely by
+            * treating the tuple as not found.
+            */
+           return false;

Why did the above two cases dealt differently? I mean if something is
not expected in the FK path, silently treating it as not found will
let the caller report an FK violation.

* RI fast path uses the wrong collation for the index probe.
build_index_scankeys() takes the scan key collation from the
referenced index. The SPI check query
resolves equality under the referenced column's collation (See
ri_GenerateQualCollation()'s header comment).

I used Claude to generate the test for the above (note you need to use
--with-icu build option) and verified that it shows the problem:
CREATE COLLATION fkfp_ci (provider = icu, locale =
'@colStrength=secondary', deterministic = false);
CREATE TABLE fkfp_pk (x text COLLATE fkfp_ci);
CREATE UNIQUE INDEX fkfp_pk_x_c ON fkfp_pk (x COLLATE "C");
INSERT INTO fkfp_pk VALUES ('ABC');

-- a non-superuser with REFERENCES but deliberately NOT SELECT on fkfp_pk,
-- so RI_Initial_Check() returns false and validation falls back to the
-- per-row RI_FKey_check_ins() loop
CREATE ROLE fkfp_r;
GRANT CREATE ON SCHEMA public TO fkfp_r;
GRANT REFERENCES ON fkfp_pk TO fkfp_r;

CREATE TABLE fkfp_fk_val (x text COLLATE fkfp_ci);
INSERT INTO fkfp_fk_val VALUES ('abc');
ALTER TABLE fkfp_fk_val OWNER TO fkfp_r;

SET ROLE fkfp_r;
-- expect: has_table_privilege = f, confirming the bulk path is unavailable
SELECT has_table_privilege('fkfp_pk', 'SELECT') AS can_select,
has_table_privilege('fkfp_pk', 'REFERENCES') AS can_reference;

-- this is the per-row fast path.
ALTER TABLE fkfp_fk_val ADD FOREIGN KEY (x) REFERENCES fkfp_pk (x);

On HEAD, above statement gives following ERROR:
ERROR: insert or update on table "fkfp_fk_val" violates foreign key
constraint "fkfp_fk_val_x_fkey"
DETAIL: Key (x)=(abc) is not present in table "fkfp_pk".

If I change code to skip fast-path and use SPI then the ALTER
statement is successful.

--
With Regards,
Amit Kapila.

Hi Amit,

On Tue, Sep 8, 2026 at 6:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Sep 7, 2026 at 6:45 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Sep 3, 2026 at 10:36 PM Amit Langote <amitlangote09@gmail.com> wrote:

Updated 0001 also pushes/pops the snapshot and adds a
regression test.

Thanks for taking a look.

One more point about divergence in fast-patch versus SPI path:
ri_FastPathCheck()
{
...
/*
* Advance the command counter so the snapshot sees the effects of prior
* triggers in this statement. Mirrors what the SPI path does in
* ri_PerformCheck().
*/
CommandCounterIncrement();

Related to above, IIUC, ri_PerformCheck() doesn't seem to be calling
CommandCounterIncrement() when called via RI_FKey_check() as it passes
detectNewRows as false for non-partitioned tables. It seems to be
calling somewhere in the SPI code path which makes the above comment
misleading. The other related point is that, in SPI path we call CCI
after acquiring lock on pk_rel which means local invalidations are
processed (CCI->AtCCI_LocalCache) after acquiring the LOCK. I am not
able to see any problem with it but maybe a comment reflecting that
difference is worth it or we can change the order to keep both paths
consistent.

You're right. The comment uses "SPI path" too loosely. How about:

/*
* Advance the command counter so the check sees the effects of prior
* triggers in this statement, as SPI does when executing the query
* issued by ri_PerformCheck().
*/

I'll also move the CCI after acquiring the lock on pk_rel and before
reloading the constraint information, so local invalidations are
processed under the lock.

Few other comments on 0001:
========================
*
ri_LockPKTuple()
{
...
+           /*
+            * In READ COMMITTED, FIND_LAST_VERSION should have chased the
+            * chain and returned TM_Ok.  Getting here means something
+            * unexpected -- fall through to error.
+            */
+           elog(ERROR, "unexpected table_tuple_lock status: %u", result);
+           break;
+
+       case TM_SelfModified:
+
+           /*
+            * The current command or a later command in this transaction
+            * modified the PK row.  This shouldn't normally happen during an
+            * FK check (we're not modifying pk_rel), but handle it safely by
+            * treating the tuple as not found.
+            */
+           return false;

Why did the above two cases dealt differently? I mean if something is
not expected in the FK path, silently treating it as not found will
let the caller report an FK violation.

I followed ExecLockRows(), but the comments don't explain the
distinction well. With FIND_LAST_VERSION in READ COMMITTED, TM_Updated
is unexpected. TM_SelfModified, however, is handled by skipping the
tuple in ExecLockRows(), which is what returning false here is
intended to match. If no match remains, the SPI path would likewise
report an FK violation.

I'll update the TM_SelfModified comment to explain that
correspondence, and remove “fall through to error” from the TM_Updated
comment since the code calls elog(ERROR) directly.

* RI fast path uses the wrong collation for the index probe.
build_index_scankeys() takes the scan key collation from the
referenced index. The SPI check query
resolves equality under the referenced column's collation (See
ri_GenerateQualCollation()'s header comment).

I used Claude to generate the test for the above (note you need to use
--with-icu build option) and verified that it shows the problem:
CREATE COLLATION fkfp_ci (provider = icu, locale =
'@colStrength=secondary', deterministic = false);
CREATE TABLE fkfp_pk (x text COLLATE fkfp_ci);
CREATE UNIQUE INDEX fkfp_pk_x_c ON fkfp_pk (x COLLATE "C");
INSERT INTO fkfp_pk VALUES ('ABC');

-- a non-superuser with REFERENCES but deliberately NOT SELECT on fkfp_pk,
-- so RI_Initial_Check() returns false and validation falls back to the
-- per-row RI_FKey_check_ins() loop
CREATE ROLE fkfp_r;
GRANT CREATE ON SCHEMA public TO fkfp_r;
GRANT REFERENCES ON fkfp_pk TO fkfp_r;

CREATE TABLE fkfp_fk_val (x text COLLATE fkfp_ci);
INSERT INTO fkfp_fk_val VALUES ('abc');
ALTER TABLE fkfp_fk_val OWNER TO fkfp_r;

SET ROLE fkfp_r;
-- expect: has_table_privilege = f, confirming the bulk path is unavailable
SELECT has_table_privilege('fkfp_pk', 'SELECT') AS can_select,
has_table_privilege('fkfp_pk', 'REFERENCES') AS can_reference;

-- this is the per-row fast path.
ALTER TABLE fkfp_fk_val ADD FOREIGN KEY (x) REFERENCES fkfp_pk (x);

On HEAD, above statement gives following ERROR:
ERROR: insert or update on table "fkfp_fk_val" violates foreign key
constraint "fkfp_fk_val_x_fkey"
DETAIL: Key (x)=(abc) is not present in table "fkfp_pk".

If I change code to skip fast-path and use SPI then the ALTER
statement is successful.

Thanks for the reproducer. I think we should fall back to SPI when the
referenced index's collation differs from the referenced column's
collation. The planner won't use that equality as an index condition
when the collations don't match. Simply changing the scan key's
collation wouldn't be correct either, since the index was built using
a different ordering. I've added an open item for this.

I'll post the updated patch set tomorrow.

--
Thanks, Amit Langote

On Thu, Sep 3, 2026 at 9:36 AM Amit Langote <amitlangote09@gmail.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?

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.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Sep 8, 2026 at 10:01 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Tue, Sep 8, 2026 at 6:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Sep 7, 2026 at 6:45 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Sep 3, 2026 at 10:36 PM Amit Langote <amitlangote09@gmail.com> wrote:

Updated 0001 also pushes/pops the snapshot and adds a
regression test.

Thanks for taking a look.

One more point about divergence in fast-patch versus SPI path:
ri_FastPathCheck()
{
...
/*
* Advance the command counter so the snapshot sees the effects of prior
* triggers in this statement. Mirrors what the SPI path does in
* ri_PerformCheck().
*/
CommandCounterIncrement();

Related to above, IIUC, ri_PerformCheck() doesn't seem to be calling
CommandCounterIncrement() when called via RI_FKey_check() as it passes
detectNewRows as false for non-partitioned tables. It seems to be
calling somewhere in the SPI code path which makes the above comment
misleading. The other related point is that, in SPI path we call CCI
after acquiring lock on pk_rel which means local invalidations are
processed (CCI->AtCCI_LocalCache) after acquiring the LOCK. I am not
able to see any problem with it but maybe a comment reflecting that
difference is worth it or we can change the order to keep both paths
consistent.

You're right. The comment uses "SPI path" too loosely. How about:

/*
* Advance the command counter so the check sees the effects of prior
* triggers in this statement, as SPI does when executing the query
* issued by ri_PerformCheck().
*/

I'll also move the CCI after acquiring the lock on pk_rel and before
reloading the constraint information, so local invalidations are
processed under the lock.

Done in the updated 0001. I folded the CCI change into it because it
belongs with the snapshot-ordering change.

Few other comments on 0001:
========================
*
ri_LockPKTuple()
{
...
+           /*
+            * In READ COMMITTED, FIND_LAST_VERSION should have chased the
+            * chain and returned TM_Ok.  Getting here means something
+            * unexpected -- fall through to error.
+            */
+           elog(ERROR, "unexpected table_tuple_lock status: %u", result);
+           break;
+
+       case TM_SelfModified:
+
+           /*
+            * The current command or a later command in this transaction
+            * modified the PK row.  This shouldn't normally happen during an
+            * FK check (we're not modifying pk_rel), but handle it safely by
+            * treating the tuple as not found.
+            */
+           return false;

Why did the above two cases dealt differently? I mean if something is
not expected in the FK path, silently treating it as not found will
let the caller report an FK violation.

I followed ExecLockRows(), but the comments don't explain the
distinction well. With FIND_LAST_VERSION in READ COMMITTED, TM_Updated
is unexpected. TM_SelfModified, however, is handled by skipping the
tuple in ExecLockRows(), which is what returning false here is
intended to match. If no match remains, the SPI path would likewise
report an FK violation.

I'll update the TM_SelfModified comment to explain that
correspondence, and remove “fall through to error” from the TM_Updated
comment since the code calls elog(ERROR) directly.

Done in the attached 0003.

* RI fast path uses the wrong collation for the index probe.
build_index_scankeys() takes the scan key collation from the
referenced index. The SPI check query
resolves equality under the referenced column's collation (See
ri_GenerateQualCollation()'s header comment).

I used Claude to generate the test for the above (note you need to use
--with-icu build option) and verified that it shows the problem:
CREATE COLLATION fkfp_ci (provider = icu, locale =
'@colStrength=secondary', deterministic = false);
CREATE TABLE fkfp_pk (x text COLLATE fkfp_ci);
CREATE UNIQUE INDEX fkfp_pk_x_c ON fkfp_pk (x COLLATE "C");
INSERT INTO fkfp_pk VALUES ('ABC');

-- a non-superuser with REFERENCES but deliberately NOT SELECT on fkfp_pk,
-- so RI_Initial_Check() returns false and validation falls back to the
-- per-row RI_FKey_check_ins() loop
CREATE ROLE fkfp_r;
GRANT CREATE ON SCHEMA public TO fkfp_r;
GRANT REFERENCES ON fkfp_pk TO fkfp_r;

CREATE TABLE fkfp_fk_val (x text COLLATE fkfp_ci);
INSERT INTO fkfp_fk_val VALUES ('abc');
ALTER TABLE fkfp_fk_val OWNER TO fkfp_r;

SET ROLE fkfp_r;
-- expect: has_table_privilege = f, confirming the bulk path is unavailable
SELECT has_table_privilege('fkfp_pk', 'SELECT') AS can_select,
has_table_privilege('fkfp_pk', 'REFERENCES') AS can_reference;

-- this is the per-row fast path.
ALTER TABLE fkfp_fk_val ADD FOREIGN KEY (x) REFERENCES fkfp_pk (x);

On HEAD, above statement gives following ERROR:
ERROR: insert or update on table "fkfp_fk_val" violates foreign key
constraint "fkfp_fk_val_x_fkey"
DETAIL: Key (x)=(abc) is not present in table "fkfp_pk".

If I change code to skip fast-path and use SPI then the ALTER
statement is successful.

Thanks for the reproducer. I think we should fall back to SPI when the
referenced index's collation differs from the referenced column's
collation. The planner won't use that equality as an index condition
when the collations don't match. Simply changing the scan key's
collation wouldn't be correct either, since the index was built using
a different ordering. I've added an open item for this.

Done in the attached 0002. The index-dependent eligibility checks are
now cached lazily, after locking the referenced table, reloading the
constraint information, and opening the index. Looking up index
properties using conindid read before that lock could race with
REINDEX CONCURRENTLY replacing and dropping the index. I moved the
existing btree eligibility check here too.

This follows the same general approach as fast-path metadata
population. Both acceptance and rejection are cached until the
constraint information is reloaded, so subsequent rows do not repeat
the collation comparisons. Both the per-row and batched paths can fall
back to SPI before probing or buffering the row. Covering the batched
path is necessary while batching remains in master. I'll follow up
separately with my thoughts on whether to retain batching there.

Batching removal patch is now 0004, the last in the series.

--
Thanks, Amit Langote

On Wed, Sep 9, 2026 at 2:59 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Sep 3, 2026 at 9:36 AM Amit Langote <amitlangote09@gmail.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

On Wed, Sep 9, 2026 at 8:26 AM Amit Langote <amitlangote09@gmail.com> wrote:

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.

I think a good start would be to revert the batching ASAP so that we
can consider the state after independently.

- Melanie

On Wed, Sep 9, 2026 at 11:41 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:

On Wed, Sep 9, 2026 at 8:26 AM Amit Langote <amitlangote09@gmail.com> wrote:

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.

I think a good start would be to revert the batching ASAP so that we
can consider the state after independently.

+1.

--
Robert Haas
EDB: http://www.enterprisedb.com