Thread: 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 be > > ereport(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
Attachment
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 be > > > > ereport(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
Attachment
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
Attachment
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
Attachment
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
On Mon, Oct 08, 2018 at 08:40:49AM +0200, Laurenz Albe wrote: > I'm fine with it. Thanks, I have pushed this version and back-patched to v11. -- Michael