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

[Ludovic Courtès]

Hi,

Thanks for the thorough investigation and for the patches!

Caleb Ristvedt <caleb.ristvedt@cune.org> skribis:

> From cce653c590be1506e15044e445aa9805370ac759 Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Mon, 1 Jun 2020 18:50:07 -0500
> Subject: [PATCH 1/4] database: work around guile-sqlite3 bug preventing
>  statement reset
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> guile-sqlite3 provides statement caching, making it unnecessary for sqlite to
> keep re-preparing statements that are frequently used.  Unfortunately it
> doesn't quite emulate the semantics of sqlite_finalize properly, because it
> doesn't cause a commit if the statement being finalized is the last "active"
> statement.  We work around this by wrapping sqlite-finalize with our own
> version that ensures sqlite-reset is called, which does The Right Thing™.
>
> * guix/store/database.scm (sqlite-finalize): new procedure that shadows the
>   sqlite-finalize from (sqlite3).

Nice.  It would be great if you could report it upstream (Danny and/or
myself can then patch it directly in guile-sqlite3 and push out a
release) and refer to the issue from here.

We can have this patch locally in the meantime, unless it would break
once the new guile-sqlite3 is out.  WDYT?

> From ee24ab21122b1c75a7d67d7062550e15e54ab62f Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Mon, 1 Jun 2020 19:21:43 -0500
> Subject: [PATCH 2/4] database: rewrite query procedures in terms of
>  with-statement.
>
> Most of our queries would fail to finalize their statements properly if sqlite
> returned an error during their execution.  This resolves that, and also makes
> them somewhat more concise as a side-effect.
>
> This also makes some small changes to improve certain queries where behavior
> was strange or overly verbose.
>
> * guix/store/database.scm (call-with-statement): new procedure.
>   (with-statement): new macro.
>   (last-insert-row-id, path-id, update-or-insert, add-references): rewrite to
>   use with-statement.
>   (update-or-insert): factor last-insert-row-id out of the end of both
>   branches.
>   (add-references): remove pointless last-insert-row-id call.
>
> * .dir-locals.el (with-statement): add indenting information.

LGTM!

> From 7d34c27c33aed3e8a49b9796a62a8c19d352e653 Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Mon, 1 Jun 2020 21:43:14 -0500
> Subject: [PATCH 3/4] database: ensure update-or-insert is run within a
>  transaction
>
> update-or-insert can break if an insert occurs between when it decides whether
> to update or insert and when it actually performs that operation.  Putting the
> check and the update/insert operation in the same transaction ensures that the
> update/insert will only succeed if no other write has occurred in the middle.
>
> * guix/store/database.scm (call-with-savepoint): new procedure.
>   (update-or-insert): use call-with-savepoint to ensure the read and the
>   insert/update occur within the same transaction.

That’s a bit beyond my understanding, but I think you can also push this
one.  :-)

Make sure “make check TESTS=tests/store-database.scm” is still happy.

Thanks a lot!

Ludo’.