Thread: isolation check takes a long time

Why does the isolation check take such a long time? On some of my slower 
buildfarm members I am thinking of disabling it because it takes so 
long. This single test typically takes longer than a full serial 
standard regression test. Is there any way we could make it faster?

cheers

andrew

Excerpts from Andrew Dunstan's message of vie jul 13 16:05:37 -0400 2012:
> Why does the isolation check take such a long time? On some of my slower
> buildfarm members I am thinking of disabling it because it takes so
> long. This single test typically takes longer than a full serial
> standard regression test. Is there any way we could make it faster?

I think the "prepared transactions" test is the one that takes the
longest.  Which is a shame when prepared xacts are not enabled, because
all it does is throw millions of "prepared transactions are not enabled"
errors.  There is one other test that takes very long because it commits
a large amount of transactions.  I found it to be much faster if run
with fsync disabled.

Maybe it'd be a good idea to disable fsync on buildfarm runs, if we
don't already do so?

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Andrew Dunstan's message of dom jul 15 16:42:22 -0400 2012:

> I'm looking into that. But given that the default is to set
> max_prepared_transactions to 0, shouldn't we just remove that test from the
> normal installcheck schedule?
>
> We could provide an alternative schedule that does include it.

That's a thought -- AFAIR we do provide a numeric_big test that's not
exercised by the regular regress schedule, for a precedent.

However, there's more work to do in isolation testing.  It'd be good to
have it being routinely run in serializable isolation level, for
example, not just in read committed.  I wouldn't want to overload the
slowest machines in the buildfarm (some of which are already barely
capable of running the tests on all branches in a 24h schedule, of which
Stefan Kaltenbrunner is so proud), but if we could have a few of the
fastest members running isolation and isolation-serializable, with
max_prepared_transactions set to a nonzero value, that'd be great.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Tue, Jul 17, 2012 at 01:56:19PM -0400, Alvaro Herrera wrote:
> Excerpts from Andrew Dunstan's message of dom jul 15 16:42:22 -0400 2012:
> > I'm looking into that. But given that the default is to set
> > max_prepared_transactions to 0, shouldn't we just remove that test from the
> > normal installcheck schedule?

> That's a thought -- AFAIR we do provide a numeric_big test that's not
> exercised by the regular regress schedule, for a precedent.

It would be nice to have a pattern for adding tests run less often than "every
commit" but more often than "whenever a human explicitly remembers the test
and invokes it manually".  Perhaps a schedule that the recommended buildfarm
configuration would somehow run every two weeks and before each release
(including betas and branch releases).

> However, there's more work to do in isolation testing.  It'd be good to
> have it being routinely run in serializable isolation level, for
> example, not just in read committed.

Except for the foreign key specs, isolation test specs request a specific
isolation level when starting their transactions.  Running such specs under
different default_transaction_isolation settings primarily confirms that
"BEGIN TRANSACTION ISOLATION LEVEL x" is indistinguishable from "BEGIN" under
default_transaction_isolation = x.  It might also discover transaction
isolation sensitivity in the setup/cleanup steps, which often omit explicit
transaction control.  I don't think such verification justifies regularly
running thousands of tests.  The foreign key tests, however, would benefit
from running under all three isolation levels.  Let's control it per-spec
instead of repeating the entire suite.

On 07/17/2012 04:28 PM, Noah Misch wrote:
> On Tue, Jul 17, 2012 at 01:56:19PM -0400, Alvaro Herrera wrote:
>> Excerpts from Andrew Dunstan's message of dom jul 15 16:42:22 -0400 2012:
>>> I'm looking into that. But given that the default is to set
>>> max_prepared_transactions to 0, shouldn't we just remove that test from the
>>> normal installcheck schedule?
>> That's a thought -- AFAIR we do provide a numeric_big test that's not
>> exercised by the regular regress schedule, for a precedent.
> It would be nice to have a pattern for adding tests run less often than "every
> commit" but more often than "whenever a human explicitly remembers the test
> and invokes it manually".  Perhaps a schedule that the recommended buildfarm
> configuration would somehow run every two weeks and before each release
> (including betas and branch releases).


We have some support for that sort of thing. The optional_steps feature 
can run with a minimum number of hours between runs. Currently the only 
supported such steps are build_docs and find_typedefs. If there are 
extra tests we'd want run in that fashion then they would need code 
added for them.

Meanwhile, I would like to remove the prepared_transactions test from 
the main isolation schedule, and add a new Make target which runs that 
test explicitly. Is there any objection to that?

