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

Greetings Michael, Daniel, all,

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> 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.

Looks like this discussion has progressed to where this patch should
really be marked as Rejected. Does anyone else want to voice an opinion
regarding it, or perhaps Daniel could post a rebuttal to the concerns
raised here?

Thinking through it, for my own 2c on this, I tend to agree with Tom
that, really, we should be using strcmp() for anything coming out of the
parser and that the backward-compatibility break from that is
acceptable. I also understand Robert's concern that there may be other
bugs hiding and I wonder if there might be a way to check for them,
though no great ideas spring to mind offhand. Would be great to hear
your thoughts, Daniel, so leaving this in Waiting on Author for now.

Thanks!

Stephen