Re: message style

Hi,

On 2019-04-30 10:58:13 -0400, Alvaro Herrera wrote:
> I have this two message patches that I've been debating with myself
> about:
>
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -1282,7 +1282,7 @@ heap_getnext(TableScanDesc sscan, ScanDirection direction)
> if (unlikely(sscan->rs_rd->rd_tableam != GetHeapamTableAmRoutine()))
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> - errmsg("only heap AM is supported")));
> + errmsg("only heap table access method is supported")));
>
>
> I think the original is not great

Agreed.

> but I'm not sure that the new is much
> better either. I think this message says "only AMs that behave using
> the heapam routines are supported"; we cannot say use the literal
> "heapam" AM name because, as the comment two lines above says, it's
> possible to copy the AM with a different name and it would be
> acceptable.

I'm not sure that's something worth being bothered about - the only
reason to do that is for testing. I don't think that needs to be
refelected in error messages.

> OTOH maybe this code will not survive for long, so it
> doesn't matter much that the message is 100% correct; perhaps we should
> just change errmsg() to errmsg_internal() and be done with it.

I'd suspect some of them will survive for a while. What should a heap
specific pageinspect function do if not called for heap etc?

> diff --git a/src/backend/access/table/tableamapi.c b/src/backend/access/table/tableamapi.c
> index 0053dc95cab..c8b7598f785 100644
> --- a/src/backend/access/table/tableamapi.c
> +++ b/src/backend/access/table/tableamapi.c
> @@ -103,7 +103,8 @@ check_default_table_access_method(char **newval, void **extra, GucSource source)
> {
> if (**newval == '\0')
> {
> - GUC_check_errdetail("default_table_access_method may not be empty.");
> + GUC_check_errdetail("%s may not be empty.",
> + "default_table_access_method");
> return false;
> }
>
> My problem here is not really the replacement of the name to %s, but the
> "may not be" part of it. We don't use "may not be" anywhere else; most
> places seem to use "foo cannot be X" and a small number of other places
> use "foo must not be Y". I'm not able to judge which of the two is
> better (so change all messages to use that form), or if there's a
> semantic difference and if so which one to use in this case.

No idea about what's better here either. I don't think there's an
intentional semantic difference.

Thanks for looking at this!

Greetings,

Andres Freund