From 46e3442c9ec858071d60a1c0fae2e9868aeaa0c8 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 6 Apr 2019 15:09:09 -0400 Subject: [PATCH] Fix failures in validateForeignKeyConstraint's slow path. The foreign-key-checking loop in ATRewriteTables failed to ignore relations without storage (e.g., partitioned tables), unlike the initial loop. This accidentally worked as long as RI_Initial_Check succeeded, which it does in most practical cases (including all the ones exercised in the existing regression tests :-(). However, if that failed, as for instance when there are permissions issues, then we entered the slow fire-the-trigger-on-each-tuple path. And that would try to read from the referencing relation, and fail if it lacks storage. A second problem, recently introduced in HEAD, was that this loop had been broken by sloppy refactoring for the tableam API changes. Repair both issues, and add a regression test case so we have some coverage on this code path. Back-patch as needed to v11. (It looks like this code could do with additional bulletproofing, but let's get a working test case in place first.) Hadi Moshayedi, Tom Lane, Andres Freund Discussion: https://postgr.es/m/CAK=1=WrnNmBbe5D9sm3t0a6dnAq3cdbF1vXY816j1wsMqzC8bw@mail.gmail.com Discussion: https://postgr.es/m/19030.1554574075@sss.pgh.pa.us Discussion: https://postgr.es/m/20190325180405.jytoehuzkeozggxx%40alap3.anarazel.de --- src/backend/commands/tablecmds.c | 15 ++++++------- src/test/regress/expected/foreign_key.out | 26 +++++++++++++++++++++++ src/test/regress/sql/foreign_key.sql | 25 ++++++++++++++++++++++ 3 files changed, 58 insertions(+), 8 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index e842f9152b..31fe40aa17 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -4459,13 +4459,8 @@ 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) + /* Relations without storage may be ignored here */ + if (!RELKIND_HAS_STORAGE(tab->relkind)) continue; /* @@ -4645,6 +4640,10 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode) Relation rel = NULL; ListCell *lcon; + /* Relations without storage may be ignored here too */ + if (!RELKIND_HAS_STORAGE(tab->relkind)) + continue; + foreach(lcon, tab->constraints) { NewConstraint *con = lfirst(lcon); @@ -9647,7 +9646,7 @@ validateForeignKeyConstraint(char *conname, trigdata.type = T_TriggerData; trigdata.tg_event = TRIGGER_EVENT_INSERT | TRIGGER_EVENT_ROW; trigdata.tg_relation = rel; - trigdata.tg_trigtuple = ExecFetchSlotHeapTuple(slot, true, NULL); + trigdata.tg_trigtuple = ExecFetchSlotHeapTuple(slot, false, NULL); trigdata.tg_trigslot = slot; trigdata.tg_newtuple = NULL; trigdata.tg_trigger = &trig; diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index 620eb4392b..3dd33f6aa1 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -1895,6 +1895,32 @@ INSERT INTO fk_notpartitioned_pk VALUES (1600, 601), (1600, 1601); ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES IN (1600); -- leave these tables around intentionally +-- test the case when the referenced table is owned by a different user +create role regress_other_partitioned_fk_owner; +grant references on fk_notpartitioned_pk to regress_other_partitioned_fk_owner; +set role regress_other_partitioned_fk_owner; +create table other_partitioned_fk(a int, b int) partition by list (a); +create table other_partitioned_fk_1 partition of other_partitioned_fk + for values in (2048); +insert into other_partitioned_fk + select 2048, x from generate_series(1,10) x; +-- this should fail +alter table other_partitioned_fk add foreign key (a, b) + references fk_notpartitioned_pk(a, b); +ERROR: insert or update on table "other_partitioned_fk_1" violates foreign key constraint "other_partitioned_fk_a_b_fkey" +DETAIL: Key (a, b)=(2048, 1) is not present in table "fk_notpartitioned_pk". +-- add the missing keys and retry +reset role; +insert into fk_notpartitioned_pk (a, b) + select 2048, x from generate_series(1,10) x; +set role regress_other_partitioned_fk_owner; +alter table other_partitioned_fk add foreign key (a, b) + references fk_notpartitioned_pk(a, b); +-- clean up +drop table other_partitioned_fk; +reset role; +revoke all on fk_notpartitioned_pk from regress_other_partitioned_fk_owner; +drop role regress_other_partitioned_fk_owner; -- Test creating a constraint at the parent that already exists in partitions. -- There should be no duplicated constraints, and attempts to drop the -- constraint in partitions should raise appropriate errors. diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index 1a8685099e..672fce317f 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -1365,6 +1365,31 @@ ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 -- leave these tables around intentionally +-- test the case when the referenced table is owned by a different user +create role regress_other_partitioned_fk_owner; +grant references on fk_notpartitioned_pk to regress_other_partitioned_fk_owner; +set role regress_other_partitioned_fk_owner; +create table other_partitioned_fk(a int, b int) partition by list (a); +create table other_partitioned_fk_1 partition of other_partitioned_fk + for values in (2048); +insert into other_partitioned_fk + select 2048, x from generate_series(1,10) x; +-- this should fail +alter table other_partitioned_fk add foreign key (a, b) + references fk_notpartitioned_pk(a, b); +-- add the missing keys and retry +reset role; +insert into fk_notpartitioned_pk (a, b) + select 2048, x from generate_series(1,10) x; +set role regress_other_partitioned_fk_owner; +alter table other_partitioned_fk add foreign key (a, b) + references fk_notpartitioned_pk(a, b); +-- clean up +drop table other_partitioned_fk; +reset role; +revoke all on fk_notpartitioned_pk from regress_other_partitioned_fk_owner; +drop role regress_other_partitioned_fk_owner; + -- Test creating a constraint at the parent that already exists in partitions. -- There should be no duplicated constraints, and attempts to drop the -- constraint in partitions should raise appropriate errors.