Fix foreign key constraint check for partitioned tables

Started by Hadi Moshayedialmost 7 years ago6 messages
#1Hadi Moshayedi
hadi@moshayedi.net
1 attachment(s)

Yesterday while doing some tests, I noticed that the following doesn't work
properly:

create role test_role with login;
create table ref(a int primary key);
grant references on ref to test_role;
set role test_role;
create table t1(a int, b int) partition by list (a);
alter table t1 add constraint t1_b_key foreign key (b) references ref(a);

In postgres 11.2, this results in the following error:

ERROR: could not open file "base/12537/16390": No such file or directory

and in the master branch it simply crashes.

It seems that validateForeignKeyConstraint() in tablecmds.c cannot
use RI_Initial_Check() to check the foreign key constraint, so it tries to
open the relation and scan it and verify each row by a call
to RI_FKey_check_ins(). Opening and scanning the relation fails, because it
is a partitioned table and has no storage.

The attached patch fixes the problem by skipping foreign key constraint
check for relations with no storage. In partitioned table case, it will be
verified by scanning the partitions, so we are safe to skip the parent
table.

-- Hadi

Attachments:

fix-foreign-key-check.patchapplication/octet-stream; name=fix-foreign-key-check.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3183b2aaa1..f3889cbe70 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4584,6 +4584,15 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 		Relation	rel = NULL;
 		ListCell   *lcon;
 
