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

> On 26 Jan 2018, at 23:58, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
>>> On 26 Jan 2018, at 22:32, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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