Unclear error message
In backend/commands/tablecmds.c, function ATAddForeignKeyConstraint, I find:
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
if (!recurse)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("foreign key referencing partitioned table \"%s\" must not be ONLY",
RelationGetRelationName(pkrel))));
That message is wrong, because "rel" and not "pkrel" is the partitioned table.
I think it should be
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("foreign key cannot be defined on ONLY \"%s\" for a partitioned table",
Relatio
nGetRelationName(rel))));
Yours,
Laurenz Albe
On Thu, Sep 20, 2018 at 09:45:09PM +0200, Laurenz Albe wrote:
That message is wrong, because "rel" and not "pkrel" is the partitioned table.
I think it should beereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("foreign key cannot be defined on ONLY \"%s\" for a partitioned table",
Relatio
nGetRelationName(rel))));
Hmm... There are no test cases for this error message, nor for the one
below "cannot add NOT VALID foreign key to relation foo", which is a bad
idea.
The referenced relation has to be RELKIND_RELATION. Here is another
idea of error message:
cannot use ONLY on partitioned table "foo" with foreign key
--
Michael
Michael Paquier wrote:
On Thu, Sep 20, 2018 at 09:45:09PM +0200, Laurenz Albe wrote:
That message is wrong, because "rel" and not "pkrel" is the partitioned table.
I think it should beereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("foreign key cannot be defined on ONLY \"%s\" for a partitioned table",
Relatio
nGetRelationName(rel))));Hmm... There are no test cases for this error message, nor for the one
below "cannot add NOT VALID foreign key to relation foo", which is a bad
idea.The referenced relation has to be RELKIND_RELATION. Here is another
idea of error message:
cannot use ONLY on partitioned table "foo" with foreign key
True; how about the attached?
I think this should go in before v11.
Yours,
Laurenz Albe
Attachments:
0001-Fix-an-error-message.patchtext/x-patch; charset=UTF-8; name=0001-Fix-an-error-message.patchDownload
From d305cfe821decbe9ff113626c945a521fb55191c Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Sat, 6 Oct 2018 23:13:39 +0200
Subject: [PATCH] Fix an error message
The message for creating a foreign key on a partitioned
table using ONLY was wrong.
Also, there were regression tests missing for this and another
error message.
Laurenz Albe, reviewed by Michael Paquier
---
src/backend/commands/tablecmds.c | 4 ++--
src/test/regress/expected/foreign_key.out | 7 +++++++
src/test/regress/sql/foreign_key.sql | 5 +++++
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c145385f84..b3050ee95a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7365,8 +7365,8 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
if (!recurse)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("foreign key referencing partitioned table \"%s\" must not be ONLY",
- RelationGetRelationName(pkrel))));
+ errmsg("cannot define foreign key on partitioned table \"%s\" with ONLY",
+ RelationGetRelationName(rel))));
if (fkconstraint->skip_validation && !fkconstraint->initially_valid)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index fc3bbe4deb..e423b8ffb2 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1465,6 +1465,13 @@ CREATE TABLE fk_partitioned_fk_3_0 PARTITION OF fk_partitioned_fk_3 FOR VALUES W
CREATE TABLE fk_partitioned_fk_3_1 PARTITION OF fk_partitioned_fk_3 FOR VALUES WITH (MODULUS 5, REMAINDER 1);
ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_3
FOR VALUES FROM (2000,2000) TO (3000,3000);
+-- but creating a foreign key with ONLY on a partitioned table doesn't work
+ALTER TABLE ONLY fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk;
+ERROR: cannot define foreign key on partitioned table "fk_partitioned_fk" with ONLY
+-- also, adding a NOT VALID foreign key should fail
+ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID;
+ERROR: cannot add NOT VALID foreign key to relation "fk_notpartitioned_pk"
+DETAIL: This feature is not yet supported on partitioned tables.
-- these inserts, targetting both the partition directly as well as the
-- partitioned table, should all fail
INSERT INTO fk_partitioned_fk (a,b) VALUES (500, 501);
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index d2cecdf4eb..85540a0fae 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -1106,6 +1106,11 @@ CREATE TABLE fk_partitioned_fk_3_1 PARTITION OF fk_partitioned_fk_3 FOR VALUES W
ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_3
FOR VALUES FROM (2000,2000) TO (3000,3000);
+-- but creating a foreign key with ONLY on a partitioned table doesn't work
+ALTER TABLE ONLY fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk;
+-- also, adding a NOT VALID foreign key should fail
+ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID;
+
-- these inserts, targetting both the partition directly as well as the
-- partitioned table, should all fail
INSERT INTO fk_partitioned_fk (a,b) VALUES (500, 501);
--
2.17.1
On Sat, Oct 06, 2018 at 11:16:09PM +0200, Laurenz Albe wrote:
True; how about the attached?
I think this should go in before v11.
Thanks for adding regression tests for that.
- errmsg("foreign key referencing partitioned table \"%s\" must not be ONLY",
- RelationGetRelationName(pkrel))));
Here is a counter-proposal:
"cannot use ONLY for foreign key on partitioned table \"%s\" referencing
relation \"%s\""
+-- also, adding a NOT VALID foreign key should fail
+ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID;
+ERROR: cannot add NOT VALID foreign key to relation "fk_notpartitioned_pk"
+DETAIL: This feature is not yet supported on partitioned tables.This error should mention "fk_partitioned_fk", and not
"fk_notpartitioned_pk", no? The foreign key is added to the former, not
the latter.
--
Michael
On Sun, Oct 07, 2018 at 05:14:30PM +0900, Michael Paquier wrote:
Here is a counter-proposal:
"cannot use ONLY for foreign key on partitioned table \"%s\" referencing
relation \"%s\""+-- also, adding a NOT VALID foreign key should fail +ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID; +ERROR: cannot add NOT VALID foreign key to relation "fk_notpartitioned_pk" +DETAIL: This feature is not yet supported on partitioned tables.This error should mention "fk_partitioned_fk", and not
"fk_notpartitioned_pk", no? The foreign key is added to the former, not
the latter.
And after some more study, I finish with the attached. Thoughts?
--
Michael
Attachments:
fk-partition-error.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 0a5eb68eb6..8554c0a3cb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7365,12 +7365,14 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
if (!recurse)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("foreign key referencing partitioned table \"%s\" must not be ONLY",
+ errmsg("cannot use ONLY for foreign key on partitioned table \"%s\" referencing relation \"%s\"",
+ RelationGetRelationName(rel),
RelationGetRelationName(pkrel))));
if (fkconstraint->skip_validation && !fkconstraint->initially_valid)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot add NOT VALID foreign key to relation \"%s\"",
+ errmsg("cannot add NOT VALID foreign key on partitioned relation \"%s\" referencing relation \"%s\"",
+ RelationGetRelationName(rel),
RelationGetRelationName(pkrel)),
errdetail("This feature is not yet supported on partitioned tables.")));
}
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index fc3bbe4deb..a1dbfabdd6 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1465,6 +1465,13 @@ CREATE TABLE fk_partitioned_fk_3_0 PARTITION OF fk_partitioned_fk_3 FOR VALUES W
CREATE TABLE fk_partitioned_fk_3_1 PARTITION OF fk_partitioned_fk_3 FOR VALUES WITH (MODULUS 5, REMAINDER 1);
ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_3
FOR VALUES FROM (2000,2000) TO (3000,3000);
+-- but creating a foreign key with ONLY on a partitioned table doesn't work
+ALTER TABLE ONLY fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk;
+ERROR: cannot use ONLY for foreign key on partitioned table "fk_partitioned_fk" referencing relation "fk_notpartitioned_pk"
+-- also, adding a NOT VALID foreign key should fail
+ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID;
+ERROR: cannot add NOT VALID foreign key on partitioned relation "fk_partitioned_fk" referencing relation "fk_notpartitioned_pk"
+DETAIL: This feature is not yet supported on partitioned tables.
-- these inserts, targetting both the partition directly as well as the
-- partitioned table, should all fail
INSERT INTO fk_partitioned_fk (a,b) VALUES (500, 501);
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index d2cecdf4eb..85540a0fae 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -1106,6 +1106,11 @@ CREATE TABLE fk_partitioned_fk_3_1 PARTITION OF fk_partitioned_fk_3 FOR VALUES W
ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_3
FOR VALUES FROM (2000,2000) TO (3000,3000);
+-- but creating a foreign key with ONLY on a partitioned table doesn't work
+ALTER TABLE ONLY fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk;
+-- also, adding a NOT VALID foreign key should fail
+ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID;
+
-- these inserts, targetting both the partition directly as well as the
-- partitioned table, should all fail
INSERT INTO fk_partitioned_fk (a,b) VALUES (500, 501);
Michael Paquier wrote:
On Sun, Oct 07, 2018 at 05:14:30PM +0900, Michael Paquier wrote:
Here is a counter-proposal:
"cannot use ONLY for foreign key on partitioned table \"%s\" referencing
relation \"%s\""+-- also, adding a NOT VALID foreign key should fail +ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk NOT VALID; +ERROR: cannot add NOT VALID foreign key to relation "fk_notpartitioned_pk" +DETAIL: This feature is not yet supported on partitioned tables.This error should mention "fk_partitioned_fk", and not
"fk_notpartitioned_pk", no? The foreign key is added to the former, not
the latter.And after some more study, I finish with the attached. Thoughts?
I'm fine with it.
"cannot use ONLY for foreign key on partitioned table" has a funny ring
to it (after all, ONLY was used for the table, not the foreign key), but
since I could not come up with anything better, I guess there is just
no entirely satisfactory way to phrase it tersely.
Yours,
Laurenz Albe