> On 11 Jan 2018, at 09:01, Michael Paquier <michael.paqu...@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 <robertmh...@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
defname_strcmp-v5.patch
Description: Binary data
