On Wed, Oct 7, 2026 at 8:47 PM Amit Langote <[email protected]> wrote: > > Hi Matheus, > > On Wed, Oct 7, 2026 at 5:33 AM Matheus Alcantara > <[email protected]> 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