cheers

andrew

On 07/19/2012 09:54 AM, Andrew Dunstan wrote:
>
>
>
> Meanwhile, I would like to remove the prepared_transactions test from
> the main isolation schedule, and add a new Make target which runs that
> test explicitly. Is there any objection to that?
>
>


Here's the patch for that.

cheers

andrew

Excerpts from Andrew Dunstan's message of vie jul 20 13:15:12 -0400 2012:
>
> On 07/19/2012 09:54 AM, Andrew Dunstan wrote:
> >
> >
> >
> > Meanwhile, I would like to remove the prepared_transactions test from
> > the main isolation schedule, and add a new Make target which runs that
> > test explicitly. Is there any objection to that?
> >
> >
>
>
> Here's the patch for that.

Looks reasonable.  I'd like to have an "include" directive in regress
files so that we don't have to repeat the whole set in the second file,
but please don't let that wishlist item to stop you from committing this
patch.

Are you planning to have one of your animals run the prepared xacts
schedule? :-)

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Noah Misch's message of mar jul 17 16:28:32 -0400 2012:
>
> On Tue, Jul 17, 2012 at 01:56:19PM -0400, Alvaro Herrera wrote:

> > However, there's more work to do in isolation testing.  It'd be good to
> > have it being routinely run in serializable isolation level, for
> > example, not just in read committed.
>
> Except for the foreign key specs, isolation test specs request a specific
> isolation level when starting their transactions.  Running such specs under
> different default_transaction_isolation settings primarily confirms that
> "BEGIN TRANSACTION ISOLATION LEVEL x" is indistinguishable from "BEGIN" under
> default_transaction_isolation = x.  It might also discover transaction
> isolation sensitivity in the setup/cleanup steps, which often omit explicit
> transaction control.  I don't think such verification justifies regularly
> running thousands of tests.  The foreign key tests, however, would benefit
> from running under all three isolation levels.  Let's control it per-spec
> instead of repeating the entire suite.

Understood and agreed.  Maybe we could use a new directive in the spec
file format for this.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 07/20/2012 01:37 PM, Alvaro Herrera wrote:
> Excerpts from Andrew Dunstan's message of vie jul 20 13:15:12 -0400 2012:
>> On 07/19/2012 09:54 AM, Andrew Dunstan wrote:
>>>
>>>
>>> Meanwhile, I would like to remove the prepared_transactions test from
>>> the main isolation schedule, and add a new Make target which runs that
>>> test explicitly. Is there any objection to that?
>>>
>>>
>>
>> Here's the patch for that.
> Looks reasonable.  I'd like to have an "include" directive in regress
> files so that we don't have to repeat the whole set in the second file,
> but please don't let that wishlist item to stop you from committing this
> patch.
>
> Are you planning to have one of your animals run the prepared xacts
> schedule? :-)
>


Possibly - I'm tossing up in my mind the best way to do that. (Hacking 
the script on a particular client would be the wrong way.)

cheers

andrew

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Excerpts from Andrew Dunstan's message of vie jul 20 13:15:12 -0400 2012:
>>> Meanwhile, I would like to remove the prepared_transactions test from 
>>> the main isolation schedule, and add a new Make target which runs that 
>>> test explicitly. Is there any objection to that?

> Looks reasonable.  I'd like to have an "include" directive in regress
> files so that we don't have to repeat the whole set in the second file,
> but please don't let that wishlist item to stop you from committing this
> patch.

I'm not thrilled with replicating the test-list file either.  But it is
not necessary: look at the way the "bigtest" target is defined in the
main regression makefile.  You can just add some more test names on the
command line, to be done in addition to what's in the schedule file.
I think we should do it that way here too.
        regards, tom lane

On 07/20/2012 01:56 PM, Tom Lane wrote:
> I'm not thrilled with replicating the test-list file either.  But it is
> not necessary: look at the way the "bigtest" target is defined in the
> main regression makefile.  You can just add some more test names on the
> command line, to be done in addition to what's in the schedule file.
> I think we should do it that way here too.
>
>             


Oh, I wasn't aware of that. Will do.

cheers

andrew

On Fri, Jul 20, 2012 at 01:39:34PM -0400, Alvaro Herrera wrote:
> Excerpts from Noah Misch's message of mar jul 17 16:28:32 -0400 2012:
> > The foreign key tests, however, would benefit
> > from running under all three isolation levels.  Let's control it per-spec
> > instead of repeating the entire suite.
> 
> Understood and agreed.  Maybe we could use a new directive in the spec
> file format for this.

