additional foreign key test coverage

Started by Peter Eisentrautabout 7 years ago6 messages

peter.eisentraut@2ndquadrant.com

about 7 years ago

4 attachment(s)

During the development of my recent patch "unused/redundant foreign key
code" [0]/messages/by-id/2fb8d28c-a4e1-f206-898b-69cd22a393a1@2ndquadrant.com/, I had developed a few additional test cases to increase the
coverage in ri_triggers.c. They are in the attached patches with
explanations. With these, coverage should be pretty complete, except
hard-to-trigger error cases. Interested reviewers can also follow along
on coverage.postgresql.org.

[0]: /messages/by-id/2fb8d28c-a4e1-f206-898b-69cd22a393a1@2ndquadrant.com/
/messages/by-id/2fb8d28c-a4e1-f206-898b-69cd22a393a1@2ndquadrant.com/

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

0001-Add-test-case-for-ON-DELETE-NO-ACTION-RESTRICT.patchtext/plain; charset=UTF-8; name=0001-Add-test-case-for-ON-DELETE-NO-ACTION-RESTRICT.patch; x-mac-creator=0; x-mac-type=0Download

From 8eb6be9c2d458af9abf58d6c55929c54036cd9db Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Wed, 6 Jun 2018 22:30:16 -0400
Subject: [PATCH 1/4] Add test case for ON DELETE NO ACTION/RESTRICT

This was previously not covered at all; function
RI_FKey_restrict_del() was not exercised in the tests.
---
 src/test/regress/expected/foreign_key.out | 10 ++++++++--
 src/test/regress/sql/foreign_key.sql      |  6 ++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 085c9aba11..5525dd75b9 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1339,7 +1339,7 @@ DETAIL:  Key (f1)=(1) is still referenced from table "defc".
 -- Test the difference between NO ACTION and RESTRICT
 --
 create temp table pp (f1 int primary key);
-create temp table cc (f1 int references pp on update no action);
+create temp table cc (f1 int references pp on update no action on delete no action);
 insert into pp values(12);
 insert into pp values(11);
 update pp set f1=f1+1;
@@ -1348,9 +1348,12 @@ update pp set f1=f1+1;
 update pp set f1=f1+1; -- fail
 ERROR:  update or delete on table "pp" violates foreign key constraint "cc_f1_fkey" on table "cc"
 DETAIL:  Key (f1)=(13) is still referenced from table "cc".
+delete from pp where f1 = 13; -- fail
+ERROR:  update or delete on table "pp" violates foreign key constraint "cc_f1_fkey" on table "cc"
+DETAIL:  Key (f1)=(13) is still referenced from table "cc".
 drop table pp, cc;
 create temp table pp (f1 int primary key);
-create temp table cc (f1 int references pp on update restrict);
+create temp table cc (f1 int references pp on update restrict on delete restrict);
 insert into pp values(12);
 insert into pp values(11);
 update pp set f1=f1+1;
@@ -1358,6 +1361,9 @@ insert into cc values(13);
 update pp set f1=f1+1; -- fail
 ERROR:  update or delete on table "pp" violates foreign key constraint "cc_f1_fkey" on table "cc"
 DETAIL:  Key (f1)=(13) is still referenced from table "cc".
