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
