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

On Wed, Oct 7, 2026 at 8:47 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> 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.

Forgot the Backpatch-through: 19 tag, which I've added in my local tree.

--
Thanks, Amit Langote