Fix optimization of foreign-key on update actions
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
Attachments:
0001-Test-case-for-keys-that-look-different-but-compare-a.patchtext/plain; charset=UTF-8; name=0001-Test-case-for-keys-that-look-different-but-compare-a.patch; x-mac-creator=0; x-mac-type=0Download
From 8193fe1560c9e7aaa48c039fb3c85ab93558d596 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@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
0002-Fix-optimization-of-foreign-key-on-update-actions.patchtext/plain; charset=UTF-8; name=0002-Fix-optimization-of-foreign-key-on-update-actions.patch; x-mac-creator=0; x-mac-type=0Download
From 083363ef132829bbf6b990317253868b5e48a137 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@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
"Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
Peter> The SQL standard seems clear
ha
hahaha
HAHAHAHAHAHA
(since when has the SQL standard ever been clear?)
SQL2016, 15.17 Execution of referential actions
10) If a non-null value of a referenced column RC in the referenced
table is updated to a value that is distinct from the current value
of RC, then,
[snip all the stuff about how ON UPDATE actions work]
does that "is distinct from" mean that IS DISTINCT FROM would be true,
or does it mean "is in some way distinguishable from"? Nothing I can see
in the spec suggests the latter.
--
Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
"Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
Peter> The SQL standard seems clear
(since when has the SQL standard ever been clear?)
Point to Andrew ;-). However, I kind of like Peter's idea anyway
on the grounds that byte-wise comparison is probably faster than
invoking the datatype comparators. Also, language lawyering aside,
I think he may be right about what people would expect "should" happen.
What I *don't* like about the proposed patch is that it installs a
new, different comparison rule for the ON UPDATE CASCADE case only.
If we were to go in this direction, I'd think we should try to use
the same comparison rule for all FK row comparisons.
The inconsistencies get messier the more you think about it,
really. If a referencing row was datatype-equal, but not byte-equal,
to the PK row to start with, why would an update changing the PK row
(perhaps to another datatype-equal value) result in forcing the
referencing row to become byte-equal? How does this fit in with
the fact that our notion of what uniqueness means in the PK table
is no-datatype-equality, rather than no-byte-equality?
On the whole we might be better off leaving this alone.
regards, tom lane
Andrew Gierth wrote:
SQL2016, 15.17 Execution of referential actions
10) If a non-null value of a referenced column RC in the referenced
table is updated to a value that is distinct from the current value
of RC, then,[snip all the stuff about how ON UPDATE actions work]
does that "is distinct from" mean that IS DISTINCT FROM would be true,
or does it mean "is in some way distinguishable from"? Nothing I can see
in the spec suggests the latter.
My 2003 standard defines, and even condescends to be informal:
3.1.6.8 distinct (of a pair of comparable values): Capable of being distinguished within a given context.
Informally, not equal, not both null. A null value and a non-null value are distinct.
Yours,
Laurenz Albe
"Laurenz" == Laurenz Albe <laurenz.albe@cybertec.at> writes:
Laurenz> Andrew Gierth wrote:
SQL2016, 15.17 Execution of referential actions
10) If a non-null value of a referenced column RC in the referenced
table is updated to a value that is distinct from the current value
of RC, then,[snip all the stuff about how ON UPDATE actions work]
does that "is distinct from" mean that IS DISTINCT FROM would be true,
or does it mean "is in some way distinguishable from"? Nothing I can see
in the spec suggests the latter.
Laurenz> My 2003 standard defines, and even condescends to be informal:
Laurenz> 3.1.6.8 distinct (of a pair of comparable values): Capable of
Laurenz> being distinguished within a given context. Informally, not
Laurenz> equal, not both null. A null value and a non-null value are
Laurenz> distinct.
Hrm. SQL2016 has similar language which I previously missed, but I don't
think it actually helps:
3.1.6.9 distinct (of a pair of comparable values)
capable of being distinguished within a given context
NOTE 8 -- Informally, two values are distinct if neither
is null and the values are not equal. A null value and a
non- null value are distinct. Two null values are not
distinct. See Subclause 4.1.5, "Properties of distinct",
and the General Rules of Subclause 8.15, "<distinct
predicate>".
Two values which are sql-equal but not identical, such as two strings in
a case-insensitive collation that differ only by case, are
distinguishable in some contexts but not others, so what context
actually applies to the quoted rule?
I think the only reasonable interpretation is that it should use the
same kind of distinctness that is being used by the unique constraint
and the equality comparison that define whether the FK is satisfied.
--
Andrew.
On 06/02/2019 12:23, Andrew Gierth wrote:
Two values which are sql-equal but not identical, such as two strings in
a case-insensitive collation that differ only by case, are
distinguishable in some contexts but not others, so what context
actually applies to the quoted rule?I think the only reasonable interpretation is that it should use the
same kind of distinctness that is being used by the unique constraint
and the equality comparison that define whether the FK is satisfied.
By that logic, a command such as
UPDATE t1 SET x = '0' WHERE x = '-0';
could be optimized away as a noop, because in that world there is no
construct by which you can prove whether the update happened.
I think that would not be satisfactory.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 05/02/2019 17:20, Tom Lane wrote:
What I *don't* like about the proposed patch is that it installs a
new, different comparison rule for the ON UPDATE CASCADE case only.
If we were to go in this direction, I'd think we should try to use
the same comparison rule for all FK row comparisons.
That's easy to change. I had it like that in earlier versions of the
patch. I agree it would be better for consistency, but it would create
some cases where we do unnecessary extra work.
The inconsistencies get messier the more you think about it,
really. If a referencing row was datatype-equal, but not byte-equal,
to the PK row to start with, why would an update changing the PK row
(perhaps to another datatype-equal value) result in forcing the
referencing row to become byte-equal? How does this fit in with
the fact that our notion of what uniqueness means in the PK table
is no-datatype-equality, rather than no-byte-equality?
This patch doesn't actually change the actual foreign key behavior.
Foreign keys already work like that. The patch only touches an
optimization that checks whether it's worth running the full foreign key
behavior because the change was significant enough. That shouldn't
affect the outcome. It should be the case that if you replace
RI_FKey_pk_upd_check_required() by just "return true", then nothing
user-visible changes.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-02-06 23:15, Peter Eisentraut wrote:
On 05/02/2019 17:20, Tom Lane wrote:
What I *don't* like about the proposed patch is that it installs a
new, different comparison rule for the ON UPDATE CASCADE case only.
If we were to go in this direction, I'd think we should try to use
the same comparison rule for all FK row comparisons.That's easy to change. I had it like that in earlier versions of the
patch. I agree it would be better for consistency, but it would create
some cases where we do unnecessary extra work.
Updated patch with this change made, and some conflicts resolved.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
v2-0001-Test-case-for-keys-that-look-different-but-compar.patchtext/plain; charset=UTF-8; name=v2-0001-Test-case-for-keys-that-look-different-but-compar.patch; x-mac-creator=0; x-mac-type=0Download
From d8615fe224eed4747682e88202faad1251c317f6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 5 Feb 2019 16:27:00 +0100
Subject: [PATCH v2 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: f2d84a4a6b4ec891a0a52f583ed5aa081c71acc6
--
2.21.0
v2-0002-Fix-optimization-of-foreign-key-on-update-actions.patchtext/plain; charset=UTF-8; name=v2-0002-Fix-optimization-of-foreign-key-on-update-actions.patch; x-mac-creator=0; x-mac-type=0Download
From f9f05aceaed972147719345c65cce9222c50ef5e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
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
On 2019-03-11 12:57, Peter Eisentraut wrote:
On 2019-02-06 23:15, Peter Eisentraut wrote:
On 05/02/2019 17:20, Tom Lane wrote:
What I *don't* like about the proposed patch is that it installs a
new, different comparison rule for the ON UPDATE CASCADE case only.
If we were to go in this direction, I'd think we should try to use
the same comparison rule for all FK row comparisons.That's easy to change. I had it like that in earlier versions of the
patch. I agree it would be better for consistency, but it would create
some cases where we do unnecessary extra work.Updated patch with this change made, and some conflicts resolved.
Committed like this.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services