On 07/10/26 08:47, Amit Langote wrote:
> 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.
>
Thanks for the new version. I've tested v2 and it looks good to me.
> I'll push 0001 and 0002 tomorrow barring objections.
>
+1 from me.
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
