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

On Wed, Jan 10, 2018 at 11:49:47PM +0100, Daniel Gustafsson wrote:
>> On 08 Jan 2018, at 17:27, Robert Haas <robertmhaas(at)gmail(dot)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. 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 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.

> All possible options aren’t excercised, and I hope I didn’t miss any
> statements that should’ve been covered. The options.sql file makes it
> quite obvious that we currently have quite a mix of case sensitive and
> insensitive commands. Is this in line with what you were thinking
> Robert? I’m definitely up for better ideas.

I would think that one option tested in the series is fine to cover
grounds. Most of those code paths are made of a series of if/elif using
strcmp so all of them should be consistent..

> 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 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.

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

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? On HEAD:
=# create table a1 (a text) with ( "Toast"."Autovacuum_vacuum_cost_limit" = 1);
CREATE TABLE
=# create table a2 (a text) with ( "Toast.Autovacuum_vacuum_cost_limit" = 1);
ERROR: 22023: unrecognized parameter "Toast.Autovacuum_vacuum_cost_limit"
=# create table a3 (a text) with ( "Toast".autovacuum_vacuum_cost_limit = 1);
CREATE TABLE
=# create table a4 (a text) with ( toast."Autovacuum_vacuum_cost_limit" = 1);
CREATE TABLE

With your patch:
=# create table a1 (a text) with ( "Toast"."Autovacuum_vacuum_cost_limit" = 1);
ERROR: 22023: unrecognized parameter "Autovacuum_vacuum_cost_limit"
=# create table a2 (a text) with ( "Toast.Autovacuum_vacuum_cost_limit" = 1);
ERROR: 22023: unrecognized parameter "Toast.Autovacuum_vacuum_cost_limit"
=# create table a3 (a text) with ( "Toast".autovacuum_vacuum_cost_limit = 1);
CREATE TABLE
=# create table a4 (a text) with ( toast."Autovacuum_vacuum_cost_limit" = 1);
ERROR: 22023: unrecognized parameter "Autovacuum_vacuum_cost_limit"
LOCATION: parseRelOptions, reloptions.c:1103

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.
--
Michael