+		/*
+		 * Foreign tables have no storage, nor do partitioned tables and
+		 * indexes.
+		 */
+		if (tab->relkind == RELKIND_FOREIGN_TABLE ||
+			tab->relkind == RELKIND_PARTITIONED_TABLE ||
+			tab->relkind == RELKIND_PARTITIONED_INDEX)
+			continue;
+
 		foreach(lcon, tab->constraints)
 		{
 			NewConstraint *con = lfirst(lcon);
#2Edmund Horner
ejrh00@gmail.com
In reply to: Hadi Moshayedi (#1)
Re: Fix foreign key constraint check for partitioned tables

On Sat, 23 Mar 2019 at 12:01, Hadi Moshayedi <hadi@moshayedi.net> wrote:

Yesterday while doing some tests, I noticed that the following doesn't work properly:

create role test_role with login;
create table ref(a int primary key);
grant references on ref to test_role;
set role test_role;
create table t1(a int, b int) partition by list (a);
alter table t1 add constraint t1_b_key foreign key (b) references ref(a);

In postgres 11.2, this results in the following error:

ERROR: could not open file "base/12537/16390": No such file or directory

and in the master branch it simply crashes.

It seems that validateForeignKeyConstraint() in tablecmds.c cannot use RI_Initial_Check() to check the foreign key constraint, so it tries to open the relation and scan it and verify each row by a call to RI_FKey_check_ins(). Opening and scanning the relation fails, because it is a partitioned table and has no storage.

The attached patch fixes the problem by skipping foreign key constraint check for relations with no storage. In partitioned table case, it will be verified by scanning the partitions, so we are safe to skip the parent table.

Hi Hadi,

I reproduced the problem and tested your fix. It looks simple and
correct to me.

I was a bit curious about the need for "set role" in the reproduction,
but I see that it's because RI_Initial_Check does some checks to see
if a simple SELECT can be used, and one of the checks is for basic
table permissions.

I wonder if the macro RELKIND_HAS_STORAGE should be used instead of
checking for each relkind? This would apply to the check on line 4405
too.

Edmund

#3Hadi Moshayedi
hadi@moshayedi.net
In reply to: Edmund Horner (#2)
1 attachment(s)
Re: Fix foreign key constraint check for partitioned tables

Hello Edmund,

Thanks for the review.

I was a bit curious about the need for "set role" in the reproduction,
but I see that it's because RI_Initial_Check does some checks to see
if a simple SELECT can be used, and one of the checks is for basic
table permissions.

I think to reproduce this the current user shouldn't be able to SELECT on
both tables, so RI_Initial_Check fails. Setting the owner of one of the
tables isn't always enough as the current user can be a super user.

I wonder if the macro RELKIND_HAS_STORAGE should be used instead of
checking for each relkind? This would apply to the check on line 4405
too.

done.

This patch also changed the output of some of tests, i.e. previously
foreign key constraint failures errored on the partitioned table itself,
but now it shows the child table's name in the error message. I hope it is
ok.

I also added a regression test which would fail without this patch.

Thanks,
Hadi

Attachments:

fix-foreign-key-check.patchapplication/octet-stream; name=fix-foreign-key-check.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3183b2aaa1..44d173b07b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4398,13 +4398,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 	{
 		AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab);
 
-		/*
-		 * Foreign tables have no storage, nor do partitioned tables and
-		 * indexes.
-		 */
-		if (tab->relkind == RELKIND_FOREIGN_TABLE ||
-			tab->relkind == RELKIND_PARTITIONED_TABLE ||
-			tab->relkind == RELKIND_PARTITIONED_INDEX)
+		if (!RELKIND_HAS_STORAGE(tab->relkind))
 			continue;
 
 		/*
@@ -4584,6 +4578,9 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 		Relation	rel = NULL;
 		ListCell   *lcon;
 
+		if (!RELKIND_HAS_STORAGE(tab->relkind))
+			continue;
+
 		foreach(lcon, tab->constraints)
 		{
 			NewConstraint *con = lfirst(lcon);
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 401514a3e0..5735264963 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1674,7 +1674,7 @@ CREATE TABLE fk_partitioned_fk_full (x int, y int) PARTITION BY RANGE (x);
 CREATE TABLE fk_partitioned_fk_full_1 PARTITION OF fk_partitioned_fk_full DEFAULT;
 INSERT INTO fk_partitioned_fk_full VALUES (1, NULL);
 ALTER TABLE fk_partitioned_fk_full ADD FOREIGN KEY (x, y) REFERENCES fk_notpartitioned_pk MATCH FULL;  -- fails
-ERROR:  insert or update on table "fk_partitioned_fk_full" violates foreign key constraint "fk_partitioned_fk_full_x_y_fkey"
+ERROR:  insert or update on table "fk_partitioned_fk_full_1" violates foreign key constraint "fk_partitioned_fk_full_x_y_fkey"
 DETAIL:  MATCH FULL does not allow mixing of null and nonnull key values.
 TRUNCATE fk_partitioned_fk_full;
 ALTER TABLE fk_partitioned_fk_full ADD FOREIGN KEY (x, y) REFERENCES fk_notpartitioned_pk MATCH FULL;
@@ -1890,7 +1890,7 @@ CREATE TABLE fk_partitioned_fk_2_2 PARTITION OF fk_partitioned_fk_2 FOR VALUES F
 INSERT INTO fk_partitioned_fk_2 VALUES (1600, 601), (1600, 1601);
 ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2
   FOR VALUES IN (1600);
-ERROR:  insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_a_b_fkey"
+ERROR:  insert or update on table "fk_partitioned_fk_2_1" violates foreign key constraint "fk_partitioned_fk_a_b_fkey"
 DETAIL:  Key (a, b)=(1600, 601) is not present in table "fk_notpartitioned_pk".
 INSERT INTO fk_notpartitioned_pk VALUES (1600, 601), (1600, 1601);
 ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2
@@ -2015,6 +2015,28 @@ alter table fkpart2.fk_part detach partition fkpart2.fk_part_1;
 alter table fkpart2.fk_part_1 drop constraint fkey;	-- ok
 alter table fkpart2.fk_part_1_1 drop constraint my_fkey;	-- doesn't exist
 ERROR:  constraint "my_fkey" of relation "fk_part_1_1" does not exist
+-- test the case when the referenced table is owned by a different user
+DROP TABLE fk_partitioned_fk;
+CREATE ROLE fk_partitioned_fk_owner;
+GRANT REFERENCES ON fk_notpartitioned_pk TO fk_partitioned_fk_owner;
+SET ROLE fk_partitioned_fk_owner;
+CREATE TABLE fk_partitioned_fk(a int, b int) PARTITION BY LIST (a);
+CREATE TABLE fk_partitioned_fk_1 PARTITION OF fk_partitioned_fk FOR VALUES IN (2048);
+INSERT INTO fk_partitioned_fk VALUES (2048, 4096);
+-- this should fail
+ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b)
+  REFERENCES fk_notpartitioned_pk(a, b);
+ERROR:  insert or update on table "fk_partitioned_fk_1" violates foreign key constraint "fk_partitioned_fk_a_b_fkey"
+DETAIL:  Key (a, b)=(2048, 4096) is not present in table "fk_notpartitioned_pk".
+-- add the missing key and retry
+RESET ROLE;
+INSERT INTO fk_notpartitioned_pk (a, b) VALUES (2048, 4096);
+-- We set back the role so we make sure current user doesn't have SELECT
+-- access to both tables.
+SET ROLE fk_partitioned_fk_owner;
+ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b)
+  REFERENCES fk_notpartitioned_pk(a, b);
+RESET ROLE;
 \set VERBOSITY terse	\\ -- suppress cascade details
 drop schema fkpart0, fkpart1, fkpart2 cascade;
 NOTICE:  drop cascades to 8 other objects
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index beeaf3277d..78c86b6277 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -1440,6 +1440,27 @@ alter table fkpart2.fk_part detach partition fkpart2.fk_part_1;
 alter table fkpart2.fk_part_1 drop constraint fkey;	-- ok
 alter table fkpart2.fk_part_1_1 drop constraint my_fkey;	-- doesn't exist
 
+-- test the case when the referenced table is owned by a different user
+DROP TABLE fk_partitioned_fk;
+CREATE ROLE fk_partitioned_fk_owner;
+GRANT REFERENCES ON fk_notpartitioned_pk TO fk_partitioned_fk_owner;
+SET ROLE fk_partitioned_fk_owner;
+CREATE TABLE fk_partitioned_fk(a int, b int) PARTITION BY LIST (a);
+CREATE TABLE fk_partitioned_fk_1 PARTITION OF fk_partitioned_fk FOR VALUES IN (2048);
+INSERT INTO fk_partitioned_fk VALUES (2048, 4096);
+-- this should fail
+ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b)
+  REFERENCES fk_notpartitioned_pk(a, b);
+-- add the missing key and retry
+RESET ROLE;
+INSERT INTO fk_notpartitioned_pk (a, b) VALUES (2048, 4096);
+-- We set back the role so we make sure current user doesn't have SELECT
+-- access to both tables.
+SET ROLE fk_partitioned_fk_owner;
+ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b)
+  REFERENCES fk_notpartitioned_pk(a, b);
+RESET ROLE;
+
 \set VERBOSITY terse	\\ -- suppress cascade details
 drop schema fkpart0, fkpart1, fkpart2 cascade;
 \set VERBOSITY default
#4Hadi Moshayedi
hadi@moshayedi.net
In reply to: Hadi Moshayedi (#3)
Re: Fix foreign key constraint check for partitioned tables

Posted this at the commitfest tool:
https://commitfest.postgresql.org/23/2075/

Show quoted text
#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Hadi Moshayedi (#3)
Re: Fix foreign key constraint check for partitioned tables

Hadi Moshayedi <hadi@moshayedi.net> writes:

[ fix-foreign-key-check.patch ]

Pushed with some adjustments, as discussed over at
/messages/by-id/19030.1554574075@sss.pgh.pa.us

This patch also changed the output of some of tests, i.e. previously
foreign key constraint failures errored on the partitioned table itself,
but now it shows the child table's name in the error message. I hope it is
ok.

Yeah, I think that's OK. Interestingly, no such changes appear in
HEAD's version of the regression test --- probably Alvaro's earlier
changes had the same effect.

I also added a regression test which would fail without this patch.

This needed a fair amount of work. You shouldn't have summarily
dropped a table that the test script specifically says it meant
to leave around, and we have a convention that role names created by
the regression test scripts always should begin with "regress_",
and you didn't clean up the role at the end (which would lead to
failures in repeated installcheck runs).

regards, tom lane

#6Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Tom Lane (#5)
Re: Fix foreign key constraint check for partitioned tables

On 2019-Apr-06, Tom Lane wrote:

Hadi Moshayedi <hadi@moshayedi.net> writes:

This patch also changed the output of some of tests, i.e. previously
foreign key constraint failures errored on the partitioned table itself,
but now it shows the child table's name in the error message. I hope it is
ok.

Yeah, I think that's OK. Interestingly, no such changes appear in
HEAD's version of the regression test --- probably Alvaro's earlier
changes had the same effect.

Yeah, they did.

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