Allowing regular identifiers in isolationtester scripts - Mailing list pgsql-hackers

[ new subject, new thread, new patch ]

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Jun-16, Tom Lane wrote:
>> BTW, as long as we're thinking of back-patching nontrivial specfile
>> changes, I have another modest proposal.  What do people think of
>> removing the requirement for step/session names to be double-quoted,
>> and instead letting them work like SQL identifiers?

> Yes *please*.

Here's a draft patch for that.  I converted one specfile just as
proof-of-concept, but I don't want to touch the rest until the other
patch has gone in, or I'll have merge problems.  (This'll have some
merge problems with that anyway I fear, but they'll be minor.)

I decided to follow the standard SQL rule that you can use "foo""bar"
to include a double-quote in a quoted identifier.  This broke one
place in test_decoding's oldest_xmin.spec where somebody had left out
a space.  So maybe there's an argument for not doing that --- but I'd
rather not document more inconsistencies than I have to.

            regards, tom lane

diff --git a/contrib/test_decoding/specs/oldest_xmin.spec b/contrib/test_decoding/specs/oldest_xmin.spec
index da3a8cd512..88bd30f5ff 100644
--- a/contrib/test_decoding/specs/oldest_xmin.spec
+++ b/contrib/test_decoding/specs/oldest_xmin.spec
@@ -39,4 +39,4 @@ step "s1_commit" { COMMIT; }
 # composite type is a rare form of DDL which allows T1 to see the tuple which
 # will be removed (xmax set) before T1 commits. That is, interlocking doesn't
 # forbid modifying catalog after someone read it (and didn't commit yet).
-permutation "s0_begin" "s0_getxid" "s1_begin" "s1_insert" "s0_alter" "s0_commit" "s0_checkpoint" "s0_get_changes"
"s0_get_changes""s1_commit""s0_vacuum" "s0_get_changes" 
+permutation "s0_begin" "s0_getxid" "s1_begin" "s1_insert" "s0_alter" "s0_commit" "s0_checkpoint" "s0_get_changes"
"s0_get_changes""s1_commit" "s0_vacuum" "s0_get_changes" 
diff --git a/src/test/isolation/README b/src/test/isolation/README
index 6ae7152325..439b1d32b2 100644
--- a/src/test/isolation/README
+++ b/src/test/isolation/README
@@ -75,7 +75,7 @@ teardown { <SQL> }
   this to clean up in preparation for the next permutation, e.g dropping
   any test tables created by setup. This part is optional.

-session "<name>"
+session <name>

   There are normally several "session" parts in a spec file. Each
   session is executed in its own connection. A session part consists
@@ -86,13 +86,13 @@ session "<name>"

   Each step has the syntax

-  step "<name>" { <SQL> }
+  step <name> { <SQL> }

   where <name> is a name identifying this step, and SQL is a SQL statement
   (or statements, separated by semicolons) that is executed in the step.
   Step names must be unique across the whole spec file.

-permutation "<step name>" ...
+permutation <step name> ...

   A permutation line specifies a list of steps that are run in that order.
   Any number of permutation lines can appear.  If no permutation lines are
@@ -103,6 +103,13 @@ permutation "<step name>" ...
   available steps; it could for instance repeat some steps more than once,
   or leave others out.

