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. -- Thanks, Amit Langote
v2-0002-Remove-RI-fast-path-code-for-calling-cast-functio.patch
Description: Binary data
v2-0003-Don-t-call-cast-functions-when-comparing-FK-keys-.patch
Description: Binary data
v2-0001-Use-SPI-for-RI-checks-whose-FK-values-need-a-cast.patch
Description: Binary data
