I came across an edge case in how our foreign key implementation works that I think is not SQL conforming. It has to do with how updates to values that "look" different but compare as equal are cascaded. A simple case involves float -0 vs. 0, but relevant cases also arise with citext and case-insensitive collations.
Consider this example: create table pktable2 (a float8, b float8, primary key (a, b)); create table fktable2 (x float8, y float8, foreign key (x, y) references pktable2 (a, b) on update cascade); insert into pktable2 values ('-0', '-0'); insert into fktable2 values ('-0', '-0'); update pktable2 set a = '0' where a = '-0'; What happens now? select * from pktable2; a | b ---+---- 0 | -0 (1 row) -- buggy: did not update fktable2.x select * from fktable2; x | y ----+---- -0 | -0 (1 row) This happens because ri_KeysEqual() compares the old and new rows and decides that because they are "equal", the ON UPDATE actions don't need to be run. The SQL standard seems clear that ON CASCADE UPDATE means that an analogous UPDATE should be run on matching rows in the foreign key table, so the current behavior is wrong. Moreover, if another column is also updated, like update pktable2 set a = '0', b = '5', then the old and new rows are no longer equal, and so x will get updated in fktable2. So the context creates inconsistencies. The fix is that in these cases we have ri_KeysEqual() use a more low-level form of equality, like for example record_image_eq does. In fact, we can take the same code. See attached patches. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 8193fe1560c9e7aaa48c039fb3c85ab93558d596 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pe...@eisentraut.org> Date: Tue, 5 Feb 2019 16:27:00 +0100 Subject: [PATCH 1/2] Test case for keys that "look" different but compare as equal --- src/test/regress/expected/foreign_key.out | 34 +++++++++++++++++++++++ src/test/regress/sql/foreign_key.sql | 20 +++++++++++++ 2 files changed, 54 insertions(+) diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index bf2c91d9f0..a68d055e40 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -1474,6 +1474,40 @@ delete from pktable2 where f1 = 1; alter table fktable2 drop constraint fktable2_f1_fkey; ERROR: cannot ALTER TABLE "pktable2" because it has pending trigger events commit; +drop table pktable2, fktable2; +-- +-- Test keys that "look" different but compare as equal +-- +create table pktable2 (a float8, b float8, primary key (a, b)); +create table fktable2 (x float8, y float8, foreign key (x, y) references pktable2 (a, b) on update cascade); +insert into pktable2 values ('-0', '-0'); +insert into fktable2 values ('-0', '-0'); +select * from pktable2; + a | b +----+---- + -0 | -0 +(1 row) + +select * from fktable2; + x | y +----+---- + -0 | -0 +(1 row) + +update pktable2 set a = '0' where a = '-0'; +select * from pktable2; + a | b +---+---- + 0 | -0 +(1 row) + +-- buggy: did not update fktable2.x +select * from fktable2; + x | y +----+---- + -0 | -0 +(1 row) + drop table pktable2, fktable2; -- -- Foreign keys and partitioned tables diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index c8d1214d02..c1fc7d30b1 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -1106,6 +1106,26 @@ CREATE TEMP TABLE tasks ( drop table pktable2, fktable2; +-- +-- Test keys that "look" different but compare as equal +-- +create table pktable2 (a float8, b float8, primary key (a, b)); +create table fktable2 (x float8, y float8, foreign key (x, y) references pktable2 (a, b) on update cascade); + +insert into pktable2 values ('-0', '-0'); +insert into fktable2 values ('-0', '-0'); + +select * from pktable2; +select * from fktable2; + +update pktable2 set a = '0' where a = '-0'; + +select * from pktable2; +-- buggy: did not update fktable2.x +select * from fktable2; + +drop table pktable2, fktable2; + -- -- Foreign keys and partitioned tables base-commit: 24114e8b4df4a4ff2db9e608742768d219b1067c -- 2.20.1
From 083363ef132829bbf6b990317253868b5e48a137 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pe...@eisentraut.org> Date: Tue, 5 Feb 2019 16:31:33 +0100 Subject: [PATCH 2/2] Fix optimization of foreign-key on update actions In RI_FKey_pk_upd_check_required(), we check among other things whether the old and new key are equal, so that we don't need to run cascade actions when nothing has actually changed. This was using the equality operator. But the effect of this is that if a value in the primary key is changed to one that "looks" different but compares as equal, the update is not propagated. (Examples are float -0 and 0 and case-insensitive text.) This appears to violate the SQL standard, and it also behaves inconsistently if in a multicolumn key another key is also updated that would cause the row to compare as not equal. To fix, if we are looking at the PK table in ri_KeysEqual() and the constraint action is ON UPDATE CASCADE, then we must do a bytewise comparison similar to record_image_eq() instead of using the equality operators. For the FK table and other constraint actions, we continue to use the equality operators. --- src/backend/utils/adt/datum.c | 57 +++++++++++++++++++++++ src/backend/utils/adt/ri_triggers.c | 18 +++++++ src/backend/utils/adt/rowtypes.c | 41 +--------------- src/include/utils/datum.h | 9 ++++ src/test/regress/expected/foreign_key.out | 8 ++-- src/test/regress/sql/foreign_key.sql | 2 +- 6 files changed, 91 insertions(+), 44 deletions(-) diff --git a/src/backend/utils/adt/datum.c b/src/backend/utils/adt/datum.c index af5944d395..81ea5a48e5 100644 --- a/src/backend/utils/adt/datum.c +++ b/src/backend/utils/adt/datum.c @@ -42,6 +42,8 @@ #include "postgres.h" +#include "access/tuptoaster.h" +#include "fmgr.h" #include "utils/datum.h" #include "utils/expandeddatum.h" @@ -251,6 +253,61 @@ datumIsEqual(Datum value1, Datum value2, bool typByVal, int typLen) return res; } +/*------------------------------------------------------------------------- + * datum_image_eq + * + * Compares two datums for identical contents, based on byte images. Return + * true if the two datums are equal, false otherwise. + *------------------------------------------------------------------------- + */ +bool +datum_image_eq(Datum value1, Datum value2, bool typByVal, int typLen) +{ + bool result = true; + + if (typLen == -1) + { + Size len1, + len2; + + len1 = toast_raw_datum_size(value1); + len2 = toast_raw_datum_size(value2); + /* No need to de-toast if lengths don't match. */ + if (len1 != len2) + result = false; + else + { + struct varlena *arg1val; + struct varlena *arg2val; + + arg1val = PG_DETOAST_DATUM_PACKED(value1); + arg2val = PG_DETOAST_DATUM_PACKED(value2); + + result = (memcmp(VARDATA_ANY(arg1val), + VARDATA_ANY(arg2val), + len1 - VARHDRSZ) == 0); + + /* Only free memory if it's a copy made here. */ + if ((Pointer) arg1val != (Pointer) value1) + pfree(arg1val); + if ((Pointer) arg2val != (Pointer) value2) + pfree(arg2val); + } + } + else if (typByVal) + { + result = (value1 == value2); + } + else + { + result = (memcmp(DatumGetPointer(value1), + DatumGetPointer(value2), + typLen) == 0); + } + + return result; +} + /*------------------------------------------------------------------------- * datumEstimateSpace * diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index e1aa3d0044..9fe8406e08 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -49,6 +49,7 @@ #include "storage/bufmgr.h" #include "utils/acl.h" #include "utils/builtins.h" +#include "utils/datum.h" #include "utils/fmgroids.h" #include "utils/guc.h" #include "utils/inval.h" @@ -2861,12 +2862,29 @@ ri_KeysEqual(Relation rel, HeapTuple oldtup, HeapTuple newtup, if (isnull) return false; + if (rel_is_pk && riinfo->confupdtype == FKCONSTR_ACTION_CASCADE) + { + /* + * If we are looking at the PK table and the constraint is ON + * UPDATE CASCADE, then we must do a bytewise comparison. We must + * propagate PK changes if the value is changed to one that + * "looks" different but would compare as equal using the equality + * operator. + */ + Form_pg_attribute att = TupleDescAttr(tupdesc, attnums[i] - 1); + + if (!datum_image_eq(oldvalue, newvalue, att->attbyval, att->attlen)) + return false; + } + else + { /* * Compare them with the appropriate equality operator. */ if (!ri_AttributesEqual(eq_oprs[i], RIAttType(rel, attnums[i]), oldvalue, newvalue)) return false; + } } return true; diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c index 5bbf568610..aa7ec8735c 100644 --- a/src/backend/utils/adt/rowtypes.c +++ b/src/backend/utils/adt/rowtypes.c @@ -23,6 +23,7 @@ #include "libpq/pqformat.h" #include "miscadmin.h" #include "utils/builtins.h" +#include "utils/datum.h" #include "utils/lsyscache.h" #include "utils/typcache.h" @@ -1671,45 +1672,7 @@ record_image_eq(PG_FUNCTION_ARGS) } /* Compare the pair of elements */ - if (att1->attlen == -1) - { - Size len1, - len2; - - len1 = toast_raw_datum_size(values1[i1]); - len2 = toast_raw_datum_size(values2[i2]); - /* No need to de-toast if lengths don't match. */ - if (len1 != len2) - result = false; - else - { - struct varlena *arg1val; - struct varlena *arg2val; - - arg1val = PG_DETOAST_DATUM_PACKED(values1[i1]); - arg2val = PG_DETOAST_DATUM_PACKED(values2[i2]); - - result = (memcmp(VARDATA_ANY(arg1val), - VARDATA_ANY(arg2val), - len1 - VARHDRSZ) == 0); - - /* Only free memory if it's a copy made here. */ - if ((Pointer) arg1val != (Pointer) values1[i1]) - pfree(arg1val); - if ((Pointer) arg2val != (Pointer) values2[i2]) - pfree(arg2val); - } - } - else if (att1->attbyval) - { - result = (values1[i1] == values2[i2]); - } - else - { - result = (memcmp(DatumGetPointer(values1[i1]), - DatumGetPointer(values2[i2]), - att1->attlen) == 0); - } + result = datum_image_eq(values1[i1], values2[i2], att1->attbyval, att2->attlen); if (!result) break; } diff --git a/src/include/utils/datum.h b/src/include/utils/datum.h index 7b913578ab..4365bf06e6 100644 --- a/src/include/utils/datum.h +++ b/src/include/utils/datum.h @@ -46,6 +46,15 @@ extern Datum datumTransfer(Datum value, bool typByVal, int typLen); extern bool datumIsEqual(Datum value1, Datum value2, bool typByVal, int typLen); +/* + * datum_image_eq + * + * Compares two datums for identical contents, based on byte images. Return + * true if the two datums are equal, false otherwise. + */ +extern bool datum_image_eq(Datum value1, Datum value2, + bool typByVal, int typLen); + /* * Serialize and restore datums so that we can transfer them to parallel * workers. diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index a68d055e40..31f5739fcb 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -1501,11 +1501,11 @@ select * from pktable2; 0 | -0 (1 row) --- buggy: did not update fktable2.x +-- should update fktable2.x select * from fktable2; - x | y -----+---- - -0 | -0 + x | y +---+---- + 0 | -0 (1 row) drop table pktable2, fktable2; diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index c1fc7d30b1..dead30a0c1 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -1121,7 +1121,7 @@ CREATE TEMP TABLE tasks ( update pktable2 set a = '0' where a = '-0'; select * from pktable2; --- buggy: did not update fktable2.x +-- should update fktable2.x select * from fktable2; drop table pktable2, fktable2; -- 2.20.1