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