+delete from pp where f1 = 13; -- fail
+ERROR:  update or delete on table "pp" violates foreign key constraint "cc_f1_fkey" on table "cc"
+DETAIL:  Key (f1)=(13) is still referenced from table "cc".
 drop table pp, cc;
 --
 -- Test interaction of foreign-key optimization with rules (bug #14219)
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 068ab2aab7..615588c318 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -992,22 +992,24 @@ CREATE TEMP TABLE tasks (
 -- Test the difference between NO ACTION and RESTRICT
 --
 create temp table pp (f1 int primary key);
-create temp table cc (f1 int references pp on update no action);
+create temp table cc (f1 int references pp on update no action on delete no action);
 insert into pp values(12);
 insert into pp values(11);
 update pp set f1=f1+1;
 insert into cc values(13);
 update pp set f1=f1+1;
 update pp set f1=f1+1; -- fail
+delete from pp where f1 = 13; -- fail
 drop table pp, cc;
 
 create temp table pp (f1 int primary key);
-create temp table cc (f1 int references pp on update restrict);
+create temp table cc (f1 int references pp on update restrict on delete restrict);
 insert into pp values(12);
 insert into pp values(11);
 update pp set f1=f1+1;
 insert into cc values(13);
 update pp set f1=f1+1; -- fail
+delete from pp where f1 = 13; -- fail
 drop table pp, cc;
 
 --

base-commit: ee2b37ae044f34851baba69e9ba737077326414e
-- 
2.19.2

0002-Increase-test-coverage-in-RI_FKey_pk_upd_check_requi.patchtext/plain; charset=UTF-8; name=0002-Increase-test-coverage-in-RI_FKey_pk_upd_check_requi.patch; x-mac-creator=0; x-mac-type=0Download

From 01e43469973e9055da6f9fa16c95efac636b886a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Sat, 16 Jun 2018 08:31:52 -0400
Subject: [PATCH 2/4] Increase test coverage in RI_FKey_pk_upd_check_required()

This checks the case where the primary key has at least one null
column.
---
 src/test/regress/expected/foreign_key.out | 24 +++++++++++++++++++++++
 src/test/regress/sql/foreign_key.sql      | 19 ++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 5525dd75b9..421ffbeae7 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -397,6 +397,30 @@ SELECT * from FKTABLE;
         |      3 |      4 |      5
 (5 rows)
 
+DROP TABLE FKTABLE;
+DROP TABLE PKTABLE;
+-- restrict with null values
+CREATE TABLE PKTABLE ( ptest1 int, ptest2 int, ptest3 int, ptest4 text, UNIQUE(ptest1, ptest2, ptest3) );
+CREATE TABLE FKTABLE ( ftest1 int, ftest2 int, ftest3 int, ftest4 int,  CONSTRAINT constrname3
+			FOREIGN KEY(ftest1, ftest2, ftest3) REFERENCES PKTABLE (ptest1, ptest2, ptest3));
+INSERT INTO PKTABLE VALUES (1, 2, 3, 'test1');
+INSERT INTO PKTABLE VALUES (1, 3, NULL, 'test2');
+INSERT INTO PKTABLE VALUES (2, NULL, 4, 'test3');
+INSERT INTO FKTABLE VALUES (1, 2, 3, 1);
+DELETE FROM PKTABLE WHERE ptest1 = 2;
+SELECT * FROM PKTABLE;
+ ptest1 | ptest2 | ptest3 | ptest4 
+--------+--------+--------+--------
+      1 |      2 |      3 | test1
+      1 |      3 |        | test2
+(2 rows)
+
+SELECT * FROM FKTABLE;
+ ftest1 | ftest2 | ftest3 | ftest4 
+--------+--------+--------+--------
+      1 |      2 |      3 |      1
+(1 row)
+
 DROP TABLE FKTABLE;
 DROP TABLE PKTABLE;
 -- cascade update/delete
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 615588c318..d3ed72b1fc 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -260,6 +260,25 @@ CREATE TABLE FKTABLE ( ftest1 int, ftest2 int, ftest3 int, ftest4 int,  CONSTRAI
 DROP TABLE FKTABLE;
 DROP TABLE PKTABLE;
 
+-- restrict with null values
+CREATE TABLE PKTABLE ( ptest1 int, ptest2 int, ptest3 int, ptest4 text, UNIQUE(ptest1, ptest2, ptest3) );
+CREATE TABLE FKTABLE ( ftest1 int, ftest2 int, ftest3 int, ftest4 int,  CONSTRAINT constrname3
+			FOREIGN KEY(ftest1, ftest2, ftest3) REFERENCES PKTABLE (ptest1, ptest2, ptest3));
+
+INSERT INTO PKTABLE VALUES (1, 2, 3, 'test1');
+INSERT INTO PKTABLE VALUES (1, 3, NULL, 'test2');
+INSERT INTO PKTABLE VALUES (2, NULL, 4, 'test3');
+
+INSERT INTO FKTABLE VALUES (1, 2, 3, 1);
+
+DELETE FROM PKTABLE WHERE ptest1 = 2;
+
+SELECT * FROM PKTABLE;
+SELECT * FROM FKTABLE;
+
+DROP TABLE FKTABLE;
+DROP TABLE PKTABLE;
+
 -- cascade update/delete
 CREATE TABLE PKTABLE ( ptest1 int, ptest2 int, ptest3 int, ptest4 text, PRIMARY KEY(ptest1, ptest2, ptest3) );
 CREATE TABLE FKTABLE ( ftest1 int, ftest2 int, ftest3 int, ftest4 int,  CONSTRAINT constrname3
-- 
2.19.2

0003-Increase-test-coverage-in-RI_FKey_fk_upd_check_requi.patchtext/plain; charset=UTF-8; name=0003-Increase-test-coverage-in-RI_FKey_fk_upd_check_requi.patch; x-mac-creator=0; x-mac-type=0Download

From 98e3a93d90ff21a18a95e759ac49b04b58c1cf70 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Wed, 21 Nov 2018 13:18:14 +0100
Subject: [PATCH 3/4] Increase test coverage in RI_FKey_fk_upd_check_required()

This checks the code path of FKCONSTR_MATCH_FULL and
RI_KEYS_SOME_NULL.
---
 src/test/regress/expected/foreign_key.out | 8 +++++++-
 src/test/regress/sql/foreign_key.sql      | 6 ++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 421ffbeae7..6b2df98c7e 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -143,6 +143,12 @@ SELECT * FROM FKTABLE;
         |        |      8
 (5 rows)
 
+-- Check update with part of key null
+UPDATE FKTABLE SET ftest1 = NULL WHERE ftest1 = 1;
+ERROR:  insert or update on table "fktable" violates foreign key constraint "constrname"
+DETAIL:  MATCH FULL does not allow mixing of null and nonnull key values.
+-- Check update with old and new key values equal
+UPDATE FKTABLE SET ftest1 = 1 WHERE ftest1 = 1;
 -- Try altering the column type where foreign keys are involved
 ALTER TABLE PKTABLE ALTER COLUMN ptest1 TYPE bigint;
 ALTER TABLE FKTABLE ALTER COLUMN ftest1 TYPE bigint;
@@ -158,11 +164,11 @@ SELECT * FROM PKTABLE;
 SELECT * FROM FKTABLE;
  ftest1 | ftest2 | ftest3 
 --------+--------+--------
-      1 |      3 |      5
       3 |      6 |     12
         |        |      0
         |        |      4
         |        |      8
+      1 |      3 |      5
 (5 rows)
 
 DROP TABLE PKTABLE CASCADE;
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index d3ed72b1fc..9dd5e29c75 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -97,6 +97,12 @@ CREATE TABLE FKTABLE ( ftest1 int, ftest2 int, ftest3 int, CONSTRAINT constrname
 -- Check FKTABLE for update of matched row
 SELECT * FROM FKTABLE;
 
+-- Check update with part of key null
+UPDATE FKTABLE SET ftest1 = NULL WHERE ftest1 = 1;
+
+-- Check update with old and new key values equal
+UPDATE FKTABLE SET ftest1 = 1 WHERE ftest1 = 1;
+
 -- Try altering the column type where foreign keys are involved
 ALTER TABLE PKTABLE ALTER COLUMN ptest1 TYPE bigint;
 ALTER TABLE FKTABLE ALTER COLUMN ftest1 TYPE bigint;
-- 
2.19.2

0004-Increase-test-coverage-in-RI_Initial_Check.patchtext/plain; charset=UTF-8; name=0004-Increase-test-coverage-in-RI_Initial_Check.patch; x-mac-creator=0; x-mac-type=0Download

From 2b5ab918e7706639b960d9a053dfc97176df4546 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Sun, 25 Nov 2018 16:27:37 +0100
Subject: [PATCH 4/4] Increase test coverage in RI_Initial_Check()

This covers the special error handling of FKCONSTR_MATCH_FULL.
---
 src/test/regress/expected/foreign_key.out | 12 ++++++++++++
 src/test/regress/sql/foreign_key.sql      | 14 ++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 6b2df98c7e..1241aab6e3 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -339,6 +339,18 @@ SELECT * FROM PKTABLE;
       0 | Test4
 (4 rows)
 
+DROP TABLE FKTABLE;
+DROP TABLE PKTABLE;
+--
+-- Check initial check upon ALTER TABLE
+--
+CREATE TABLE PKTABLE ( ptest1 int, ptest2 int, PRIMARY KEY(ptest1, ptest2) );
+CREATE TABLE FKTABLE ( ftest1 int, ftest2 int );
+INSERT INTO PKTABLE VALUES (1, 2);
+INSERT INTO FKTABLE VALUES (1, NULL);
+ALTER TABLE FKTABLE ADD FOREIGN KEY(ftest1, ftest2) REFERENCES PKTABLE MATCH FULL;
+ERROR:  insert or update on table "fktable" violates foreign key constraint "fktable_ftest1_fkey"
+DETAIL:  MATCH FULL does not allow mixing of null and nonnull key values.
 DROP TABLE FKTABLE;
 DROP TABLE PKTABLE;
 -- MATCH SIMPLE
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 9dd5e29c75..9ff2611377 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -219,6 +219,20 @@ CREATE TABLE FKTABLE ( ftest1 int REFERENCES PKTABLE MATCH FULL, ftest2 int );
 DROP TABLE FKTABLE;
 DROP TABLE PKTABLE;
 
+--
+-- Check initial check upon ALTER TABLE
+--
+CREATE TABLE PKTABLE ( ptest1 int, ptest2 int, PRIMARY KEY(ptest1, ptest2) );
+CREATE TABLE FKTABLE ( ftest1 int, ftest2 int );
+
+INSERT INTO PKTABLE VALUES (1, 2);
+INSERT INTO FKTABLE VALUES (1, NULL);
+
+ALTER TABLE FKTABLE ADD FOREIGN KEY(ftest1, ftest2) REFERENCES PKTABLE MATCH FULL;
+
+DROP TABLE FKTABLE;
+DROP TABLE PKTABLE;
+
 
 -- MATCH SIMPLE
 
-- 
2.19.2

Alvaro Herrera

alvherre@2ndquadrant.com

about 7 years ago

In reply to: Peter Eisentraut (#1)

Re: additional foreign key test coverage

On 2018-Dec-04, Peter Eisentraut wrote:

During the development of my recent patch "unused/redundant foreign key
code" [0], I had developed a few additional test cases to increase the
coverage in ri_triggers.c. They are in the attached patches with
explanations. With these, coverage should be pretty complete, except
hard-to-trigger error cases. Interested reviewers can also follow along
on coverage.postgresql.org.

Hmm. One of the things I did for FKs on partitioned tables was remove
all the cases involving only unpartitioned tables, then run just the
foreign_key test and see what the coverage looked like -- in the first
versions, there were large swaths of uncovered code. That guided me to
add a few more tests to increase coverage in later versions. This is
all to say that I think it would be useful to include the case of
partitioned tables in the tests you add, where relevant.

--
ï¿½lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

about 7 years ago

In reply to: Alvaro Herrera (#2)

Re: additional foreign key test coverage

On 04/12/2018 14:23, Alvaro Herrera wrote:

On 2018-Dec-04, Peter Eisentraut wrote:

During the development of my recent patch "unused/redundant foreign key
code" [0], I had developed a few additional test cases to increase the
coverage in ri_triggers.c. They are in the attached patches with
explanations. With these, coverage should be pretty complete, except
hard-to-trigger error cases. Interested reviewers can also follow along
on coverage.postgresql.org.

Hmm. One of the things I did for FKs on partitioned tables was remove
all the cases involving only unpartitioned tables, then run just the
foreign_key test and see what the coverage looked like -- in the first
versions, there were large swaths of uncovered code. That guided me to
add a few more tests to increase coverage in later versions. This is
all to say that I think it would be useful to include the case of
partitioned tables in the tests you add, where relevant.

I'm not sure I understand where partitioned tables come in here. In
ri_triggers.c, it's all dealing with single base tables. Certainly
other code elsewhere needs to know about partitions.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera

alvherre@2ndquadrant.com

about 7 years ago

In reply to: Peter Eisentraut (#3)

Re: additional foreign key test coverage

On 2018-Dec-07, Peter Eisentraut wrote:

On 04/12/2018 14:23, Alvaro Herrera wrote:

Hmm. One of the things I did for FKs on partitioned tables was remove
all the cases involving only unpartitioned tables, then run just the
foreign_key test and see what the coverage looked like -- in the first
versions, there were large swaths of uncovered code. That guided me to
add a few more tests to increase coverage in later versions. This is
all to say that I think it would be useful to include the case of
partitioned tables in the tests you add, where relevant.

I'm not sure I understand where partitioned tables come in here. In
ri_triggers.c, it's all dealing with single base tables. Certainly
other code elsewhere needs to know about partitions.

Well, certain features (say, referential actions) needed some specific
code changes when FKs appeared in partitioned tables. I didn't notice
those at first, and only noticed when I added tests involving
partitioned tables. I'm just saying if you add for the simple case, you
might miss bugs when whatever feature you're covering is used with
partitioned tables.

I see one example right in your 0001 patch, where your code calls
ri_restrict. That one needs to add ONLY or not depending on
partitionedness. I think you don't need to do anything here because
the !is_no_action case is already covered for partitioned tables.

Another potential example in 0002 (and 0003): in the covered function we
do this,
if (ri_NullCheck(RelationGetDescr(pk_rel), old_row, riinfo, true) != RI_KEYS_NONE_NULL)
are we using the correct tuple descriptor? Keep in mind that partition
can have different column layout than parent. (In this case it's not a
problem, because the pk_rel is not yet allowed to be partitioned, so if
you commit this soon, it will be my problem not yours).

--
ï¿½lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Mi Tar

mmitar@gmail.com

about 7 years ago

In reply to: Alvaro Herrera (#4)

Re: additional foreign key test coverage

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

Hi!

I tested this patch and it applied cleanly and all tests passed. I haven't looked if the changes to tests are reasonable or extensive to cover all aspects of what they want to cover.

Mitar

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

almost 7 years ago

In reply to: Mi Tar (#5)

Re: additional foreign key test coverage

On 09/01/2019 09:20, Mi Tar wrote:

I tested this patch and it applied cleanly and all tests passed. I haven't looked if the changes to tests are reasonable or extensive to cover all aspects of what they want to cover.

I have committed this with additional tests for partitioned tables, as
requested by Álvaro.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services