[bug#42023] [PATCH] database: register-items: reduce transaction scope.

["Ludovic Courtès" <ludo@gnu.org>]

Hi Caleb,

And apologies for the delay.

I think we’ve drifted from the original patch and that’s become a tricky
7-patch series, which partly explains the delay—not that I’m looking for
an excuse.  ;-)

I decided to go ahead and apply some of these on your behalf.  Comments
below.

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

> From 4c8f0cc50e2a1a33d9ce2f8e58cc426872676a7f Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Wed, 24 Jun 2020 01:00:40 -0500
> Subject: [PATCH 1/6] .dir-locals.el: fix call-with-{retrying-}transaction
>  indenting.
>
> * .dir-locals.el (call-with-transaction, call-with-retrying-transaction):
>   change scheme-indent-function property from 2 to 1.

Applied.

> From 9717568f922e0921e5fdc320cbe6689768d29a29 Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Sat, 8 Aug 2020 10:05:22 -0500
> Subject: [PATCH 2/6] deduplication: pass store directory to replace-with-link.
>
> This causes with-writable-file to take into consideration the actual store
> being used, as passed to 'deduplicate', rather than
> whatever (%store-directory) may return.
>
> * guix/store/deduplication.scm (replace-with-link): new keyword argument
>   'store'.  Pass to with-writable-file.
>   (with-writable-file, call-with-writable-file): new store argument.
>   (deduplicate): pass store to replace-with-link.

Applied.

> From b992a3aaac7e3b30222e0bf1df09093f18e25e6a Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Sat, 8 Aug 2020 11:25:57 -0500
> Subject: [PATCH 3/6] deduplication: retry on ENOENT.
>
> It's possible for the garbage collector to remove the "canonical" link after
> it's been detected as existing by 'deduplicate'.  This would cause an ENOENT
> error when replace-with-link attempts to create the temporary link.  This
> changes it so that it will properly handle that by retrying.

Would that ENOENT cause an error, or just a missed deduplication opportunity?

> * guix/store/deduplication.scm (replace-with-link): renamed to
>   canonicalize-with-link, now also handles the case where the target link
>   doesn't exist yet, and retries on ENOENT.
>   (deduplicate): modified to use canonicalize-with-link.

There’s an issue with this patch.  I gave it a spin (offloading a few
builds) and it got stuck in a infinite loop:

--8<---------------cut here---------------start------------->8---
stat("/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk", 0x7ffe43898cd0) = -1 ENOENT (Dosiero aŭ dosierujo ne ekzistas)
link("/gnu/store/83jy739bn644w3pnwgb5kwjig0kzs92f-libwps-0.4.12/lib/libwps-0.4.so", "/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk") = -1 EEXIST (Dosiero jam ekzistas)
stat("/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk", 0x7ffe43898cd0) = -1 ENOENT (Dosiero aŭ dosierujo ne ekzistas)
link("/gnu/store/83jy739bn644w3pnwgb5kwjig0kzs92f-libwps-0.4.12/lib/libwps-0.4.so", "/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk") = -1 EEXIST (Dosiero jam ekzistas)
stat("/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk", 0x7ffe43898cd0) = -1 ENOENT (Dosiero aŭ dosierujo ne ekzistas)
link("/gnu/store/83jy739bn644w3pnwgb5kwjig0kzs92f-libwps-0.4.12/lib/libwps-0.4.so", "/gnu/store/.links/0k63r0n3681r2gqd00blq4z5xd7cw1knv0x049p99f0pq31brhrk") = -1 EEXIST (Dosiero jam ekzistas)
--8<---------------cut here---------------end--------------->8---

I think we should work on reducing the complexity of that code (e.g.,
there are several layers of system-error handling).

I’ve omitted it and I propose discussing it in a separate issue if we
need to.

> From 6b7d011680c77642f24396be0eb0015c20413d1a Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Sat, 8 Aug 2020 08:31:38 -0500
> Subject: [PATCH 4/6] nar: ensure finalization target is a temp root during
>  registration.
>
> Note that this is currently unnecessary, as finalize-store-file is only used
> from the offload hook, and the daemon process that spawned the offload hook
> will have already registered the derivation outputs as temp roots prior to
> attempting to offload (see haveDerivation() in nix/libstore/build.cc).  But
> it's necessary to ensure that the register-items invocation works properly
> when finalize-store-file is used in other contexts.
>
> * guix/nar.scm (finalize-store-file): make target a temp root during extent of
>   register-items invocation.

[...]

> +          ;; TODO: don't use an RPC for this, add it to *this process's* temp
> +          ;; roots file.
> +          (with-store store
> +            (add-temp-root store target)

I agree that this is the right thing but as you note, it’s currently
unnecessary in the context of ‘guix offload’, and I’d rather avoid
opening more connections to the daemon from ‘guix offload’ and this
increases load, pressure on the store database, etc.

> From 55dd48e88d641bbc17b4d9484d6ee84acfb29766 Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Wed, 8 Jul 2020 11:33:23 -0500
> Subject: [PATCH 5/6] database: document extra registration requirements.
>
> It's necessary that store items be locked and protected from garbage
> collection while they are being registered.  This documents that.
>
> * guix/store/database.scm (register-path, register-items): document GC
>   protection and locking requirements.

Applied.

> From 30afb453ce4eb161bb87645a0e6314e6af82a61f Mon Sep 17 00:00:00 2001
> From: Christopher Baines <mail@cbaines.net>
> Date: Tue, 23 Jun 2020 17:36:49 +0100
> Subject: [PATCH 6/6] database: register-items: reduce transaction scope.
>
> It was made transactional in a4678c6ba18d8dbd79d931f80426eebf61be7ebe, with
> the reasoning to prevent broken intermediate states from being visible. I
> think this means something like an entry being in ValidPaths, but the Refs not
> being inserted.
>
> Using a transaction for this makes sense, but I think using one single
> transaction for the whole register-items call is unnecessary to avoid broken
> states from being visible, and could block other writes to the store database
> while register-items is running. Because the deduplication and resetting
> timestamps happens within the transaction as well, even though these things
> don't involve the database, writes to the database will still be blocked while
> this is happening.
>
> To reduce the potential for register-items to block other writers to the
> database for extended periods, this commit moves the transaction to just wrap
> the call to sqlite-register. This is the one place where writes occur, so that
> should prevent the broken intermediate states issue above. The one difference
> this will make is some of the registered items will be visible to other
> connections while others may be still being added. I think this is OK, as it's
> equivalent to just registering different items.
>
> * guix/store/database.scm (register-items): Reduce transaction scope.

Applied.

Thanks Caleb & Chris!

Ludo’.