Multiple setup steps for isolation tests

Started by Kevin Grittnerover 13 years ago6 messages
#1Kevin Grittner
Kevin.Grittner@wicourts.gov
1 attachment(s)

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:
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#1)
Re: Multiple setup steps for isolation tests

"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

#3Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#2)
1 attachment(s)
Re: Multiple setup steps for isolation tests

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:
#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#3)
Re: Multiple setup steps for isolation tests

"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

#5Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#4)
Re: Multiple setup steps for isolation tests

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

#6Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Kevin Grittner (#5)
Re: Multiple setup steps for isolation tests

"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