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

On Fri, 16 Jul 2021 at 10:26, Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)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