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

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] Refactoring identifier checks to consistently use strcmp
Date: 2017-11-27 15:11:39
Message-ID: 636F9683-C4B7-47EF-A5BB-57735B6A0894@yesql.se
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On 17 Nov 2017, at 03:31, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>
> On Tue, Sep 5, 2017 at 5:34 PM, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>>> On 17 Aug 2017, at 11:08, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>>>> On 16 Aug 2017, at 17:51, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> My thought is that if we are looking at words that have been through the
>>>> parser, then it should *always* be plain strcmp; we should expect that
>>>> the parser already did the appropriate case-folding.
>>>
>>> +1
>>>
>>>> pg_strcasecmp would be appropriate, perhaps, if we're dealing with stuff
>>>> that somehow came in without going through the parser.
>>>
>>> In that case it would be up to the consumer of the data to handle required
>>> case-folding for the expected input, so pg_strcasecmp or strcmp depending on
>>> situation.
>>
>> This patch has been marked “Waiting on Author”, but I’m not sure what the
>> concensus of this thread came to with regards to quoted keywords and backwards
>> compatibility. There seems to be a 2-1 vote for allowing a break, and forcing
>> all keywords out of the parser to be casefolded. Any other opinions?
>
> This patch impacts the DDL grammar of aggregates, operators,
> collations, text search, views, etc. Still I agree with the purpose of
> this thread that it would be nice to get a more consistent behavior
> even if it breaks some queries, so +1 for the argument with the
> post-parser comparison which should use strcmp.

Thanks for reviewing!

> The patch needs a rebase, and there are a couple of places that need
> an extra lookup I think:
> $ git grep defname -- *.c | grep strcasecmp | wc -l
> 39

Rebased and handled a few more places which I had either missed in the last
round, or that had been added in the meantime. “PARALLEL” in aggregatecmds.c
is intentionally using pg_strcasecmp() due to the old-style syntax which is
still supported. AFAICS this covers all relevant codepaths from the 39 above.

cheers ./daniel

Attachment Content-Type Size
defname_strcmp-v2.patch application/octet-stream 26.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2017-11-27 15:49:32 Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables
Previous Message Oliver Ford 2017-11-27 15:01:06 Re: Add RANGE with values and exclusions clauses to the Window Functions