+Session and step names are SQL identifiers or quoted identifiers.
+A difference from standard SQL is that no case-folding occurs, so
+FOO and "FOO" are the same name while FOO and Foo are different,
+whether you quote them or not.  You must use double quotes if you
+want to use an isolation test keyword (such as "permutation") as
+a name.
+
 Lines beginning with a # are considered comments.

 For each permutation of the session steps (whether these are manually
diff --git a/src/test/isolation/specparse.y b/src/test/isolation/specparse.y
index ae3a999961..efdf333b00 100644
--- a/src/test/isolation/specparse.y
+++ b/src/test/isolation/specparse.y
@@ -39,12 +39,12 @@ TestSpec        parseresult;            /* result of parsing is left here */
 %type <str>  opt_setup opt_teardown
 %type <str> setup
 %type <ptr_list> step_list session_list permutation_list opt_permutation_list
-%type <ptr_list> string_literal_list
+%type <ptr_list> identifier_list
 %type <session> session
 %type <step> step
 %type <permutation> permutation

-%token <str> sqlblock string_literal
+%token <str> sqlblock identifier
 %token PERMUTATION SESSION SETUP STEP TEARDOWN TEST

 %%
@@ -111,7 +111,7 @@ session_list:
         ;

 session:
-            SESSION string_literal opt_setup step_list opt_teardown
+            SESSION identifier opt_setup step_list opt_teardown
             {
                 $$ = pg_malloc(sizeof(Session));
                 $$->name = $2;
@@ -140,7 +140,7 @@ step_list:


 step:
-            STEP string_literal sqlblock
+            STEP identifier sqlblock
             {
                 $$ = pg_malloc(sizeof(Step));
                 $$->name = $2;
@@ -180,7 +180,7 @@ permutation_list:


 permutation:
-            PERMUTATION string_literal_list
+            PERMUTATION identifier_list
             {
                 $$ = pg_malloc(sizeof(Permutation));
                 $$->stepnames = (char **) $2.elements;
@@ -188,15 +188,15 @@ permutation:
             }
         ;

-string_literal_list:
-            string_literal_list string_literal
+identifier_list:
+            identifier_list identifier
             {
                 $$.elements = pg_realloc($1.elements,
                                          ($1.nelements + 1) * sizeof(void *));
                 $$.elements[$1.nelements] = $2;
                 $$.nelements = $1.nelements + 1;
             }
-            | string_literal
+            | identifier
             {
                 $$.nelements = 1;
                 $$.elements = pg_malloc(sizeof(void *));
diff --git a/src/test/isolation/specs/aborted-keyrevoke.spec b/src/test/isolation/specs/aborted-keyrevoke.spec
index 08945d8b31..4f6f9027cc 100644
--- a/src/test/isolation/specs/aborted-keyrevoke.spec
+++ b/src/test/isolation/specs/aborted-keyrevoke.spec
@@ -17,30 +17,30 @@ teardown
   DROP TABLE foo;
 }

-session "s1"
+session s1
 setup        { BEGIN; }
-step "s1s"    { SAVEPOINT f; }
-step "s1u"    { UPDATE foo SET key = 2; }    # obtain KEY REVOKE
-step "s1r"    { ROLLBACK TO f; } # lose KEY REVOKE
-step "s1l"    { SELECT * FROM foo FOR KEY SHARE; }
-step "s1c"    { COMMIT; }
+step s1s    { SAVEPOINT f; }
+step s1u    { UPDATE foo SET key = 2; }    # obtain KEY REVOKE
+step s1r    { ROLLBACK TO f; } # lose KEY REVOKE
+step s1l    { SELECT * FROM foo FOR KEY SHARE; }
+step s1c    { COMMIT; }

-session "s2"
+session s2
 setup        { BEGIN; }
-step "s2l"    { SELECT * FROM foo FOR KEY SHARE; }
-step "s2c"    { COMMIT; }
+step s2l    { SELECT * FROM foo FOR KEY SHARE; }
+step s2c    { COMMIT; }

-permutation "s1s" "s1u" "s1r" "s1l" "s1c" "s2l" "s2c"
-permutation "s1s" "s1u" "s1r" "s1l" "s2l" "s1c" "s2c"
-permutation "s1s" "s1u" "s1r" "s1l" "s2l" "s2c" "s1c"
-permutation "s1s" "s1u" "s1r" "s2l" "s1l" "s1c" "s2c"
-permutation "s1s" "s1u" "s1r" "s2l" "s1l" "s2c" "s1c"
-permutation "s1s" "s1u" "s1r" "s2l" "s2c" "s1l" "s1c"
-permutation "s1s" "s1u" "s2l" "s1r" "s1l" "s1c" "s2c"
-permutation "s1s" "s1u" "s2l" "s1r" "s1l" "s2c" "s1c"
-permutation "s1s" "s1u" "s2l" "s1r" "s2c" "s1l" "s1c"
-permutation "s1s" "s2l" "s1u" "s2c" "s1r" "s1l" "s1c"
-permutation "s1s" "s2l" "s2c" "s1u" "s1r" "s1l" "s1c"
-permutation "s2l" "s1s" "s1u" "s2c" "s1r" "s1l" "s1c"
-permutation "s2l" "s1s" "s2c" "s1u" "s1r" "s1l" "s1c"
-permutation "s2l" "s2c" "s1s" "s1u" "s1r" "s1l" "s1c"
+permutation s1s s1u s1r s1l s1c s2l s2c
+permutation s1s s1u s1r s1l s2l s1c s2c
+permutation s1s s1u s1r s1l s2l s2c s1c
+permutation s1s s1u s1r s2l s1l s1c s2c
+permutation s1s s1u s1r s2l s1l s2c s1c
+permutation s1s s1u s1r s2l s2c s1l s1c
+permutation s1s s1u s2l s1r s1l s1c s2c
+permutation s1s s1u s2l s1r s1l s2c s1c
+permutation s1s s1u s2l s1r s2c s1l s1c
+permutation s1s s2l s1u s2c s1r s1l s1c
+permutation s1s s2l s2c s1u s1r s1l s1c
+permutation s2l s1s s1u s2c s1r s1l s1c
+permutation s2l s1s s2c s1u s1r s1l s1c
+permutation s2l s2c s1s s1u s1r s1l s1c
diff --git a/src/test/isolation/specscanner.l b/src/test/isolation/specscanner.l
index 19883c265b..01f39fe05f 100644
--- a/src/test/isolation/specscanner.l
+++ b/src/test/isolation/specscanner.l
@@ -34,13 +34,19 @@ static void addlitchar(char c);


 %x sql
-%x qstr
+%x qident

 non_newline        [^\n\r]
 space            [ \t\r\f]

 comment            ("#"{non_newline}*)

+digit            [0-9]
+ident_start        [A-Za-z\200-\377_]
+ident_cont        [A-Za-z\200-\377_0-9\$]
+
+identifier        {ident_start}{ident_cont}*
+
 %%

 %{
@@ -48,32 +54,42 @@ comment            ("#"{non_newline}*)
     litbufsize = LITBUF_INIT;
 %}

+ /* Keywords (must appear before the {identifier} rule!) */
 permutation        { return PERMUTATION; }
 session            { return SESSION; }
 setup            { return SETUP; }
 step            { return STEP; }
 teardown        { return TEARDOWN; }

+ /* Whitespace and comments */
 [\n]            { yyline++; }
 {comment}        { /* ignore */ }
 {space}            { /* ignore */ }

- /* Quoted strings: "foo" */
+ /* Plain identifiers */
+{identifier}    {
+                    yylval.str = pg_strdup(yytext);
+                    return(identifier);
+                }
+
+ /* Quoted identifiers: "foo" */
 \"                {
                     litbufpos = 0;
-                    BEGIN(qstr);
+                    BEGIN(qident);
                 }
-<qstr>\"        {
+<qident>\"\"    { addlitchar(yytext[0]); }
+<qident>\"        {
                     litbuf[litbufpos] = '\0';
                     yylval.str = pg_strdup(litbuf);
                     BEGIN(INITIAL);
-                    return(string_literal);
+                    return(identifier);
                 }
-<qstr>.            { addlitchar(yytext[0]); }
-<qstr>\n        { yyerror("unexpected newline in quoted string"); }
-<qstr><<EOF>>    { yyerror("unterminated quoted string"); }
+<qident>.        { addlitchar(yytext[0]); }
+<qident>\n        { yyerror("unexpected newline in quoted identifier"); }
+<qident><<EOF>>    { yyerror("unterminated quoted identifier"); }

  /* SQL blocks: { UPDATE ... } */
+ /* We trim leading/trailing whitespace, otherwise they're unprocessed */
 "{"{space}*        {

                     litbufpos = 0;
@@ -96,6 +112,7 @@ teardown        { return TEARDOWN; }
                     yyerror("unterminated sql block");
                 }

+ /* Anything else is an error */
 .                {
                     fprintf(stderr, "syntax error at line %d: unexpected character \"%s\"\n", yyline, yytext);
                     exit(1);