Re: Enhanced error message to include hint messages for redundant options error

On Thu, 8 Jul 2021 at 14:40, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Thu, Jul 8, 2021 at 1:52 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> >
> > I sort of like the visual cue of seeing ereport(ERROR .. since it makes it
> > clear it will break execution then and there, this will require a lookup for
> > anyone who don't know the function by heart. That being said, reducing
> > duplicated boilerplate has clear value and this reduce the risk of introducing
> > strings which are complicated to translate. On the whole I think this is a net
> > win, and the patch looks pretty good.
> >

Bikeshedding the function name, there are several similar examples in
the existing code, but the closest analogs are probably
errorMissingColumn() and errorMissingRTE(). So I think
errorConflictingDefElem() would be better, since it's slightly more
obviously an error.

Also, I don't think this function should be marked inline -- using a
normal function ought to help make the compiled code smaller.

A bigger problem is that the patch replaces about 100 instances of the
error "conflicting or redundant options" with "option \"%s\" specified
more than once", but that's not always the appropriate thing to do.
For example, in the walsender code, the error isn't necessarily due to
the option being specified more than once.

Also, there are cases where def->defname isn't actually the name of
the option specified, so including it in the error is misleading. For
example:

CREATE OR REPLACE FUNCTION foo() RETURNS int
AS $$ SELECT 1 $$ STABLE IMMUTABLE;

ERROR: option "volatility" specified more than once
LINE 2: AS $$ SELECT 1 $$ STABLE IMMUTABLE;
^

and in this case "volatility" is an internal string, so it won't get translated.

I'm inclined to think that it isn't worth the effort trying to
distinguish between conflicting options, options specified more than
once and faked-up options that weren't really specified. If we just
make errorConflictingDefElem() report "conflicting or redundant
options", then it's much easier to update calling code without making
mistakes. The benefit then comes from the reduced code size and the
fact that the patch includes pstate in more places, so the
parser_errposition() indicator helps the user identify the problem.

In file_fdw_validator(), where there is no pstate, it's already using
"specified more than once" as a hint to clarify the "conflicting or
redundant options" error, so I think we should leave that alone.

Regards,
Dean