[OSSTEST PATCH 0/9] db retry: fixes and workarounds

[OSSTEST PATCH 0/9] db retry: fixes and workarounds

[Ian Jackson]

We have here:
 * Two patches to improve mg-schema-test-database a bit.
 * Fixes to cs-bisection-step, to survive and give right answers
   on db retry.
 * An attempt at a workarounds for a strange DBD::Pg behaviour.

The workaround seems to WFM in my ad-hoc tests and I propose to deploy
it in the hope that it will work in production too.


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

[OSSTEST PATCH 1/9] mg-schema-test-database: Revamp sequence handling

[Ian Jackson]

The initial value (at creation time) of a sequence appears in the
schema, but is not of any consequence.  To avoid the schema diff check
failing in databases created in a slightly different way, it is
necessary to copy the actual original initial sequence value for each
sequence.

Replace the sequence handling code with a setup which, for each
sequence, copies the START WITH and calculates a fresh RESTART WITH.

This replaces both the unconditional copy (done with pgdump) and the
special calculation of the next flight number.  Now all sequences have
the "bump the number somewhat" treatment, which seems nice.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 mg-schema-test-database | 43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/mg-schema-test-database b/mg-schema-test-database
index 5ebba39..a82c044 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -413,6 +413,28 @@ END
 
 	withtest ./mg-schema-update -q apply $wantupdates
 
+        printf " (seqs)"
+        seq_alters=""
+        for seq in $sequences; do
+        	orgseqval=$(psql_query <<END
+			SELECT start_value || ' ' || last_value FROM $seq
+END
+                )
+                read orgstart orglast <<END
+$orgseqval
+END
+		newlast=$(( 10000 * (2 + $orglast / 10000) ))
+                seq_alters+="
+			ALTER SEQUENCE $seq
+				START WITH $orgstart
+				RESTART WITH $newlast;
+"
+        done
+	(withtest psql_do <<END
+$seq_alters
+END
+        )
+
 	printf ".\n"
 
 	# Schema should now be identical to main DB
@@ -444,14 +466,6 @@ END
 		SET CONSTRAINTS ALL DEFERRED;
 END
 
-	$(get_pgdump_cmd) -a -O -x ${sequences// / -t } >$t.sequences-import
-	perl <$t.sequences-import >>$t.import -ne '
-		next if m/^--/;
-		next if m/^SET /;
-		next unless m/\S/;
-		print or die $!;
-	'
-
 	for table in $tables; do
 		case " $ftables " in
 		*" $table "*)	condition="flight >= $minflight" ;;
@@ -490,19 +504,6 @@ END
 
 	rm -f $t.tabledata.*
 
-	printf "flightseq..."
-
-	lastflight=$(psql_query <<END
-		SELECT last_value FROM flights_flight_seq
-END
-		)
-	newlastflight=$(( 10000 * (2 + $lastflight / 10000) ))
-
-	withtest psql_do <<END
-		ALTER SEQUENCE flights_flight_seq
-			RESTART WITH $newlastflight;
-END
-
 	#---------- actually borrow resources ----------
 
 	printf "borrow..."
-- 
2.1.4


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

[OSSTEST PATCH 2/9] mg-schema-test-database: Wrap some withtest psql_do in subshells

[Ian Jackson]

Otherwise it takes effect for the rest of the script, which is not
what is wanted !  As it happens, there are no accesses to the real db
after this point, so this bug is latent.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 mg-schema-test-database | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/mg-schema-test-database b/mg-schema-test-database
index a82c044..0517ca3 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -509,19 +509,20 @@ END
 	printf "borrow..."
 
 	for task in $tasks; do
-		withtest psql_do <<END
+		(withtest psql_do <<END
 			BEGIN;
 			UPDATE resources
 				SET owntaskid = $(taskid magic idle)
 				WHERE owntaskid = $(borrowtaskid $task);
 			COMMIT;
 END
+                )
 	done
