Re: Enhanced error message to include hint messages for redundant options error - Mailing list pgsql-hackers

On Mon, Apr 26, 2021 at 8:06 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Apr-26, Bharath Rupireddy wrote:
>
> > Thanks! IMO, it is better to change the error message to "option
> > \"%s\" specified more than once" instead of adding an error detail.
> > Let's hear other hackers' opinions.
>
> Many other places have the message "conflicting or redundant options",
> and then parser_errposition() shows the problem option.  That seems
> pretty unhelpful, so whenever the problem is the redundancy I would have
> the message be explicit about that, and about which option is the
> problem:
>   errmsg("option \"%s\" specified more than once", "someopt")
> Do note that wording it this way means only one translatable message,
> not dozens.

I agree that we can just be clear about the problem. Looks like the
majority of the errors "conflicting or redundant options" are for
redundant options. So, wherever "conflicting or redundant options"
exists: 1) change the message to "option \"%s\" specified more than
once" and remove parser_errposition if it's there because the option
name in the error message would give the info with which user can
point to the location 2) change the message to something like "option
\"%s\" is conflicting with option \"%s\"".

> In some cases it is possible that you'd end up with two messages, one
> for "redundant" and one for the other ways for options to conflict with
> others; for example collationcmds.c has one that's not as obvious, and

And yes, we need to divide up the message for conflicting and
redundant options on a case-to-case basis.

In createdb: we just need to modify the error message to "conflicting
options" or we could just get rid of errdetail and have the error
message directly saying "LOCALE cannot be specified together with
LC_COLLATE or LC_CTYPE". Redundant options are just caught in the
above for loop in createdb.
    if (dlocale && (dcollate || dctype))
        ereport(ERROR,
                (errcode(ERRCODE_SYNTAX_ERROR),
                 errmsg("conflicting or redundant options"),
                 errdetail("LOCALE cannot be specified together with
LC_COLLATE or LC_CTYPE.")));

In AlterDatabase: we can remove parser_errposition because the option
name in the error message would give the right information.
        if (list_length(stmt->options) != 1)
            ereport(ERROR,
                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                     errmsg("option \"%s\" cannot be specified with
other options",
                            dtablespace->defname),
                     parser_errposition(pstate, dtablespace->location)));

In compute_common_attribute: we can remove goto duplicate_error and
have the message change to "option \"%s\" specified more than once".

In DefineType: we need to rework for loop.
I found another problem with collationcmds.c is that it doesn't error
out if some of the options are specified more than once, something
like below. I think the option checking "for loop" in DefineCollation
needs to be reworked.
CREATE COLLATION case_insensitive (provider = icu, provider =
someother locale = '@colStrength=secondary', deterministic = false,
deterministic = true);

> force_quote/force_quote_all in COPY have their own thing too.

We can remove the errhint for force_not_null and force_null along with
the error message wording change to "option \"%s\" specified more than
once".

Upon looking at error "conflicting or redundant options" instances, to
do the above we need a lot of code changes, I'm not sure that will be
acceptable.

One thing is that all the option checking for loops are doing these
things in common: 1) fetching the values bool, int, float, string of
the options 2) redundant checking. I feel we need to invent a common
API to which we pass in 1) a list of allowed options for a particular
command, we can have these as static data structure
{allowed_option_name, data_type}, 2) a list of user specified options
3) the API will return a list of fetched i.e. parsed values
{user_specified_option_name, data_type, value}. Maybe the API can
return a hash table of these values so that the callers can look up
faster for the required option. The advantage of this API is that we
don't need to have many for-loops for options checking in the code.
I'm not sure it is worth doing though. Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com