| Message ID | 1452195496-16016-6-git-send-email-ian.jackson@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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} {
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.
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)
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} {
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(-)