All of lore.kernel.org
 help / color / mirror / Atom feed
* [OSSTEST PATCH 0/7] Better database handling in Tcl
@ 2016-01-07 19:38 Ian Jackson
  2016-01-07 19:38 ` [OSSTEST PATCH 1/7] Tcl database debugging: Actually work Ian Jackson
                   ` (7 more replies)
  0 siblings, 8 replies; 14+ messages in thread
From: Ian Jackson @ 2016-01-07 19:38 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Campbell

This series arranges for the owner daemon to be able to tolerate
database restarts, and is generally much more careful about database
errors in Tcl.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [OSSTEST PATCH 1/7] Tcl database debugging: Actually work
  2016-01-07 19:38 [OSSTEST PATCH 0/7] Better database handling in Tcl Ian Jackson
@ 2016-01-07 19:38 ` Ian Jackson
  2016-01-07 19:38 ` [OSSTEST PATCH 2/7] Database locking: Tcl: Use db-execute-array Ian Jackson
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Ian Jackson @ 2016-01-07 19:38 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

Setting OSSTEST_TCL_JOBDB_DEBUG was ineffective (ever since it was
introduced in 44dad3d8) because `env' wasn't imported from the global
scope.

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

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 7dba497..239f22c 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -116,6 +116,7 @@ proc db-update-1 {stmt} {
 }
 
 proc db-execute-debug {stmt} {
+    global env
     if {[var-or-default env(OSSTEST_TCL_JOBDB_DEBUG) 0]} {
 	puts stderr "EXECUTING >$stmt<"
     }
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [OSSTEST PATCH 2/7] Database locking: Tcl: Use db-execute-array
  2016-01-07 19:38 [OSSTEST PATCH 0/7] Better database handling in Tcl Ian Jackson
  2016-01-07 19:38 ` [OSSTEST PATCH 1/7] Tcl database debugging: Actually work Ian Jackson
@ 2016-01-07 19:38 ` Ian Jackson
  2016-01-07 19:38 ` [OSSTEST PATCH 3/7] Database locking: Tcl: Use db-execute Ian Jackson
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Ian Jackson @ 2016-01-07 19:38 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

Replace open-coded uses of pg_execute -array ARRAYVAR dbh STMT
with jobdb::db-execute-array ARRAYVAR STMT.

The only functional change is that if OSSTEST_TCL_JOBDB_DEBUG is set,
there will be debugging output.

