Thread: Fix foreign key constraint check for partitioned tables
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);
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
Attachment
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 andscanning 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 partitionedtable 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
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
Attachment
Hadi Moshayedi <hadi@moshayedi.net> writes: > [ fix-foreign-key-check.patch ] Pushed with some adjustments, as discussed over at https://postgr.es/m/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
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