From: Tom Lane Date: Tue, 17 Jul 2007 17:45:57 +0000 (+0000) Subject: Fix incorrect optimization of foreign-key checks. When an UPDATE on the X-Git-Tag: REL8_0_14~32 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=a41c4b218fc7a67234fa657a0b611fea70ddd394;p=thirdparty%2Fpostgresql.git Fix incorrect optimization of foreign-key checks. When an UPDATE on the referencing table does not change the tuple's FK column(s), we don't bother to check the PK table since the constraint was presumably already valid. However, the check is still necessary if the tuple was inserted by our own transaction, since in that case the INSERT trigger will conclude it need not make the check (since its version of the tuple has been deleted). We got this right for simple cases, but not when the insert and update are in different subtransactions of the current top-level transaction; in such cases the FK check would never be made at all. (Hence, problem dates back to 8.0 when subtransactions were added --- it's actually the subtransaction version of a bug fixed in 7.3.5.) Fix, and add regression test cases. Report and fix by Affan Salman. --- diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 72ae3b8ddea..46653423c1c 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -17,7 +17,7 @@ * * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group * - * $PostgreSQL: pgsql/src/backend/utils/adt/ri_triggers.c,v 1.76 2004/12/31 22:01:22 pgsql Exp $ + * $PostgreSQL: pgsql/src/backend/utils/adt/ri_triggers.c,v 1.76.4.1 2007/07/17 17:45:56 tgl Exp $ * * ---------- */ @@ -377,12 +377,13 @@ RI_FKey_check(PG_FUNCTION_ARGS) /* * No need to check anything if old and new references are the same on - * UPDATE. + * UPDATE; unless the updated row was inserted by our own transaction. + * This is because the UPDATE invalidated the INSERT, so the INSERT + * trigger won't have checked. */ if (TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event)) { - if (HeapTupleHeaderGetXmin(old_row->t_data) != - GetCurrentTransactionId() && + if (!TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetXmin(old_row->t_data)) && ri_KeysEqual(fk_rel, old_row, new_row, &qkey, RI_KEYPAIR_FK_IDX)) { diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index ae80b33c6f7..a479f4932a8 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -1128,3 +1128,82 @@ WARNING: foreign key constraint "fk_241_132" will require costly sequential sca DETAIL: Key columns "x2" and "id1" are of different types: character varying and integer. WARNING: foreign key constraint "fk_241_132" will require costly sequential scans DETAIL: Key columns "x4" and "id3" are of different types: text and real. +DROP TABLE pktable, fktable CASCADE; +NOTICE: drop cascades to constraint fk_241_132 on table fktable +NOTICE: drop cascades to constraint fk_123_231 on table fktable +NOTICE: drop cascades to constraint fk_253_213 on table fktable +NOTICE: drop cascades to constraint fk_213_213 on table fktable +NOTICE: drop cascades to constraint fk_123_123 on table fktable +NOTICE: drop cascades to constraint fk_5_1 on table fktable +NOTICE: drop cascades to constraint fk_3_1 on table fktable +NOTICE: drop cascades to constraint fk_2_1 on table fktable +NOTICE: drop cascades to constraint fktable_x1_fkey on table fktable +NOTICE: drop cascades to constraint fk_4_2 on table fktable +NOTICE: drop cascades to constraint fk_1_2 on table fktable +NOTICE: drop cascades to constraint fktable_x2_fkey on table fktable +NOTICE: drop cascades to constraint fk_1_3 on table fktable +NOTICE: drop cascades to constraint fk_2_3 on table fktable +NOTICE: drop cascades to constraint fktable_x3_fkey on table fktable +-- test a tricky case: we can elide firing the FK check trigger during +-- an UPDATE if the UPDATE did not change the foreign key +-- field. However, we can't do this if our transaction was the one that +-- created the updated row and the trigger is deferred, since our UPDATE +-- will have invalidated the original newly-inserted tuple, and therefore +-- cause the on-INSERT RI trigger not to be fired. +CREATE TEMP TABLE pktable ( + id int primary key, + other int +); +NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "pktable_pkey" for table "pktable" +CREATE TEMP TABLE fktable ( + id int primary key, + fk int references pktable deferrable initially deferred +); +NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "fktable_pkey" for table "fktable" +INSERT INTO pktable VALUES (5, 10); +BEGIN; +-- doesn't match PK, but no error yet +INSERT INTO fktable VALUES (0, 20); +-- don't change FK +UPDATE fktable SET id = id + 1; +-- should catch error from initial INSERT +COMMIT; +ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_fk_fkey" +DETAIL: Key (fk)=(20) is not present in table "pktable". +-- check same case when insert is in a different subtransaction than update +BEGIN; +-- doesn't match PK, but no error yet +INSERT INTO fktable VALUES (0, 20); +-- UPDATE will be in a subxact +SAVEPOINT savept1; +-- don't change FK +UPDATE fktable SET id = id + 1; +-- should catch error from initial INSERT +COMMIT; +ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_fk_fkey" +DETAIL: Key (fk)=(20) is not present in table "pktable". +BEGIN; +-- INSERT will be in a subxact +SAVEPOINT savept1; +-- doesn't match PK, but no error yet +INSERT INTO fktable VALUES (0, 20); +RELEASE SAVEPOINT savept1; +-- don't change FK +UPDATE fktable SET id = id + 1; +-- should catch error from initial INSERT +COMMIT; +ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_fk_fkey" +DETAIL: Key (fk)=(20) is not present in table "pktable". +BEGIN; +-- doesn't match PK, but no error yet +INSERT INTO fktable VALUES (0, 20); +-- UPDATE will be in a subxact +SAVEPOINT savept1; +-- don't change FK +UPDATE fktable SET id = id + 1; +-- Roll back the UPDATE +ROLLBACK TO savept1; +-- should catch error from initial INSERT +COMMIT; +ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_fk_fkey" +DETAIL: Key (fk)=(20) is not present in table "pktable". diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index ad5865683fb..dee046a1952 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -774,3 +774,84 @@ FOREIGN KEY (x1,x2,x3) REFERENCES pktable(id2,id3,id1); ALTER TABLE fktable ADD CONSTRAINT fk_241_132 FOREIGN KEY (x2,x4,x1) REFERENCES pktable(id1,id3,id2); + +DROP TABLE pktable, fktable CASCADE; + +-- test a tricky case: we can elide firing the FK check trigger during +-- an UPDATE if the UPDATE did not change the foreign key +-- field. However, we can't do this if our transaction was the one that +-- created the updated row and the trigger is deferred, since our UPDATE +-- will have invalidated the original newly-inserted tuple, and therefore +-- cause the on-INSERT RI trigger not to be fired. + +CREATE TEMP TABLE pktable ( + id int primary key, + other int +); + +CREATE TEMP TABLE fktable ( + id int primary key, + fk int references pktable deferrable initially deferred +); + +INSERT INTO pktable VALUES (5, 10); + +BEGIN; + +-- doesn't match PK, but no error yet +INSERT INTO fktable VALUES (0, 20); + +-- don't change FK +UPDATE fktable SET id = id + 1; + +-- should catch error from initial INSERT +COMMIT; + +-- check same case when insert is in a different subtransaction than update + +BEGIN; + +-- doesn't match PK, but no error yet +INSERT INTO fktable VALUES (0, 20); + +-- UPDATE will be in a subxact +SAVEPOINT savept1; + +-- don't change FK +UPDATE fktable SET id = id + 1; + +-- should catch error from initial INSERT +COMMIT; + +BEGIN; + +-- INSERT will be in a subxact +SAVEPOINT savept1; + +-- doesn't match PK, but no error yet +INSERT INTO fktable VALUES (0, 20); + +RELEASE SAVEPOINT savept1; + +-- don't change FK +UPDATE fktable SET id = id + 1; + +-- should catch error from initial INSERT +COMMIT; + +BEGIN; + +-- doesn't match PK, but no error yet +INSERT INTO fktable VALUES (0, 20); + +-- UPDATE will be in a subxact +SAVEPOINT savept1; + +-- don't change FK +UPDATE fktable SET id = id + 1; + +-- Roll back the UPDATE +ROLLBACK TO savept1; + +-- should catch error from initial INSERT +COMMIT;