But we are going to want to make db-execute-array do something more
complicated involving pg_exec.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 ms-ownerdaemon |    2 +-
 ms-queuedaemon |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index 33ee238..a228e3c 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -67,7 +67,7 @@ proc cmd/create-task {chan desc} {
                         ( type,  refkey,               refinfo,         live)
                  VALUES ('ownd', [pg_quote $taskdesc], [clock seconds], 't')
         "
-        set nrows [pg_execute -array av dbh "
+        set nrows [jobdb::db-execute-array av "
             SELECT taskid
               FROM tasks
              WHERE live AND refkey = [pg_quote $taskdesc]
diff --git a/ms-queuedaemon b/ms-queuedaemon
index 08f88cf..b3a05ee 100755
--- a/ms-queuedaemon
+++ b/ms-queuedaemon
@@ -444,7 +444,7 @@ proc cmd/unwait {chan desc} {
 
 proc for-free-resources {varname body} {
     jobdb::transaction resources {
-	pg_execute -array free_resources_row dbh {
+	jobdb::db-execute-array free_resources_row {
 		SELECT (restype || '/' || resname || '/' || shareix) AS r
 		  FROM resources
 	     WHERE NOT (SELECT live FROM tasks WHERE taskid=owntaskid)
@@ -636,7 +636,7 @@ proc cmd/uptime {chan desc seconds} {
     set descpat "[regsub {\:\d+$} $desc {:%}]"
     transaction resources {
         set keys {}
-        pg_execute -array task dbh "
+        jobdb::db-execute-array task "
             SELECT * FROM tasks
                     WHERE type = 'ownd'
                       AND ( refkey LIKE [pg_quote $descpat]
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [OSSTEST PATCH 3/7] Database locking: Tcl: Use db-execute
  2016-01-07 19:38 [OSSTEST PATCH 0/7] Better database handling in Tcl Ian Jackson
  2016-01-07 19:38 ` [OSSTEST PATCH 1/7] Tcl database debugging: Actually work Ian Jackson
  2016-01-07 19:38 ` [OSSTEST PATCH 2/7] Database locking: Tcl: Use db-execute-array Ian Jackson
@ 2016-01-07 19:38 ` Ian Jackson
  2016-01-08  9:28   ` Ian Campbell
  2016-01-07 19:38 ` [OSSTEST PATCH 4/7] Database locking: Tcl: Always use db-execute-array for SELECT Ian Jackson
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 14+ messages in thread
From: Ian Jackson @ 2016-01-07 19:38 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

Replace open-coded uses of pg_execute dbh STMT with
jobdb::db-execute-array STMT.

The only functional change is that if OSSTEST_TCL_JOBDB_DEBUG is set,
there will be debugging output.

But we are going to want to make db-execute do something more
complicated involving pg_exec.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 ms-ownerdaemon |    6 +++---
 ms-queuedaemon |    6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index a228e3c..502dcfe 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -32,7 +32,7 @@ proc chan-destroy-stuff {chan} {
     jobdb::transaction resources {
         puts-chan-desc $chan "-- $tasks"
         foreach task $tasks {
-            pg_execute dbh "
+            jobdb::db-execute "
                 UPDATE tasks
                    SET live = 'f'
                  WHERE taskid = $task
@@ -62,7 +62,7 @@ proc cmd/create-task {chan desc} {
         set taskdesc $desc
     }
     jobdb::transaction resources {
-        pg_execute dbh "
+        jobdb::db-execute "
             INSERT INTO tasks
                         ( type,  refkey,               refinfo,         live)
                  VALUES ('ownd', [pg_quote $taskdesc], [clock seconds], 't')
@@ -93,7 +93,7 @@ main-daemon Owner {
     jobdb::db-open
     
     jobdb::transaction resources {
-        set nrows [pg_execute dbh "
+        set nrows [jobdb::db-execute "
             UPDATE tasks
                SET refkey = 'previous ' || refkey
              WHERE type = 'ownd'
diff --git a/ms-queuedaemon b/ms-queuedaemon
index b3a05ee..9325d46 100755
--- a/ms-queuedaemon
+++ b/ms-queuedaemon
@@ -155,7 +155,7 @@ proc runneeded-perhaps-start {} {
     }
 
     jobdb::transaction resources {
-        set nrows [pg_execute dbh {
+        set nrows [jobdb::db-execute {
             UPDATE resources
                SET owntaskid= (SELECT taskid FROM tasks
                                WHERE type='magic' AND refkey='allocatable')
@@ -167,7 +167,7 @@ proc runneeded-perhaps-start {} {
     if {!($nrows || $needed>=2)} return
 
     jobdb::transaction resources {
-        set cleaned [pg_execute dbh {
+        set cleaned [jobdb::db-execute {
             DELETE FROM tasks
              WHERE type='ownd'
                AND live='f'
@@ -650,7 +650,7 @@ proc cmd/uptime {chan desc seconds} {
                 continue
             }
             if {$refinfo > $before} continue
-            pg_execute dbh "
+            jobdb::db-execute "
                 UPDATE tasks
                    SET live = false,
                        refinfo = refinfo || ' stale'
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [OSSTEST PATCH 4/7] Database locking: Tcl: Always use db-execute-array for SELECT
  2016-01-07 19:38 [OSSTEST PATCH 0/7] Better database handling in Tcl Ian Jackson
                   ` (2 preceding siblings ...)
  2016-01-07 19:38 ` [OSSTEST PATCH 3/7] Database locking: Tcl: Use db-execute Ian Jackson
@ 2016-01-07 19:38 ` Ian Jackson
  2016-01-07 19:38 ` [OSSTEST PATCH 5/7] Database locking: Tcl: for errorCode, use pg_exec, not pg_execute Ian Jackson
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Ian Jackson @ 2016-01-07 19:38 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

We are going to make it wrong to use db-execute for SELECT statements.

Convert the two existing violation sites (providing a dummy arrayvar).

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

diff --git a/sg-execute-flight b/sg-execute-flight
index 4e3fcf2..008e661 100755
--- a/sg-execute-flight
+++ b/sg-execute-flight
@@ -30,7 +30,7 @@ proc check {} {
 
     jobdb::db-open
 
-    set nqueued [jobdb::db-execute "
+    set nqueued [jobdb::db-execute-array dummy "
         SELECT job FROM jobs j
          WHERE j.flight = $flight
            AND j.status = 'queued'
diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 239f22c..bbce6fc 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -275,7 +275,7 @@ proc become-task {comment} {
     set username "[id user]@$hostname"
 
     transaction resources {
-        set nrows [db-execute "
+        set nrows [db-execute-array dummy "
             UPDATE tasks
                SET username = [pg_quote $username],
                    comment = [pg_quote $comment]
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [OSSTEST PATCH 5/7] Database locking: Tcl: for errorCode, use pg_exec, not pg_execute
  2016-01-07 19:38 [OSSTEST PATCH 0/7] Better database handling in Tcl Ian Jackson
                   ` (3 preceding siblings ...)
  2016-01-07 19:38 ` [OSSTEST PATCH 4/7] Database locking: Tcl: Always use db-execute-array for SELECT Ian Jackson
@ 2016-01-07 19:38 ` Ian Jackson
  2016-01-08  9:32   ` Ian Campbell
  2016-01-07 19:38 ` [OSSTEST PATCH 6/7] Database locking: Tcl: Retry only on DEADLOCK DETECTED Ian Jackson
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 14+ messages in thread
From: Ian Jackson @ 2016-01-07 19:38 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

We would like to be able to retry db transactions.  To do this we need
to know why they failed (if they did).

But pg_execute does not set errorCode.  (This is clearly a bug.)  And
since it immediately discards a failed statement, any error
information has been lost by the time pg_execute returns.

So, instead, use pg_exec, and manually mess about with fishing
suitable information out of a failed statement handle, and generating
an appropriate errorCode.

There are no current consumers of this errorCode: that will come in a
moment.

A wrinkle is that as a result it is no longer possible to use
db-execute on a SELECT statement nor db-execute-array on a non-SELECT
statement.  This is because the 1. the `ok' status that we have to
check for is different for statements which are commands and ones
which return tupes and 2. we need to fish a different return value out
of the statement handle (-cmdTuples vs -numTuples).  But all uses in
the codebase are now fine for this distinction.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tcl/JobDB-Executive.tcl |   54 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index bbce6fc..ed9abbb 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -121,13 +121,61 @@ proc db-execute-debug {stmt} {
 	puts stderr "EXECUTING >$stmt<"
     }
 }
+
+proc db--exec-check {shvar stmt expected_status body} {
+    # pg_execute does not set errorCode and it throws away the
+    # statement handle so we can't get the error out.  So
+    # use pg_exec, as wrapped up here.
+
+    # db--exec-check executes stmt and checks that the status is
+    # `expected_status'.  If OK, executes body with $shvar set to the
+    # stmt handle.   Otherwise throws with errorCode
+    #   {OSSTEST-PSQL <pg-status> <pg-sqlstate>}
+
+    global errorInfo errorCode
+    upvar 1 $shvar sh
+
+    set sh [pg_exec dbh $stmt]
+
+    set rc [catch {
+	set status [pg_result $sh -status]
+	if {[string compare $status $expected_status]} {
+	    set emsg [pg_result $sh -error]
+	    set sqlstate [pg_result $sh -error sqlstate]
+	    if {![string length $emsg]} {
+		set emsg "osstest expected status $expected_status got $status"
+	    }
+	    set context [pg_result $sh -error context]
+	    error $emsg \
+		"    while executing SQL\n$stmt\n    in SQL context\n$context" \
+		[list OSSTEST-PSQL $status $sqlstate]
+	}
+	uplevel 1 $body
+    } emsg]
+
+    set ei $errorInfo
+    set ec $errorCode
+    catch { pg_result $sh -clear }
+
+    return -code $rc -errorinfo $ei -errorcode $ec $emsg
+}
+
 proc db-execute {stmt} {
     db-execute-debug $stmt
-    uplevel 1 [list pg_execute dbh $stmt]
+    db--exec-check sh $stmt PGRES_COMMAND_OK {
+	return [pg_result $sh -cmdTuples]
+    }
 }
-proc db-execute-array {arrayvar stmt args} {
+proc db-execute-array {arrayvar stmt {body {}}} {
     db-execute-debug $stmt
-    uplevel 1 [list pg_execute -array $arrayvar dbh $stmt] $args
+    db--exec-check sh $stmt PGRES_TUPLES_OK {
+	set nrows [pg_result $sh -numTuples]
+	for {set row 0} {$row < $nrows} {incr row} {
+	    uplevel 1 [list pg_result $sh -tupleArray $row $arrayvar]
+	    uplevel 1 $body
+	}
+	return $nrows
+    }
 }
 
 proc lock-tables {tables} {
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [OSSTEST PATCH 6/7] Database locking: Tcl: Retry only on DEADLOCK DETECTED
  2016-01-07 19:38 [OSSTEST PATCH 0/7] Better database handling in Tcl Ian Jackson
                   ` (4 preceding siblings ...)
  2016-01-07 19:38 ` [OSSTEST PATCH 5/7] Database locking: Tcl: for errorCode, use pg_exec, not pg_execute Ian Jackson
@ 2016-01-07 19:38 ` Ian Jackson
  2016-01-07 19:38 ` [OSSTEST PATCH 7/7] ms-ownerdaemon: Cope with db restart. Retry recording dead tasks Ian Jackson
  2016-01-08  9:40 ` [OSSTEST PATCH 0/7] Better database handling in Tcl Ian Campbell
  7 siblings, 0 replies; 14+ messages in thread
From: Ian Jackson @ 2016-01-07 19:38 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

Use the new errorCode coming out of db-execute* to tell when the error
is that we got a database deadlock, which is the situation in which we
should retry.

This involves combining the two catch blocks, so that there is only
one error handling strategy.  Previously errors on COMMIT would be
retried and others would not.  Now errors anywhere might be retried
but only if the DB indicated deadlock.

We now unconditionally execute ROLLBACK.  This is more correct, since
we always previously executed BEGIN.

And, we pass the errorInfo and errorCode from the $body to the caller.

I have tested this with a test db instance, using contrived means to
generate a database deadlock, and it does actually retry.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tcl/JobDB-Executive.tcl |   30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index ed9abbb..63db4f0 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -283,25 +283,27 @@ proc transaction {tables script} {
 	    db-execute "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"
 	    lock-tables $tables
 	    uplevel 1 $script
+	    db-execute COMMIT
 	} result]
-	if {!$rc} {
-	    if {[catch {
-		db-execute COMMIT
-	    } emsg]} {
-		puts "commit failed: $emsg; retrying ..."
-		db-execute ROLLBACK
-		if {[incr retries -1] <= 0} {
-		    error \
- "commit failed, too many retries: $emsg\n$errorInfo\n$errorCode\n"
+	set ei $errorInfo
+	set ec $errorCode
+	if {$rc} {
+	    db-execute ROLLBACK
+	    switch -glob $errorCode {
+		{OSSTEST-PSQL * 40P01} {
+		    # DEADLOCK DETECTED
+		    puts "transaction deadlock ($result) retrying ..."
+		    if {[incr retries -1] <= 0} {
+			error \
+ "transaction failed, too many retries: $result\n$errorInfo\n$errorCode\n"
+		    }
+		    after 500
+		    continue
 		}
-		after 500
-		continue
 	    }
-	} else {
-	    db-execute ROLLBACK
 	}
         db-close
-	return -code $rc $result
+	return -code $rc -errorinfo $ei -errorcode $ec $result
     }
 }
 
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [OSSTEST PATCH 7/7] ms-ownerdaemon: Cope with db restart. Retry recording dead tasks.
  2016-01-07 19:38 [OSSTEST PATCH 0/7] Better database handling in Tcl Ian Jackson
                   ` (5 preceding siblings ...)
  2016-01-07 19:38 ` [OSSTEST PATCH 6/7] Database locking: Tcl: Retry only on DEADLOCK DETECTED Ian Jackson
@ 2016-01-07 19:38 ` Ian Jackson
  2016-01-08  9:40 ` [OSSTEST PATCH 0/7] Better database handling in Tcl Ian Campbell
  7 siblings, 0 replies; 14+ messages in thread
From: Ian Jackson @ 2016-01-07 19:38 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

In chan-destroy-stuff, instead of accessing the db directly, add the
dead task(s) to a queue, and arrange to look at that queue.

Errors are handled by setting an `after' handler which we cancel if we
are successful.

The after handler requeues a queue run attempt as the first thing
(which will arrange that a further retry will occur if things are
still broken) and then attempts to reconnect to the database.

I have tested this with a test instance by renaming the `tasks' table
under its feet, and it functions as expected.

DEPLOYMENT NOTE: The owner daemon cannot be restarted without shutting
everything down.  So this update should first be deployed in
Cambridge, probably, to see how it goes.  Also, it is less critical in
the main Xen production test lab because there the db and the owner
daemon are co-hosted on the same VM.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 Osstest/Executive.pm |    1 +
 ms-ownerdaemon       |   37 +++++++++++++++++++++++++++++++++----
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/Osstest/Executive.pm b/Osstest/Executive.pm
index 2314577..d31fafb 100644
--- a/Osstest/Executive.pm
+++ b/Osstest/Executive.pm
@@ -113,6 +113,7 @@ augmentconfigdefaults(
 augmentconfigdefaults(
     OwnerDaemonHost => $c{ControlDaemonHost},
     QueueDaemonHost => $c{ControlDaemonHost},
+    OwnerDaemonDbRetry => $c{QueueDaemonRetry},
 );
 
 #---------- configuration reader etc. ----------
diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index 502dcfe..318549a 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -22,16 +22,37 @@
 source ./tcl/daemonlib.tcl
 
 
+set dead_tasks {}
+
 proc chan-destroy-stuff {chan} {
+    global dead_tasks
+
     upvar #0 chanawait($chan) await
     catch { unset await }
 
     upvar #0 chantasks($chan) tasks
     if {![info exists tasks]} return
 
+    puts-chan-desc $chan "-- $tasks"
+
+    foreach task $tasks {
+	lappend dead_tasks $task
+    }
+    after idle record-dead-tasks
+}
+
+proc record-dead-tasks {} {
+    global c dead_tasks
+
+    if {![llength $dead_tasks]} return
+
+    puts "record-dead-tasks ... $dead_tasks"
+
+    set retry [expr {$c(OwnerDaemonDbRetry) * 1000}]
+    set eafter [after $retry record-dead-tasks-retry]
+
     jobdb::transaction resources {
-        puts-chan-desc $chan "-- $tasks"
-        foreach task $tasks {
+        foreach task $dead_tasks {
             jobdb::db-execute "
                 UPDATE tasks
                    SET live = 'f'
@@ -39,12 +60,20 @@ proc chan-destroy-stuff {chan} {
             "
         }
     }
-    puts-chan-desc $chan "== $tasks"
-    unset tasks
 
+    after cancel $eafter
+    puts "record-dead-tasks OK. $dead_tasks"
+    set dead_tasks {}
     after idle await-endings-notify
 }
 
+proc record-dead-tasks-retry {} {
+    after idle record-dead-tasks
+    puts "** reconnecting/retrying **"
+    catch { jobdb::db-close }
+    jobdb::db-open
+}
+
 proc await-endings-notify {} {
     global chanawait
     foreach chan [array names chanawait] {
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [OSSTEST PATCH 3/7] Database locking: Tcl: Use db-execute
  2016-01-07 19:38 ` [OSSTEST PATCH 3/7] Database locking: Tcl: Use db-execute Ian Jackson
@ 2016-01-08  9:28   ` Ian Campbell
  2016-01-12 15:38     ` Ian Jackson
  0 siblings, 1 reply; 14+ messages in thread
From: Ian Campbell @ 2016-01-08  9:28 UTC (permalink / raw)
  To: Ian Jackson, xen-devel

On Thu, 2016-01-07 at 19:38 +0000, Ian Jackson wrote:
> Replace open-coded uses of pg_execute dbh STMT with
> jobdb::db-execute-array STMT.

ITYM jobdb::db-execute?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [OSSTEST PATCH 5/7] Database locking: Tcl: for errorCode, use pg_exec, not pg_execute
  2016-01-07 19:38 ` [OSSTEST PATCH 5/7] Database locking: Tcl: for errorCode, use pg_exec, not pg_execute Ian Jackson
@ 2016-01-08  9:32   ` Ian Campbell
  2016-01-12 15:39     ` Ian Jackson
  0 siblings, 1 reply; 14+ messages in thread
From: Ian Campbell @ 2016-01-08  9:32 UTC (permalink / raw)
  To: Ian Jackson, xen-devel

On Thu, 2016-01-07 at 19:38 +0000, Ian Jackson wrote:
> We would like to be able to retry db transactions.  To do this we need
> to know why they failed (if they did).
> 
> But pg_execute does not set errorCode.  (This is clearly a bug.)  And
> since it immediately discards a failed statement, any error
> information has been lost by the time pg_execute returns.
> 
> So, instead, use pg_exec, and manually mess about with fishing
> suitable information out of a failed statement handle, and generating
> an appropriate errorCode.
> 
> There are no current consumers of this errorCode: that will come in a
> moment.
> 
> A wrinkle is that as a result it is no longer possible to use
> db-execute on a SELECT statement nor db-execute-array on a non-SELECT
> statement.  This is because the 1. the `ok' status that we have to

Stray "the" before "1.".

> check for is different for statements which are commands and ones
> which return tupes and 2. we need to fish a different return value out

"tuples"

> of the statement handle (-cmdTuples vs -numTuples).  But all uses in
> the codebase are now fine for this distinction.

Does this imply that db-execute-array could be renamed db-execute-select,
or even just db-select?

> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  tcl/JobDB-Executive.tcl |   54
> ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
> index bbce6fc..ed9abbb 100644
> --- a/tcl/JobDB-Executive.tcl
> +++ b/tcl/JobDB-Executive.tcl
> @@ -121,13 +121,61 @@ proc db-execute-debug {stmt} {
>  	puts stderr "EXECUTING >$stmt<"
>      }
>  }
> +
> +proc db--exec-check {shvar stmt expected_status body} {
> +    # pg_execute does not set errorCode and it throws away the
> +    # statement handle so we can't get the error out.  So
> +    # use pg_exec, as wrapped up here.
> +
> +    # db--exec-check executes stmt and checks that the status is
> +    # `expected_status'.  If OK, executes body with $shvar set to the
> +    # stmt handle.   Otherwise throws with errorCode
> +    #   {OSSTEST-PSQL <pg-status> <pg-sqlstate>}
> +
> +    global errorInfo errorCode
> +    upvar 1 $shvar sh
> +
> +    set sh [pg_exec dbh $stmt]
> +
> +    set rc [catch {
> +	set status [pg_result $sh -status]
> +	if {[string compare $status $expected_status]} {
> +	    set emsg [pg_result $sh -error]
> +	    set sqlstate [pg_result $sh -error sqlstate]
> +	    if {![string length $emsg]} {
> +		set emsg "osstest expected status $expected_status got
> $status"
> +	    }
> +	    set context [pg_result $sh -error context]
> +	    error $emsg \
> +		"    while executing SQL\n$stmt\n    in SQL
> context\n$context" \
> +		[list OSSTEST-PSQL $status $sqlstate]
> +	}
> +	uplevel 1 $body
> +    } emsg]
> +
> +    set ei $errorInfo
> +    set ec $errorCode
> +    catch { pg_result $sh -clear }
> +
> +    return -code $rc -errorinfo $ei -errorcode $ec $emsg
> +}
> +
>  proc db-execute {stmt} {
>      db-execute-debug $stmt
> -    uplevel 1 [list pg_execute dbh $stmt]
> +    db--exec-check sh $stmt PGRES_COMMAND_OK {
> +	return [pg_result $sh -cmdTuples]
> +    }
>  }
> -proc db-execute-array {arrayvar stmt args} {
> +proc db-execute-array {arrayvar stmt {body {}}} {
>      db-execute-debug $stmt
> -    uplevel 1 [list pg_execute -array $arrayvar dbh $stmt] $args
> +    db--exec-check sh $stmt PGRES_TUPLES_OK {
> +	set nrows [pg_result $sh -numTuples]
> +	for {set row 0} {$row < $nrows} {incr row} {
> +	    uplevel 1 [list pg_result $sh -tupleArray $row $arrayvar]
> +	    uplevel 1 $body
> +	}
> +	return $nrows
> +    }
>  }
>  
>  proc lock-tables {tables} {

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [OSSTEST PATCH 0/7] Better database handling in Tcl
  2016-01-07 19:38 [OSSTEST PATCH 0/7] Better database handling in Tcl Ian Jackson
                   ` (6 preceding siblings ...)
  2016-01-07 19:38 ` [OSSTEST PATCH 7/7] ms-ownerdaemon: Cope with db restart. Retry recording dead tasks Ian Jackson
@ 2016-01-08  9:40 ` Ian Campbell
  7 siblings, 0 replies; 14+ messages in thread
From: Ian Campbell @ 2016-01-08  9:40 UTC (permalink / raw)
  To: Ian Jackson, xen-devel

On Thu, 2016-01-07 at 19:38 +0000, Ian Jackson wrote:
> This series arranges for the owner daemon to be able to tolerate
> database restarts, and is generally much more careful about database
> errors in Tcl.

I had a trivial comment on #3's commit message, assuming that rather than
the code is wrong, Ack.

I spotted a few typos in #5's commit message too. I asked about renaming
db-execute-array db-{execute-,}select, but either way Ack.

I'm not going to claim I really understood all the Tcl (especially towards
the end of the series), but you've tested it, which is good enough I think.

IOW, all Acked-by: Ian Campbell <ian.campbell@citrix.com>
(with small caveats on #3 and #5)

Ian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [OSSTEST PATCH 3/7] Database locking: Tcl: Use db-execute
  2016-01-08  9:28   ` Ian Campbell
@ 2016-01-12 15:38     ` Ian Jackson
  0 siblings, 0 replies; 14+ messages in thread
From: Ian Jackson @ 2016-01-12 15:38 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel

Ian Campbell writes ("Re: [OSSTEST PATCH 3/7] Database locking: Tcl: Use db-execute"):
> On Thu, 2016-01-07 at 19:38 +0000, Ian Jackson wrote:
> > Replace open-coded uses of pg_execute dbh STMT with
> > jobdb::db-execute-array STMT.
> 
> ITYM jobdb::db-execute?

Yes, fixed, thanks.

Ian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [OSSTEST PATCH 5/7] Database locking: Tcl: for errorCode, use pg_exec, not pg_execute
  2016-01-08  9:32   ` Ian Campbell
@ 2016-01-12 15:39     ` Ian Jackson
  2016-01-14 10:33       ` Ian Campbell
  0 siblings, 1 reply; 14+ messages in thread
From: Ian Jackson @ 2016-01-12 15:39 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel, Ian Jackson

Ian Campbell writes ("Re: [OSSTEST PATCH 5/7] Database locking: Tcl: for errorCode, use pg_exec, not pg_execute"):
> On Thu, 2016-01-07 at 19:38 +0000, Ian Jackson wrote:
> > A wrinkle is that as a result it is no longer possible to use
> > db-execute on a SELECT statement nor db-execute-array on a non-SELECT
> > statement.  This is because the 1. the `ok' status that we have to

(fixed the typos, thanks)

> > of the statement handle (-cmdTuples vs -numTuples).  But all uses in
> > the codebase are now fine for this distinction.
> 
> Does this imply that db-execute-array could be renamed db-execute-select,
> or even just db-select?

Yes, indeed.  Do you think I should ?

Ian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [OSSTEST PATCH 5/7] Database locking: Tcl: for errorCode, use pg_exec, not pg_execute
  2016-01-12 15:39     ` Ian Jackson
@ 2016-01-14 10:33       ` Ian Campbell
  0 siblings, 0 replies; 14+ messages in thread
From: Ian Campbell @ 2016-01-14 10:33 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel

On Tue, 2016-01-12 at 15:39 +0000, Ian Jackson wrote:
> > > of the statement handle (-cmdTuples vs -numTuples).  But all uses in
> > > the codebase are now fine for this distinction.
> > 
> > Does this imply that db-execute-array could be renamed db-execute-
> > select,
> > or even just db-select?
> 
> Yes, indeed.  Do you think I should ?

Unless its unexpectedly more complicated than running sed I think you may
as well (maybe as a new patch at the end rather than trying to do it with
the other stuff)


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2016-01-14 10:35 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-07 19:38 [OSSTEST PATCH 0/7] Better database handling in Tcl Ian Jackson
2016-01-07 19:38 ` [OSSTEST PATCH 1/7] Tcl database debugging: Actually work Ian Jackson
2016-01-07 19:38 ` [OSSTEST PATCH 2/7] Database locking: Tcl: Use db-execute-array Ian Jackson
2016-01-07 19:38 ` [OSSTEST PATCH 3/7] Database locking: Tcl: Use db-execute Ian Jackson
2016-01-08  9:28   ` Ian Campbell
2016-01-12 15:38     ` Ian Jackson
2016-01-07 19:38 ` [OSSTEST PATCH 4/7] Database locking: Tcl: Always use db-execute-array for SELECT Ian Jackson
2016-01-07 19:38 ` [OSSTEST PATCH 5/7] Database locking: Tcl: for errorCode, use pg_exec, not pg_execute Ian Jackson
2016-01-08  9:32   ` Ian Campbell
2016-01-12 15:39     ` Ian Jackson
2016-01-14 10:33       ` Ian Campbell
2016-01-07 19:38 ` [OSSTEST PATCH 6/7] Database locking: Tcl: Retry only on DEADLOCK DETECTED Ian Jackson
2016-01-07 19:38 ` [OSSTEST PATCH 7/7] ms-ownerdaemon: Cope with db restart. Retry recording dead tasks Ian Jackson
2016-01-08  9:40 ` [OSSTEST PATCH 0/7] Better database handling in Tcl Ian Campbell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.