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

On Mon, 12 Jul 2021 at 17:39, vignesh C <vignesh21@gmail.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).

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.

Thoughts?

Regards,
Dean