[bug#41658] [PATCH] fixes / improvements for (guix store database)

[Caleb Ristvedt]

Ludovic Courtès <ludo@gnu.org> writes:

> Hi,
>
> Thanks for the thorough investigation and for the patches!
>
> Caleb Ristvedt <caleb.ristvedt@cune.org> skribis:
>
>> From cce653c590be1506e15044e445aa9805370ac759 Mon Sep 17 00:00:00 2001
>> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
>> Date: Mon, 1 Jun 2020 18:50:07 -0500
>> Subject: [PATCH 1/4] database: work around guile-sqlite3 bug preventing
>>  statement reset
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
>>
>> guile-sqlite3 provides statement caching, making it unnecessary for sqlite to
>> keep re-preparing statements that are frequently used.  Unfortunately it
>> doesn't quite emulate the semantics of sqlite_finalize properly, because it
>> doesn't cause a commit if the statement being finalized is the last "active"
>> statement.  We work around this by wrapping sqlite-finalize with our own
>> version that ensures sqlite-reset is called, which does The Right Thing™.
>>
>> * guix/store/database.scm (sqlite-finalize): new procedure that shadows the
>>   sqlite-finalize from (sqlite3).
>
> Nice.  It would be great if you could report it upstream (Danny and/or
> myself can then patch it directly in guile-sqlite3 and push out a
> release) and refer to the issue from here.

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 <caleb.ristvedt@cune.org>
>> Date: Mon, 1 Jun 2020 21:43:14 -0500
>> Subject: [PATCH 3/4] database: ensure update-or-insert is run within a
>>  transaction
>>
>> update-or-insert can break if an insert occurs between when it decides 
>> whether
>> to update or insert and when it actually performs that operation.  Putting 
>> the
>> check and the update/insert operation in the same transaction ensures that 
>> the
>> update/insert will only succeed if no other write has occurred in the middle.
>>
>> * guix/store/database.scm (call-with-savepoint): new procedure.
>>   (update-or-insert): use call-with-savepoint to ensure the read and the
>>   insert/update occur within the same transaction.
>
> That’s a bit beyond my understanding, but I think you can also push this
> one.  :-)

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

0001-database-work-around-guile-sqlite3-bug-preventing-st.patch
Description: Text Data

0002-gnu-guile-sqlite3-add-patch-to-fix-sqlite-finalize-b.patch
Description: Text Data

0003-database-rewrite-query-procedures-in-terms-of-with-s.patch
Description: Text Data

0004-database-ensure-update-or-insert-is-run-within-a-tra.patch
Description: Text Data

0005-database-separate-transaction-handling-and-retry-han.patch
Description: Text Data

signature.asc
Description: PGP signature