From f9f05aceaed972147719345c65cce9222c50ef5e Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Mon, 11 Mar 2019 12:55:22 +0100 Subject: [PATCH v2 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(), then do a bytewise comparison similar to record_image_eq() instead of using the equality operators. This only makes a difference for ON UPDATE CASCADE, but for consistency we treat all changes to the PK the same. For the FK table, we continue to use the equality operators. Discussion: https://www.postgresql.org/message-id/flat/3326fc2e-bc02-d4c5-e3e5-e54da466e89a@2ndquadrant.com --- src/backend/utils/adt/datum.c | 57 +++++++++++++++++++++++ src/backend/utils/adt/ri_triggers.c | 40 ++++++++++------ 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, 100 insertions(+), 57 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 ef04fa5009..0a6548a797 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -42,6 +42,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" @@ -2419,18 +2420,11 @@ ri_KeysEqual(Relation rel, TupleTableSlot *oldslot, TupleTableSlot *newslot, const RI_ConstraintInfo *riinfo, bool rel_is_pk) { const int16 *attnums; - const Oid *eq_oprs; if (rel_is_pk) - { attnums = riinfo->pk_attnums; - eq_oprs = riinfo->pp_eq_oprs; - } else - { attnums = riinfo->fk_attnums; - eq_oprs = riinfo->ff_eq_oprs; - } /* XXX: could be worthwhile to fetch all necessary attrs at once */ for (int i = 0; i < riinfo->nkeys; i++) @@ -2453,12 +2447,32 @@ ri_KeysEqual(Relation rel, TupleTableSlot *oldslot, TupleTableSlot *newslot, if (isnull) return false; - /* - * Compare them with the appropriate equality operator. - */ - if (!ri_AttributesEqual(eq_oprs[i], RIAttType(rel, attnums[i]), - oldvalue, newvalue)) - return false; + if (rel_is_pk) + { + /* + * If we are looking at the PK table, then 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. This only makes a + * difference for ON UPDATE CASCADE, but for consistency we treat + * all changes to the PK the same. + */ + Form_pg_attribute att = TupleDescAttr(oldslot->tts_tupleDescriptor, attnums[i] - 1); + + if (!datum_image_eq(oldvalue, newvalue, att->attbyval, att->attlen)) + return false; + } + else + { + /* + * For the FK table, compare with the appropriate equality + * operator. Changes that compare equal will still satisfy the + * constraint after the update. + */ + if (!ri_AttributesEqual(riinfo->ff_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.21.0