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

On 04/04/2017 10:13 AM, Daniel Gustafsson wrote:
>> On 04 Apr 2017, at 05:52, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>>
>> Daniel Gustafsson wrote:
>>> Testing DefElem options is done with both strcmp() and pg_strcasecmp() a bit
>>> mixed.  Since the option defnames are all lowercased, either via IDENT, keyword
>>> rules or “by hand” with makeString(), using strcmp() is safe (and assumed to be
>>> so in quite a lot of places).
>>>
>>> While it’s not incorrect per se to use pg_strcasecmp(), it has the potential to
>>> hide a DefElem created with a mixed-case defname where it in other places is
>>> expected to be in lowercase, which may lead to subtle bugs.
>>>
>>> The attached patch refactors to use strcmp() consistently for option processing
>>> in the command code as a pre-emptive belts+suspenders move against such subtle
>>> bugs and to make the code more consistent.  Also reorders a few checks to have
>>> all in the same “format” and removes a comment related to the above.
>>>
>>> Tested with randomizing case on options in make check (not included in patch).
>>
>> Does it work correctly in the Turkish locale?
> 
> Yes, running make check with random case defnames under tr_TR works fine.

This no longer works:

postgres=# CREATE TEXT SEARCH DICTIONARY public.simple_dict (    TEMPLATE = pg_catalog.simple,    "STOPWORds" =
english
);
ERROR:  unrecognized simple dictionary parameter: "STOPWORds"

In hindsight, perhaps we should always have been more strict about that 
to begin wtih, but let's not break backwards-compatibility without a 
better reason. I didn't thoroughly check all of the cases here, to see 
if there are more like this.

It'd be nice to have some kind of a rule on when pg_strcasecmp should be 
used and when strcmp() is enough. Currently, by looking at the code, I 
can't tell.

- Heikki