Re: PG19: two RI fast-path issues found while testing the batching revert

Hi Matheus,

On Wed, Oct 7, 2026 at 5:33 AM Matheus Alcantara
<matheusssilv97(at)gmail(dot)com> wrote:
>
> On Mon Oct 5, 2026 at 10:18 AM -03, Amit Langote wrote:
> > So I propose fixing the fast-path selection criteria to avoid it when
> > such a cast is necessary, as done in the attached 0001. 0002 is a
> > patch to drop the code added in the fast-path commit to facilitate
> > this cast handling as it is now dead code. 0001+0002 apply on top of
> > the other fix I posted at [1], which I plan to commit tomorrow.
> >
> > [ ... ]
> >
> > I would like to commit 0001 and 0002 before RC1 next week, since
> > without them a foreign key using such a cast is checked differently
> > depending on whether the fast path or SPI handles it. I can wait for
> > others to opine on whether 0003 is warranted (I haven't checked the
> > back-patching pain).
> >
>
> Hi, thanks for the patches! I've reviewed and tested 0001 and 0002 and I
> dind't find any issues.

Thanks for the review.

> I also noticed that a user-defined WITH INOUT AS IMPLICIT cast now works
> through SPI. Before 0001 it failed with "no conversion function" from
> ri_HashCompareOp(), so perhaps it's worth adding a test for it.

+1, added in v2 (0001's ri_cast tests).

> The pg_cast invalidation also seems to work fine. I tested it with a
> session that had cached the fast path for a FK using a binary cast,
> while another session replaced the cast with a function returning NULL,
> and the next insert on the first session went through SPI and reported
> the FK violation. The same works when the cast is replaced within the
> same transaction.
>
> I have just a single comment: I'm wondering if the tests that 0001
> rewords (fk_defer_main, the fkint invalidation test, fp_reentry and the
> ri_snapshot STABLE test) now all seems to go through SPI, so they no
> longer test the fast path issues they were written for, what do you
> think?

Good point. fk_defer_main and fp_reentry were written for batching,
which is gone, so there's no fast-path code left for them to test;
I've kept them as they are, now going through SPI. The fkint
invalidation test and the ri_snapshot test cover the fast path's
handling of its cached metadata being invalidated mid-check and its
pushing of the check's snapshot as the active one, so in v2 they use
the equality function of a custom opclass instead of a cast, the only
user code a fast-path check still runs, which also makes the pk_fast
name accurate again.

> Some minor comments:
>
> - "getBaseType(fk_type) != righttype &&" on the new check seems
> redundant since IsBinaryCoercible() already handles the same type and
> domain cases.
>
> - The "typedef struct RI_CompareHashEntry RI_CompareHashEntry;" added
> by 2da86c1ef9 before FastPathMeta is not needed anymore.
>
> - The ri_check_fastpath_index() header comment only mentions index
> properties, but the new check depends on the FK table. The comment
> before the fast path call on RI_FKey_check() also only mentions
> non-partitioned and non-temporal FKs.
>
> - "pk_fast" on the ri_snapshot test now also goes through SPI, so the
> name is a may be misleading.

All addressed in v2.

> I'll review 0003 next.

In no hurry, but thanks. I've added a test case with a WITH INOUT
cast to 0003 as well, which covers UPDATEs of the referencing row:
without 0003, they fail with "no conversion function" even when the
key doesn't change.

I'll push 0001 and 0002 tomorrow barring objections.

--
Thanks, Amit Langote