From 8f5b05ec9ab66a9209f1e952b1db083d5e0cd650 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Wed, 7 Oct 2026 11:07:16 +0900 Subject: [PATCH v2 3/3] Don't call cast functions when comparing FK keys on UPDATE On UPDATE of a referencing row, ri_KeysEqual() compares the old and new key to decide whether the constraint needs checking again, whether or not the UPDATE changes the key. If the key's type needs a cast function to reach the equality operator's input type, it casts both values by calling that function directly, without the input collation, NULL handling and invalidation the cast gets as an expression in the check's query, and it can't apply an array coercion or a coercion via I/O at all. So depending on the cast, even UPDATEs that don't change the key could fail; e.g., with a collation-sensitive cast, "could not determine which collation to use for lower() function", and after the cast's function was dropped and replaced, "cache lookup failed for function". In that case, compare the images of the old and new values instead, as is already done for the referenced side. Identical images are equal under the operator; treating different ones as unequal at worst runs a check that could have been skipped. Backpatch-through: 14 Reviewed-by: Matheus Alcantara Discussion: https://postgr.es/m/CAM527d9BgPjeOOYmbCBTd57R145qHCk-dzw9qNq+nOrDq1j__A@mail.gmail.com --- src/backend/utils/adt/ri_triggers.c | 79 +++++--------- src/test/regress/expected/foreign_key.out | 127 ++++++++++++++++++++++ src/test/regress/sql/foreign_key.sql | 99 +++++++++++++++++ 3 files changed, 251 insertions(+), 54 deletions(-) diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 2e7a815105f..d2e307dcce4 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -209,8 +209,10 @@ typedef struct RI_CompareHashEntry { RI_CompareKey key; bool valid; /* successfully initialized? */ + bool needs_coercion; /* values not binary-coercible to op input? */ + int16 typlen; /* typlen and typbyval of typeid, to compare */ + bool typbyval; /* images when needs_coercion is set */ FmgrInfo eq_opr_finfo; /* call info for equality fn */ - FmgrInfo cast_func_finfo; /* in case we must coerce input */ } RI_CompareHashEntry; /* @@ -3803,7 +3805,9 @@ ri_KeysEqual(Relation rel, TupleTableSlot *oldslot, TupleTableSlot *newslot, * Call the appropriate comparison operator for two values. * Normally this is equality, but for the PERIOD part of foreign keys * it is ContainedBy, so the order of lhs vs rhs is significant. - * See below for how the collation is applied. + * See below for how the collation is applied. If the values are not + * binary-coercible to the operator's input type, their images are compared + * instead. * * NB: we have already checked that neither value is null. */ @@ -3813,18 +3817,16 @@ ri_CompareWithCast(Oid eq_opr, Oid typeid, Oid collid, { RI_CompareHashEntry *entry = ri_HashCompareOp(eq_opr, typeid); - /* Do we need to cast the values? */ - if (OidIsValid(entry->cast_func_finfo.fn_oid)) - { - lhs = FunctionCall3(&entry->cast_func_finfo, - lhs, - Int32GetDatum(-1), /* typmod */ - BoolGetDatum(false)); /* implicit coercion */ - rhs = FunctionCall3(&entry->cast_func_finfo, - rhs, - Int32GetDatum(-1), /* typmod */ - BoolGetDatum(false)); /* implicit coercion */ - } + /* + * If the values are not binary-coercible to the operator's input type, + * compare their images instead of coercing them. Coercing them here + * would mean reproducing what the check's query does when it applies the + * coercion as an expression, and isn't needed: identical images satisfy + * the operator, and treating different images as unequal only means the + * caller runs a check it might have skipped. + */ + if (entry->needs_coercion) + return datum_image_eq(lhs, rhs, entry->typbyval, entry->typlen); /* * Apply the comparison operator. @@ -3886,55 +3888,24 @@ ri_HashCompareOp(Oid eq_opr, Oid typeid) if (!entry->valid) { Oid lefttype, - righttype, - castfunc; - CoercionPathType pathtype; + righttype; /* We always need to know how to call the equality operator */ fmgr_info_cxt(get_opcode(eq_opr), &entry->eq_opr_finfo, TopMemoryContext); /* - * If we chose to use a cast from FK to PK type, we may have to apply - * the cast function to get to the operator's input type. - * - * XXX eventually it would be good to support array-coercion cases - * here and in ri_CompareWithCast(). At the moment there is no point - * because cases involving nonidentical array types will be rejected - * at constraint creation time. - * - * XXX perhaps also consider supporting CoerceViaIO? No need at the - * moment since that will never be generated for implicit coercions. + * If we chose to use a cast from FK to PK type, the values may need a + * coercion to get to the operator's input type: a cast function, an + * array coercion, or a coercion via I/O. ri_CompareWithCast() then + * compares their images instead; see there. IsBinaryCoercible() also + * accepts a polymorphic input type such as ANYARRAY or ANYENUM, and + * RECORD for a composite type, which need no coercion either. */ op_input_types(eq_opr, &lefttype, &righttype); Assert(lefttype == righttype); - if (typeid == lefttype) - castfunc = InvalidOid; /* simplest case */ - else - { - pathtype = find_coercion_pathway(lefttype, typeid, - COERCION_IMPLICIT, - &castfunc); - if (pathtype != COERCION_PATH_FUNC && - pathtype != COERCION_PATH_RELABELTYPE) - { - /* - * The declared input type of the eq_opr might be a - * polymorphic type such as ANYARRAY or ANYENUM, or other - * special cases such as RECORD; find_coercion_pathway - * currently doesn't subsume these special cases. - */ - if (!IsBinaryCoercible(typeid, lefttype)) - elog(ERROR, "no conversion function from %s to %s", - format_type_be(typeid), - format_type_be(lefttype)); - } - } - if (OidIsValid(castfunc)) - fmgr_info_cxt(castfunc, &entry->cast_func_finfo, - TopMemoryContext); - else - entry->cast_func_finfo.fn_oid = InvalidOid; + entry->needs_coercion = !IsBinaryCoercible(typeid, lefttype); + get_typlenbyval(typeid, &entry->typlen, &entry->typbyval); entry->valid = true; } diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index 4d7cfc31c60..6b7157f4849 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -4206,3 +4206,130 @@ ERROR: insert or update on table "ri_fk_io" violates foreign key constraint "ri DETAIL: Key (a)=(2) is not present in table "ri_pk". ROLLBACK TO SAVEPOINT s; ROLLBACK; +-- +-- UPDATEs of referencing rows whose key type needs a coercion to reach the +-- equality operator's input type. ri_KeysEqual() decides whether the key +-- changed without applying the coercion; a check it can't skip applies it in +-- its query. Each statement runs in its own transaction, because +-- ri_KeysEqual() is not consulted for rows inserted by the same transaction. +-- +CREATE SCHEMA ri_upd_cast; +SET search_path = ri_upd_cast; +CREATE TABLE pk (id int4 PRIMARY KEY); +INSERT INTO pk VALUES (1); +-- A cast that returns NULL for one value, later replaced and dropped. +CREATE TYPE word AS ENUM ('one', 'none'); +CREATE FUNCTION word_to_int4(word) RETURNS int4 + LANGUAGE sql IMMUTABLE AS $$ SELECT CASE WHEN $1::text = 'one' THEN 1 END $$; +CREATE CAST (word AS int4) WITH FUNCTION word_to_int4(word) AS IMPLICIT; +CREATE TABLE fk_word (a word REFERENCES pk, n int); +INSERT INTO fk_word VALUES ('one', 0); +UPDATE fk_word SET n = n + 1; +UPDATE fk_word SET a = 'none'; -- fails, as a violation +ERROR: insert or update on table "fk_word" violates foreign key constraint "fk_word_a_fkey" +DETAIL: Key (a)=(none) is not present in table "pk". +CREATE FUNCTION word_to_int4_v2(word) RETURNS int4 + LANGUAGE sql IMMUTABLE AS $$ SELECT 1 $$; +DROP CAST (word AS int4); +CREATE CAST (word AS int4) WITH FUNCTION word_to_int4_v2(word) AS IMPLICIT; +DROP FUNCTION word_to_int4(word); +UPDATE fk_word SET n = n + 1; -- must not call the dropped function +UPDATE fk_word SET a = 'none'; -- succeeds through the new cast +SELECT * FROM fk_word; + a | n +------+--- + none | 2 +(1 row) + +-- A collation-sensitive cast from a collatable type. +CREATE TYPE ctext; +CREATE FUNCTION ctext_in(cstring) RETURNS ctext + AS 'textin' LANGUAGE internal IMMUTABLE STRICT; +NOTICE: return type ctext is only a shell +CREATE FUNCTION ctext_out(ctext) RETURNS cstring + AS 'textout' LANGUAGE internal IMMUTABLE STRICT; +NOTICE: argument type ctext is only a shell +LINE 1: CREATE FUNCTION ctext_out(ctext) RETURNS cstring + ^ +CREATE TYPE ctext (INPUT = ctext_in, OUTPUT = ctext_out, + LIKE = text, COLLATABLE = true); +CREATE FUNCTION ctext_lower(ctext) RETURNS text + AS 'lower' LANGUAGE internal IMMUTABLE STRICT; +CREATE CAST (ctext AS text) WITH FUNCTION ctext_lower(ctext) AS IMPLICIT; +CREATE TABLE pk_text (k text PRIMARY KEY); +INSERT INTO pk_text VALUES ('abc'); +CREATE TABLE fk_text (a ctext REFERENCES pk_text, n int); +INSERT INTO fk_text VALUES ('ABC', 0); +UPDATE fk_text SET n = n + 1; +SELECT * FROM fk_text; + a | n +-----+--- + ABC | 1 +(1 row) + +-- An array coercion (int2[] to int4[], allowed with an opclass declared for +-- int4[]), which has no single cast function. +CREATE FUNCTION i4a_lt(int4[], int4[]) RETURNS bool + AS 'array_lt' LANGUAGE internal IMMUTABLE STRICT; +CREATE FUNCTION i4a_le(int4[], int4[]) RETURNS bool + AS 'array_le' LANGUAGE internal IMMUTABLE STRICT; +CREATE FUNCTION i4a_eq(int4[], int4[]) RETURNS bool + AS 'array_eq' LANGUAGE internal IMMUTABLE STRICT; +CREATE FUNCTION i4a_ge(int4[], int4[]) RETURNS bool + AS 'array_ge' LANGUAGE internal IMMUTABLE STRICT; +CREATE FUNCTION i4a_gt(int4[], int4[]) RETURNS bool + AS 'array_gt' LANGUAGE internal IMMUTABLE STRICT; +CREATE FUNCTION i4a_cmp(int4[], int4[]) RETURNS int4 + AS 'btarraycmp' LANGUAGE internal IMMUTABLE STRICT; +CREATE OPERATOR ri_upd_cast.<<< (LEFTARG = int4[], RIGHTARG = int4[], FUNCTION = i4a_lt); +CREATE OPERATOR ri_upd_cast.<<= (LEFTARG = int4[], RIGHTARG = int4[], FUNCTION = i4a_le); +CREATE OPERATOR ri_upd_cast.=== (LEFTARG = int4[], RIGHTARG = int4[], FUNCTION = i4a_eq); +CREATE OPERATOR ri_upd_cast.>>= (LEFTARG = int4[], RIGHTARG = int4[], FUNCTION = i4a_ge); +CREATE OPERATOR ri_upd_cast.>>> (LEFTARG = int4[], RIGHTARG = int4[], FUNCTION = i4a_gt); +CREATE OPERATOR CLASS i4a_ops FOR TYPE int4[] USING btree AS + OPERATOR 1 ri_upd_cast.<<<, OPERATOR 2 ri_upd_cast.<<=, + OPERATOR 3 ri_upd_cast.===, OPERATOR 4 ri_upd_cast.>>=, + OPERATOR 5 ri_upd_cast.>>>, FUNCTION 1 i4a_cmp(int4[], int4[]); +CREATE TABLE pk_arr (k int4[]); +CREATE UNIQUE INDEX ON pk_arr (k i4a_ops); +INSERT INTO pk_arr VALUES ('{1,2}'); +CREATE TABLE fk_arr (a int2[] REFERENCES pk_arr (k), n int); +INSERT INTO fk_arr VALUES ('{1,2}', 0); +UPDATE fk_arr SET n = n + 1; +UPDATE fk_arr SET a = '{3}'; -- fails, as a violation +ERROR: insert or update on table "fk_arr" violates foreign key constraint "fk_arr_a_fkey" +DETAIL: Key (a)=({3}) is not present in table "pk_arr". +SELECT * FROM fk_arr; + a | n +-------+--- + {1,2} | 1 +(1 row) + +-- A coercion via I/O, which has no cast function either. +CREATE TYPE ioint; +CREATE FUNCTION ioint_in(cstring) RETURNS ioint + AS 'int4in' LANGUAGE internal IMMUTABLE STRICT; +NOTICE: return type ioint is only a shell +CREATE FUNCTION ioint_out(ioint) RETURNS cstring + AS 'int4out' LANGUAGE internal IMMUTABLE STRICT; +NOTICE: argument type ioint is only a shell +LINE 1: CREATE FUNCTION ioint_out(ioint) RETURNS cstring + ^ +CREATE TYPE ioint (INPUT = ioint_in, OUTPUT = ioint_out, LIKE = int4); +CREATE CAST (ioint AS int4) WITH INOUT AS IMPLICIT; +CREATE TABLE fk_io (a ioint REFERENCES pk, n int); +INSERT INTO fk_io VALUES ('1', 0); +UPDATE fk_io SET n = n + 1; +UPDATE fk_io SET a = '2'; -- fails, as a violation +ERROR: insert or update on table "fk_io" violates foreign key constraint "fk_io_a_fkey" +DETAIL: Key (a)=(2) is not present in table "pk". +SELECT * FROM fk_io; + a | n +---+--- + 1 | 1 +(1 row) + +RESET search_path; +SET client_min_messages = warning; +DROP SCHEMA ri_upd_cast CASCADE; +RESET client_min_messages; diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index 4104e13a04b..5934c9b2031 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -3145,3 +3145,102 @@ SAVEPOINT s; INSERT INTO ri_fk_io VALUES ('2'); -- fails ROLLBACK TO SAVEPOINT s; ROLLBACK; + +-- +-- UPDATEs of referencing rows whose key type needs a coercion to reach the +-- equality operator's input type. ri_KeysEqual() decides whether the key +-- changed without applying the coercion; a check it can't skip applies it in +-- its query. Each statement runs in its own transaction, because +-- ri_KeysEqual() is not consulted for rows inserted by the same transaction. +-- +CREATE SCHEMA ri_upd_cast; +SET search_path = ri_upd_cast; +CREATE TABLE pk (id int4 PRIMARY KEY); +INSERT INTO pk VALUES (1); + +-- A cast that returns NULL for one value, later replaced and dropped. +CREATE TYPE word AS ENUM ('one', 'none'); +CREATE FUNCTION word_to_int4(word) RETURNS int4 + LANGUAGE sql IMMUTABLE AS $$ SELECT CASE WHEN $1::text = 'one' THEN 1 END $$; +CREATE CAST (word AS int4) WITH FUNCTION word_to_int4(word) AS IMPLICIT; +CREATE TABLE fk_word (a word REFERENCES pk, n int); +INSERT INTO fk_word VALUES ('one', 0); +UPDATE fk_word SET n = n + 1; +UPDATE fk_word SET a = 'none'; -- fails, as a violation +CREATE FUNCTION word_to_int4_v2(word) RETURNS int4 + LANGUAGE sql IMMUTABLE AS $$ SELECT 1 $$; +DROP CAST (word AS int4); +CREATE CAST (word AS int4) WITH FUNCTION word_to_int4_v2(word) AS IMPLICIT; +DROP FUNCTION word_to_int4(word); +UPDATE fk_word SET n = n + 1; -- must not call the dropped function +UPDATE fk_word SET a = 'none'; -- succeeds through the new cast +SELECT * FROM fk_word; + +-- A collation-sensitive cast from a collatable type. +CREATE TYPE ctext; +CREATE FUNCTION ctext_in(cstring) RETURNS ctext + AS 'textin' LANGUAGE internal IMMUTABLE STRICT; +CREATE FUNCTION ctext_out(ctext) RETURNS cstring + AS 'textout' LANGUAGE internal IMMUTABLE STRICT; +CREATE TYPE ctext (INPUT = ctext_in, OUTPUT = ctext_out, + LIKE = text, COLLATABLE = true); +CREATE FUNCTION ctext_lower(ctext) RETURNS text + AS 'lower' LANGUAGE internal IMMUTABLE STRICT; +CREATE CAST (ctext AS text) WITH FUNCTION ctext_lower(ctext) AS IMPLICIT; +CREATE TABLE pk_text (k text PRIMARY KEY); +INSERT INTO pk_text VALUES ('abc'); +CREATE TABLE fk_text (a ctext REFERENCES pk_text, n int); +INSERT INTO fk_text VALUES ('ABC', 0); +UPDATE fk_text SET n = n + 1; +SELECT * FROM fk_text; + +-- An array coercion (int2[] to int4[], allowed with an opclass declared for +-- int4[]), which has no single cast function. +CREATE FUNCTION i4a_lt(int4[], int4[]) RETURNS bool + AS 'array_lt' LANGUAGE internal IMMUTABLE STRICT; +CREATE FUNCTION i4a_le(int4[], int4[]) RETURNS bool + AS 'array_le' LANGUAGE internal IMMUTABLE STRICT; +CREATE FUNCTION i4a_eq(int4[], int4[]) RETURNS bool + AS 'array_eq' LANGUAGE internal IMMUTABLE STRICT; +CREATE FUNCTION i4a_ge(int4[], int4[]) RETURNS bool + AS 'array_ge' LANGUAGE internal IMMUTABLE STRICT; +CREATE FUNCTION i4a_gt(int4[], int4[]) RETURNS bool + AS 'array_gt' LANGUAGE internal IMMUTABLE STRICT; +CREATE FUNCTION i4a_cmp(int4[], int4[]) RETURNS int4 + AS 'btarraycmp' LANGUAGE internal IMMUTABLE STRICT; +CREATE OPERATOR ri_upd_cast.<<< (LEFTARG = int4[], RIGHTARG = int4[], FUNCTION = i4a_lt); +CREATE OPERATOR ri_upd_cast.<<= (LEFTARG = int4[], RIGHTARG = int4[], FUNCTION = i4a_le); +CREATE OPERATOR ri_upd_cast.=== (LEFTARG = int4[], RIGHTARG = int4[], FUNCTION = i4a_eq); +CREATE OPERATOR ri_upd_cast.>>= (LEFTARG = int4[], RIGHTARG = int4[], FUNCTION = i4a_ge); +CREATE OPERATOR ri_upd_cast.>>> (LEFTARG = int4[], RIGHTARG = int4[], FUNCTION = i4a_gt); +CREATE OPERATOR CLASS i4a_ops FOR TYPE int4[] USING btree AS + OPERATOR 1 ri_upd_cast.<<<, OPERATOR 2 ri_upd_cast.<<=, + OPERATOR 3 ri_upd_cast.===, OPERATOR 4 ri_upd_cast.>>=, + OPERATOR 5 ri_upd_cast.>>>, FUNCTION 1 i4a_cmp(int4[], int4[]); +CREATE TABLE pk_arr (k int4[]); +CREATE UNIQUE INDEX ON pk_arr (k i4a_ops); +INSERT INTO pk_arr VALUES ('{1,2}'); +CREATE TABLE fk_arr (a int2[] REFERENCES pk_arr (k), n int); +INSERT INTO fk_arr VALUES ('{1,2}', 0); +UPDATE fk_arr SET n = n + 1; +UPDATE fk_arr SET a = '{3}'; -- fails, as a violation +SELECT * FROM fk_arr; + +-- A coercion via I/O, which has no cast function either. +CREATE TYPE ioint; +CREATE FUNCTION ioint_in(cstring) RETURNS ioint + AS 'int4in' LANGUAGE internal IMMUTABLE STRICT; +CREATE FUNCTION ioint_out(ioint) RETURNS cstring + AS 'int4out' LANGUAGE internal IMMUTABLE STRICT; +CREATE TYPE ioint (INPUT = ioint_in, OUTPUT = ioint_out, LIKE = int4); +CREATE CAST (ioint AS int4) WITH INOUT AS IMPLICIT; +CREATE TABLE fk_io (a ioint REFERENCES pk, n int); +INSERT INTO fk_io VALUES ('1', 0); +UPDATE fk_io SET n = n + 1; +UPDATE fk_io SET a = '2'; -- fails, as a violation +SELECT * FROM fk_io; + +RESET search_path; +SET client_min_messages = warning; +DROP SCHEMA ri_upd_cast CASCADE; +RESET client_min_messages; -- 2.47.3