Re: CREATE COLLATION - check for duplicate options and error out if found one

On Fri, Jul 16, 2021 at 1:04 AM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> Having pushed [1], I started looking at this, and I think it's mostly
> in good shape.

Thanks a lot for taking a look at this.

> Since we're improving the CREATE COLLATION errors, I think it's also
> worth splitting out the error for LOCALE + LC_COLLATE/LC_CTYPE from
> the error for FROM + any other option.
>
> In the case of LOCALE + LC_COLLATE/LC_CTYPE, there is an identical
> error in CREATE DATABASE, so we should use the same error message and
> detail text here.
>
> Then logically, FROM + any other option should have an error of the
> same general form.
>
> And finally, it then makes sense to make the other errors follow the
> same pattern (with the "specified more than once" text in the detail),
> which is also where we ended up in the discussion over in [1].
>
> So, attached is what I propose.

Here are some comments:

1) Duplicate option check for "FROM" option is unnecessary and will be
a dead code. Because the syntaxer anyways would catch if FROM is
specified more than once, something like CREATE COLLATION mycoll1 FROM
FROM "C";.
+ {
+ if (fromEl)
+ errorDuplicateDefElem(defel, pstate);
defelp = &fromEl;

And we might think to catch below errors:

postgres=# CREATE COLLATION coll_dup_chk (FROM = "C", FROM = "C",
VERSION = "1");
ERROR: conflicting or redundant options
LINE 1: CREATE COLLATION coll_dup_chk (FROM = "C", FROM = "C", VERSI...
^
DETAIL: Option "from" specified more than once.

But IMO, the above should fail with:

postgres=# CREATE COLLATION coll_dup_chk (FROM = "C", FROM = "C",
VERSION = "1");
ERROR: conflicting or redundant options
DETAIL: FROM cannot be specified together with any other options.

2) I don't understand the difference between errorConflictingDefElem
and errorDuplicateDefElem. Isn't the following statement "This should
only be used if defel->defname is guaranteed to match the user-entered
option name"
true with errorConflictingDefElem? I mean, aren't we calling
errorConflictingDefElem only if the defel->defname is guaranteed to
match the user-entered option name? I don't see much use of
errdetail("Option \"%s\" specified more than once.", defel->defname),
in errorDuplicateDefElem when we have pstate and that sort of points
to the option that's specified more than once.
+
+/*
+ * Raise an error about a duplicate DefElem.
+ *
+ * This is similar to errorConflictingDefElem(), except that it is intended for
+ * an option that the user explicitly specified more than once. This should
+ * only be used if defel->defname is guaranteed to match the user-entered
+ * option name, otherwise the detail text might be confusing.
+ */

I personally don't like the new function errorDuplicateDefElem as it
doesn't add any value given the presence of pstate.

Regards,
Bharath Rupireddy.