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

> On 26 Jan 2018, at 23:58, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 26 Jan 2018, at 22:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I notice that there are two reloptions-related
>>> "pg_strncasecmp" calls that did not get converted to "strncmp":
>>> reloptions.c:804
> 
>> The way I read transformRelOptions(), oldOptions is not guaranteed to
>> come from the parser (though in reality it probably will be).
> 
> Well, one response to that is that it should contain values that were
> deemed acceptable at some previous time.  If we allowed catalog contents
> to be migrated physically across major releases, we'd need to worry about
> having mixed-case reloptions in a v11 catalog ... but we don't, so we
> won't.  The old reloptions should always be ones that got through
> parseRelOptions before, and those now will always have to be exact case.

That’s a good point, the reloptions will only ever come from the same major
version.

> Another response is that leaving it like this would mean that
> transformRelOptions and parseRelOptions have different opinions about
> whether two option names are the same or not, which seems to me to be
> exactly the same sort of bug hazard that you were on about at the
> beginning of this thread.

Completely agree.

>> The namespace isn’t either
>> but passing an uppercase namespace should never be valid AFAICT, hence the
>> patch changing it to case sensitive comparison.
> 
> Well, it's no more or less valid than an uppercase option name …

Agreed.  Taking a step back and thinking it’s clear that the comparison in the
oldoptions loop should’ve been changed in the patch for it to be consistent
with the original objective.

cheers ./daniel