-	withtest psql_do <<END
+	(withtest psql_do <<END
 		DELETE FROM tasks
 			WHERE type='xdbref' AND refkey='$dbname';
 END
-
+        )
 	printf "\n"
 
 	cat <<END
-- 
2.1.4


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

[OSSTEST PATCH 3/9] cs-bisection-step: Do not acquire the repo lock

[Ian Jackson]

This is only required to avoid a lock inversion between the repo lock
and database table locks, but we have no explicit database table locks
any more.

We do not want to hold the repo lock for an extended period,
particularly when we are running a database retry loop.

In practice, currently, cs-bisection-step always fails with a database
serialisation error.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 cs-bisection-step | 1 -
 1 file changed, 1 deletion(-)

diff --git a/cs-bisection-step b/cs-bisection-step
index a0595b2..4c71587 100755
--- a/cs-bisection-step
+++ b/cs-bisection-step
@@ -1405,7 +1405,6 @@ sub compute_exitstatus () {
 }
 
 csreadconfig();
-grabrepolock_reexec(@org_argv);
 reportcmdline();
 findbasics();
 digraph_whole();
-- 
2.1.4


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

[OSSTEST PATCH 4/9] db retry, bisection: Reset %jobs_created on db retry

[Ian Jackson]

%jobs_created is used for memoisation while populating the destination
flight.  We need to reset it when we restart flight construction,
because those jobs were created in the discarded transaction.

Otherwise we could create a flight with missing jobs.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 cs-bisection-step | 1 +
 1 file changed, 1 insertion(+)

