Обсуждение: Multiple setup steps for isolation tests

I just today found that the index-only scan feature has broken SSI.
I don't think it will take much to fix, and I'm looking at that, but
the first thing I wanted was a test to show the breakage. I couldn't
find a way to do that without running VACUUM after loading data to
the test tables, and because VACUUM refuses to run in a
multi-statement batch I propose the following patch to the isolation
testing code, which allows multiple setup blocks. Using this code I
now have an isolation test to show the breakage.
 
If there are no objections, I will apply this to HEAD and 9.2.
 
I'm working on a fix to the bug itself.
 
-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> I just today found that the index-only scan feature has broken SSI. 
> I don't think it will take much to fix, and I'm looking at that, but
> the first thing I wanted was a test to show the breakage.

Ugh.  That sounds like a release-blocker.  What's your ETA for a fix?

> I couldn't
> find a way to do that without running VACUUM after loading data to
> the test tables, and because VACUUM refuses to run in a
> multi-statement batch I propose the following patch to the isolation
> testing code, which allows multiple setup blocks.  Using this code I
> now have an isolation test to show the breakage.
> If there are no objections, I will apply this to HEAD and 9.2.

The grammar changes look wrong: I think you eliminated the ability to
have zero setup steps, no?  Instead, setup_list should expand to either
empty or "setup_list setup".
        regards, tom lane

> Tom Lane  wrote:
> "Kevin Grittner"  writes:
>> I just today found that the index-only scan feature has broken
>> SSI.  I don't think it will take much to fix, and I'm looking at
>> that, but the first thing I wanted was a test to show the
>> breakage.
>
> Ugh. That sounds like a release-blocker. What's your ETA for a fix?

I have a fix now.  I just got done testing it.  I will post right
after this, and can apply as soon as I know there are no objections.

>> I couldn't find a way to do that without running VACUUM after
>> loading data to the test tables, and because VACUUM refuses to run
>> in a multi-statement batch I propose the following patch to the
>> isolation testing code, which allows multiple setup blocks. Using
>> this code I now have an isolation test to show the breakage.
>
>> If there are no objections, I will apply this to HEAD and 9.2.
>
> The grammar changes look wrong: I think you eliminated the ability
> to have zero setup steps, no? Instead, setup_list should expand to
> either empty or "setup_list setup".

I tried that first, but had shift/reduce conflicts.  I noticed that
there were no *tests* without setup so far, and it's hard to imagine
when that would be sensible, so I didn't feel too bad requiring the
setup list for the test but leaving a single, optional, setup for
each connection.  If you can suggest how I could move to a list and
still keep it optional without the shift/reduce problems, I'd be
happy to do it.  I just didn't see any obvious way to do it.  But
then, I haven't done a lot in flex.

New version of this patch attached.  I think the only change is that
I modified the README file.

-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
>> Tom Lane  wrote:
>> The grammar changes look wrong: I think you eliminated the ability
>> to have zero setup steps, no? Instead, setup_list should expand to
>> either empty or "setup_list setup".

> I tried that first, but had shift/reduce conflicts.

[ scratches head ... ]  Dunno what you did exactly, but the attached
version works fine for me.

            regards, tom lane


diff --git a/src/test/isolation/README b/src/test/isolation/README
index dc96242883ab850e4883780b2f8893bd6eb64844..69095778c6c4c331c6d72e4d4dbcd48b8edd9b3d 100644
*** a/src/test/isolation/README
--- b/src/test/isolation/README
*************** subdirectory. A test specification consi
*** 49,56 ****
  setup { <SQL> }

    The given SQL block is executed once, in one session only, before running
!   the test. Create any test tables or other required objects here. This
!   part is optional.

  teardown { <SQL> }

--- 49,60 ----
  setup { <SQL> }

    The given SQL block is executed once, in one session only, before running
!   the test.  Create any test tables or other required objects here.  This
!   part is optional.  Multiple setup blocks are allowed if needed; each is
!   run separately, in the given order.  (The reason for allowing multiple
!   setup blocks is that each block is run as a single PQexec submission,
!   and some statements such as VACUUM cannot be combined with others in such
!   a block.)

  teardown { <SQL> }

diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index 98f89da6bff842bfbc21be3f92691561c9b23f3a..4c4556654b390b8520289a942f8454f867bbbe51 100644
*** a/src/test/isolation/isolationtester.c
--- b/src/test/isolation/isolationtester.c
*************** run_permutation(TestSpec * testspec, int
*** 512,520 ****
      printf("\n");

      /* Perform setup */
!     if (testspec->setupsql)
      {
!         res = PQexec(conns[0], testspec->setupsql);
          if (PQresultStatus(res) != PGRES_COMMAND_OK)
          {
              fprintf(stderr, "setup failed: %s", PQerrorMessage(conns[0]));
--- 512,520 ----
      printf("\n");

      /* Perform setup */
!     for (i = 0; i < testspec->nsetupsqls; i++)
      {
!         res = PQexec(conns[0], testspec->setupsqls[i]);
          if (PQresultStatus(res) != PGRES_COMMAND_OK)
          {
              fprintf(stderr, "setup failed: %s", PQerrorMessage(conns[0]));
diff --git a/src/test/isolation/isolationtester.h b/src/test/isolation/isolationtester.h
index 1f61c6f1275df2d2e0516311eb7ee7e135181401..4c986bb52334be6bae4c9561d425cb6bf0fdc0ff 100644
*** a/src/test/isolation/isolationtester.h
--- b/src/test/isolation/isolationtester.h
*************** typedef struct
*** 42,48 ****

  typedef struct
  {
!     char       *setupsql;
      char       *teardownsql;
      Session   **sessions;
      int            nsessions;
--- 42,49 ----

  typedef struct
  {
!     char      **setupsqls;
!     int         nsetupsqls;
      char       *teardownsql;
      Session   **sessions;
      int            nsessions;
diff --git a/src/test/isolation/specparse.y b/src/test/isolation/specparse.y
index 9d2b1a277f03a5e2ddc05b4c5dc408f961bb12bf..bf3a9f3b5058c4c08b06870b39e4bc9fa458b205 100644
*** a/src/test/isolation/specparse.y
--- b/src/test/isolation/specparse.y
*************** TestSpec        parseresult;            /* result of pa
*** 35,41 ****
--- 35,43 ----
      }            ptr_list;
  }

+ %type <ptr_list> setup_list
  %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_list
  %type <session> session
*************** TestSpec        parseresult;            /* result of pa
*** 48,59 ****
  %%

  TestSpec:
!             opt_setup
              opt_teardown
              session_list
              opt_permutation_list
              {
!                 parseresult.setupsql = $1;
                  parseresult.teardownsql = $2;
                  parseresult.sessions = (Session **) $3.elements;
                  parseresult.nsessions = $3.nelements;
--- 50,62 ----
  %%

  TestSpec:
!             setup_list
              opt_teardown
              session_list
              opt_permutation_list
              {
!                 parseresult.setupsqls = (char **) $1.elements;
!                 parseresult.nsetupsqls = $1.nelements;
                  parseresult.teardownsql = $2;
                  parseresult.sessions = (Session **) $3.elements;
                  parseresult.nsessions = $3.nelements;
*************** TestSpec:
*** 62,70 ****
              }
          ;

  opt_setup:
              /* EMPTY */            { $$ = NULL; }
!             | SETUP sqlblock    { $$ = $2; }
          ;

  opt_teardown:
--- 65,92 ----
              }
          ;

+ setup_list:
+             /* EMPTY */
+             {
+                 $$.elements = NULL;
+                 $$.nelements = 0;
+             }
+             | setup_list setup
+             {
+                 $$.elements = realloc($1.elements,
+                                       ($1.nelements + 1) * sizeof(void *));
+                 $$.elements[$1.nelements] = $2;
+                 $$.nelements = $1.nelements + 1;
+             }
+         ;
+
  opt_setup:
              /* EMPTY */            { $$ = NULL; }
!             | setup                { $$ = $1; }
!         ;
!
! setup:
!             SETUP sqlblock        { $$ = $2; }
          ;

  opt_teardown:

> Tom Lane  wrote:
> "Kevin Grittner"  writes:
>>> Tom Lane wrote:
>>> The grammar changes look wrong: I think you eliminated the
>>> ability to have zero setup steps, no? Instead, setup_list should
>>> expand to either empty or "setup_list setup".
> 
>> I tried that first, but had shift/reduce conflicts.
> 
> [ scratches head ... ] Dunno what you did exactly, but the attached
> version works fine for me.
[ slaps forhead ]
Yeah, that should do it.  Will apply.
Thanks.
-Kevin

"Kevin Grittner"  wrote:
> Tom Lane  wrote:
>> the attached version works fine for me.> 
> Yeah, that should do it.  Will apply.
Pushed to master and REL9_2_STABLE.
-Kevin