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

On Sat, Jul 10, 2021 at 4:14 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> 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.
>

Ok, I will change it to keep it similar.

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

inline instructs the compiler to attempt to embed the function content
into the calling code instead of executing an actual call. I think we
should keep it inline to reduce the function call.

> 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.
>

This patch intended to change "conflicting or redundant options" to
"option \"%s\" specified more than once" only in case that error is
for option specified more than once. This change is not required. I
will remove it.

> 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.

This patch intended to change "conflicting or redundant options" to
"option \"%s\" specified more than once" only in case that error is
for option specified more than once. Thanks for pointing out a few
places where the actual error "conflicting or redundant options"
should be left as it is. I will post a new patch which will remove the
conflicting options error scenarios, which were not targeted in this
patch.

Regards,
Vignesh