Обсуждение: 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

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

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

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