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
Attachments:
isolation-setuplist-v1.patchapplication/octet-stream; name=isolation-setuplist-v1.patchDownload
*** a/src/test/isolation/isolationtester.c
--- b/src/test/isolation/isolationtester.c
***************
*** 512,520 **** run_permutation(TestSpec * testspec, int nsteps, Step ** steps)
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]));
*** a/src/test/isolation/isolationtester.h
--- b/src/test/isolation/isolationtester.h
***************
*** 42,48 **** typedef struct
typedef struct
{
! char *setupsql;
char *teardownsql;
Session **sessions;
int nsessions;
--- 42,49 ----
typedef struct
{
! char **setupsqls;
! int nsetupsqls;
char *teardownsql;
Session **sessions;
int nsessions;
*** a/src/test/isolation/specparse.y
--- b/src/test/isolation/specparse.y
***************
*** 35,41 **** TestSpec parseresult; /* result of parsing is left here */
--- 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
***************
*** 48,59 **** TestSpec parseresult; /* result of parsing is left here */
%%
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;
***************
*** 62,70 **** TestSpec:
}
;
opt_setup:
/* EMPTY */ { $$ = NULL; }
! | SETUP sqlblock { $$ = $2; }
;
opt_teardown:
--- 65,93 ----
}
;
+ setup_list:
+ setup_list setup
+ {
+ $$.elements = realloc($1.elements,
+ ($1.nelements + 1) * sizeof(void *));
+ $$.elements[$1.nelements] = $2;
+ $$.nelements = $1.nelements + 1;
+ }
+ | setup
+ {
+ $$.nelements = 1;
+ $$.elements = malloc(sizeof(void *));
+ $$.elements[0] = $1;
+ }
+ ;
+
opt_setup:
/* EMPTY */ { $$ = NULL; }
! | setup { $$ = $1; }
! ;
!
! setup:
! SETUP sqlblock { $$ = $2; }
;
opt_teardown:
"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
Attachments:
isolation-setuplist-v2.patchapplication/octet-stream; name=isolation-setuplist-v2.patchDownload
*** a/src/test/isolation/README
--- b/src/test/isolation/README
***************
*** 48,56 **** subdirectory. A test specification consists of four parts, in this order:
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> }
--- 48,58 ----
setup { <SQL> }
! Multiple setup blocks are allowed. Each is run separately (in the given
! order). Some statements, like VACUUM, need to be split to a separate
! block. Each 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 required.
teardown { <SQL> }
*** a/src/test/isolation/isolationtester.c
--- b/src/test/isolation/isolationtester.c
***************
*** 512,520 **** run_permutation(TestSpec * testspec, int nsteps, Step ** steps)
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]));
*** a/src/test/isolation/isolationtester.h
--- b/src/test/isolation/isolationtester.h
***************
*** 42,48 **** typedef struct
typedef struct
{
! char *setupsql;
char *teardownsql;
Session **sessions;
int nsessions;
--- 42,49 ----
typedef struct
{
! char **setupsqls;
! int nsetupsqls;
char *teardownsql;
Session **sessions;
int nsessions;
*** a/src/test/isolation/specparse.y
--- b/src/test/isolation/specparse.y
***************
*** 35,41 **** TestSpec parseresult; /* result of parsing is left here */
--- 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
***************
*** 48,59 **** TestSpec parseresult; /* result of parsing is left here */
%%
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;
***************
*** 62,70 **** TestSpec:
}
;
opt_setup:
/* EMPTY */ { $$ = NULL; }
! | SETUP sqlblock { $$ = $2; }
;
opt_teardown:
--- 65,93 ----
}
;
+ setup_list:
+ setup_list setup
+ {
+ $$.elements = realloc($1.elements,
+ ($1.nelements + 1) * sizeof(void *));
+ $$.elements[$1.nelements] = $2;
+ $$.nelements = $1.nelements + 1;
+ }
+ | setup
+ {
+ $$.nelements = 1;
+ $$.elements = malloc(sizeof(void *));
+ $$.elements[0] = $1;
+ }
+ ;
+
opt_setup:
/* EMPTY */ { $$ = NULL; }
! | setup { $$ = $1; }
! ;
!
! setup:
! SETUP sqlblock { $$ = $2; }
;
opt_teardown:
Import Notes
Resolved by subject fallback
"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
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
Import Notes
Resolved by subject fallback