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

On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> On Mon, 12 Jul 2021 at 17:39, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > Thanks for your comments, I have made the changes for the same in the
> > V10 patch attached.
> > Thoughts?
> >
>
> I'm still not happy about changing so many error messages.
>
> Some of the changes might be OK, but aren't strictly necessary. For example:
>
> COPY x from stdin (force_not_null (a), force_not_null (b));
> -ERROR: conflicting or redundant options
> +ERROR: option "force_not_null" specified more than once
> LINE 1: COPY x from stdin (force_not_null (a), force_not_null (b));
> ^
>
> I actually prefer the original primary error message, for consistency
> with other similar cases, and I think the error position indicator is
> sufficient to identify the problem. If it were to include the
> "specified more than once" text, I would put that in DETAIL.
>
> Other changes are wrong though. For example:
>
> COPY x from stdin (format CSV, FORMAT CSV);
> -ERROR: conflicting or redundant options
> +ERROR: redundant options specified
> LINE 1: COPY x from stdin (format CSV, FORMAT CSV);
> ^
>
> The problem here is that the code that throws this error throws the
> same error if the second format is different, which would make it a
> conflicting option, not a redundant one. And I don't think we should
> add more code to test whether it's conflicting or redundant, so again,
> I think we should just keep the original error message.
>
> Similarly, this error is wrong:
>
> CREATE OR REPLACE FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STABLE IMMUTABLE;
> ERROR: redundant options specified
> LINE 1: ...NCTION foo() RETURNS int AS $$ SELECT 1 $$ STABLE IMMUTABLE;
> ^
>
> And even this error:
>
> CREATE OR REPLACE FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT STRICT;
> ERROR: redundant options specified
> LINE 1: ... FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT STRICT;
> ^
>
> which looks OK, is actually problematic because the same code also
> handles the alternate syntax, which leads to this (which is now wrong
> because it's conflicting not redundant):
>
> CREATE OR REPLACE FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT
> CALLED ON NULL INPUT;
> ERROR: redundant options specified
> LINE 1: ...NCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT CALLED ON ...
> ^
>
> The problem is it's actually quite hard to decide in each case whether
> the option is redundant or conflicting. Sometimes, it might look
> obvious in the code, but actually be much more subtle, due to an
> earlier transformation of the grammar. Likewise redundant doesn't
> necessarily mean literally specified more than once.
>
> Also, most of these don't have regression test cases, and I'm very
> reluctant to change them without proper testing, and that would make
> the patch much bigger. To me, this patch is already attempting to
> change too much in one go, which is causing problems.
>
> So I suggest a more incremental approach, starting by keeping the
> original error message, but improving it where possible with the error
> position. Then maybe move on to look at specific cases that can be
> further improved with additional detail (keeping the same primary
> error message, for consistency).

I'm fine with this approach as we do not have tests to cover all the
error conditions, also I'm not sure if it is worth adding tests for
all the error conditions and as the patch changes a large number of
error conditions, an incremental approach is better.

> Here is an updated version, following that approach. It does the following:
>
> 1). Keeps the same primary error message ("conflicting or redundant
> options") in all cases.
>
> 2). Uses errorConflictingDefElem() to throw it, to ensure consistency
> and reduce the executable size.
>
> 3). Includes your enhancements to make the ParseState available in
> more places, so that the error position indicator is shown to indicate
> the cause of the error.
>
> IMO, this makes for a much safer incremental change, that is more committable.
>
> As it turns out, there are 110 cases of this error that now use
> errorConflictingDefElem(), and of those, just 10 (in 3 functions)
> don't have a ParseState readily available to them:
>
> - ATExecSetIdentity()
> - parse_output_parameters() x5
> - parseCreateReplSlotOptions() x4
>
> It might be possible to improve those (and possibly some of the others
> too) by adding some appropriate DETAIL to the error, but as I said, I
> suggest doing that in a separate follow-on patch, and only with
> careful analysis and testing of each case.
>
> As it stands, the improvements from (3) seem quite worthwhile. Also,
> the patch saves a couple of hundred lines of code, and for me an
> optimised executable is around 30 kB smaller, which is more than I
> expected.

Agreed, it can be handled as part of the 2nd patch. The changes you
made apply neatly and the test passes.

Regards,
Vignesh