From debbugs-submit-bounces@debbugs.gnu.org Tue Jun 02 02:31:55 2020 Received: (at submit) by debbugs.gnu.org; 2 Jun 2020 06:31:55 +0000 Received: from localhost ([127.0.0.1]:37710 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jg0So-0006ni-Qg for submit@debbugs.gnu.org; Tue, 02 Jun 2020 02:31:55 -0400 Received: from lists.gnu.org ([209.51.188.17]:58026) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jg0Sm-0006na-Gc for submit@debbugs.gnu.org; Tue, 02 Jun 2020 02:31:53 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:44188) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jg0Sm-0005QZ-AI for guix-patches@gnu.org; Tue, 02 Jun 2020 02:31:52 -0400 Received: from mail-il1-x12d.google.com ([2607:f8b0:4864:20::12d]:41927) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jg0Sj-0002vG-KU for guix-patches@gnu.org; Tue, 02 Jun 2020 02:31:51 -0400 Received: by mail-il1-x12d.google.com with SMTP id d1so11792522ila.8 for ; Mon, 01 Jun 2020 23:31:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cune-org.20150623.gappssmtp.com; s=20150623; h=from:to:subject:date:message-id:user-agent:mime-version; bh=0Hv+5EliMxBI2r7Kdfa5detqIH1SOtXpJjvG2m1ohUY=; b=miGmYGS+Dcxez+wwuHvzo394g1Z/mWtAHfHZ748IzWrVOzYvfXIloejVqqFmA9VkFp 3wcY/JX2vQmMDhgTyO9zgP49XsFN5dnVbbEnrokJHEUVRnLLFb2XhgCxIpERyyHfVh89 Zr6/JPEgRgBZPNeFrs1riRQetFO5EBTMdxVXA/OMEhcJvMuEGGWyqE2ybDWWOxUP4qaS jxSgNASNHvtk29HRfXxA7XF37VsgbzZlkXivXyS65NgF5LTXVcH8aOxAE8zUDowrpmYc sa+TvvIAY00xNqVam9KFAo1OHhK8zK0YhmSvkPaWRHhK31lvCOOdAuZ/9iTgujRo9F+R YW1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:user-agent :mime-version; bh=0Hv+5EliMxBI2r7Kdfa5detqIH1SOtXpJjvG2m1ohUY=; b=ZGzP0F8wXgwPtmzytbbPjP4gFxd66Ygaye06CQ0HdSYBUasRqk+fJ0bQLwJumN5BAD 8GVaay23Rqg5GNyH2VZQjjejEfJdmkquoYDcgzhFUmwCJrBf63XppNiPggK3eqlFNwI2 iBYl4bS7EacKWOF2/bfZzf8V7bm52cg+tvn50O1mntAntnlxpMuS1aMbb6Rt8a3BHyh2 XIVw0uqrrpPe0rSzRvIbi9PvnkUFIp4ZjoLJ5ayiGh3mpCSD7FnhE7A9Jnx2xgtpPiMM d8SinW8YDCXFYSlfzM2u7rc0Qn/uUW4jP7Cy5sKdkhjoiPcA/xpAWJzSVQV6IBqGaPdY rWrw== X-Gm-Message-State: AOAM532c3kHeYEuZU4opBpVk6wG8IHbOLf4rhZN80SPNtvk7egAlCKMT rhbfDWZmYzAZUHxynwv7G3RoTCdm6yg= X-Google-Smtp-Source: ABdhPJy1b/qpnItrtxbrYPRYHjnTVYFxZ/++41AyK1+OpHiK/M3lDh0VxNocXOsv5tqHg2KZLPzB1w== X-Received: by 2002:a92:5c18:: with SMTP id q24mr3666503ilb.181.1591079503689; Mon, 01 Jun 2020 23:31:43 -0700 (PDT) Received: from GuixPotato ([208.89.170.24]) by smtp.gmail.com with ESMTPSA id y13sm701102iob.51.2020.06.01.23.31.42 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 01 Jun 2020 23:31:42 -0700 (PDT) From: Caleb Ristvedt To: guix-patches@gnu.org Subject: [PATCH] fixes / improvements for (guix store database) Date: Tue, 02 Jun 2020 01:31:38 -0500 Message-ID: <87ftbernat.fsf@cune.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: multipart/signed; boundary="==-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Received-SPF: pass client-ip=2607:f8b0:4864:20::12d; envelope-from=caleb.ristvedt@cune.org; helo=mail-il1-x12d.google.com X-detected-operating-system: by eggs.gnu.org: No matching host in p0f cache. That's all we know. X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001 autolearn=_AUTOLEARN X-Spam_action: no action X-Spam-Score: -1.4 (-) X-Debbugs-Envelope-To: submit X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -2.4 (--) --==-=-= Content-Type: multipart/mixed; boundary="=-=-=" --=-=-= Content-Type: text/plain After some pondering about why the database might be locked so frequently, this is what I've managed to come up with. The first patch is the most likely to actually help with that, and the others mostly involve improving robustness. Ideally we'd come up with a test to quantify how much these kinds of changes affect contention over the database. For now, though, all that I can think of is seeing how this affects the systems that have had issues with that. - reepca --=-=-= Content-Type: text/x-patch; charset=utf-8 Content-Disposition: attachment; filename=0001-database-work-around-guile-sqlite3-bug-preventing-st.patch Content-Transfer-Encoding: quoted-printable From=20cce653c590be1506e15044e445aa9805370ac759 Mon Sep 17 00:00:00 2001 From: Caleb Ristvedt 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=3DUTF-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=E2= =84=A2. * guix/store/database.scm (sqlite-finalize): new procedure that shadows the sqlite-finalize from (sqlite3). =2D-- guix/store/database.scm | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/guix/store/database.scm b/guix/store/database.scm index ef52036ede..d4251e580e 100644 =2D-- a/guix/store/database.scm +++ b/guix/store/database.scm @@ -130,6 +130,36 @@ transaction after it finishes." If FILE doesn't exist, create it and initialize it as a new database." (call-with-database file (lambda (db) exp ...))) =20 +(define (sqlite-finalize stmt) + ;; 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 t= he + ;; last active statement finishes. A statement finishes when its last cu= rsor + ;; 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 aro= und + ;; 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 state= ment + ;; 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 th= an + ;; it should be. + ;; + ;; (see https://www.sqlite.org/lang_transaction.html) + ;; To work around this, we wrap sqlite-finalize so that sqlite-reset is + ;; always called. This will continue working even when the behavior is f= ixed + ;; in guile-sqlite3, since resetting twice doesn't cause any problems. We + ;; can remove this once the fixed guile-sqlite3 is widespread. + (sqlite-reset stmt) + ((@ (sqlite3) sqlite-finalize) stmt)) + (define (last-insert-row-id db) ;; XXX: (sqlite3) currently lacks bindings for 'sqlite3_last_insert_rowi= d'. ;; Work around that. =2D-=20 2.26.2 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0002-database-rewrite-query-procedures-in-terms-of-with-s.patch Content-Transfer-Encoding: quoted-printable From=20ee24ab21122b1c75a7d67d7062550e15e54ab62f Mon Sep 17 00:00:00 2001 From: Caleb Ristvedt 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 sql= ite returned an error during their execution. This resolves that, and also mak= es 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. =2D-- .dir-locals.el | 1 + guix/store/database.scm | 53 ++++++++++++++++++++++------------------- 2 files changed, 30 insertions(+), 24 deletions(-) diff --git a/.dir-locals.el b/.dir-locals.el index fcde914e60..a085269e85 100644 =2D-- a/.dir-locals.el +++ b/.dir-locals.el @@ -89,6 +89,7 @@ =20 (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)) =20 (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 d4251e580e..2209da3df1 100644 =2D-- a/guix/store/database.scm +++ b/guix/store/database.scm @@ -160,14 +160,26 @@ If FILE doesn't exist, create it and initialize it as= a new database." (sqlite-reset stmt) ((@ (sqlite3) sqlite-finalize) stmt)) =20 +(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_rowi= d'. ;; Work around that. =2D (let* ((stmt (sqlite-prepare db "SELECT last_insert_rowid();" =2D #:cache? #t)) =2D (result (sqlite-fold cons '() stmt))) =2D (sqlite-finalize stmt) =2D (match result + (with-statement db "SELECT last_insert_rowid();" stmt + (match (sqlite-fold cons '() stmt) ((#(id)) id) (_ #f)))) =20 @@ -177,13 +189,11 @@ If FILE doesn't exist, create it and initialize it as= a new database." (define* (path-id db path) "If PATH exists in the 'ValidPaths' table, return its numerical identifier. Otherwise, return #f." =2D (let ((stmt (sqlite-prepare db path-id-sql #:cache? #t))) + (with-statement db path-id-sql stmt (sqlite-bind-arguments stmt #:path path) =2D (let ((result (sqlite-fold cons '() stmt))) =2D (sqlite-finalize stmt) =2D (match result =2D ((#(id) . _) id) =2D (_ #f))))) + (match (sqlite-fold cons '() stmt) + ((#(id) . _) id) + (_ #f)))) =20 (define update-sql "UPDATE ValidPaths SET hash =3D :hash, registrationTime =3D :time, deriv= er =3D @@ -200,20 +210,17 @@ and re-inserting instead of updating, which causes pr= oblems with foreign keys, of course. Returns the row id of the row that was modified or inserted." (let ((id (path-id db path))) (if id =2D (let ((stmt (sqlite-prepare db update-sql #:cache? #t))) + (with-statement db update-sql stmt (sqlite-bind-arguments stmt #:id id #:deriver deriver #:hash hash #:size nar-size #:time time) =2D (sqlite-fold cons '() stmt) =2D (sqlite-finalize stmt) =2D (last-insert-row-id db)) =2D (let ((stmt (sqlite-prepare db insert-sql #:cache? #t))) + (sqlite-fold cons '() stmt)) + (with-statement db insert-sql stmt (sqlite-bind-arguments stmt #:path path #:deriver deriver #:hash hash #:size nar-size #:time time) =2D (sqlite-fold cons '() stmt) ;execute it =2D (sqlite-finalize stmt) =2D (last-insert-row-id db))))) + (sqlite-fold cons '() stmt))) + (last-insert-row-id db))) =20 (define add-reference-sql "INSERT OR REPLACE INTO Refs (referrer, reference) VALUES (:referrer, :r= eference);") @@ -221,15 +228,13 @@ of course. Returns the row id of the row that was mod= ified or inserted." (define (add-references db referrer references) "REFERRER is the id of the referring store item, REFERENCES is a list ids of items referred to." =2D (let ((stmt (sqlite-prepare db add-reference-sql #:cache? #t))) + (with-statement db add-reference-sql stmt (for-each (lambda (reference) (sqlite-reset stmt) (sqlite-bind-arguments stmt #:referrer referrer #:reference reference) =2D (sqlite-fold cons '() stmt) ;execute it =2D (last-insert-row-id db)) =2D references) =2D (sqlite-finalize stmt))) + (sqlite-fold cons '() stmt)) + references))) =20 (define* (sqlite-register db #:key path (references '()) deriver hash nar-size time) =2D-=20 2.26.2 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0003-database-ensure-update-or-insert-is-run-within-a-tra.patch Content-Transfer-Encoding: quoted-printable From=207d34c27c33aed3e8a49b9796a62a8c19d352e653 Mon Sep 17 00:00:00 2001 From: Caleb Ristvedt 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 whet= her 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 middl= e. * 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. =2D-- .dir-locals.el | 1 + guix/store/database.scm | 68 +++++++++++++++++++++++++++++++++-------- 2 files changed, 56 insertions(+), 13 deletions(-) diff --git a/.dir-locals.el b/.dir-locals.el index a085269e85..ef25cb100a 100644 =2D-- a/.dir-locals.el +++ b/.dir-locals.el @@ -90,6 +90,7 @@ (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-savepoint 'scheme-indent-function 1)) =20 (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 2209da3df1..3955c48b1f 100644 =2D-- a/guix/store/database.scm +++ b/guix/store/database.scm @@ -120,6 +120,26 @@ transaction after it finishes." (begin (sqlite-exec db "rollback;") (throw 'sqlite-error who error description)))))) +(define* (call-with-savepoint db proc + #:optional (savepoint-name "SomeSavepoint")) + "Call PROC after creating a savepoint named SAVEPOINT-NAME. If PROC exi= ts +abnormally, rollback to that savepoint. In all cases, remove the savepoint +prior to returning." + (define (exec sql) + (with-statement db sql stmt + (sqlite-fold cons '() stmt))) + + (dynamic-wind + (lambda () + (exec (string-append "SAVEPOINT " savepoint-name ";"))) + (lambda () + (catch #t + proc + (lambda args + (exec (string-append "ROLLBACK TO " savepoint-name ";")) + (apply throw args)))) + (lambda () + (exec (string-append "RELEASE " savepoint-name ";"))))) =20 (define %default-database-file ;; Default location of the store database. @@ -208,19 +228,41 @@ VALUES (:path, :hash, :time, :deriver, :size)") doesn't exactly have... they've got something close, but it involves delet= ing and re-inserting instead of updating, which causes problems with foreign k= eys, of course. Returns the row id of the row that was modified or inserted." =2D (let ((id (path-id db path))) =2D (if id =2D (with-statement db update-sql stmt =2D (sqlite-bind-arguments stmt #:id id =2D #:deriver deriver =2D #:hash hash #:size nar-size #:time time) =2D (sqlite-fold cons '() stmt)) =2D (with-statement db insert-sql stmt =2D (sqlite-bind-arguments stmt =2D #:path path #:deriver deriver =2D #:hash hash #:size nar-size #:time time) =2D (sqlite-fold cons '() stmt))) =2D (last-insert-row-id db))) + + ;; It's important that querying the path-id and the insert/update operat= ion + ;; take place in the same transaction, as otherwise some other + ;; process/thread/fiber could register the same path between when we che= ck + ;; whether it's already registered and when we register it, resulting in + ;; duplicate paths (which, due to a 'unique' constraint, would cause an + ;; exception to be thrown). With the default journaling mode this will + ;; prevent writes from occurring during that sensitive time, but with WAL + ;; mode it will instead arrange to return SQLITE_BUSY when a write occurs + ;; between the start of a read transaction and its upgrading to a write + ;; transaction (see https://sqlite.org/rescode.html#busy_snapshot). + ;; Experimentally, it seems this SQLITE_BUSY will ignore a busy_timeout = and + ;; immediately return (makes sense, since waiting won't change anything). + + ;; Note that when that kind of SQLITE_BUSY error is returned, it will ke= ep + ;; being returned every time we try to upgrade the same outermost + ;; transaction to a write transaction. So when retrying, we have to res= tart + ;; the *outermost* write transaction. We can't inherently tell whether + ;; we're the outermost write transaction, so we leave the retry-handling= to + ;; the caller. + (call-with-savepoint db + (lambda () + (let ((id (path-id db path))) + (if id + (with-statement db update-sql stmt + (sqlite-bind-arguments stmt #:id id + #:deriver deriver + #:hash hash #:size nar-size #:time ti= me) + (sqlite-fold cons '() stmt)) + (with-statement db insert-sql stmt + (sqlite-bind-arguments stmt + #:path path #:deriver deriver + #:hash hash #:size nar-size #:time ti= me) + (sqlite-fold cons '() stmt))) + (last-insert-row-id db))))) =20 (define add-reference-sql "INSERT OR REPLACE INTO Refs (referrer, reference) VALUES (:referrer, :r= eference);") =2D-=20 2.26.2 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0004-database-separate-transaction-handling-and-retry-han.patch Content-Transfer-Encoding: quoted-printable From=20e30271728dfb23324c981d226c752b17689c9eef Mon Sep 17 00:00:00 2001 From: Caleb Ristvedt 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 w= ere 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. =2D-- .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 =2D-- 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)) =20 (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 =2D-- 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) =20 =2D(define (call-with-transaction db proc) =2D "Start a transaction with DB (make as many attempts as necessary) and = run =2DPROC. If PROC exits abnormally, abort the transaction, otherwise commit= the =2Dtransaction 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 (=3D 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, ab= ort +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 mult= iple +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 fig= ure + ;; that out immediately rather than because some SQLITE_BUSY exception g= ets + ;; 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 () =2D ;; We use begin immediate here so that if we need to retry, we =2D ;; figure that out immediately rather than because some SQLITE_BUSY =2D ;; exception gets thrown partway through PROC - in which case the =2D ;; part already executed (which may contain side-effects!) would be =2D ;; executed again for every retry. =2D (sqlite-exec db "begin immediate;") =2D (let ((result (proc))) =2D (sqlite-exec db "commit;") =2D result)) =2D (lambda (key who error description) =2D (if (=3D error SQLITE_BUSY) =2D (call-with-transaction db proc) =2D (begin =2D (sqlite-exec db "rollback;") =2D (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 si= gnal + ;; 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 exi= ts @@ -141,6 +158,18 @@ prior to returning." (lambda () (exec (string-append "RELEASE " savepoint-name ";"))))) =20 +(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 =2D (call-with-transaction db + (call-with-retrying-transaction db (lambda () (let* ((prefix (format #f "registering ~a items" (length ite= ms))) (progress (progress-reporter/bar (length items) =2D-=20 2.26.2 --=-=-=-- --==-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEEdNapMPRLm4SepVYGwWaqSV9/GJwFAl7V8koACgkQwWaqSV9/ GJwHXgf/R/mpXxFexv9B/V22BwF8+bNmDr6L0KVdFmaKz15nmPbMnnLtqmJTkrik vbEtwQ9MT4we6yNRD76VaVEe+iE1Yk9WvCyBNUcoPL07dlzY/hN+0frSjQI4MqSc GIktVbdt6Bf3bloCl5SHg2KrGEYY+ptWliEuY3AfQnBL/7YoSKfd4me5TLa1PB/4 RhyVqVFJHq/jeSQvirDYQoMCiRaDCxu8g+xV5/7ITE3Ue+gkM2aYKSrWz8cFhiJ4 /S3zFZYuI31d2J8jdkbcz5tuVG/ALpTTCIJa8cwIpRB2Yon6yZN4zoDX8ue3P8we qEnsPdkxSPu7lckVk7OqQNQTvxpknQ== =2ARa -----END PGP SIGNATURE----- --==-=-=-- From debbugs-submit-bounces@debbugs.gnu.org Thu Jun 04 12:40:47 2020 Received: (at 41658) by debbugs.gnu.org; 4 Jun 2020 16:40:47 +0000 Received: from localhost ([127.0.0.1]:47372 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jgsv9-0002Mq-3m for submit@debbugs.gnu.org; Thu, 04 Jun 2020 12:40:47 -0400 Received: from eggs.gnu.org ([209.51.188.92]:40948) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jgsv7-0002Md-Ho for 41658@debbugs.gnu.org; Thu, 04 Jun 2020 12:40:46 -0400 Received: from fencepost.gnu.org ([2001:470:142:3::e]:36129) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jgsv2-0001kc-1l; Thu, 04 Jun 2020 12:40:40 -0400 Received: from [2a01:e0a:1d:7270:af76:b9b:ca24:c465] (port=46088 helo=ribbon) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1jgsv1-0006Um-D1; Thu, 04 Jun 2020 12:40:39 -0400 From: =?utf-8?Q?Ludovic_Court=C3=A8s?= To: Caleb Ristvedt Subject: Re: [bug#41658] [PATCH] fixes / improvements for (guix store database) References: <87ftbernat.fsf@cune.org> Date: Thu, 04 Jun 2020 18:40:35 +0200 In-Reply-To: <87ftbernat.fsf@cune.org> (Caleb Ristvedt's message of "Tue, 02 Jun 2020 01:31:38 -0500") Message-ID: <87a71i943g.fsf@gnu.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Score: -2.3 (--) X-Debbugs-Envelope-To: 41658 Cc: 41658@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -3.3 (---) Hi, Thanks for the thorough investigation and for the patches! Caleb Ristvedt skribis: > From cce653c590be1506e15044e445aa9805370ac759 Mon Sep 17 00:00:00 2001 > From: Caleb Ristvedt > 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=3DUTF-8 > Content-Transfer-Encoding: 8bit > > guile-sqlite3 provides statement caching, making it unnecessary for sqlit= e 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 "acti= ve" > statement. We work around this by wrapping sqlite-finalize with our own > version that ensures sqlite-reset is called, which does The Right Thing= =E2=84=A2. > > * guix/store/database.scm (sqlite-finalize): new procedure that shadows t= he > 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 > 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 s= qlite > returned an error during their execution. This resolves that, and also m= akes > them somewhat more concise as a side-effect. > > This also makes some small changes to improve certain queries where behav= ior > 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): rewrit= e 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 > 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 wh= ether > to update or insert and when it actually performs that operation. Puttin= g the > check and the update/insert operation in the same transaction ensures tha= t the > update/insert will only succeed if no other write has occurred in the mid= dle. > > * 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=E2=80=99s a bit beyond my understanding, but I think you can also push= this one. :-) Make sure =E2=80=9Cmake check TESTS=3Dtests/store-database.scm=E2=80=9D is = still happy. Thanks a lot! Ludo=E2=80=99. From debbugs-submit-bounces@debbugs.gnu.org Thu Jun 04 13:01:01 2020 Received: (at 41658) by debbugs.gnu.org; 4 Jun 2020 17:01:01 +0000 Received: from localhost ([127.0.0.1]:47391 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jgtEU-0002uV-Ly for submit@debbugs.gnu.org; Thu, 04 Jun 2020 13:01:01 -0400 Received: from dd26836.kasserver.com ([85.13.145.193]:48230) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jgtES-0002uN-UI for 41658@debbugs.gnu.org; Thu, 04 Jun 2020 13:00:46 -0400 Received: from localhost (80-110-127-207.cgn.dynamic.surfer.at [80.110.127.207]) by dd26836.kasserver.com (Postfix) with ESMTPSA id 1076A3362236; Thu, 4 Jun 2020 19:00:43 +0200 (CEST) Date: Thu, 4 Jun 2020 19:00:40 +0200 From: Danny Milosavljevic To: Ludovic =?ISO-8859-1?Q?Court=E8s?= , Caleb Ristvedt Subject: Re: [bug#41658] [PATCH] fixes / improvements for (guix store database) Message-ID: <20200604190000.3c454a83@scratchpost.org> In-Reply-To: <87a71i943g.fsf@gnu.org> References: <87ftbernat.fsf@cune.org> <87a71i943g.fsf@gnu.org> X-Mailer: Claws Mail 3.17.5 (GTK+ 2.24.32; x86_64-unknown-linux-gnu) MIME-Version: 1.0 Content-Type: multipart/signed; boundary="Sig_//Q/Ho3.GHjQ9ZFu5708gxoh"; protocol="application/pgp-signature"; micalg=pgp-sha512 X-Spam-Score: -0.7 (/) X-Debbugs-Envelope-To: 41658 Cc: 41658@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) --Sig_//Q/Ho3.GHjQ9ZFu5708gxoh Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi Ludo, Hi Caleb, On Thu, 04 Jun 2020 18:40:35 +0200 Ludovic Court=C3=A8s 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 woul= dn'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 t= he + ;; last active statement finishes. A statement finishes when its last cu= rsor + ;; 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 aro= und + ;; 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 state= ment + ;; 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 th= an + ;; 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? --Sig_//Q/Ho3.GHjQ9ZFu5708gxoh Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iQEzBAEBCgAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAl7ZKLgACgkQ5xo1VCww uqW8HAf+LapIRcQdeHgWmjm9LAW3w8Ww1MFSZhulb/a/GKz4tC5EMCOlnIs7Otaj kdpeQopGmt8lPHNQNYc85jszOZe+ROzDC7+iraeYIXtoVdQGm5uROts8DH25YGDc GiEg+Q77vJtS73u+f2iG/18UCWOsM0Czkgnv1kV7fFb+yedhl+OoPmueyHbZVl/n dHhut5EwuyXRimz0yErqY4atoFrqU9fPWCe7JZHhz+krHhHavE2AUL6TFBroayDf O2m5aWECT7yUNR0wuyUN9NCLQPNuJ9D4WtHn8B+3KYQ6IT9Mj0jtIhwkRk2k/dy4 T0WyJaaKSB5WgM5WYouCeR7OMKfSSg== =hCaa -----END PGP SIGNATURE----- --Sig_//Q/Ho3.GHjQ9ZFu5708gxoh-- From debbugs-submit-bounces@debbugs.gnu.org Fri Jun 05 12:19:49 2020 Received: (at 41658) by debbugs.gnu.org; 5 Jun 2020 16:19:49 +0000 Received: from localhost ([127.0.0.1]:49696 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jhF4P-0001e4-EO for submit@debbugs.gnu.org; Fri, 05 Jun 2020 12:19:49 -0400 Received: from eggs.gnu.org ([209.51.188.92]:50822) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jhF4O-0001dq-1G for 41658@debbugs.gnu.org; Fri, 05 Jun 2020 12:19:48 -0400 Received: from fencepost.gnu.org ([2001:470:142:3::e]:40557) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jhF4I-0002e2-IW; Fri, 05 Jun 2020 12:19:42 -0400 Received: from [2a01:e0a:1d:7270:af76:b9b:ca24:c465] (port=50280 helo=ribbon) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1jhF4I-0007MH-1u; Fri, 05 Jun 2020 12:19:42 -0400 From: =?utf-8?Q?Ludovic_Court=C3=A8s?= To: Danny Milosavljevic Subject: Re: [bug#41658] [PATCH] fixes / improvements for (guix store database) References: <87ftbernat.fsf@cune.org> <87a71i943g.fsf@gnu.org> <20200604190000.3c454a83@scratchpost.org> X-URL: http://www.fdn.fr/~lcourtes/ X-Revolutionary-Date: 18 Prairial an 228 de la =?utf-8?Q?R=C3=A9volution?= X-PGP-Key-ID: 0x090B11993D9AEBB5 X-PGP-Key: http://www.fdn.fr/~lcourtes/ludovic.asc X-PGP-Fingerprint: 3CE4 6455 8A84 FDC6 9DB4 0CFB 090B 1199 3D9A EBB5 X-OS: x86_64-pc-linux-gnu Date: Fri, 05 Jun 2020 18:19:40 +0200 In-Reply-To: <20200604190000.3c454a83@scratchpost.org> (Danny Milosavljevic's message of "Thu, 4 Jun 2020 19:00:40 +0200") Message-ID: <87bllx32oz.fsf@gnu.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Score: -2.3 (--) X-Debbugs-Envelope-To: 41658 Cc: Caleb Ristvedt , 41658@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -3.3 (---) Hi Danny! Danny Milosavljevic 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-sqlite= 3/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=E2=80=99. From debbugs-submit-bounces@debbugs.gnu.org Sat Jun 06 17:38:32 2020 Received: (at control) by debbugs.gnu.org; 6 Jun 2020 21:38:32 +0000 Received: from localhost ([127.0.0.1]:52600 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jhgWO-0005We-7Q for submit@debbugs.gnu.org; Sat, 06 Jun 2020 17:38:32 -0400 Received: from eggs.gnu.org ([209.51.188.92]:48184) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jhgWM-0005WQ-CT for control@debbugs.gnu.org; Sat, 06 Jun 2020 17:38:30 -0400 Received: from fencepost.gnu.org ([2001:470:142:3::e]:39149) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jhgWH-0005Gt-3n for control@debbugs.gnu.org; Sat, 06 Jun 2020 17:38:25 -0400 Received: from [2a01:e0a:1d:7270:af76:b9b:ca24:c465] (port=38976 helo=ribbon) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1jhgWD-0007zY-LU for control@debbugs.gnu.org; Sat, 06 Jun 2020 17:38:22 -0400 Date: Sat, 06 Jun 2020 23:38:19 +0200 Message-Id: <87lfkzub78.fsf@gnu.org> To: control@debbugs.gnu.org From: =?utf-8?Q?Ludovic_Court=C3=A8s?= Subject: control message for bug #41658 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Spam-Score: -2.3 (--) X-Debbugs-Envelope-To: control X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -3.3 (---) severity 41658 important quit From debbugs-submit-bounces@debbugs.gnu.org Mon Jun 08 01:53:13 2020 Received: (at 41658) by debbugs.gnu.org; 8 Jun 2020 05:53:13 +0000 Received: from localhost ([127.0.0.1]:55718 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jiAiZ-0003aD-QC for submit@debbugs.gnu.org; Mon, 08 Jun 2020 01:53:13 -0400 Received: from mail-io1-f68.google.com ([209.85.166.68]:36321) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jiAiT-0003Zd-P6 for 41658@debbugs.gnu.org; Mon, 08 Jun 2020 01:53:06 -0400 Received: by mail-io1-f68.google.com with SMTP id r77so4297185ior.3 for <41658@debbugs.gnu.org>; Sun, 07 Jun 2020 22:53:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cune-org.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version; bh=3GVCNg+XuGEy84hQI6TztS6EcxTNZ3n9GFaC5+vc5Ts=; b=zL13OUxMO9OEAWfKS/Od8GHzKyL7bb/6Z1CvewahPHD3cDrj6wv7QKuY8vCQyeunrL f861jnswSgYXgGnEPA8odJk4VWpe3+mHJAGmplcIdmPiF6tCZlDcvEIuwH+8UtrPAmaa g4a12BMg57zs8nuq1NuZ8uUn2PptzQMKjZjne0tQ/yZz91jNI/v+qYgu7Fuh2ZoTvlUD ngFq+MKOLtHK8HXJty1xd8hN178nNFoc6JBTOAsjBVOWIPIEieE2xP30jBWZYJHDoy7z UbFcX1NoL5zKIRCgAT+BogdlKZgmmjMB3FLnmNZ8tNCcC4uxDAOgmM/GF+WbXjHnOBV0 xzvQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version; bh=3GVCNg+XuGEy84hQI6TztS6EcxTNZ3n9GFaC5+vc5Ts=; b=M9zT9KQhGxL/ZfgbKsiHPywHPWB1kDpQ3wluw1DiBf6O5lSEnk8tdH3ml0txFIBd/t drc0AftxqT/IXkhXLwL6PenU+GnkRV2GEV4M72D+PQMgWdoB05CE7q/OM06s47VO0qXT dJMlPnGGNNUcmxOh/Q9OQF3XbD+dvCfZnnI0mhekNZ3w5NSM8QkEhnD1cJxqCvPW+gs4 uA4gBc9ly+Kai00glGkR1TTocN0C9Q8zEYJhmczjJz3VKRchXaxlNTuGHKqj+ZiyXOMC er3c/FbYFmKBSc3Pq8ZltAM7ydsfgjSB3KLvlmyOmI0IsZUyH4iOpUqFDzJGqj08d4Sk vsZg== X-Gm-Message-State: AOAM532q3Yyve5kWYkIh5+XNAfpO1qJLSb7oSbRIGIAbVIZbL5X/EnGI nKYZ2GD2TqqBMr51+StdaBLK8g== X-Google-Smtp-Source: ABdhPJzicVpzqS0jTuLL0QiJklPeXRKAvq69TmHdbGEqLkB8DUSDH1wVPtR5+Zk4S66gfV9Ud44tPw== X-Received: by 2002:a02:7707:: with SMTP id g7mr20401656jac.141.1591595575702; Sun, 07 Jun 2020 22:52:55 -0700 (PDT) Received: from GuixPotato ([208.89.170.24]) by smtp.gmail.com with ESMTPSA id t5sm5573659iov.53.2020.06.07.22.52.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 07 Jun 2020 22:52:54 -0700 (PDT) From: Caleb Ristvedt To: Ludovic =?utf-8?Q?Court=C3=A8s?= Subject: Re: [bug#41658] [PATCH] fixes / improvements for (guix store database) References: <87ftbernat.fsf@cune.org> <87a71i943g.fsf@gnu.org> Date: Mon, 08 Jun 2020 00:52:50 -0500 In-Reply-To: <87a71i943g.fsf@gnu.org> ("Ludovic \=\?utf-8\?Q\?Court\=C3\=A8s\=22'\?\= \=\?utf-8\?Q\?s\?\= message of "Thu, 04 Jun 2020 18:40:35 +0200") Message-ID: <87o8pu5cjx.fsf@cune.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: multipart/signed; boundary="==-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" X-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 41658 Cc: Danny Milosavljevic , 41658@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) --==-=-= Content-Type: multipart/mixed; boundary="=-=-=" --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Ludovic Court=C3=A8s writes: > Hi, > > Thanks for the thorough investigation and for the patches! > > Caleb Ristvedt skribis: > >> From cce653c590be1506e15044e445aa9805370ac759 Mon Sep 17 00:00:00 2001 >> From: Caleb Ristvedt >> 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=3DUTF-8 >> Content-Transfer-Encoding: 8bit >> >> guile-sqlite3 provides statement caching, making it unnecessary for sqli= te 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 "act= ive" >> statement. We work around this by wrapping sqlite-finalize with our own >> version that ensures sqlite-reset is called, which does The Right Thing= =E2=84=A2. >> >> * 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. Reported as https://notabug.org/guile-sqlite3/guile-sqlite3/issues/12, with corresponding pull request https://notabug.org/guile-sqlite3/guile-sqlite3/pulls/13. > We can have this patch locally in the meantime, unless it would break > once the new guile-sqlite3 is out. WDYT? I've attached an updated patch series that both includes the guile-sqlite3 fix as a patch to the guile-sqlite3 package and adopts the workaround for situations where older guile-sqlite3's must be used (for example, when building guix from scratch on foreign distros that haven't incorporated the fix yet). The only changes are the addition of the now-second patch and fixing up some spacing in the comment in the first patch. >> From 7d34c27c33aed3e8a49b9796a62a8c19d352e653 Mon Sep 17 00:00:00 2001 >> From: Caleb Ristvedt >> 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 w= hether >> to update or insert and when it actually performs that operation. Putti= ng the >> check and the update/insert operation in the same transaction ensures th= at the >> update/insert will only succeed if no other write has occurred in the mi= ddle. >> >> * 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=E2=80=99s a bit beyond my understanding, but I think you can also pu= sh this > one. :-) Basically, it's like combining the body of two separate compare-and-swap loops into a single compare-and-swap loop. This ensures that the view is consistent (since if it isn't, the "compare" will fail and we'll retry). It addresses a problem that doesn't exist in practice yet, since update-or-insert is only called from within a call-with-transaction currently. But if someone ever wanted to call it from outside of a call-with-transaction, this would ensure that it still worked correctly. > Make sure =E2=80=9Cmake check TESTS=3Dtests/store-database.scm=E2=80=9D i= s still happy. Works on my system. Related question: does berlin export /var/guix over NFS as per http://hpc.guix.info/blog/2017/11/installing-guix-on-a-cluster? If so, that could interact poorly with our use of WAL mode: "All processes using a database must be on the same host computer; WAL does not work over a network filesystem." - https://sqlite.org/wal.html. =2D reepca --=-=-= Content-Type: text/x-patch; charset=utf-8 Content-Disposition: attachment; filename=0001-database-work-around-guile-sqlite3-bug-preventing-st.patch Content-Transfer-Encoding: quoted-printable From=20614213c80a7ea15f7aab9502e6c33206ac089d05 Mon Sep 17 00:00:00 2001 From: Caleb Ristvedt Date: Mon, 1 Jun 2020 18:50:07 -0500 Subject: [PATCH 1/5] database: work around guile-sqlite3 bug preventing statement reset MIME-Version: 1.0 Content-Type: text/plain; charset=3DUTF-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 (see https://notabug.org/guile-sqlite3/guile-sqlite3/issues/12). = We work around this by wrapping sqlite-finalize with our own version that ensu= res sqlite-reset is called, which does The Right Thing=E2=84=A2. * guix/store/database.scm (sqlite-finalize): new procedure that shadows the sqlite-finalize from (sqlite3). =2D-- guix/store/database.scm | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/guix/store/database.scm b/guix/store/database.scm index ef52036ede..15f5791a08 100644 =2D-- a/guix/store/database.scm +++ b/guix/store/database.scm @@ -130,6 +130,38 @@ transaction after it finishes." If FILE doesn't exist, create it and initialize it as a new database." (call-with-database file (lambda (db) exp ...))) =20 +(define (sqlite-finalize stmt) + ;; As of guile-sqlite3 0.1.0, cached statements aren't reset when + ;; sqlite-finalize is invoked on them (see + ;; https://notabug.org/guile-sqlite3/guile-sqlite3/issues/12). 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 t= he + ;; last active statement finishes. A statement finishes when its last c= ursor + ;; 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 aro= und + ;; until sqlite-reset is called when the cached statement is next + ;; used. Because the transaction is committed automatically only when t= he + ;; *last active statement* finishes, the implicitly-started transaction = may + ;; later be upgraded to a write transaction (!) and this non-reset state= ment + ;; 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 th= an + ;; it should be. + ;; + ;; (see https://www.sqlite.org/lang_transaction.html) + ;; To work around this, we wrap sqlite-finalize so that sqlite-reset is + ;; always called. This will continue working even when the behavior is = fixed + ;; in guile-sqlite3, since resetting twice doesn't cause any problems. = We + ;; can remove this once the fixed guile-sqlite3 is widespread. + (sqlite-reset stmt) + ((@ (sqlite3) sqlite-finalize) stmt)) + (define (last-insert-row-id db) ;; XXX: (sqlite3) currently lacks bindings for 'sqlite3_last_insert_rowi= d'. ;; Work around that. =2D-=20 2.26.2 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0002-gnu-guile-sqlite3-add-patch-to-fix-sqlite-finalize-b.patch Content-Transfer-Encoding: quoted-printable From=20e3cf7be4491f465d3041933596d3caad1ea64e83 Mon Sep 17 00:00:00 2001 From: Caleb Ristvedt Date: Sun, 7 Jun 2020 22:30:41 -0500 Subject: [PATCH 2/5] gnu: guile-sqlite3: add patch to fix sqlite-finalize b= ug Adds patch that fixes https://notabug.org/guile-sqlite3/guile-sqlite3/issues/12. This can be discarded once the patch is integrated into the next guile-sqlite3 release. Note that the patch is identical to the pull request at https://notabug.org/guile-sqlite3/guile-sqlite3/pulls/13. * gnu/packages/patches/guile-sqlite3-reset-on-sqlite-finalize.patch: new patch. * gnu/packages/guile.scm (guile-sqlite3): use it. * gnu/local.mk (dist_patch_DATA): add it. =2D-- gnu/local.mk | 1 + gnu/packages/guile.scm | 3 +- ...ile-sqlite3-reset-on-sqlite-finalize.patch | 74 +++++++++++++++++++ 3 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 gnu/packages/patches/guile-sqlite3-reset-on-sqlite-fina= lize.patch diff --git a/gnu/local.mk b/gnu/local.mk index ae8a2275f7..d382205a03 100644 =2D-- a/gnu/local.mk +++ b/gnu/local.mk @@ -1059,6 +1059,7 @@ dist_patch_DATA =3D \ %D%/packages/patches/guile-rsvg-pkgconfig.patch \ %D%/packages/patches/guile-emacs-fix-configure.patch \ %D%/packages/patches/guile-sqlite3-fix-cross-compilation.patch \ + %D%/packages/patches/guile-sqlite3-reset-on-sqlite-finalize.patch \ %D%/packages/patches/gtk2-respect-GUIX_GTK2_PATH.patch \ %D%/packages/patches/gtk2-respect-GUIX_GTK2_IM_MODULE_FILE.patch \ %D%/packages/patches/gtk2-theme-paths.patch \ diff --git a/gnu/packages/guile.scm b/gnu/packages/guile.scm index c2dc7f6d5f..63cd91a1cb 100644 =2D-- a/gnu/packages/guile.scm +++ b/gnu/packages/guile.scm @@ -645,7 +645,8 @@ Guile's foreign function interface.") "1nv8j7wk6b5n4p22szyi8lv8fs31rrzxhzz16gyj8r38c1fyp9qp")) (file-name (string-append name "-" version "-checkout")) (patches =2D (search-patches "guile-sqlite3-fix-cross-compilation.patc= h")) + (search-patches "guile-sqlite3-fix-cross-compilation.patch" + "guile-sqlite3-reset-on-sqlite-finalize.pat= ch")) (modules '((guix build utils))) (snippet '(begin diff --git a/gnu/packages/patches/guile-sqlite3-reset-on-sqlite-finalize.pa= tch b/gnu/packages/patches/guile-sqlite3-reset-on-sqlite-finalize.patch new file mode 100644 index 0000000000..b6bf5325ad =2D-- /dev/null +++ b/gnu/packages/patches/guile-sqlite3-reset-on-sqlite-finalize.patch @@ -0,0 +1,74 @@ +From c59db66f9ac754b40463c6788ab9bad4f045cc92 Mon Sep 17 00:00:00 2001 +From: Caleb Ristvedt +Date: Sun, 7 Jun 2020 18:27:44 -0500 +Subject: [PATCH] Reset statement when sqlite-finalize is called on cached + statement + +Automatically-started transactions only end when a statement finishes, whi= ch is +normally either when sqlite-reset or sqlite-finalize is called for that +statement. Consequently, transactions automatically started by cached +statements won't end until the statement is next reused by sqlite-prepare = or +sqlite-reset is called on it. This changes sqlite-finalize so that it pre= serves +the statement-finishing (and thus transaction-finishing) behavior of +sqlite_finalize. +--- + sqlite3.scm.in | 43 ++++++++++++++++++++++++++++++++++--------- + 1 file changed, 34 insertions(+), 9 deletions(-) + +diff --git a/sqlite3.scm.in b/sqlite3.scm.in +index 77b5032..19241dc 100644 +--- a/sqlite3.scm.in ++++ b/sqlite3.scm.in +@@ -274,15 +274,40 @@ statements, into DB. The result is unspecified." + (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 co= de. +- (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)))))) ++ ;; Note: When STMT is cached, this merely resets. This ensures cac= hing ++ ;; actually works while still separating concerns: users can turn c= aching ++ ;; on and off without having to change the rest of their code. ++ (if (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)) ++ ;; It's necessary to reset cached statements due to the followi= ng: ++ ;; ++ ;; "An implicit transaction (a transaction that is started ++ ;; automatically, not a transaction started by BEGIN) is commit= ted ++ ;; automatically when the last active statement finishes. A st= atement ++ ;; finishes when its last cursor closes, which is guaranteed to= happen ++ ;; when the prepared statement is reset or finalized. Some sta= tements ++ ;; might "finish" for the purpose of transaction control prior = to ++ ;; being reset or finalized, but there is no guarantee of this." ++ ;; ++ ;; (see https://www.sqlite.org/lang_transaction.html) ++ ;; ++ ;; Thus, it's possible for an implicitly-started transaction to= hang ++ ;; around until sqlite-reset is called when the cached statemen= t is ++ ;; next used. Because the transaction is committed automatical= ly only ++ ;; when the *last active statement* finishes, the implicitly-st= arted ++ ;; transaction may later be upgraded to a write transaction (!)= and ++ ;; this non-reset statement will still be keeping the transacti= on from ++ ;; committing until it is next used or the database connection = is ++ ;; closed. This has the potential to make (exclusive) write ac= cess to ++ ;; the database necessary for much longer than it should be. ++ ;; ++ ;; So it's necessary to preserve the statement-finishing behavi= or of ++ ;; sqlite_finalize here, which we do by calling sqlite-reset. ++ (sqlite-reset stmt))))) +=20 + (define *stmt-map* (make-weak-key-hash-table)) + (define (stmt->db stmt) +--=20 +2.26.2 + =2D-=20 2.26.2 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0003-database-rewrite-query-procedures-in-terms-of-with-s.patch Content-Transfer-Encoding: quoted-printable From=201d0b2a6742935b812fe56ae97e34f4a35fed3348 Mon Sep 17 00:00:00 2001 From: Caleb Ristvedt Date: Mon, 1 Jun 2020 19:21:43 -0500 Subject: [PATCH 3/5] database: rewrite query procedures in terms of with-statement. Most of our queries would fail to finalize their statements properly if sql= ite returned an error during their execution. This resolves that, and also mak= es 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. =2D-- .dir-locals.el | 1 + guix/store/database.scm | 53 ++++++++++++++++++++++------------------- 2 files changed, 30 insertions(+), 24 deletions(-) diff --git a/.dir-locals.el b/.dir-locals.el index dc8bc0e437..77c12f9411 100644 =2D-- a/.dir-locals.el +++ b/.dir-locals.el @@ -89,6 +89,7 @@ =20 (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)) =20 (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 15f5791a08..2a943a7eb0 100644 =2D-- a/guix/store/database.scm +++ b/guix/store/database.scm @@ -162,14 +162,26 @@ If FILE doesn't exist, create it and initialize it as= a new database." (sqlite-reset stmt) ((@ (sqlite3) sqlite-finalize) stmt)) =20 +(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_rowi= d'. ;; Work around that. =2D (let* ((stmt (sqlite-prepare db "SELECT last_insert_rowid();" =2D #:cache? #t)) =2D (result (sqlite-fold cons '() stmt))) =2D (sqlite-finalize stmt) =2D (match result + (with-statement db "SELECT last_insert_rowid();" stmt + (match (sqlite-fold cons '() stmt) ((#(id)) id) (_ #f)))) =20 @@ -179,13 +191,11 @@ If FILE doesn't exist, create it and initialize it as= a new database." (define* (path-id db path) "If PATH exists in the 'ValidPaths' table, return its numerical identifier. Otherwise, return #f." =2D (let ((stmt (sqlite-prepare db path-id-sql #:cache? #t))) + (with-statement db path-id-sql stmt (sqlite-bind-arguments stmt #:path path) =2D (let ((result (sqlite-fold cons '() stmt))) =2D (sqlite-finalize stmt) =2D (match result =2D ((#(id) . _) id) =2D (_ #f))))) + (match (sqlite-fold cons '() stmt) + ((#(id) . _) id) + (_ #f)))) =20 (define update-sql "UPDATE ValidPaths SET hash =3D :hash, registrationTime =3D :time, deriv= er =3D @@ -202,20 +212,17 @@ and re-inserting instead of updating, which causes pr= oblems with foreign keys, of course. Returns the row id of the row that was modified or inserted." (let ((id (path-id db path))) (if id =2D (let ((stmt (sqlite-prepare db update-sql #:cache? #t))) + (with-statement db update-sql stmt (sqlite-bind-arguments stmt #:id id #:deriver deriver #:hash hash #:size nar-size #:time time) =2D (sqlite-fold cons '() stmt) =2D (sqlite-finalize stmt) =2D (last-insert-row-id db)) =2D (let ((stmt (sqlite-prepare db insert-sql #:cache? #t))) + (sqlite-fold cons '() stmt)) + (with-statement db insert-sql stmt (sqlite-bind-arguments stmt #:path path #:deriver deriver #:hash hash #:size nar-size #:time time) =2D (sqlite-fold cons '() stmt) ;execute it =2D (sqlite-finalize stmt) =2D (last-insert-row-id db))))) + (sqlite-fold cons '() stmt))) + (last-insert-row-id db))) =20 (define add-reference-sql "INSERT OR REPLACE INTO Refs (referrer, reference) VALUES (:referrer, :r= eference);") @@ -223,15 +230,13 @@ of course. Returns the row id of the row that was mod= ified or inserted." (define (add-references db referrer references) "REFERRER is the id of the referring store item, REFERENCES is a list ids of items referred to." =2D (let ((stmt (sqlite-prepare db add-reference-sql #:cache? #t))) + (with-statement db add-reference-sql stmt (for-each (lambda (reference) (sqlite-reset stmt) (sqlite-bind-arguments stmt #:referrer referrer #:reference reference) =2D (sqlite-fold cons '() stmt) ;execute it =2D (last-insert-row-id db)) =2D references) =2D (sqlite-finalize stmt))) + (sqlite-fold cons '() stmt)) + references))) =20 (define* (sqlite-register db #:key path (references '()) deriver hash nar-size time) =2D-=20 2.26.2 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0004-database-ensure-update-or-insert-is-run-within-a-tra.patch Content-Transfer-Encoding: quoted-printable From=2000b220d1c380004a9bb128b73bb8027f2ff156f0 Mon Sep 17 00:00:00 2001 From: Caleb Ristvedt Date: Mon, 1 Jun 2020 21:43:14 -0500 Subject: [PATCH 4/5] database: ensure update-or-insert is run within a transaction update-or-insert can break if an insert occurs between when it decides whet= her 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 middl= e. * 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. =2D-- .dir-locals.el | 1 + guix/store/database.scm | 68 +++++++++++++++++++++++++++++++++-------- 2 files changed, 56 insertions(+), 13 deletions(-) diff --git a/.dir-locals.el b/.dir-locals.el index 77c12f9411..d9c81b2a48 100644 =2D-- a/.dir-locals.el +++ b/.dir-locals.el @@ -90,6 +90,7 @@ (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-savepoint 'scheme-indent-function 1)) =20 (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 2a943a7eb0..14aaeef176 100644 =2D-- a/guix/store/database.scm +++ b/guix/store/database.scm @@ -120,6 +120,26 @@ transaction after it finishes." (begin (sqlite-exec db "rollback;") (throw 'sqlite-error who error description)))))) +(define* (call-with-savepoint db proc + #:optional (savepoint-name "SomeSavepoint")) + "Call PROC after creating a savepoint named SAVEPOINT-NAME. If PROC exi= ts +abnormally, rollback to that savepoint. In all cases, remove the savepoint +prior to returning." + (define (exec sql) + (with-statement db sql stmt + (sqlite-fold cons '() stmt))) + + (dynamic-wind + (lambda () + (exec (string-append "SAVEPOINT " savepoint-name ";"))) + (lambda () + (catch #t + proc + (lambda args + (exec (string-append "ROLLBACK TO " savepoint-name ";")) + (apply throw args)))) + (lambda () + (exec (string-append "RELEASE " savepoint-name ";"))))) =20 (define %default-database-file ;; Default location of the store database. @@ -210,19 +230,41 @@ VALUES (:path, :hash, :time, :deriver, :size)") doesn't exactly have... they've got something close, but it involves delet= ing and re-inserting instead of updating, which causes problems with foreign k= eys, of course. Returns the row id of the row that was modified or inserted." =2D (let ((id (path-id db path))) =2D (if id =2D (with-statement db update-sql stmt =2D (sqlite-bind-arguments stmt #:id id =2D #:deriver deriver =2D #:hash hash #:size nar-size #:time time) =2D (sqlite-fold cons '() stmt)) =2D (with-statement db insert-sql stmt =2D (sqlite-bind-arguments stmt =2D #:path path #:deriver deriver =2D #:hash hash #:size nar-size #:time time) =2D (sqlite-fold cons '() stmt))) =2D (last-insert-row-id db))) + + ;; It's important that querying the path-id and the insert/update operat= ion + ;; take place in the same transaction, as otherwise some other + ;; process/thread/fiber could register the same path between when we che= ck + ;; whether it's already registered and when we register it, resulting in + ;; duplicate paths (which, due to a 'unique' constraint, would cause an + ;; exception to be thrown). With the default journaling mode this will + ;; prevent writes from occurring during that sensitive time, but with WAL + ;; mode it will instead arrange to return SQLITE_BUSY when a write occurs + ;; between the start of a read transaction and its upgrading to a write + ;; transaction (see https://sqlite.org/rescode.html#busy_snapshot). + ;; Experimentally, it seems this SQLITE_BUSY will ignore a busy_timeout = and + ;; immediately return (makes sense, since waiting won't change anything). + + ;; Note that when that kind of SQLITE_BUSY error is returned, it will ke= ep + ;; being returned every time we try to upgrade the same outermost + ;; transaction to a write transaction. So when retrying, we have to res= tart + ;; the *outermost* write transaction. We can't inherently tell whether + ;; we're the outermost write transaction, so we leave the retry-handling= to + ;; the caller. + (call-with-savepoint db + (lambda () + (let ((id (path-id db path))) + (if id + (with-statement db update-sql stmt + (sqlite-bind-arguments stmt #:id id + #:deriver deriver + #:hash hash #:size nar-size #:time ti= me) + (sqlite-fold cons '() stmt)) + (with-statement db insert-sql stmt + (sqlite-bind-arguments stmt + #:path path #:deriver deriver + #:hash hash #:size nar-size #:time ti= me) + (sqlite-fold cons '() stmt))) + (last-insert-row-id db))))) =20 (define add-reference-sql "INSERT OR REPLACE INTO Refs (referrer, reference) VALUES (:referrer, :r= eference);") =2D-=20 2.26.2 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0005-database-separate-transaction-handling-and-retry-han.patch Content-Transfer-Encoding: quoted-printable From=20af87a556dab110ed7a6ef2fa1584778cc60be682 Mon Sep 17 00:00:00 2001 From: Caleb Ristvedt Date: Mon, 1 Jun 2020 22:15:21 -0500 Subject: [PATCH 5/5] database: separate transaction-handling and retry-handling. Previously call-with-transaction would both retry when SQLITE_BUSY errors w= ere 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. =2D-- .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 d9c81b2a48..b88ec7a795 100644 =2D-- 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)) =20 (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 14aaeef176..4921e9f0e2 100644 =2D-- 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) =20 =2D(define (call-with-transaction db proc) =2D "Start a transaction with DB (make as many attempts as necessary) and = run =2DPROC. If PROC exits abnormally, abort the transaction, otherwise commit= the =2Dtransaction 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 (=3D 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, ab= ort +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 mult= iple +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 fig= ure + ;; that out immediately rather than because some SQLITE_BUSY exception g= ets + ;; 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 () =2D ;; We use begin immediate here so that if we need to retry, we =2D ;; figure that out immediately rather than because some SQLITE_BUSY =2D ;; exception gets thrown partway through PROC - in which case the =2D ;; part already executed (which may contain side-effects!) would be =2D ;; executed again for every retry. =2D (sqlite-exec db "begin immediate;") =2D (let ((result (proc))) =2D (sqlite-exec db "commit;") =2D result)) =2D (lambda (key who error description) =2D (if (=3D error SQLITE_BUSY) =2D (call-with-transaction db proc) =2D (begin =2D (sqlite-exec db "rollback;") =2D (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 si= gnal + ;; 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 exi= ts @@ -141,6 +158,18 @@ prior to returning." (lambda () (exec (string-append "RELEASE " savepoint-name ";"))))) =20 +(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")) @@ -433,7 +462,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 =2D (call-with-transaction db + (call-with-retrying-transaction db (lambda () (let* ((prefix (format #f "registering ~a items" (length ite= ms))) (progress (progress-reporter/bar (length items) =2D-=20 2.26.2 --=-=-=-- --==-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEEdNapMPRLm4SepVYGwWaqSV9/GJwFAl7d0jIACgkQwWaqSV9/ GJzIwQgAiQz4aUAmaoawzhYslWzEytu5mokJi5mv4+ZRFQlI4JD4oK1hw/PjWC3X vj05QIAfJywvw4x1mOB9p58Du+8/KV+CYaNJGoh4jyu6gxudPzdhIy7Nwxjor7v2 Lpjm6j7AHlOfWgls8oGFTzcsU70F33yD8BKEKN8Uwq5GDhWP7o1xDKOuKHsLoUg1 EBzFD90Nw3RUBtQIbPC8ep+YNIgcRuW1NXPu1wH1p8SSSOzM1WiltO5ul6mvDkc4 TXRFYq9hrvHrMb1FVMaxfLxtcdbzDl0nG0eEbKjpZIbBjEqcLia4ZZtoBBaunCTf 0qez5U5yBqivwczlFK2zqPVmBYpsuA== =QgPR -----END PGP SIGNATURE----- --==-=-=-- From debbugs-submit-bounces@debbugs.gnu.org Tue Jun 09 04:42:23 2020 Received: (at 41658) by debbugs.gnu.org; 9 Jun 2020 08:42:23 +0000 Received: from localhost ([127.0.0.1]:58890 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jiZpr-0005sv-Gx for submit@debbugs.gnu.org; Tue, 09 Jun 2020 04:42:23 -0400 Received: from eggs.gnu.org ([209.51.188.92]:40160) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jiZpm-0005sf-0e for 41658@debbugs.gnu.org; Tue, 09 Jun 2020 04:42:17 -0400 Received: from fencepost.gnu.org ([2001:470:142:3::e]:37419) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jiZpg-0002Rx-6w; Tue, 09 Jun 2020 04:42:08 -0400 Received: from [2a01:e0a:1d:7270:af76:b9b:ca24:c465] (port=59270 helo=ribbon) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1jiZpZ-0004tv-Cn; Tue, 09 Jun 2020 04:42:05 -0400 From: =?utf-8?Q?Ludovic_Court=C3=A8s?= To: Caleb Ristvedt Subject: Re: [bug#41658] [PATCH] fixes / improvements for (guix store database) References: <87ftbernat.fsf@cune.org> <87a71i943g.fsf@gnu.org> <87o8pu5cjx.fsf@cune.org> Date: Tue, 09 Jun 2020 10:42:00 +0200 In-Reply-To: <87o8pu5cjx.fsf@cune.org> (Caleb Ristvedt's message of "Mon, 08 Jun 2020 00:52:50 -0500") Message-ID: <87h7vkmy07.fsf@gnu.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Score: -2.3 (--) X-Debbugs-Envelope-To: 41658 Cc: Danny Milosavljevic , 41658@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -3.3 (---) Hi, Caleb Ristvedt skribis: [...] >> 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. > > Reported as https://notabug.org/guile-sqlite3/guile-sqlite3/issues/12, > with corresponding pull request > https://notabug.org/guile-sqlite3/guile-sqlite3/pulls/13. Awesome, thank you. >> We can have this patch locally in the meantime, unless it would break >> once the new guile-sqlite3 is out. WDYT? > > I've attached an updated patch series that both includes the > guile-sqlite3 fix as a patch to the guile-sqlite3 package and adopts the > workaround for situations where older guile-sqlite3's must be used (for > example, when building guix from scratch on foreign distros that haven't > incorporated the fix yet). The only changes are the addition of the > now-second patch and fixing up some spacing in the comment in the first > patch. OK. >>> * 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=E2=80=99s a bit beyond my understanding, but I think you can also p= ush this >> one. :-) > > Basically, it's like combining the body of two separate compare-and-swap > loops into a single compare-and-swap loop. This ensures that the view is > consistent (since if it isn't, the "compare" will fail and we'll > retry). It addresses a problem that doesn't exist in practice yet, since > update-or-insert is only called from within a call-with-transaction > currently. But if someone ever wanted to call it from outside of a > call-with-transaction, this would ensure that it still worked correctly. Makes sense, thanks for explaining. > Related question: does berlin export /var/guix over NFS as per > http://hpc.guix.info/blog/2017/11/installing-guix-on-a-cluster? If so, > that could interact poorly with our use of WAL mode: No, it doesn=E2=80=99t. (Also, in the setup described above, there=E2=80= =99s only one guix-daemon instance and it accesses the database via the local file system.) > From 614213c80a7ea15f7aab9502e6c33206ac089d05 Mon Sep 17 00:00:00 2001 > From: Caleb Ristvedt > Date: Mon, 1 Jun 2020 18:50:07 -0500 > Subject: [PATCH 1/5] database: work around guile-sqlite3 bug preventing > statement reset > MIME-Version: 1.0 > Content-Type: text/plain; charset=3DUTF-8 > Content-Transfer-Encoding: 8bit > > guile-sqlite3 provides statement caching, making it unnecessary for sqlit= e 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 "acti= ve" > statement (see https://notabug.org/guile-sqlite3/guile-sqlite3/issues/12)= . We > work around this by wrapping sqlite-finalize with our own version that en= sures > sqlite-reset is called, which does The Right Thing=E2=84=A2. > > * guix/store/database.scm (sqlite-finalize): new procedure that shadows t= he > sqlite-finalize from (sqlite3). [...] > +(define (sqlite-finalize stmt) > + ;; As of guile-sqlite3 0.1.0, cached statements aren't reset when > + ;; sqlite-finalize is invoked on them (see > + ;; https://notabug.org/guile-sqlite3/guile-sqlite3/issues/12). This c= an > + ;; cause problems with automatically-started transactions: I think it=E2=80=99s enough to link to the upstream issue, which has the pr= oblem well documented. > From e3cf7be4491f465d3041933596d3caad1ea64e83 Mon Sep 17 00:00:00 2001 > From: Caleb Ristvedt > Date: Sun, 7 Jun 2020 22:30:41 -0500 > Subject: [PATCH 2/5] gnu: guile-sqlite3: add patch to fix sqlite-finalize= bug > > Adds patch that fixes > https://notabug.org/guile-sqlite3/guile-sqlite3/issues/12. This can be > discarded once the patch is integrated into the next guile-sqlite3 releas= e. > Note that the patch is identical to the pull request at > https://notabug.org/guile-sqlite3/guile-sqlite3/pulls/13. > > * gnu/packages/patches/guile-sqlite3-reset-on-sqlite-finalize.patch: new > patch. > * gnu/packages/guile.scm (guile-sqlite3): use it. > * gnu/local.mk (dist_patch_DATA): add it. I=E2=80=99d skip it: we have a workaround and the release may be out soon. Danny, thoughts on getting a new release out? The rest is still fine with me, thank you! Ludo=E2=80=99. From debbugs-submit-bounces@debbugs.gnu.org Tue Jun 23 17:51:10 2020 Received: (at control) by debbugs.gnu.org; 23 Jun 2020 21:51:10 +0000 Received: from localhost ([127.0.0.1]:37308 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jnqov-0005IH-Rm for submit@debbugs.gnu.org; Tue, 23 Jun 2020 17:51:10 -0400 Received: from eggs.gnu.org ([209.51.188.92]:46126) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jnqou-0005I2-Iz for control@debbugs.gnu.org; Tue, 23 Jun 2020 17:51:08 -0400 Received: from fencepost.gnu.org ([2001:470:142:3::e]:40781) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jnqop-0004iL-9h for control@debbugs.gnu.org; Tue, 23 Jun 2020 17:51:03 -0400 Received: from [2a01:e0a:1d:7270:af76:b9b:ca24:c465] (port=58518 helo=ribbon) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1jnqoo-0000Qd-QP for control@debbugs.gnu.org; Tue, 23 Jun 2020 17:51:03 -0400 Date: Tue, 23 Jun 2020 23:51:01 +0200 Message-Id: <87imfhxxh6.fsf@gnu.org> To: control@debbugs.gnu.org From: =?utf-8?Q?Ludovic_Court=C3=A8s?= Subject: control message for bug #41658 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Spam-Score: -2.3 (--) X-Debbugs-Envelope-To: control X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -3.3 (---) tags 41658 fixed close 41658 quit From unknown Sun Jun 22 17:13:16 2025 Received: (at fakecontrol) by fakecontrolmessage; To: internal_control@debbugs.gnu.org From: Debbugs Internal Request Subject: Internal Control Message-Id: bug archived. Date: Wed, 22 Jul 2020 11:24:05 +0000 User-Agent: Fakemail v42.6.9 # This is a fake control message. # # The action: # bug archived. thanks # This fakemail brought to you by your local debbugs # administrator