From patchwork Mon Jun 8 05:52:50 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Caleb Ristvedt X-Patchwork-Id: 22603 Return-Path: X-Original-To: patchwork@mira.cbaines.net Delivered-To: patchwork@mira.cbaines.net Received: by mira.cbaines.net (Postfix, from userid 113) id 11C5627BBE1; Mon, 8 Jun 2020 06:54:13 +0100 (BST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on mira.cbaines.net X-Spam-Level: X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, MAILING_LIST_MULTI,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,T_DKIM_INVALID, URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.2 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mira.cbaines.net (Postfix) with ESMTP id 2CDB827BBE3 for ; Mon, 8 Jun 2020 06:54:10 +0100 (BST) Received: from localhost ([::1]:48924 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jiAjZ-0007a8-Ik for patchwork@mira.cbaines.net; Mon, 08 Jun 2020 01:54:09 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:58940) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jiAjS-0007YK-R3 for guix-patches@gnu.org; Mon, 08 Jun 2020 01:54:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:44173) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jiAjS-0000kO-Hz for guix-patches@gnu.org; Mon, 08 Jun 2020 01:54:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jiAjS-0003bX-GM for guix-patches@gnu.org; Mon, 08 Jun 2020 01:54:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#41658] [PATCH] fixes / improvements for (guix store database) Resent-From: Caleb Ristvedt Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Mon, 08 Jun 2020 05:54:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 41658 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Ludovic =?utf-8?q?Court=C3=A8s?= Cc: Danny Milosavljevic , 41658@debbugs.gnu.org Received: via spool by 41658-submit@debbugs.gnu.org id=B41658.159159559313789 (code B ref 41658); Mon, 08 Jun 2020 05:54:02 +0000 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 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 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org Sender: "Guix-patches" X-getmail-retrieved-from-mailbox: Patches Ludovic Courtès 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=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. 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 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. :-) 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 “make check TESTS=tests/store-database.scm” is 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. - reepca From af87a556dab110ed7a6ef2fa1584778cc60be682 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 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 d9c81b2a48..b88ec7a795 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 14aaeef176..4921e9f0e2 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")) @@ -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 - (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