Re: Fix optimization of foreign-key on update actions
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
Re: Fix optimization of foreign-key on update actions
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
From d8615fe224eed4747682e88202faad1251c317f6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
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
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-e54da466e...@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
Re: Fix optimization of foreign-key on update actions
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
Re: Fix optimization of foreign-key on update actions
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
Re: Fix optimization of foreign-key on update actions
> "Laurenz" == Laurenz Albe 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, "". 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.
Re: Fix optimization of foreign-key on update actions
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
Re: Fix optimization of foreign-key on update actions
Andrew Gierth writes: > "Peter" == Peter Eisentraut 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
Re: Fix optimization of foreign-key on update actions
> "Peter" == Peter Eisentraut 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)
