[OSSTEST PATCH 1/2] Executive DB: Eliminate SQL locking for read-only transactions

[OSSTEST PATCH 1/2] Executive DB: Eliminate SQL locking for read-only transactions

[Ian Jackson]

Our transactions generally run with
  SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
(which, incidentally, does not mean that the transactions are
necessarily serialisable!)

In SQL in general it is possible for a read-only transaction to fail
and need to be retried because some writer has updated things.

However, in PostgreSQL this is not possible because Postgres uses
multi-version concurrency control: it retains the old version of the
data while the read transaction is open:
  http://www.postgresql.org/docs/8.3/static/transaction-iso.html

(And, of course, SQLite uses MVCC too, and all transactions in SQLite
are fully serialisable.)

So it is not necessary for these read-only operations to take out
locks.  When they do so they can unnecessarily block other important
work for long periods of time.

With this change, we go further from the ability to support databases
other than PostgreSQL and SQLite.  However, such support was very
distant anyway because of differences in SQL syntax and semantics, our
reliance in Executive mode on sql's command line utilities, and so on.

We retain the db_retry framing because (a) although the retry loop is
not necessary in these cases, the transaction framing is (b) it will
make it slightly easier to reverse this decision in the future if we
ever decide to do so (c) it is less code churn.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 Osstest/Executive.pm   |    6 ++++--
 cr-ensure-disk-space   |    2 +-
 ms-planner             |    3 ++-
 sg-report-flight       |    2 +-
 sg-report-host-history |    8 ++++----
 sg-report-job-history  |    2 +-
 6 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/Osstest/Executive.pm b/Osstest/Executive.pm
index 84c7d46..d9c698a 100644
--- a/Osstest/Executive.pm
+++ b/Osstest/Executive.pm
@@ -90,8 +90,10 @@ our (@all_lock_tables) = qw(flights resources);
 #
 # READS:
 #
-#  Nontransactional reads are also permitted
-#  Transactional reads must take out locks as if they were modifying
+#  Nontransactional and transactional reads are also permitted
+#  and do not need to take out any locks.  (We support only databases
+#  using multi-version concurrency control, which includes PostgreSQL
+#  and SQLite.)
 
 augmentconfigdefaults(
     ControlDaemonHost => 'control-daemons',
diff --git a/cr-ensure-disk-space b/cr-ensure-disk-space
index 138f3f4..a1f838f 100755
--- a/cr-ensure-disk-space
+++ b/cr-ensure-disk-space
@@ -145,7 +145,7 @@ END
     return 1;
 }
 