I was pondering something like this:
setting "i-rc" "isolation" = "READ COMMITTED"setting "i-rr" "isolation" = "REPEATABLE READ"
session "s1"setup        { BEGIN TRANSACTION ISOLATION LEVEL :isolation; }step "foo"    { SELECT 1; }
permutation "i-rc" "foo"permutation "i-rr" "foo"

That is, introduce psql-style variable substitutions in per-session "setup",
"step" and "teardown" directives.  Introduce the "setting" directive to
declare possible values for each variable.  Each permutation may name settings
as well as steps.  Order within the permutation would not matter; we could
allow them anywhere in the list or only at the beginning.  When the tester
generates permutations, it would include all variable setting combinations.

Thoughts?

On fre, 2012-07-20 at 13:15 -0400, Andrew Dunstan wrote:
> On 07/19/2012 09:54 AM, Andrew Dunstan wrote:
> >
> >
> >
> > Meanwhile, I would like to remove the prepared_transactions test from 
> > the main isolation schedule, and add a new Make target which runs that 
> > test explicitly. Is there any objection to that?
> >
> >
> 
> 
> Here's the patch for that.

Why was this backpatched to 9.1?  That doesn't seem appropriate.

Peter Eisentraut <peter_e@gmx.net> writes:
> On fre, 2012-07-20 at 13:15 -0400, Andrew Dunstan wrote:
>>> Meanwhile, I would like to remove the prepared_transactions test from 
>>> the main isolation schedule, and add a new Make target which runs that 
>>> test explicitly. Is there any objection to that?

> Why was this backpatched to 9.1?  That doesn't seem appropriate.

AIUI the point was to cut the load on buildfarm machines, so it won't
help (as much) if it's only applied to a subset of the branches that
have this test.  In any case, I don't follow your objection.  Removing
regression tests from stable branches is probably safer than removing
them from HEAD.
        regards, tom lane

Excerpts from Noah Misch's message of dom jul 22 17:11:53 -0400 2012:

> I was pondering something like this:
>
>     setting "i-rc" "isolation" = "READ COMMITTED"
>     setting "i-rr" "isolation" = "REPEATABLE READ"
>
>     session "s1"
>     setup        { BEGIN TRANSACTION ISOLATION LEVEL :isolation; }
>     step "foo"    { SELECT 1; }
>
>     permutation "i-rc" "foo"
>     permutation "i-rr" "foo"
>
> That is, introduce psql-style variable substitutions in per-session "setup",
> "step" and "teardown" directives.  Introduce the "setting" directive to
> declare possible values for each variable.  Each permutation may name settings
> as well as steps.  Order within the permutation would not matter; we could
> allow them anywhere in the list or only at the beginning.  When the tester
> generates permutations, it would include all variable setting combinations.

The idea of using psql-style variables seems good to me.

Offhand having the setting names appear in permutations just like step
names seems a bit weird to me, though I admit that I don't see any other
way that's not overly verbose.

I would expect that if no permutations are specified, all possible
values for a certain setting would be generated.  That way it'd be easy
to define tests that run through all possible permutations of two (or
more) sequences of commands on all isolation levels, without having the
specify them all by hand.  With that in mind, having each possible value
for a setting be declared independently might be a bit troublesome.

Something like
    setting "isolevel" "isolation" = { "READ COMMITTED", "REPEATABLE READ" }    session "s1"    setup        { BEGIN
TRANSACTIONISOLATION LEVEL :isolation; }    step "foo"    { SELECT 1; }    permutation "isolevel" "foo" 

Maybe we can have a single name for both the setting specification and
variable name, instead of having to invent two names; in that case, it'd
reduce to
    setting "isolation" = { "READ COMMITTED", "REPEATABLE READ" }    session "s1"    setup        { BEGIN TRANSACTION
ISOLATIONLEVEL :isolation; }    step "foo"    { SELECT 1; }    permutation "isolation" "foo" 

Thoughts?

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Tue, Jul 24, 2012 at 11:08:09AM -0400, Alvaro Herrera wrote:
> Excerpts from Noah Misch's message of dom jul 22 17:11:53 -0400 2012:
> >     setting "i-rc" "isolation" = "READ COMMITTED"
> >     setting "i-rr" "isolation" = "REPEATABLE READ"
> > 
> >     session "s1"
> >     setup        { BEGIN TRANSACTION ISOLATION LEVEL :isolation; }
> >     step "foo"    { SELECT 1; }
> > 
> >     permutation "i-rc" "foo"
> >     permutation "i-rr" "foo"

