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.