| Message ID | c4bf41b5c91623ee93285f35516265cfdb0ab401.1708457946.git.mail@cbaines.net |
|---|---|
| State | New |
| Headers | show |
| Series | Prepare the database code for use in the daemon | expand |
Christopher Baines <mail@cbaines.net> writes: > Especially since we're asking for these to be cached. > > Management of prepared statements isn't trivial, since you don't want to keep > them forever as this can lead to poor query performance, but I don't think > that finalizing them immediately is the right solution. guile-sqlite3 arranges for cached statements to only be reset, not finalized, when sqlite-finalize is called on them (see https://notabug.org/guile-sqlite3/guile-sqlite3/src/master/sqlite3.scm.in#L283). The idea behind this admittedly-unintuitive behavior is that it allows for the caching behavior of a statement to be decided independently of the code that actually uses it: if it's been decided elsewhere that a prepared statement is worth keeping around, it will reuse it, but if it hasn't, it will still properly clean up what it created. Perhaps reusing the name 'sqlite-finalize' to make that behavior transparent wasn't the best choice in the long run. I hope that makes the way it was written a bit less baffling. - reepca
reepca@russelstein.xyz writes: > Christopher Baines <mail@cbaines.net> writes: > >> Especially since we're asking for these to be cached. >> >> Management of prepared statements isn't trivial, since you don't want to keep >> them forever as this can lead to poor query performance, but I don't think >> that finalizing them immediately is the right solution. > > guile-sqlite3 arranges for cached statements to only be reset, not > finalized, when sqlite-finalize is called on them (see > https://notabug.org/guile-sqlite3/guile-sqlite3/src/master/sqlite3.scm.in#L283). > The idea behind this admittedly-unintuitive behavior is that it allows > for the caching behavior of a statement to be decided independently of > the code that actually uses it: if it's been decided elsewhere that a > prepared statement is worth keeping around, it will reuse it, but if it > hasn't, it will still properly clean up what it created. > > Perhaps reusing the name 'sqlite-finalize' to make that behavior > transparent wasn't the best choice in the long run. > > I hope that makes the way it was written a bit less baffling. Right, this is something I hadn't realised. I don't think this causes any problems for how I'm using sqlite-finalize though. Thanks for pointing this out.
diff --git a/guix/store/database.scm b/guix/store/database.scm index 7e3a2873ce..8d8b7346e0 100644 --- a/guix/store/database.scm +++ b/guix/store/database.scm @@ -167,16 +167,19 @@ (define-syntax with-database ((_ file db exp ...) (call-with-database file (lambda (db) exp ...))))) +(define (sqlite-step-and-reset statement) + (let ((val (sqlite-step statement))) + (sqlite-reset statement) + val)) + (define (last-insert-row-id db) ;; XXX: (sqlite3) currently lacks bindings for 'sqlite3_last_insert_rowid'. ;; Work around that. - (let* ((stmt (sqlite-prepare db "SELECT last_insert_rowid();" - #:cache? #t)) - (result (sqlite-fold cons '() stmt))) - (sqlite-finalize stmt) - (match result - ((#(id)) id) - (_ #f)))) + (let ((stmt (sqlite-prepare db + "SELECT last_insert_rowid();" + #:cache? #t))) + (vector-ref (sqlite-step-and-reset stmt) + 0))) (define* (path-id db path) "If PATH exists in the 'ValidPaths' table, return its numerical @@ -187,11 +190,9 @@ (define* (path-id db path) SELECT id FROM ValidPaths WHERE path = :path" #:cache? #t))) (sqlite-bind-arguments stmt #:path path) - (let ((result (sqlite-fold cons '() stmt))) - (sqlite-finalize stmt) - (match result - ((#(id) . _) id) - (_ #f))))) + (match (sqlite-step-and-reset stmt) + (#(id) id) + (#f #f)))) (define-inlinable (assert-integer proc in-range? key number) (unless (integer? number) @@ -228,9 +229,8 @@ (define* (update-or-insert db #:key path deriver hash nar-size time) (sqlite-bind-arguments stmt #:id id #:deriver deriver #:hash hash #:size nar-size #:time time) - (sqlite-fold cons '() stmt) - (sqlite-finalize stmt) - (last-insert-row-id db)) + (sqlite-step-and-reset stmt) + id) (let ((stmt (sqlite-prepare db " @@ -240,8 +240,7 @@ (define* (update-or-insert db #:key path deriver hash nar-size time) (sqlite-bind-arguments stmt #:path path #:deriver deriver #:hash hash #:size nar-size #:time time) - (sqlite-fold cons '() stmt) ;execute it - (sqlite-finalize stmt) + (sqlite-step-and-reset stmt) (last-insert-row-id db))))) (define (add-references db referrer references) @@ -254,13 +253,10 @@ (define (add-references db referrer references) VALUES (:referrer, :reference)" #:cache? #t))) (for-each (lambda (reference) - (sqlite-reset stmt) (sqlite-bind-arguments stmt #:referrer referrer #:reference reference) - (sqlite-fold cons '() stmt) ;execute it - (last-insert-row-id db)) - references) - (sqlite-finalize stmt))) + (sqlite-step-and-reset stmt)) + references))) (define (timestamp) "Return a timestamp, either the current time of SOURCE_DATE_EPOCH."