From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@lists.postgresql.org, Hadi Moshayedi <hadi@moshayedi.net>
Subject: tableam scan-API patch broke foreign key validation
Date: Sat, 06 Apr 2019 14:07:55 -0400
Message-ID: <19030.1554574075@sss.pgh.pa.us> (raw)
It seems that the fire-the-triggers code path in
validateForeignKeyConstraint isn't being exercised; at least, that's
what coverage.postgresql.org says right now, and I'm afraid that may
have been true for quite some time. The attached regression-test
addition causes it to be exercised, and guess what: it blows up real
good.
This is a slightly adapted version of the test Hadi proposed in
https://postgr.es/m/CAK=1=WonwcuN_0KiZwQO3SQxse41jZ5hOJRpFCvZ3qa8n9cssw@mail.gmail.com
Since he didn't mention anything about core dumps or assertion
failures, one assumes that it did work as of the version he was
testing against.
What it looks like to me is that because of this hunk in c2fe139c2:
@@ -8962,7 +8981,8 @@ validateForeignKeyConstraint(char *conname,
trigdata.type = T_TriggerData;
trigdata.tg_event = TRIGGER_EVENT_INSERT | TRIGGER_EVENT_ROW;
trigdata.tg_relation = rel;
- trigdata.tg_trigtuple = tuple;
+ trigdata.tg_trigtuple = ExecFetchSlotHeapTuple(slot, true, NULL);
+ trigdata.tg_trigslot = slot;
trigdata.tg_newtuple = NULL;
trigdata.tg_trigger = &trig;
validateForeignKeyConstraint asks ExecFetchSlotHeapTuple to materialize
the tuple, which causes it to no longer be associated with a buffer,
which causes heapam_tuple_satisfies_snapshot to be very unhappy.
I can make the case not crash by s/true/false/ in the above call,
but I wonder whether that's an appropriate fix. It seems rather
fragile that things work like this.
I plan to go ahead and commit Hadi's fix with that change included
(as below), but I wonder whether anything else needs to be revisited.
regards, tom lane
Attachments:
[text/x-diff] fix-foreign-key-check-3.patch (4.9K, ../19030.1554574075@sss.pgh.pa.us/2-fix-foreign-key-check-3.patch)
download | inline diff:diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.cindex e842f91..31fe40a 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.outindex 620eb43..e62b220 100644--- a/src/test/regress/expected/foreign_key.out+++ b/src/test/regress/expected/foreign_key.out@@ -1895,6 +1895,30 @@ 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 values (2048, 4096);+-- 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, 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);+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.sqlindex 1a86850..090bd09 100644--- a/src/test/regress/sql/foreign_key.sql+++ b/src/test/regress/sql/foreign_key.sql@@ -1365,6 +1365,29 @@ 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 values (2048, 4096);+-- this should fail+alter table other_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);+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.
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, andres@anarazel.de, hadi@moshayedi.net
Subject: Re: tableam scan-API patch broke foreign key validation
In-Reply-To: <19030.1554574075@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox