Re: [HACKERS] Refactoring identifier checks to consistently use strcmp

On Fri, Dec 1, 2017 at 4:14 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I think the changes in DefineView and ATExecSetRelOptions is wrong,
> because transformRelOptions() is still using pg_strcasecmp. With the
> patch:
>
> rhaas=# create view v(x) with ("Check_option"="local") as select 1;
> CREATE VIEW
> rhaas=# create view v(x) with ("check_option"="local") as select 1;
> ERROR: WITH CHECK OPTION is supported only on automatically updatable views
> HINT: Views that do not select from a single table or view are not
> automatically updatable.
>
> Oops.

I am getting the feeling that there could be other issues than this
one... If this patch ever gets integrated, I think that this should
actually be shaped as two patches:
- One patch with the set of DDL queries taking advantage of the
current grammar with pg_strcasecmp.
- A second patch that does the switch from pg_strcasecmp to strcmp,
and allows checking which paths of patch 1 are getting changed.
Patch 1 is the hard part to figure out all the possible patterns that
could be changed.

> I am actually pretty dubious that we want to do this. I found one bug
> already (see above), and I don't think there's any guarantee that it's
> the only one, because it's pretty hard to be sure that none of the
> remaining calls to pg_strcasecmp are being applied to any of these
> values in some other part of the code. I'm not sure that the backward
> compatibility issue is a huge deal, but from my point of view this
> carries a significant risk of introducing new bugs, might annoy users
> who spell any of these keywords in all caps with surrounding quotation
> marks, and really has no clear benefit that I can see. The
> originally-offered justification is that making this consistent would
> help us avoid subtle bugs, but it seems at least as likely to CREATE
> subtle bugs, and the status quo is AFAICT harming nobody.

Changing opinion here ;)
Yes, I agree that the risk of bugs may not be worth the compatibility
effort at the end. I still see value in this patch for long-term
purposes by making the code more consistent though.
--
Michael