From: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
---|---|
To: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> |
Cc: | vignesh C <vignesh21(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: CREATE COLLATION - check for duplicate options and error out if found one |
Date: | 2021-07-16 05:40:14 |
Message-ID: | CALj2ACXChAksgEDneF=qUiaqSG6Oex8LpNCVULvs_ZhSRuPSyw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
From | Date | Subject | |
---|---|---|---|
Next Message | Vladimir Sitnikov | 2021-07-16 05:44:06 | Re: speed up verifying UTF-8 |
Previous Message | Michael Paquier | 2021-07-16 05:08:57 | Re: Introduce pg_receivewal gzip compression tests |