| Message ID | b7360abb08559073653effea98a99332fc1f8071.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> skribis: > I think using dynamic-wind to finalize all statements is the wrong > approach. Firstly it would be good to allow reseting statements rather than > finalizing them. Then for the problem of handling errors, the approach I've > settled on in the build coordinator is to close the database connection, since > that'll trigger guile-sqlite3 to finalize all the cached statements. > > This reverts commit 5d6e2255286e591def122ec2f4a3cbda497fea21. > > * .dir-locals.el (scheme-mode): Remove with-statement. > * guix/store/database.scm (call-with-statement): Remove procedure. > (with-statement): Remove syntax rule. > (call-with-transaction, last-insert-row-id, path-id, update-or-insert, > add-references): Don't use with-statement. > > Change-Id: I2fd976b3f12ec8105cc56350933a953cf53647e8 I’m all for removing ‘dynamic-wind’, we’ll have to do it to make it usable in a fiberized context anyway. I’ll let reepca comment. Ludo’.
>> I think using dynamic-wind to finalize all statements is the wrong >> approach. Firstly it would be good to allow reseting statements rather than >> finalizing them. Let me once again mention https://issues.guix.gnu.org/69292#7 with this much more magnificent Cc list. >> Then for the problem of handling errors, the approach I've >> settled on in the build coordinator is to close the database connection, since >> that'll trigger guile-sqlite3 to finalize all the cached statements. And in the event that a statement is *not* cached, it will hang around until the gc next pumps the statement guardian, at which point it will do... whatever happens when a statement is finalized after the database connection it was created with has already been closed, I guess. I don't know if that's a problem or not. On further investigation, it appears that sqlite_close would return SQLITE_BUSY, but guile-sqlite3's sqlite-close doesn't throw any exceptions, and according to https://www.sqlite.org/c3ref/close.html it would just hold off on actually closing the database until all statements have been finalized. So I guess that works. >> >> This reverts commit 5d6e2255286e591def122ec2f4a3cbda497fea21. >> >> * .dir-locals.el (scheme-mode): Remove with-statement. >> * guix/store/database.scm (call-with-statement): Remove procedure. >> (with-statement): Remove syntax rule. >> (call-with-transaction, last-insert-row-id, path-id, update-or-insert, >> add-references): Don't use with-statement. >> >> Change-Id: I2fd976b3f12ec8105cc56350933a953cf53647e8 > > I’m all for removing ‘dynamic-wind’, we’ll have to do it to make it > usable in a fiberized context anyway. > > I’ll let reepca comment. What is the proper fibers-friendly replacement for dynamic-wind, anyway, that is "like dynamic-wind, except when suspending a fiber"? It feels like the current interaction between dynamic-wind and fibers is more of an accident of how the implementation works; I don't suppose fibers could export a transparent replacement, like how it already exports a replacement for 'sleep'? Or perhaps the underlying issue is that we keep using 'dynamic-wind' in situations where it only makes sense to enter or exit the dynamic extent once? Is it time to bring 'unwind-protect' back into style? I see that fibers now has a dynamic-wind*, should that be preferred? I don't have a strong opinion on these changes, I just want to make sure we're all aware of how guile-sqlite3's sqlite-finalize acts with cached statements. - reepca
Hi, Reepca Russelstein <reepca@russelstein.xyz> skribis: >> I’m all for removing ‘dynamic-wind’, we’ll have to do it to make it >> usable in a fiberized context anyway. >> >> I’ll let reepca comment. > > What is the proper fibers-friendly replacement for dynamic-wind, anyway, > that is "like dynamic-wind, except when suspending a fiber"? It feels > like the current interaction between dynamic-wind and fibers is more of > an accident of how the implementation works; I don't suppose fibers could > export a transparent replacement, like how it already exports a > replacement for 'sleep'? Or perhaps the underlying issue is that we > keep using 'dynamic-wind' in situations where it only makes sense to > enter or exit the dynamic extent once? Is it time to bring > 'unwind-protect' back into style? I see that fibers now has a > dynamic-wind*, should that be preferred? I’ve come to think that ‘dynamic-wind’ is a problematic abstraction. What we really want is to perform an effect after a normal exit or when an exception is thrown. The latter case is actually best handled with ‘with-exception-handler’ (this is what I did for ‘with-store’; see commit 8ed597f4a261fe188de82cd1f5daed83dba948eb). (In Cuirass I added ‘unwind-protect’ at one point but in the end it was not so nice and I eventually removed it.) > I don't have a strong opinion on these changes, I just want to make sure > we're all aware of how guile-sqlite3's sqlite-finalize acts with cached > statements. Agreed. Thank you for chiming in! Ludo’.
diff --git a/.dir-locals.el b/.dir-locals.el index f135eb69a5..2d1a03c313 100644 --- a/.dir-locals.el +++ b/.dir-locals.el @@ -131,7 +131,6 @@ (eval . (put 'with-database 'scheme-indent-function 2)) (eval . (put 'call-with-database 'scheme-indent-function 1)) (eval . (put 'call-with-transaction 'scheme-indent-function 1)) - (eval . (put 'with-statement 'scheme-indent-function 3)) (eval . (put 'call-with-retrying-transaction 'scheme-indent-function 1)) (eval . (put 'call-with-container 'scheme-indent-function 1)) diff --git a/guix/store/database.scm b/guix/store/database.scm index 3093fd816a..de72b79860 100644 --- a/guix/store/database.scm +++ b/guix/store/database.scm @@ -130,25 +130,22 @@ (define* (call-with-transaction db proc #:key restartable?) the transaction, otherwise commit the transaction after it finishes. RESTARTABLE? may be set to a non-#f value when it is safe to run PROC multiple times. This may reduce contention for the database somewhat." - (define (exec sql) - (with-statement db sql stmt - (sqlite-fold cons '() stmt))) ;; We might use begin immediate here so that if we need to retry, we figure ;; that out immediately rather than because some SQLITE_BUSY exception gets ;; thrown partway through PROC - in which case the part already executed ;; (which may contain side-effects!) might have to be executed again for ;; every retry. - (exec (if restartable? "begin;" "begin immediate;")) + (sqlite-exec db (if restartable? "begin;" "begin immediate;")) (catch #t (lambda () (let-values ((result (proc))) - (exec "commit;") + (sqlite-exec db "commit;") (apply values result))) (lambda args ;; The roll back may or may not have occurred automatically when the ;; error was generated. If it has occurred, this does nothing but signal ;; an error. If it hasn't occurred, this needs to be done. - (false-if-exception (exec "rollback;")) + (false-if-exception (sqlite-exec db "rollback;")) (apply throw args)))) (define* (call-with-retrying-transaction db proc #:key restartable?) @@ -170,26 +167,14 @@ (define-syntax with-database ((_ file db exp ...) (call-with-database file (lambda (db) exp ...))))) -(define (call-with-statement db sql proc) - (let ((stmt (sqlite-prepare db sql #:cache? #t))) - (dynamic-wind - (const #t) - (lambda () - (proc stmt)) - (lambda () - (sqlite-finalize stmt))))) - -(define-syntax-rule (with-statement db sql stmt exp ...) - "Run EXP... with STMT bound to a prepared statement corresponding to the sql -string SQL for DB." - (call-with-statement db sql - (lambda (stmt) exp ...))) - (define (last-insert-row-id db) ;; XXX: (sqlite3) currently lacks bindings for 'sqlite3_last_insert_rowid'. ;; Work around that. - (with-statement db "SELECT last_insert_rowid();" stmt - (match (sqlite-fold cons '() stmt) + (let* ((stmt (sqlite-prepare db "SELECT last_insert_rowid();" + #:cache? #t)) + (result (sqlite-fold cons '() stmt))) + (sqlite-finalize stmt) + (match result ((#(id)) id) (_ #f)))) @@ -199,11 +184,13 @@ (define path-id-sql (define* (path-id db path) "If PATH exists in the 'ValidPaths' table, return its numerical identifier. Otherwise, return #f." - (with-statement db path-id-sql stmt + (let ((stmt (sqlite-prepare db path-id-sql #:cache? #t))) (sqlite-bind-arguments stmt #:path path) - (match (sqlite-fold cons '() stmt) - ((#(id) . _) id) - (_ #f)))) + (let ((result (sqlite-fold cons '() stmt))) + (sqlite-finalize stmt) + (match result + ((#(id) . _) id) + (_ #f))))) (define update-sql "UPDATE ValidPaths SET hash = :hash, registrationTime = :time, deriver = @@ -235,17 +222,20 @@ (define* (update-or-insert db #:key path deriver hash nar-size time) (let ((id (path-id db path))) (if id - (with-statement db update-sql stmt + (let ((stmt (sqlite-prepare db update-sql #:cache? #t))) (sqlite-bind-arguments stmt #:id id #:deriver deriver #:hash hash #:size nar-size #:time time) - (sqlite-fold cons '() stmt)) - (with-statement db insert-sql stmt + (sqlite-fold cons '() stmt) + (sqlite-finalize stmt) + (last-insert-row-id db)) + (let ((stmt (sqlite-prepare db insert-sql #:cache? #t))) (sqlite-bind-arguments stmt #:path path #:deriver deriver #:hash hash #:size nar-size #:time time) - (sqlite-fold cons '() stmt))) - (last-insert-row-id db))) + (sqlite-fold cons '() stmt) ;execute it + (sqlite-finalize stmt) + (last-insert-row-id db))))) (define add-reference-sql "INSERT OR REPLACE INTO Refs (referrer, reference) VALUES (:referrer, :reference);") @@ -253,13 +243,15 @@ (define add-reference-sql (define (add-references db referrer references) "REFERRER is the id of the referring store item, REFERENCES is a list ids of items referred to." - (with-statement db add-reference-sql stmt + (let ((stmt (sqlite-prepare db add-reference-sql #:cache? #t))) (for-each (lambda (reference) (sqlite-reset stmt) (sqlite-bind-arguments stmt #:referrer referrer #:reference reference) - (sqlite-fold cons '() stmt)) - references))) + (sqlite-fold cons '() stmt) ;execute it + (last-insert-row-id db)) + references) + (sqlite-finalize stmt))) (define (timestamp) "Return a timestamp, either the current time of SOURCE_DATE_EPOCH."