Re: Revert RI fast-path batching from REL_19_STABLE

On Tue, Sep 8, 2026 at 10:01 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Tue, Sep 8, 2026 at 6:52 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Mon, Sep 7, 2026 at 6:45 PM 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:
> > >
> > > 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