Re: CREATE COLLATION - check for duplicate options and error out if found one
On Sat, 17 Jul 2021 at 05:24, Bharath Rupireddy wrote: > > I also think that if it is specified as CREATE FUNCTION ... STRICT > STRICT, CREATE FUNCTION ... CALLED ON NULL INPUT RETURNS NULL ON NULL > INPUT etc. isn't the syntaxer catching that error while parsing the > SQL text, similar to CREATE COLLATION mycoll1 FROM FROM "C";? No, they're processed quite differently. The initial parsing of CREATE FUNCTION allows an arbitrary list of things like STRICT, CALLED ON NULL INPUT, etc., which it turns into a list of DefElem that is only checked later on. That's the most natural way to do it, since this is really just a list of options that can appear in any order, much like the version of CREATE COLLATION that allows options in parentheses, which is quite different from the version that takes a single FROM. Reading the relevant portions of gram.y is probably the easiest way to understand it. It's actually quite instructive to search for "makeDefElem" in gram.y, and see all the places that create a DefElem that doesn't match the user-entered syntax. There are quite a few of them, and there may be others elsewhere. Regards, Dean
Re: CREATE COLLATION - check for duplicate options and error out if found one
On Fri, 16 Jul 2021 at 12:17, Dean Rasheed wrote: > > On Fri, 16 Jul 2021 at 10:26, Bharath Rupireddy > wrote: > > > > Thanks. The v5 patch LGTM. > > OK, I'll push that in a while. > Pushed, with some additional tidying up. In particular, I decided it was neater to follow the style of the code in typecmds.c, and just do a single check for duplicates at the end of the loop, since that makes for a significantly smaller patch, with less code duplication. That, of course, means duplicate "from" options are handled the same as any other option, but that's arguably more consistent, and not particularly important anyway, since it's not a documented syntax. Regards, Dean
Re: CREATE COLLATION - check for duplicate options and error out if found one
On Fri, Jul 16, 2021 at 4:47 PM Dean Rasheed wrote: > > On Fri, 16 Jul 2021 at 10:26, Bharath Rupireddy > wrote: > > > > Thanks. The v5 patch LGTM. > > OK, I'll push that in a while. Thanks. > There are a few cases where def->defname isn't necessarily the name > that was specified by the user (e.g., "volatility", "strict", > "format", and probably more cases not spotted in the other thread, > which was only a cursory review), and it would be quite onerous to go > through every one of the 100+ places in the code where this error is > raised to check them all. 2bfb50b3df was more about making pstate > available in more places to add location information to existing > errors, and did not want the risk of changing and possibly worsening > existing errors. > > I think it's preferable to have a single consistent primary error > message for all these cases. I wouldn't really want "CREATE FUNCTION > ... STRICT STRICT" to throw a different error from "CREATE FUNCTION > ... LEAKPROOF LEAKPROOF", but saying "option \"strict\" specified more > than once" would be odd for "CREATE FUNCTION ... CALLED ON NULL INPUT > RETURNS NULL ON NULL INPUT", which is indistinguishable from "STRICT > STRICT" in the code. > > In any case, as you said before, the location is sufficient to > identify the offending option. Making this kind of change would > require going through all 100+ callers quite carefully, and a lot more > testing. I'm not really convinced that it's worth it. Thanks for the examples. I get it. It doesn't make sense to show "option "foo" specified more than once" for the cases where "foo" is not necessarily an option that's specified in a WITH clause of a statement, but it can be something like CREATE FUNCTION ... foo foo, like you quoted above. I also think that if it is specified as CREATE FUNCTION ... STRICT STRICT, CREATE FUNCTION ... CALLED ON NULL INPUT RETURNS NULL ON NULL INPUT etc. isn't the syntaxer catching that error while parsing the SQL text, similar to CREATE COLLATION mycoll1 FROM FROM "C";? If we don't want to go dig why the syntaxer isn't catching such errors, I tend to agree to keep the errorConflictingDefElem as is given the effort that one needs to put in identifying, fixing and testing the changes. Regards, Bharath Rupireddy.
Re: CREATE COLLATION - check for duplicate options and error out if found one
On Fri, 16 Jul 2021 at 10:26, Bharath Rupireddy
wrote:
>
> Thanks. The v5 patch LGTM.
OK, I'll push that in a while.
> Comment on errorConflictingDefElem:
> I think that the message in errorConflictingDefElem should say
> <>. I'm not sure why it
> wasn't done. Almost, all the cases where errorConflictingDefElem is
> called from, def->defname would give the correct user specified option
> name right, as errorConflictingDefElem called in if
> (strcmp(def->defname, "foo") == 0) clause.
>
> Is there any location the function errorConflictingDefElem gets called
> when def->defname isn't a name that's specified by the user?
There are a few cases where def->defname isn't necessarily the name
that was specified by the user (e.g., "volatility", "strict",
"format", and probably more cases not spotted in the other thread,
which was only a cursory review), and it would be quite onerous to go
through every one of the 100+ places in the code where this error is
raised to check them all. 2bfb50b3df was more about making pstate
available in more places to add location information to existing
errors, and did not want the risk of changing and possibly worsening
existing errors.
> If there's a scenario, then the function
> can be something like below:
> void
> errorConflictingDefElem(DefElem *defel, ParseState *pstate, bool
> show_option_name)
> {
> if (show_option_name)
> ereport(ERROR,
> errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("option \"%s\" specified more than once",
> defel->defname),
> pstate ? parser_errposition(pstate, defel->location) : 0);
> else
> ereport(ERROR,
> errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("conflicting or redundant options"),
> pstate ? parser_errposition(pstate, defel->location) : 0);
> }
I think it's preferable to have a single consistent primary error
message for all these cases. I wouldn't really want "CREATE FUNCTION
... STRICT STRICT" to throw a different error from "CREATE FUNCTION
... LEAKPROOF LEAKPROOF", but saying "option \"strict\" specified more
than once" would be odd for "CREATE FUNCTION ... CALLED ON NULL INPUT
RETURNS NULL ON NULL INPUT", which is indistinguishable from "STRICT
STRICT" in the code.
In any case, as you said before, the location is sufficient to
identify the offending option. Making this kind of change would
require going through all 100+ callers quite carefully, and a lot more
testing. I'm not really convinced that it's worth it.
Regards,
Dean
Re: CREATE COLLATION - check for duplicate options and error out if found one
On Fri, Jul 16, 2021 at 1:32 PM Dean Rasheed wrote:
>
> On Fri, 16 Jul 2021 at 06:40, Bharath Rupireddy
> wrote:
> >
> > 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";.
>
> Hmm, it is possible to type CREATE COLLATION mycoll1 (FROM = "C", FROM
> = "POSIX") though. It will still be caught by the check at the bottom
> though, so I guess it's OK to skip the duplicate check in that case.
> Also, it's not a documented syntax, so it's unlikely to occur in
> practice anyway.
>
> > 2) I don't understand the difference between errorConflictingDefElem
> > and errorDuplicateDefElem.
> >
> > I personally don't like the new function errorDuplicateDefElem as it
> > doesn't add any value given the presence of pstate.
>
> Yeah, there was a lot of discussion on that other thread about using
> defel->defname in these kinds of errors, and that was still on my
> mind. In general there are a number of cases where defel->defname
> isn't quite right, because it doesn't match the user-entered text,
> though in this case it always does. You're right though, it's a bit
> redundant with the position indicator pointing to the offending
> option, so it's probably not worth the effort to include it here when
> we don't anywhere else. That makes the patch much simpler, and
> consistent with option-checking elsewhere -- v5 attached (which is now
> much more like your v3).
Thanks. The v5 patch LGTM.
Comment on errorConflictingDefElem:
I think that the message in errorConflictingDefElem should say
<>. I'm not sure why it
wasn't done. Almost, all the cases where errorConflictingDefElem is
called from, def->defname would give the correct user specified option
name right, as errorConflictingDefElem called in if
(strcmp(def->defname, "foo") == 0) clause.
Is there any location the function errorConflictingDefElem gets called
when def->defname isn't a name that's specified by the user? Please
point me to that location. If there's a scenario, then the function
can be something like below:
void
errorConflictingDefElem(DefElem *defel, ParseState *pstate, bool
show_option_name)
{
if (show_option_name)
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("option \"%s\" specified more than once",
defel->defname),
pstate ? parser_errposition(pstate, defel->location) : 0);
else
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options"),
pstate ? parser_errposition(pstate, defel->location) : 0);
}
Regards,
Bharath Rupireddy.
Re: CREATE COLLATION - check for duplicate options and error out if found one
On Fri, 16 Jul 2021 at 06:40, Bharath Rupireddy
wrote:
>
> 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";.
Hmm, it is possible to type CREATE COLLATION mycoll1 (FROM = "C", FROM
= "POSIX") though. It will still be caught by the check at the bottom
though, so I guess it's OK to skip the duplicate check in that case.
Also, it's not a documented syntax, so it's unlikely to occur in
practice anyway.
> 2) I don't understand the difference between errorConflictingDefElem
> and errorDuplicateDefElem.
>
> I personally don't like the new function errorDuplicateDefElem as it
> doesn't add any value given the presence of pstate.
Yeah, there was a lot of discussion on that other thread about using
defel->defname in these kinds of errors, and that was still on my
mind. In general there are a number of cases where defel->defname
isn't quite right, because it doesn't match the user-entered text,
though in this case it always does. You're right though, it's a bit
redundant with the position indicator pointing to the offending
option, so it's probably not worth the effort to include it here when
we don't anywhere else. That makes the patch much simpler, and
consistent with option-checking elsewhere -- v5 attached (which is now
much more like your v3).
Regards,
Dean
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c
new file mode 100644
index ebb0994..c6a6ed3
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -88,17 +88,41 @@ DefineCollation(ParseState *pstate, List
if (strcmp(defel->defname, "from") == 0)
defelp =
else if (strcmp(defel->defname, "locale") == 0)
+ {
+ if (localeEl)
+errorConflictingDefElem(defel, pstate);
defelp =
+ }
else if (strcmp(defel->defname, "lc_collate") == 0)
+ {
+ if (lccollateEl)
+errorConflictingDefElem(defel, pstate);
defelp =
+ }
else if (strcmp(defel->defname, "lc_ctype") == 0)
+ {
+ if (lcctypeEl)
+errorConflictingDefElem(defel, pstate);
defelp =
+ }
else if (strcmp(defel->defname, "provider") == 0)
+ {
+ if (providerEl)
+errorConflictingDefElem(defel, pstate);
defelp =
+ }
else if (strcmp(defel->defname, "deterministic") == 0)
+ {
+ if (deterministicEl)
+errorConflictingDefElem(defel, pstate);
defelp =
+ }
else if (strcmp(defel->defname, "version") == 0)
+ {
+ if (versionEl)
+errorConflictingDefElem(defel, pstate);
defelp =
+ }
else
{
ereport(ERROR,
@@ -112,11 +136,17 @@ DefineCollation(ParseState *pstate, List
*defelp = defel;
}
- if ((localeEl && (lccollateEl || lcctypeEl))
- || (fromEl && list_length(parameters) != 1))
+ if (localeEl && (lccollateEl || lcctypeEl))
ereport(ERROR,
-(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("conflicting or redundant options")));
+errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("conflicting or redundant options"),
+errdetail("LOCALE cannot be specified together with LC_COLLATE or LC_CTYPE."));
+
+ if (fromEl && list_length(parameters) != 1)
+ ereport(ERROR,
+errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("conflicting or redundant options"),
+errdetail("FROM cannot be specified together with any other options."));
if (fromEl)
{
diff --git a/src/test/regress/expected/collate.out b/src/test/regress/expected/collate.out
new file mode 100644
index 0b60b55..2468325
--- a/src/test/regress/expected/collate.out
+++ b/src/test/regress/expected/collate.out
@@ -701,6 +701,53 @@ View definition:
SELECT ss.c1 + 1 AS c1p
FROM ( SELECT 4 AS c1) ss;
+-- Check conflicting or redundant options in CREATE COLLATION
+-- LC_COLLATE
+CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LC_COLLATE = "NONSENSE", LC_CTYPE = "POSIX");
+ERROR: conflicting or redundant options
+LINE 1: ...ATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LC_COLLATE...
+ ^
+-- LC_CTYPE
+CREATE COLLATION coll_dup_chk (LC_CTYPE = "POSIX", LC_CTYPE = "NONSENSE", LC_COLLATE = "POSIX");
+ERROR: conflicting or redundant options
+LINE 1: ...REATE COLLATION coll_dup_chk (LC_CTYPE = "POSIX", LC_CTYPE =...
+ ^
+-- PROVIDER
+CREATE COLLATION coll_dup_chk (PROVIDER = icu, PROVIDER = NONSENSE, LC_COLLATE = "POSIX", LC_CTYPE = "POSIX");
+ERROR: conflicting or redundant options
+LINE 1: CREATE COLLATION coll_dup_chk (PROVIDER = icu, PROVIDER = NO...
+ ^
+-- LOCALE
+CREATE COLLATION case_sensitive (LOCALE = '', LOCALE = "NONSENSE");
+ERROR: conflicting or redundant options
+LINE 1: CREATE COLLATION case_sensitive (LOCALE = '', LOCALE = "NONS...
+ ^
+-- DETERMINISTIC
+CREATE COLLATION
Re: CREATE COLLATION - check for duplicate options and error out if found one
On Fri, Jul 16, 2021 at 1:04 AM Dean Rasheed 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 =
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.
Re: CREATE COLLATION - check for duplicate options and error out if found one
On Mon, 31 May 2021 at 15:10, vignesh C wrote:
>
> On Sat, May 29, 2021 at 9:20 PM Bharath Rupireddy
> wrote:
> >
> > Thanks. PSA v3 patch.
>
> Thanks for the updated patch, the changes look good to me.
>
Hi,
Having pushed [1], I started looking at this, and I think it's mostly
in good shape.
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.
Regards,
Dean
[1]
https://www.postgresql.org/message-id/CAEZATCXHWa9OoSAetiZiGQy1eM2raa9q-b3K4ZYDwtcARypCcA%40mail.gmail.com
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c
new file mode 100644
index ebb0994..40e2626
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -86,19 +86,47 @@ DefineCollation(ParseState *pstate, List
DefElem **defelp;
if (strcmp(defel->defname, "from") == 0)
+ {
+ if (fromEl)
+errorDuplicateDefElem(defel, pstate);
defelp =
+ }
else if (strcmp(defel->defname, "locale") == 0)
+ {
+ if (localeEl)
+errorDuplicateDefElem(defel, pstate);
defelp =
+ }
else if (strcmp(defel->defname, "lc_collate") == 0)
+ {
+ if (lccollateEl)
+errorDuplicateDefElem(defel, pstate);
defelp =
+ }
else if (strcmp(defel->defname, "lc_ctype") == 0)
+ {
+ if (lcctypeEl)
+errorDuplicateDefElem(defel, pstate);
defelp =
+ }
else if (strcmp(defel->defname, "provider") == 0)
+ {
+ if (providerEl)
+errorDuplicateDefElem(defel, pstate);
defelp =
+ }
else if (strcmp(defel->defname, "deterministic") == 0)
+ {
+ if (deterministicEl)
+errorDuplicateDefElem(defel, pstate);
defelp =
+ }
else if (strcmp(defel->defname, "version") == 0)
+ {
+ if (versionEl)
+errorDuplicateDefElem(defel, pstate);
defelp =
+ }
else
{
ereport(ERROR,
@@ -112,11 +140,17 @@ DefineCollation(ParseState *pstate, List
*defelp = defel;
}
- if ((localeEl && (lccollateEl || lcctypeEl))
- || (fromEl && list_length(parameters) != 1))
+ if (localeEl && (lccollateEl || lcctypeEl))
ereport(ERROR,
-(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("conflicting or redundant options")));
+errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("conflicting or redundant options"),
+errdetail("LOCALE cannot be specified together with LC_COLLATE or LC_CTYPE."));
+
+ if (fromEl && list_length(parameters) != 1)
+ ereport(ERROR,
+errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("conflicting or redundant options"),
+errdetail("FROM cannot be specified together with any other options."));
if (fromEl)
{
diff --git a/src/backend/commands/define.c b/src/backend/commands/define.c
new file mode 100644
index aafd755..8dd260d
--- a/src/backend/commands/define.c
+++ b/src/backend/commands/define.c
@@ -359,3 +359,21 @@ errorConflictingDefElem(DefElem *defel,
errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location));
}
+
+/*
+ * 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.
+ */
+void
+errorDuplicateDefElem(DefElem *defel, ParseState *pstate)
+{
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting or redundant options"),
+ errdetail("Option \"%s\" specified more than once.", defel->defname),
+ parser_errposition(pstate, defel->location));
+}
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
new file mode 100644
index f84d099..91743ca
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -154,5 +154,6 @@ extern TypeName *defGetTypeName(DefElem
extern int defGetTypeLength(DefElem *def);
extern List *defGetStringList(DefElem *def);
extern void errorConflictingDefElem(DefElem *defel, ParseState *pstate) pg_attribute_noreturn();
+extern void errorDuplicateDefElem(DefElem *defel, ParseState *pstate) pg_attribute_noreturn();
#endif /* DEFREM_H */
diff --git a/src/test/regress/expected/collate.out b/src/test/regress/expected/collate.out
new file mode 100644
index 0b60b55..c08d8f2
--- a/src/test/regress/expected/collate.out
+++ b/src/test/regress/expected/collate.out
@@ -701,6 +701,65 @@ View definition:
SELECT ss.c1
Re: CREATE COLLATION - check for duplicate options and error out if found one
On Sat, May 29, 2021 at 9:20 PM Bharath Rupireddy
wrote:
>
> On Sat, May 29, 2021 at 9:08 PM vignesh C wrote:
> > One minor comment:
> > You can remove the brackets around errcode, You could change:
> > + if (localeEl)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_SYNTAX_ERROR),
> > + errmsg("option \"%s\" specified more than once", defel->defname),
> > + parser_errposition(pstate, defel->location)));
> > to:
> > + if (localeEl)
> > + ereport(ERROR,
> > + errcode(ERRCODE_SYNTAX_ERROR),
> > + errmsg("option \"%s\" specified more than once", defel->defname),
> > + parser_errposition(pstate, defel->location));
>
> Thanks. PSA v3 patch.
Thanks for the updated patch, the changes look good to me.
Regards,
Vignesh
Re: CREATE COLLATION - check for duplicate options and error out if found one
On Sat, May 29, 2021 at 9:08 PM vignesh C wrote:
> One minor comment:
> You can remove the brackets around errcode, You could change:
> + if (localeEl)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("option \"%s\" specified more than once", defel->defname),
> + parser_errposition(pstate, defel->location)));
> to:
> + if (localeEl)
> + ereport(ERROR,
> + errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("option \"%s\" specified more than once", defel->defname),
> + parser_errposition(pstate, defel->location));
Thanks. PSA v3 patch.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
v3-0001-Check-duplicate-options-and-error-out-for-CREATE-.patch
Description: Binary data
Re: CREATE COLLATION - check for duplicate options and error out if found one
On Wed, May 26, 2021 at 7:44 PM Bharath Rupireddy
wrote:
>
> On Wed, May 26, 2021 at 7:18 PM vignesh C wrote:
> > +1 for fixing this issue, we have handled this error in other places.
> > The patch does not apply on head, could you rebase the patch on head
> > and post a new patch.
>
> Thanks. I thought of rebasing once the other patch (which reorganizes
> "...specified more than once" error) gets committed. Anyways, I've
> rebased for now on the latest master. Please review v2 patch.
>
Thanks for the updated patch.
One minor comment:
You can remove the brackets around errcode, You could change:
+ if (localeEl)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", defel->defname),
+ parser_errposition(pstate, defel->location)));
to:
+ if (localeEl)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", defel->defname),
+ parser_errposition(pstate, defel->location));
Regards,
Vignesh
Re: CREATE COLLATION - check for duplicate options and error out if found one
On Thu, May 27, 2021 at 8:36 PM vignesh C wrote: > > On Wed, May 26, 2021 at 7:44 PM Bharath Rupireddy > wrote: > > > > On Wed, May 26, 2021 at 7:18 PM vignesh C wrote: > > > +1 for fixing this issue, we have handled this error in other places. > > > The patch does not apply on head, could you rebase the patch on head > > > and post a new patch. > > > > Thanks. I thought of rebasing once the other patch (which reorganizes > > "...specified more than once" error) gets committed. Anyways, I've > > rebased for now on the latest master. Please review v2 patch. > > The test changes look good to me, I liked the idea of rebasing the > source changes once "specified more than once" patch gets committed. I > will review the code changes after that. I'm not sure which patch goes first. I think the review can be finished and see which one will be picked up first by the committer. Based on that, the other patch can be rebased. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: CREATE COLLATION - check for duplicate options and error out if found one
On Wed, May 26, 2021 at 7:44 PM Bharath Rupireddy wrote: > > On Wed, May 26, 2021 at 7:18 PM vignesh C wrote: > > +1 for fixing this issue, we have handled this error in other places. > > The patch does not apply on head, could you rebase the patch on head > > and post a new patch. > > Thanks. I thought of rebasing once the other patch (which reorganizes > "...specified more than once" error) gets committed. Anyways, I've > rebased for now on the latest master. Please review v2 patch. The test changes look good to me, I liked the idea of rebasing the source changes once "specified more than once" patch gets committed. I will review the code changes after that. Regards, Vignesh
Re: CREATE COLLATION - check for duplicate options and error out if found one
On Wed, May 26, 2021 at 7:18 PM vignesh C wrote: > +1 for fixing this issue, we have handled this error in other places. > The patch does not apply on head, could you rebase the patch on head > and post a new patch. Thanks. I thought of rebasing once the other patch (which reorganizes "...specified more than once" error) gets committed. Anyways, I've rebased for now on the latest master. Please review v2 patch. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com v2-0001-Check-duplicate-options-and-error-out-for-CREATE-.patch Description: Binary data
Re: CREATE COLLATION - check for duplicate options and error out if found one
On Tue, Apr 27, 2021 at 3:21 PM Bharath Rupireddy wrote: > > Hi, > > While reviewing [1], I found that the CREATE COLLATION doesn't throw an error > if duplicate options are specified, see [2] for testing a few cases on HEAD. > This may end up accepting some of the weird cases, see [2]. It's against > other option checking code in the server where the duplicate option is > detected and an error thrown if found one. Attached a patch doing that. I > chose to have the error message "option \"%s\" specified more than once" and > parser_errposition because that's kind of agreed in [3]. > > Thoughts? > > [1] > https://www.postgresql.org/message-id/CALj2ACWVd%3D-E6uG5AdHD0MvHY6e4mVzkapT%3DvLDnJJseGjaJLQ%40mail.gmail.com > > [2] > CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LC_COLLATE = "NONSENSE", > LC_CTYPE = "POSIX"); -- ERROR > CREATE COLLATION coll_dup_chk (LC_COLLATE = "NONSENSE", LC_COLLATE = "POSIX", > LC_CTYPE = "POSIX"); -- OK but it's weird > CREATE COLLATION coll_dup_chk (LC_CTYPE = "POSIX", LC_CTYPE = "NONSENSE", > LC_COLLATE = "POSIX"); -- ERROR > CREATE COLLATION coll_dup_chk (LC_CTYPE = "NONSENSE", LC_CTYPE = "POSIX", > LC_COLLATE = "POSIX",); -- OK but it's weird > CREATE COLLATION coll_dup_chk (PROVIDER = icu, PROVIDER = NONSENSE, > LC_COLLATE = "POSIX", LC_CTYPE = "POSIX"); -- ERROR > CREATE COLLATION coll_dup_chk (PROVIDER = NONSENSE, PROVIDER = icu, > LC_COLLATE = "POSIX", LC_CTYPE = "POSIX"); -- OK but it's weird > CREATE COLLATION case_sensitive (LOCALE = '', LOCALE = "NONSENSE"); -- ERROR > CREATE COLLATION coll_dup_chk (LOCALE = "NONSENSE", LOCALE = ''); -- OK but > it's weird > CREATE COLLATION coll_dup_chk (DETERMINISTIC = TRUE, DETERMINISTIC = > NONSENSE, LOCALE = ''); -- ERROR > CREATE COLLATION coll_dup_chk (DETERMINISTIC = NONSENSE, DETERMINISTIC = > TRUE, LOCALE = ''); -- OK but it's weird > > [3] > https://www.postgresql.org/message-id/CALj2ACUa%3DZM8QtOLPCHc7%3DWgFrx9P6-AgKQs8cmKLvNCvu7arQ%40mail.gmail.com +1 for fixing this issue, we have handled this error in other places. The patch does not apply on head, could you rebase the patch on head and post a new patch. Regards, Vignesh
CREATE COLLATION - check for duplicate options and error out if found one
Hi,
While reviewing [1], I found that the CREATE COLLATION doesn't throw an
error if duplicate options are specified, see [2] for testing a few cases
on HEAD. This may end up accepting some of the weird cases, see [2]. It's
against other option checking code in the server where the duplicate option
is detected and an error thrown if found one. Attached a patch doing that.
I chose to have the error message "option \"%s\" specified more than once"
and parser_errposition because that's kind of agreed in [3].
Thoughts?
[1]
https://www.postgresql.org/message-id/CALj2ACWVd%3D-E6uG5AdHD0MvHY6e4mVzkapT%3DvLDnJJseGjaJLQ%40mail.gmail.com
[2]
CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LC_COLLATE =
"NONSENSE", LC_CTYPE = "POSIX"); -- ERROR
CREATE COLLATION coll_dup_chk (LC_COLLATE = "NONSENSE", LC_COLLATE =
"POSIX", LC_CTYPE = "POSIX"); -- OK but it's weird
CREATE COLLATION coll_dup_chk (LC_CTYPE = "POSIX", LC_CTYPE = "NONSENSE",
LC_COLLATE = "POSIX"); -- ERROR
CREATE COLLATION coll_dup_chk (LC_CTYPE = "NONSENSE", LC_CTYPE = "POSIX",
LC_COLLATE = "POSIX",); -- OK but it's weird
CREATE COLLATION coll_dup_chk (PROVIDER = icu, PROVIDER = NONSENSE,
LC_COLLATE = "POSIX", LC_CTYPE = "POSIX"); -- ERROR
CREATE COLLATION coll_dup_chk (PROVIDER = NONSENSE, PROVIDER = icu,
LC_COLLATE = "POSIX", LC_CTYPE = "POSIX"); -- OK but it's weird
CREATE COLLATION case_sensitive (LOCALE = '', LOCALE = "NONSENSE"); -- ERROR
CREATE COLLATION coll_dup_chk (LOCALE = "NONSENSE", LOCALE = ''); -- OK but
it's weird
CREATE COLLATION coll_dup_chk (DETERMINISTIC = TRUE, DETERMINISTIC =
NONSENSE, LOCALE = ''); -- ERROR
CREATE COLLATION coll_dup_chk (DETERMINISTIC = NONSENSE, DETERMINISTIC =
TRUE, LOCALE = ''); -- OK but it's weird
[3]
https://www.postgresql.org/message-id/CALj2ACUa%3DZM8QtOLPCHc7%3DWgFrx9P6-AgKQs8cmKLvNCvu7arQ%40mail.gmail.com
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 555ac67a94d899b107e27a691f3a2a7340a14f64 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Tue, 27 Apr 2021 12:01:44 +0530
Subject: [PATCH v1] Check duplicate options and error out for CREATE COLLATION
command
CREATE COLLATION doesn't throw an error if duplicate options are
specified. Modify the option parsing loop in DefineCollation to
check for duplicate options and error out if found one.
---
src/backend/commands/collationcmds.c | 35 +++
src/test/regress/expected/collate.out | 35 +++
src/test/regress/sql/collate.sql | 17 +
3 files changed, 87 insertions(+)
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c
index b8ec6f5756..8d07b21eda 100644
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -86,15 +86,50 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e
if (strcmp(defel->defname, "from") == 0)
defelp =
else if (strcmp(defel->defname, "locale") == 0)
+ {
+ if (localeEl)
+ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", "locale"),
+ parser_errposition(pstate, defel->location)));
defelp =
+ }
else if (strcmp(defel->defname, "lc_collate") == 0)
+ {
+ if (lccollateEl)
+ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", "lc_collate"),
+ parser_errposition(pstate, defel->location)));
defelp =
+ }
else if (strcmp(defel->defname, "lc_ctype") == 0)
+ {
+ if (lcctypeEl)
+ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", "lc_ctype"),
+ parser_errposition(pstate, defel->location)));
defelp =
+ }
else if (strcmp(defel->defname, "provider") == 0)
+ {
+ if (providerEl)
+ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", "provider"),
+ parser_errposition(pstate, defel->location)));
defelp =
+ }
else if (strcmp(defel->defname, "deterministic") == 0)
+ {
+ if (deterministicEl)
+ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", "deterministic"),
+ parser_errposition(pstate, defel->location)));
defelp =
+ }
else
{
ereport(ERROR,
diff --git a/src/test/regress/expected/collate.out b/src/test/regress/expected/collate.out
index 0b60b55514..85a10c8198 100644
--- a/src/test/regress/expected/collate.out
+++ b/src/test/regress/expected/collate.out
@@ -701,6 +701,41 @@ View definition:
SELECT ss.c1 + 1 AS c1p
FROM ( SELECT 4 AS c1) ss;
+-- Check duplicate options in CREATE COLLATION command and throw error
+-- LC_COLLATE
+CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LC_COLLATE = "NONSENSE", LC_CTYPE = "POSIX");
+ERROR: option "lc_collate" specified more than once
+LINE 1: ...ATE COLLATION coll_dup_chk (LC_COLLATE =