diff --git a/cs-bisection-step b/cs-bisection-step
index 4c71587..1d1962a 100755
--- a/cs-bisection-step
+++ b/cs-bisection-step
@@ -1286,6 +1286,7 @@ END
 
     db_retry($popflight,'constructing', $dbh_tests,[qw(flights)], sub {
         print STDERR "Populating $popflight...\n";
+	undef %jobs_created;
         preparejob($job, $latest_flight, 0);
         foreach my $hostspec (split /,/, $hosts) { # /
             $hostspec =~ m/=/;
-- 
2.1.4


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

[OSSTEST PATCH 5/9] db retry, bisect: Cache build reuse investigations

[Ian Jackson]

If we previously searched for builds to reuse, trust our previous
answers.  We will only have seen data from committed transactions and
we will only have looked at jobs in completed flights, which won't
have changed.

So any previously reuseable build is still reuseable.  (Unless its
stash check failed, in which case we might want to search again if we
do a db retry.)  We don't care much about missing any
recently-finished jobs - there is a much bigger and unavoidable race
there anyway, where multiple bisections on different branches may
choose to pointlessly rebuild the same thing at the same time.

Not doing this search over and over again is important because it is a
very wide ranging search, which will often cause database transaction
serialisation errors.  Without some caching here, we may never
converge.

In principle we could do this another way: we could make a readonly
transaction which did all the searching.  But that's a more awkward
way to organise the code because our search uses a temporary table
which we then construct the flight from.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 cs-bisection-step | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/cs-bisection-step b/cs-bisection-step
index 1d1962a..819e519 100755
--- a/cs-bisection-step
+++ b/cs-bisection-step
@@ -1121,6 +1121,7 @@ END
 }
 
 our %jobs_created;
+our %builds_investigated; # $builds_investigated{$popjob} = 0, or {..row..}
 
 sub preparejob ($$$);
 sub preparejob ($$$) {
@@ -1194,7 +1195,11 @@ END
     my $usejob;
 
     if ($cache_option and $cacheok and $recipe =~ m/^build/ and !@$subjobs) {
-        my $reusejob= $dbh_tests->selectrow_hashref(<<END,{}, $popjob,$popjob);
+	my $reusejob= $builds_investigated{$popjob};
+	if (!defined $reusejob) {
+	    print STDERR "Searching for $popflight.$popjob to reuse...\n";
+	    $reusejob =
+		$dbh_tests->selectrow_hashref(<<END,{}, $popjob,$popjob);
             SELECT  *
             FROM    flights JOIN jobs j USING (flight)
             WHERE   j.job=?
@@ -1216,6 +1221,9 @@ END
             ORDER BY flights.started desc
             LIMIT 1
 END
+	    $reusejob //= 0; # defined but falseish
+	    $builds_investigated{$popjob}= $reusejob;
+	}
 
         if ($reusejob) {
 	    my $wantusejob= "$reusejob->{flight}.$reusejob->{job}";
@@ -1226,6 +1234,7 @@ END
 	    } else {
 		print STDERR "Can NOT reuse $recipe $wantusejob:".
 		    " $stashcheck: $!\n";
+		undef $builds_investigated{$popjob};
 	    }
         }
     }
-- 
2.1.4


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

[OSSTEST PATCH 6/9] db retry: Document $@ as an implicit parameter to need_retry

[Ian Jackson]

There are only two call sites and neither trashes $@ right now.
We are going to use a more exception-friendly style.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 Osstest/JobDB/Executive.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Osstest/JobDB/Executive.pm b/Osstest/JobDB/Executive.pm
index 7eeaf52..ebafb4c 100644
--- a/Osstest/JobDB/Executive.pm
+++ b/Osstest/JobDB/Executive.pm
@@ -59,7 +59,7 @@ sub begin_work ($$$) { #method
 }
 
 sub need_retry ($$$) {
-    my ($jd, $dbh,$committing) = @_;
+    my ($jd, $dbh,$committing) = @_; # implicitly, $@ is an argument too
     return
 	($dbh_tests->err() // 0)==7 &&
 	($dbh_tests->state =~ m/^(?:40P01|40001|23|40002)/);
-- 
2.1.4


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

[OSSTEST PATCH 7/9] db retry: Break out Osstest::Executive::JobDB::_need_retry

[Ian Jackson]

We are going to want to reorganise this.  As prep work, break the $dbh
state checking (and the corresponding comment) into a separate sub.

No functional changel.

(There is still an anomaly: need_retry passes it $dbh_tests, not the
$dbh it got from the caller.  This will go away shortly.)

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 Osstest/JobDB/Executive.pm | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Osstest/JobDB/Executive.pm b/Osstest/JobDB/Executive.pm
index ebafb4c..5f2aac9 100644
--- a/Osstest/JobDB/Executive.pm
+++ b/Osstest/JobDB/Executive.pm
@@ -58,11 +58,11 @@ sub begin_work ($$$) { #method
     }
 }
 
-sub need_retry ($$$) {
-    my ($jd, $dbh,$committing) = @_; # implicitly, $@ is an argument too
+sub _need_retry ($) {
+    my ($dbh) = @_;
     return
-	($dbh_tests->err() // 0)==7 &&
-	($dbh_tests->state =~ m/^(?:40P01|40001|23|40002)/);
+	($dbh->err() // 0)==7 &&
+	($dbh->state =~ m/^(?:40P01|40001|23|40002)/);
     # DEADLOCK DETECTED or SERIALIZATION FAILURE
     # or any Integrity Constraint Violation including
     # TRANSACTION_INTEGRITY_CONSTRAINT_VIOLATION.
@@ -109,6 +109,11 @@ sub need_retry ($$$) {
     # https://www.postgresql.org/message-id/flat/D960CB61B694CF459DCFB4B0128514C203937E44%40exadv11.host.magwien.gv.at
 }
 
+sub need_retry ($$$) {
+    my ($jd, $dbh,$committing) = @_; # implicitly, $@ is an argument too
+    return _need_retry($dbh_tests);
+}
+
 sub readonly_report ($$) { #method
     my ($jd, $dbh) = @_;
     
-- 
2.1.4


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

[OSSTEST PATCH 8/9] db retry: Use HandleError and exceptions to detect when to retry

[Ian Jackson]

It appears that sometimes, $dbh->state could be overwritten before
$mjobdb->need_retry got to run.  $dbh->err and $@ would still be
right.  I have not been able to explain this; I suspect that there is
something exciting going on in the eval exception trapping.

To try to isolate the problem, I developed this different approach: we
use the HandleError database handle feature, to cause all the
retriable errors to be thrown as a dedicated exception class.
We can then simply test ref $@.  (We don't care about subclassing
here.  And we don't want isa because $@ might just be a string.)
This is, in general, more reliable, as it cannot treat any other
failures as db retry failures.

Osstest::Executive and Osstest::JobDB::Executive become slightly more
intertwined with this patch.

Sadly this does not seem to completely eliminate the problem.  It does
allow us to present clearer evidence of problems in the underlying
DBI, DBD or PostgreSQL layers...

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

diff --git a/Osstest/Executive.pm b/Osstest/Executive.pm
index c423570..635e5dd 100644
--- a/Osstest/Executive.pm
+++ b/Osstest/Executive.pm
@@ -245,6 +245,8 @@ sub db_pg_dsn ($) {
     return $pg;
 }
 
+use Exception::Class ( 'Osstest::JobDB::Executive::RetryException' );
+
 sub opendb ($) {
     my ($dbname) = @_;
 
@@ -252,6 +254,18 @@ sub opendb ($) {
 
     my $dbh= DBI->connect("dbi:Pg:$pg", '','', {
         AutoCommit => 1,
+        HandleError => sub {
+            my ($emsg,$edbh,$firstval) = @_;
+            if (Osstest::JobDB::Executive::_need_retry($edbh)) {
+                chomp $emsg;
+                printf STDERR "DB confict (err=%s state=%s): %s\n",
+                    ($edbh->err // 'undef'),
+                    $edbh->state,
+                    $emsg;
+                die new Osstest::JobDB::Executive::RetryException $emsg;
+            }
+            undef;
+        },
         RaiseError => 1,
         ShowErrorStatement => 1,
         })
diff --git a/Osstest/JobDB/Executive.pm b/Osstest/JobDB/Executive.pm
index 5f2aac9..5545849 100644
--- a/Osstest/JobDB/Executive.pm
+++ b/Osstest/JobDB/Executive.pm
@@ -111,7 +111,7 @@ sub _need_retry ($) {
 
 sub need_retry ($$$) {
     my ($jd, $dbh,$committing) = @_; # implicitly, $@ is an argument too
-    return _need_retry($dbh_tests);
+    return ref($@) eq 'Osstest::JobDB::Executive::RetryException';
 }
 
 sub readonly_report ($$) { #method
-- 
2.1.4


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

[OSSTEST PATCH 9/9] db retry: Retry on $dbh->state eq ''

[Ian Jackson]

This is supposed to represent success.  But now that _need_retry is
only called within a HandleError hook, we know there has been a
failure.

Retry such failures, in the hope that they are stochastic.  If they
aren't, we will fail eventually when we run out of retries.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 Osstest/JobDB/Executive.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Osstest/JobDB/Executive.pm b/Osstest/JobDB/Executive.pm
index 5545849..83e19e4 100644
--- a/Osstest/JobDB/Executive.pm
+++ b/Osstest/JobDB/Executive.pm
@@ -62,11 +62,14 @@ sub _need_retry ($) {
     my ($dbh) = @_;
     return
 	($dbh->err() // 0)==7 &&
-	($dbh->state =~ m/^(?:40P01|40001|23|40002)/);
+	($dbh->state =~ m/^(?:40P01|40001|23|40002|$)/);
     # DEADLOCK DETECTED or SERIALIZATION FAILURE
     # or any Integrity Constraint Violation including
     # TRANSACTION_INTEGRITY_CONSTRAINT_VIOLATION.
     #
+    # Or the empty string, which it seems that we sometimes get on
+    # serialisation errors.
+    #
     # An Integrity Constraint Violation ought not to occur with
     # serialisable transactions, so it is aways a bug.  These bugs
     # should not be retried.  However, there is a longstanding bug in
-- 
2.1.4


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