Re: [HACKERS] Refactoring identifier checks to consistently use strcmp - Mailing list pgsql-hackers

> On 11 Jan 2018, at 09:01, Michael Paquier <michael.paquier@gmail.com> wrote:
>
> On Wed, Jan 10, 2018 at 11:49:47PM +0100, Daniel Gustafsson wrote:
>>> On 08 Jan 2018, at 17:27, Robert Haas <robertmhaas@gmail.com> wrote:
>>> nor has anyone taken the trouble to list with precision all of the
>>> places where the behavior will change.  I think the latter is
>>> absolutely indispensable,
>>
>> I had a look at the available commands in postgres and compiled a list
>> of them in options.sql based on if they have options, and how those
>> options and matched (case sensitive of insensitive).  The queries in
>> the file are nonsensical, they are just meant to show the handling of
>> the options.  The idea was to illustrate the impact of this proposal
>> by running examples. Running this file with and without the patches
>> applied shows the following commands being affected:
>>
>> <snip>
>>
>> The output with the patch is attached as options_patched.out, and the output
>> from master as options_master.out.  Diffing the two files is rather helpful I
>> think.
>
> Thanks. This is saving me hours of lookups and testing during the
> review, as now reviewers just have to map you test series with the code
> modified.

Well, being the one proposing the patch I should be the one spending those
hours and not you.  Sorry for not including in the original submission.

> I can't help to notice that tests for code paths with
> contrib modules are missing. This brings up the point: do we want those
> tests to be in the patch?

I left them out since they are version of ALTER TEXT SEARCH .. rather than new
statements.

> I would like to think that a special section
> dedicated to option compatibility for each command would be welcome to
> track which grammar is supported and which grammar is not supported.

I’m not sure I follow?

>> One open question from this excercise is how to write a good test for
>> this.  It can either be made part of the already existing test queries
>> or a separate suite.  I’m leaning on the latter simply because the
>> case-flipping existing tests seems like something that can be cleaned
>> up years from now accidentally because it looks odd.
>
> Adding them into src/test/regress/ sounds like a good plan to me.

If there is interest in this patch now that the list exists and aids review, I
can turn the list into a proper test that makes a little more sense than the
current list which is rather aimed at helping reviewers.

>> If you however combine the new and old syntax, the statement works and the WITH
>> option wins by overwriting any previous value.  The below statement creates an
>> IMMUTABLE function:
>>
>>    create function int42(cstring) returns int42 AS 'int4in'
>>    language internal strict volatile with (isstrict, iscachable);
>
> Here is another idea: nuking isstrict and iscachable from CREATE
> FUNCTION syntax and forget about them. I would be tempted of the opinion
> to do that before the rest.

Thats certainly an option, I have no idea about the prevalence in real life
production environments to have much an opinion to offer.

>
> -old_aggr_elem:  IDENT '=' def_arg
> +old_aggr_elem:  PARALLEL '=' def_arg
> +               {
> +                   $$ = makeDefElem("parallel", (Node *)$3, @1);
> +               }
> +               | IDENT '=' def_arg
> Nit: alphabetical order.

Fixed.

> I have spent a couple of hours reviewing all the calls to pg_strcasecmp,
> and the only thing I have noticed is in transformRelOptions(), where the
> namespace string should be evaluated as well by strcmp, no?
<snip>
> So per my set of examples, the evaluation of the schema name should fail
> when creating relation a3 with your patch applied. As the patch does
> things, the experience for the user is not consistent, and the schema
> name goes through parser via reloption_elem using makeDefElemExtended,
> so this should use strcmp.

I believe you are entirely correct, the attached v5 patch is updated with this
behavior.  The volatility patch is unchanged by this so didn’t submit a new
version of that one.

Thanks for the review!

cheers ./daniel