On Tue, Apr 14, 2020 at 7:02 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> On 2020-Apr-13, Robert Haas wrote:
> > + ereport(ERROR,
> > + (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> > + errmsg("action cannot be performed on relation \"%s\"",
> > + RelationGetRelationName(rel)),
> >
> > Super-vague.
>
> Maybe, but note that the patch proposed to replace this current error
> message:
> ERROR: foo is not an index or foreign table
> with
> ERROR: action cannot be performed on "foo"
> DETAIL: "foo" is a materialized view.
Sure, but the point is that this case is not improved nearly as much
as most of the others. In a whole bunch of cases, he made the error
message describe the attempted operation, but here he didn't. I'm not
saying that makes it worse than what we had before, just that it would
be better if we could make this look like the other cases the patch
also changes.
> This could be improved if we had stringification of ALTER TABLE
> subcommand types:
>
> ERROR: ALTER TABLE ... ADD COLUMN cannot be performed on "foo"
> DETAIL: "foo" is a gummy bear.
> or
> ERROR: ALTER TABLE ... ADD COLUMN cannot be performed on foo
> DETAIL: This action cannot be performed on gummy bears.
>
> but that seems material for a different patch.
Even without that, you could at least say "this form of ALTER TABLE is
not supported for foo" or something like that.
I'm not trying to block the patch. I think it's a good patch. I was
just making an observation about some parts of it where it seems like
we could try slightly harder to do better.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company