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

On Fri, Jul 16, 2021 at 1:32 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> On Fri, 16 Jul 2021 at 06:40, Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)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.