| Message ID | 87ftbernat.fsf@cune.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#41658] fixes / improvements for (guix store database) | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/applying patch | fail | View Laminar job |
Hi, Thanks for the thorough investigation and for the patches! Caleb Ristvedt <caleb.ristvedt@cune.org> skribis: > From cce653c590be1506e15044e445aa9805370ac759 Mon Sep 17 00:00:00 2001 > From: Caleb Ristvedt <caleb.ristvedt@cune.org> > Date: Mon, 1 Jun 2020 18:50:07 -0500 > Subject: [PATCH 1/4] database: work around guile-sqlite3 bug preventing > statement reset > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > guile-sqlite3 provides statement caching, making it unnecessary for sqlite to > keep re-preparing statements that are frequently used. Unfortunately it > doesn't quite emulate the semantics of sqlite_finalize properly, because it > doesn't cause a commit if the statement being finalized is the last "active" > statement. We work around this by wrapping sqlite-finalize with our own > version that ensures sqlite-reset is called, which does The Right Thing™. > > * guix/store/database.scm (sqlite-finalize): new procedure that shadows the > sqlite-finalize from (sqlite3). Nice. It would be great if you could report it upstream (Danny and/or myself can then patch it directly in guile-sqlite3 and push out a release) and refer to the issue from here. We can have this patch locally in the meantime, unless it would break once the new guile-sqlite3 is out. WDYT? > From ee24ab21122b1c75a7d67d7062550e15e54ab62f Mon Sep 17 00:00:00 2001 > From: Caleb Ristvedt <caleb.ristvedt@cune.org> > Date: Mon, 1 Jun 2020 19:21:43 -0500 > Subject: [PATCH 2/4] database: rewrite query procedures in terms of > with-statement. > > Most of our queries would fail to finalize their statements properly if sqlite > returned an error during their execution. This resolves that, and also makes > them somewhat more concise as a side-effect. > > This also makes some small changes to improve certain queries where behavior > was strange or overly verbose. > > * guix/store/database.scm (call-with-statement): new procedure. > (with-statement): new macro. > (last-insert-row-id, path-id, update-or-insert, add-references): rewrite to > use with-statement. > (update-or-insert): factor last-insert-row-id out of the end of both > branches. > (add-references): remove pointless last-insert-row-id call. > > * .dir-locals.el (with-statement): add indenting information. LGTM! > From 7d34c27c33aed3e8a49b9796a62a8c19d352e653 Mon Sep 17 00:00:00 2001 > From: Caleb Ristvedt <caleb.ristvedt@cune.org> > Date: Mon, 1 Jun 2020 21:43:14 -0500 > Subject: [PATCH 3/4] database: ensure update-or-insert is run within a > transaction > > update-or-insert can break if an insert occurs between when it decides whether > to update or insert and when it actually performs that operation. Putting the > check and the update/insert operation in the same transaction ensures that the > update/insert will only succeed if no other write has occurred in the middle. > > * guix/store/database.scm (call-with-savepoint): new procedure. > (update-or-insert): use call-with-savepoint to ensure the read and the > insert/update occur within the same transaction. That’s a bit beyond my understanding, but I think you can also push this one. :-) Make sure “make check TESTS=tests/store-database.scm” is still happy. Thanks a lot! Ludo’.
Hi Ludo, Hi Caleb, On Thu, 04 Jun 2020 18:40:35 +0200 Ludovic Courtès <ludo@gnu.org> wrote: > Nice. It would be great if you could report it upstream (Danny and/or > myself can then patch it directly in guile-sqlite3 and push out a > release) and refer to the issue from here. I agree. It's easy to change sqlite-finalize in guile-sqlite3 to call sqlite-reset, basically just adapt (define sqlite-finalize (let ((f (pointer->procedure int (dynamic-func "sqlite3_finalize" libsqlite3) (list '*)))) (lambda (stmt) ;; Note: When STMT is cached, this is a no-op. This ensures caching ;; actually works while still separating concerns: users can turn ;; caching on and off without having to change the rest of their code. (when (and (stmt-live? stmt) (not (stmt-cached? stmt))) (let ((p (stmt-pointer stmt))) (sqlite-remove-statement! (stmt->db stmt) stmt) (set-stmt-live?! stmt #f) (f p)))))) so that it calls sqlite-reset in the "when"'s new "else" branch there. (we could also always call sqlite3_reset on sqlite-finalize anyway, it wouldn't hurt but it wouldn't help either) I agree that sqlite-finalize should model sqlite's finalization behavior as much as possible. Also, the comment about this being a no-op is not true then anymore. We should definitely also pick up Caleb's comment upstream: + ;; Cached statements aren't reset when sqlite-finalize is invoked on + ;; them. This can cause problems with automatically-started transactions: + ;; + ;; "An implicit transaction (a transaction that is started automatically, + ;; not a transaction started by BEGIN) is committed automatically when the + ;; last active statement finishes. A statement finishes when its last cursor + ;; closes, which is guaranteed to happen when the prepared statement is + ;; reset or finalized. Some statements might "finish" for the purpose of + ;; transaction control prior to being reset or finalized, but there is no + ;; guarantee of this." + ;; + ;; Thus, it's possible for an implicitly-started transaction to hang around + ;; until sqlite-reset is called when the cached statement is next + ;; used. Because the transaction is committed automatically only when the + ;; *last active statement* finishes, the implicitly-started transaction may + ;; later be upgraded to a write transaction (!) and this non-reset statement + ;; will still be keeping the transaction from committing until it is next + ;; used or the database connection is closed. This has the potential to make + ;; (exclusive) write access to the database necessary for much longer than + ;; it should be. + ;; + ;; (see https://www.sqlite.org/lang_transaction.html) @Caleb: Could you file an issue at https://notabug.org/guile-sqlite3/guile-sqlite3/issues and pull request so this is auditable?
Hi Danny! Danny Milosavljevic <dannym@scratchpost.org> skribis: > I agree. It's easy to change sqlite-finalize in guile-sqlite3 to > call sqlite-reset, basically just adapt [...] > @Caleb: > > Could you file an issue at https://notabug.org/guile-sqlite3/guile-sqlite3/issues > and pull request so this is auditable? Agreed. Danny, once this is merged upstream, could you tag a new release? There are a few other useful improvements in there. Thanks, Ludo’.
From e30271728dfb23324c981d226c752b17689c9eef Mon Sep 17 00:00:00 2001 From: Caleb Ristvedt <caleb.ristvedt@cune.org> Date: Mon, 1 Jun 2020 22:15:21 -0500 Subject: [PATCH 4/4] database: separate transaction-handling and retry-handling. Previously call-with-transaction would both retry when SQLITE_BUSY errors were thrown and do what its name suggested (start and rollback/commit a transaction). This changes it to do only what its name implies, which simplifies its implementation. Retrying is provided by the new call-with-SQLITE_BUSY-retrying procedure. * guix/store/database.scm (call-with-transaction): no longer restarts, new #:restartable? argument controls whether "begin" or "begin immediate" is used. (call-with-SQLITE_BUSY-retrying, call-with-retrying-transaction, call-with-retrying-savepoint): new procedures. (register-items): use call-with-retrying-transaction to preserve old behavior. * .dir-locals.el (call-with-retrying-transaction, call-with-retrying-savepoint): add indentation information. --- .dir-locals.el | 2 ++ guix/store/database.scm | 69 +++++++++++++++++++++++++++++------------ 2 files changed, 51 insertions(+), 20 deletions(-) diff --git a/.dir-locals.el b/.dir-locals.el index ef25cb100a..e9dccd0511 100644 --- a/.dir-locals.el +++ b/.dir-locals.el @@ -90,7 +90,9 @@ (eval . (put 'with-database 'scheme-indent-function 2)) (eval . (put 'call-with-transaction 'scheme-indent-function 2)) (eval . (put 'with-statement 'scheme-indent-function 3)) + (eval . (put 'call-with-retrying-transaction 'scheme-indent-function 2)) (eval . (put 'call-with-savepoint 'scheme-indent-function 1)) + (eval . (put 'call-with-retrying-savepoint 'scheme-indent-function 1)) (eval . (put 'call-with-container 'scheme-indent-function 1)) (eval . (put 'container-excursion 'scheme-indent-function 1)) diff --git a/guix/store/database.scm b/guix/store/database.scm index 3955c48b1f..2a78379dac 100644 --- a/guix/store/database.scm +++ b/guix/store/database.scm @@ -99,27 +99,44 @@ create it and initialize it as a new database." ;; XXX: missing in guile-sqlite3@0.1.0 (define SQLITE_BUSY 5) -(define (call-with-transaction db proc) - "Start a transaction with DB (make as many attempts as necessary) and run -PROC. If PROC exits abnormally, abort the transaction, otherwise commit the -transaction after it finishes." +(define (call-with-SQLITE_BUSY-retrying thunk) + "Call THUNK, retrying as long as it exits abnormally due to SQLITE_BUSY +errors." (catch 'sqlite-error + thunk + (lambda (key who code errmsg) + (if (= code SQLITE_BUSY) + (call-with-SQLITE_BUSY-retrying thunk) + (throw key who code errmsg))))) + + + +(define* (call-with-transaction db proc #:key restartable?) + "Start a transaction with DB and run PROC. If PROC exits abnormally, abort +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;")) + (catch #t (lambda () - ;; We 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!) would be - ;; executed again for every retry. - (sqlite-exec db "begin immediate;") - (let ((result (proc))) - (sqlite-exec db "commit;") - result)) - (lambda (key who error description) - (if (= error SQLITE_BUSY) - (call-with-transaction db proc) - (begin - (sqlite-exec db "rollback;") - (throw 'sqlite-error who error description)))))) + (let-values ((result (proc))) + (exec "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;")) + (apply throw args)))) + (define* (call-with-savepoint db proc #:optional (savepoint-name "SomeSavepoint")) "Call PROC after creating a savepoint named SAVEPOINT-NAME. If PROC exits @@ -141,6 +158,18 @@ prior to returning." (lambda () (exec (string-append "RELEASE " savepoint-name ";"))))) +(define* (call-with-retrying-transaction db proc #:key restartable?) + (call-with-SQLITE_BUSY-retrying + (lambda () + (call-with-transaction db proc #:restartable? restartable?)))) + +(define* (call-with-retrying-savepoint db proc + #:optional (savepoint-name + "SomeSavepoint")) + (call-with-SQLITE_BUSY-retrying + (lambda () + (call-with-savepoint db proc savepoint-name)))) + (define %default-database-file ;; Default location of the store database. (string-append %store-database-directory "/db.sqlite")) @@ -431,7 +460,7 @@ Write a progress report to LOG-PORT." (mkdir-p db-dir) (parameterize ((sql-schema schema)) (with-database (string-append db-dir "/db.sqlite") db - (call-with-transaction db + (call-with-retrying-transaction db (lambda () (let* ((prefix (format #f "registering ~a items" (length items))) (progress (progress-reporter/bar (length items) -- 2.26.2