-db_retry($dbh_tests,[qw(flights)], sub {
+db_retry($dbh_tests,[], sub {
     @flights = ();
     for (;;) {
 	iteration_continue()
diff --git a/ms-planner b/ms-planner
index cab32c9..fc65a5b 100755
--- a/ms-planner
+++ b/ms-planner
@@ -57,7 +57,8 @@ my $fn= "data-$walker.pl";
 
 sub allocations ($$) {
     my ($init, $each) = @_;
-    db_retry($dbh_tests, \@all_lock_tables, sub {
+
+    db_retry($dbh_tests, [], sub {
 	$init->();
 	
 	our $resources_q ||= $dbh_tests->prepare(<<END);
diff --git a/sg-report-flight b/sg-report-flight
index bbe03fe..db0d922 100755
--- a/sg-report-flight
+++ b/sg-report-flight
@@ -1284,7 +1284,7 @@ END
     rename "$htmlout.new", $htmlout or die $!;
 }
 
-db_begin_work($dbh_tests, [qw(flights)]);
+db_begin_work($dbh_tests, []);
 findspecflight();
 my $fi= examineflight($specflight);
 my @fails= justifyfailures($fi);
diff --git a/sg-report-host-history b/sg-report-host-history
index 79dc519..9d39483 100755
--- a/sg-report-host-history
+++ b/sg-report-host-history
@@ -259,7 +259,7 @@ END
     rename "$html_file.new", "$html_file" or die "$html_file $!";
 }
 
-db_retry($dbh_tests, [qw(flights resources)], sub {
+db_retry($dbh_tests, [], sub {
     computeflightsrange();
 });
 
@@ -271,7 +271,7 @@ $dbh_tests->do("SET LOCAL enable_seqscan=false");
 foreach my $host (@ARGV) {
     if ($host =~ m/^flight:/) {
 	my $flight=$'; #';
-	db_retry($dbh_tests, [qw(flights)], sub {
+	db_retry($dbh_tests, [], sub {
 	    our $hostsinflightq //= db_prepare(<<END);
 	        SELECT DISTINCT val
 		  FROM runvars
@@ -292,12 +292,12 @@ END
 
 exit 0 unless %hosts;
 
-db_retry($dbh_tests, [qw(flights)], sub {
+db_retry($dbh_tests, [], sub {
     mainquery();
 });
 
 foreach my $host (sort keys %hosts) {
-    db_retry($dbh_tests, [qw(flights)], sub {
+    db_retry($dbh_tests, [], sub {
 	reporthost $host;
     });
 }
diff --git a/sg-report-job-history b/sg-report-job-history
index 0e2a3f9..0ca441c 100755
--- a/sg-report-job-history
+++ b/sg-report-job-history
@@ -298,5 +298,5 @@ sub processjob ($) {
     processjobbranch($j,$_) foreach @branches;
 }
 
-db_begin_work($dbh_tests, [qw(flights)]);
+db_begin_work($dbh_tests, []);
 foreach my $j (@jobs) { processjob($j); }
-- 
1.7.10.4

[OSSTEST PATCH 2/2] Executive DB: Reduce strength of DB locks

[Ian Jackson]

The purpose of these locks is partly to prevent transactions being
aborted (which I'm not sure the existing code would in practice cope
with, although this is a bug) and also to avoid bugs due to the fact
that
  SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
not mean that the transactions are necessarily serialisable!
  http://www.postgresql.org/docs/8.3/static/transaction-iso.html

In SQL in general it is possible for read-only transactions to
conflict with writing transactions.

However, in PostgreSQL this is not a problem because Postgres uses
multi-version concurrency control: it retains the old version of the
data while the read transaction is open:
  http://www.postgresql.org/docs/8.3/static/transaction-iso.html

So a read transaction cannot cause a write transaction to abort, nor
vice versa.  So there is no need to have the database explicit table
locks prevent concurrent read access.

Preventing concurrent read access means that simple and urgent updates
can be unnecessarily delayed by long-running reader transactions in
the history reporters and archaelogists.

So, reduce the lock mode from ACCESS EXCLUSIVE to ACCESS.  This still
conflicts with all kinds of updates and prospective updates, but no
longer with SELECT:
  http://www.postgresql.org/docs/8.3/static/explicit-locking.html

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 Osstest/JobDB/Executive.pm |    2 +-
 tcl/JobDB-Executive.tcl    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Osstest/JobDB/Executive.pm b/Osstest/JobDB/Executive.pm
index 69cb276..124e7c0 100644
--- a/Osstest/JobDB/Executive.pm
+++ b/Osstest/JobDB/Executive.pm
@@ -43,7 +43,7 @@ sub begin_work ($$$) { #method
     
     return if $ENV{'OSSTEST_DEBUG_NOSQLLOCK'};
     foreach my $tab (@$tables) {
-        $dbh->do("LOCK TABLE $tab IN ACCESS EXCLUSIVE MODE");
+        $dbh->do("LOCK TABLE $tab IN EXCLUSIVE MODE");
     }
 }
 
diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index f37bbaf..a3dbb9e 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -133,7 +133,7 @@ proc lock-tables {tables} {
     # must be inside transaction
     foreach tab $tables {
         db-execute "
-		LOCK TABLE $tab IN ACCESS EXCLUSIVE MODE
+		LOCK TABLE $tab IN EXCLUSIVE MODE
         "
     }
 }
-- 
1.7.10.4

Re: [OSSTEST PATCH 1/2] Executive DB: Eliminate SQL locking for read-only transactions

[Ian Campbell]

On Fri, 2015-12-11 at 16:23 +0000, Ian Jackson wrote:
> Our transactions generally run with
>   SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
> (which, incidentally, does not mean that the transactions are
> necessarily serialisable!)
> 
> In SQL in general it is possible for a read-only transaction to fail
> and need to be retried because some writer has updated things.
> 
> However, in PostgreSQL this is not possible because Postgres uses
> multi-version concurrency control: it retains the old version of the
> data while the read transaction is open:
>   http://www.postgresql.org/docs/8.3/static/transaction-iso.html
> 
> (And, of course, SQLite uses MVCC too, and all transactions in SQLite
> are fully serialisable.)
> 
> So it is not necessary for these read-only operations to take out
> locks.  When they do so they can unnecessarily block other important
> work for long periods of time.
> 
> With this change, we go further from the ability to support databases
> other than PostgreSQL and SQLite.  However, such support was very
> distant anyway because of differences in SQL syntax and semantics, our
> reliance in Executive mode on sql's command line utilities, and so on.
> 
> We retain the db_retry framing because (a) although the retry loop is
> not necessary in these cases, the transaction framing is (b) it will
> make it slightly easier to reverse this decision in the future if we
> ever decide to do so (c) it is less code churn.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

(I haven't actually checked that everywhere you have changed is actually
r/o, nor whether you have found every r/o operation).


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [OSSTEST PATCH 2/2] Executive DB: Reduce strength of DB locks

[Ian Campbell]

On Fri, 2015-12-11 at 16:23 +0000, Ian Jackson wrote:
> The purpose of these locks is partly to prevent transactions being
> aborted (which I'm not sure the existing code would in practice cope
> with, although this is a bug) and also to avoid bugs due to the fact
> that
>   SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
> not mean that the transactions are necessarily serialisable!

 ^does not?

>   http://www.postgresql.org/docs/8.3/static/transaction-iso.html
> 
> In SQL in general it is possible for read-only transactions to
> conflict with writing transactions.
> 
> However, in PostgreSQL this is not a problem because Postgres uses
> multi-version concurrency control: it retains the old version of the
> data while the read transaction is open:
>   http://www.postgresql.org/docs/8.3/static/transaction-iso.html
> 
> So a read transaction cannot cause a write transaction to abort, nor
> vice versa.  So there is no need to have the database explicit table
> locks prevent concurrent read access.
> 
> Preventing concurrent read access means that simple and urgent updates
> can be unnecessarily delayed by long-running reader transactions in
> the history reporters and archaelogists.

                            archaeologists

> So, reduce the lock mode from ACCESS EXCLUSIVE to ACCESS.  This still
> conflicts with all kinds of updates and prospective updates, but no
> longer with SELECT:
>   http://www.postgresql.org/docs/8.3/static/explicit-locking.html
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  Osstest/JobDB/Executive.pm |    2 +-
>  tcl/JobDB-Executive.tcl    |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Osstest/JobDB/Executive.pm b/Osstest/JobDB/Executive.pm
> index 69cb276..124e7c0 100644
> --- a/Osstest/JobDB/Executive.pm
> +++ b/Osstest/JobDB/Executive.pm
> @@ -43,7 +43,7 @@ sub begin_work ($$$) { #method
>      
>      return if $ENV{'OSSTEST_DEBUG_NOSQLLOCK'};
>      foreach my $tab (@$tables) {
> -        $dbh->do("LOCK TABLE $tab IN ACCESS EXCLUSIVE MODE");
> +        $dbh->do("LOCK TABLE $tab IN EXCLUSIVE MODE");
>      }
>  }
>  
> diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
> index f37bbaf..a3dbb9e 100644
> --- a/tcl/JobDB-Executive.tcl
> +++ b/tcl/JobDB-Executive.tcl
> @@ -133,7 +133,7 @@ proc lock-tables {tables} {
>      # must be inside transaction
>      foreach tab $tables {
>          db-execute "
> -		LOCK TABLE $tab IN ACCESS EXCLUSIVE MODE
> +		LOCK TABLE $tab IN EXCLUSIVE MODE
>          "
>      }
>  }

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [OSSTEST PATCH 1/2] Executive DB: Eliminate SQL locking for read-only transactions

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH 1/2] Executive DB: Eliminate SQL locking for read-only transactions"):
> On Fri, 2015-12-11 at 16:23 +0000, Ian Jackson wrote:
> > We retain the db_retry framing because (a) although the retry loop is
> > not necessary in these cases, the transaction framing is (b) it will
> > make it slightly easier to reverse this decision in the future if we
> > ever decide to do so (c) it is less code churn.
> > 
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> (I haven't actually checked that everywhere you have changed is actually
> r/o

Justifying that claim, from the diffstat:


Comment only:

 Osstest/Executive.pm   |    6 ++++--


These two I have checked, but their function is indeed RO:

 cr-ensure-disk-space   |    2 +-
 ms-planner             |    3 ++-

(Note that `ms-planner' just manipulates data-plan.pl et al; it is
left to the clients to actually obtain resources in the DB.)


These are clearly RO:

 sg-report-flight       |    2 +-
 sg-report-host-history |    8 ++++----
 sg-report-job-history  |    2 +-


> nor whether you have found every r/o operation).

I may have missed some, but the obvious potential remaining
long-running lockholder would be cs-bisection-step.  I have looked at
cs-bisection step and:

 - It does not seem to take any lock, or even begin a transaction, for
   its primary archaeology function.  I think this is fine because if
   some test results or whatever show up later it ought not to break
   the algorithm.

 - The flight construction _is_ within a lock, in populateflight, but
   it basically just clones a template and merges in information from
   the archaeologist part.

 - There is the check in `checkrepetition' - the flail test.  This is
   not a trivial transaction and it would be better if it didn't take
   the lock.  It is only not read-only because it ends, in the error
   case, with an UPDATE to set the flight broken.  This could be in a
   separate transaction given that no-one else is supposed to be
   messing about with the flight that cs-bisection-step is
   constructing.  But this is not a trivial change.

Ian.

Re: [OSSTEST PATCH 2/2] Executive DB: Reduce strength of DB locks

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH 2/2] Executive DB: Reduce strength of DB locks"):
> On Fri, 2015-12-11 at 16:23 +0000, Ian Jackson wrote:
> >   SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
> > not mean that the transactions are necessarily serialisable!
> 
>  ^does not?

Yes, thanks.

> > Preventing concurrent read access means that simple and urgent updates
> > can be unnecessarily delayed by long-running reader transactions in
> > the history reporters and archaelogists.
> 
>                             archaeologists

Fixed.

> > So, reduce the lock mode from ACCESS EXCLUSIVE to ACCESS.  This still
> > conflicts with all kinds of updates and prospective updates, but no
> > longer with SELECT:
> >   http://www.postgresql.org/docs/8.3/static/explicit-locking.html
> > 
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks,
Ian.