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

On Tue, Apr 27, 2021 at 3:21 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> 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
optionchecking code in the server where the duplicate option is detected and an error thrown if found one.  Attached a
patchdoing that. I chose to have the error message "option \"%s\" specified more than once" and parser_errposition
becausethat'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
butit'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

On Wed, May 26, 2021 at 7:18 PM vignesh C <vignesh21@gmail.com> 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

On Wed, May 26, 2021 at 7:44 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, May 26, 2021 at 7:18 PM vignesh C <vignesh21@gmail.com> 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

On Thu, May 27, 2021 at 8:36 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, May 26, 2021 at 7:44 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Wed, May 26, 2021 at 7:18 PM vignesh C <vignesh21@gmail.com> 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

On Wed, May 26, 2021 at 7:44 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, May 26, 2021 at 7:18 PM vignesh C <vignesh21@gmail.com> 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

On Sat, May 29, 2021 at 9:08 PM vignesh C <vignesh21@gmail.com> 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

On Sat, May 29, 2021 at 9:20 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Sat, May 29, 2021 at 9:08 PM vignesh C <vignesh21@gmail.com> 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

On Mon, 31 May 2021 at 15:10, vignesh C <vignesh21@gmail.com> wrote:
>
> On Sat, May 29, 2021 at 9:20 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> 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

On Fri, Jul 16, 2021 at 1:04 AM Dean Rasheed <dean.a.rasheed@gmail.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.

On Fri, 16 Jul 2021 at 06:40, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> 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

On Fri, Jul 16, 2021 at 1:32 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Fri, 16 Jul 2021 at 06:40, Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> 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
<<option \"%s\'' specified more than once>>. 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.

On Fri, 16 Jul 2021 at 10:26, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> 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
> <<option \"%s\'' specified more than once>>. 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

On Fri, Jul 16, 2021 at 4:47 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Fri, 16 Jul 2021 at 10:26, Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> 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.

On Fri, 16 Jul 2021 at 12:17, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Fri, 16 Jul 2021 at 10:26, Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> 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

On Sat, 17 Jul 2021 at 05:24, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> 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