> The idea of using psql-style variables seems good to me.

> I would expect that if no permutations are specified, all possible
> values for a certain setting would be generated.  That way it'd be easy
> to define tests that run through all possible permutations of two (or
> more) sequences of commands on all isolation levels, without having the
> specify them all by hand.

Yes; I intended the same.

> With that in mind, having each possible value
> for a setting be declared independently might be a bit troublesome.

I wouldn't expect any of these syntax variants to substantially simplify or
complicate the code for generating all possible permutations.  We can choose
based on convenience and flexibility for spec authors.

> Something like 
> 
>      setting "isolevel" "isolation" = { "READ COMMITTED", "REPEATABLE READ" }
>  
>      session "s1"
>      setup        { BEGIN TRANSACTION ISOLATION LEVEL :isolation; }
>      step "foo"    { SELECT 1; }
>  
>      permutation "isolevel" "foo"
> 
> Maybe we can have a single name for both the setting specification and
> variable name, instead of having to invent two names; in that case, it'd
> reduce to
> 
>      setting "isolation" = { "READ COMMITTED", "REPEATABLE READ" }
>  
>      session "s1"
>      setup        { BEGIN TRANSACTION ISOLATION LEVEL :isolation; }
>      step "foo"    { SELECT 1; }
>  
>      permutation "isolation" "foo"
> 
> Thoughts?

Between those two, I prefer the second -- having two names for each variable
does not add much.  However, both lose some flexibility by not giving a name
to each possible value.  'permutation "isolation" "foo"' actually requests two
runs through the test case, one with each variable value.  There would be no
way to specify permutations to run with fewer than the full range of values.
I lean toward preserving that flexibility.  That being said, perhaps a
one-statement syntax remains nicer?
var "isolation" = { "rc" => "READ COMMITTED", "rr" => "REPEATABLE READ" }

Excerpts from Noah Misch's message of jue jul 26 06:28:54 -0400 2012:
>
> On Tue, Jul 24, 2012 at 11:08:09AM -0400, Alvaro Herrera wrote:

> > I would expect that if no permutations are specified, all possible
> > values for a certain setting would be generated.  That way it'd be easy
> > to define tests that run through all possible permutations of two (or
> > more) sequences of commands on all isolation levels, without having the
> > specify them all by hand.
>
> Yes; I intended the same.

Okay, great.

> > With that in mind, having each possible value
> > for a setting be declared independently might be a bit troublesome.
>
> I wouldn't expect any of these syntax variants to substantially simplify or
> complicate the code for generating all possible permutations.  We can choose
> based on convenience and flexibility for spec authors.

Understood.


> Between those two, I prefer the second -- having two names for each variable
> does not add much.  However, both lose some flexibility by not giving a name
> to each possible value.  'permutation "isolation" "foo"' actually requests two
> runs through the test case, one with each variable value.  There would be no
> way to specify permutations to run with fewer than the full range of values.
> I lean toward preserving that flexibility.  That being said, perhaps a
> one-statement syntax remains nicer?
>
>     var "isolation" = { "rc" => "READ COMMITTED", "rr" => "REPEATABLE READ" }

Agreed.  What would be the syntax to specify a particular value to use
in a permutation?  Maybe

permutation isolation=rr "step1" "step2" ...

I'm not sure about requiring quotes around those identifiers.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Thu, Jul 26, 2012 at 12:16:29PM -0400, Alvaro Herrera wrote:
> Excerpts from Noah Misch's message of jue jul 26 06:28:54 -0400 2012:
> >     var "isolation" = { "rc" => "READ COMMITTED", "rr" => "REPEATABLE READ" }
> 
> Agreed.  What would be the syntax to specify a particular value to use
> in a permutation?  Maybe
> 
> permutation isolation=rr "step1" "step2" ...

I figured just 'permutation "rr" "step1" "step2"', in keeping with the
tendency toward terseness already found in the implementation.  It would then
be an error to use a name more than once among specs and variable.

> I'm not sure about requiring quotes around those identifiers.

Let's keep it consistent with the requirement in other contexts of the spec
file.  I generally favor a policy of optional quotes, but that could be its
own patch.

Thanks,
nm