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

[Ludovic Courtès]

Hi,

(+Cc: reepca)

Christopher Baines <mail@cbaines.net> skribis:

> 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.


[...]

> +        (call-with-retrying-transaction db
> +            (lambda ()
             ^^
Too much indentation (maybe we miss a rule in .dir-locals.el?).

> +              (sqlite-register db #:path to-register
> +                               #:references (store-info-references item)
> +                               #:deriver (store-info-deriver item)
> +                               #:hash (string-append
> +                                       "sha256:"
> +                                       (bytevector->base16-string hash))
> +                               #:nar-size nar-size
> +                               #:time registration-time)))

I think it would be good to have a 2-line summary of the rationale right
above ‘call-with-retrying-transaction’.

Two questions:

  1. Can another process come and fiddle with TO-REGISTER while we’re
     still in ‘reset-timestamps’?  Or can GC happen while we’re in
     ‘reset-timestamps’ and delete TO-REGISTER and remove it from the
     database?

I think none of these scenarios can happen, as long as we’ve taken the
.lock file for TO-REGISTER before, like ‘finalize-store-file’ does.

  2. After the transaction, TO-REGISTER is considered valid.  But are
     the effects of the on-going deduplication observable, due to
     non-atomicity of some operation?

I think the ‘replace-with-link’ dance is atomic, so we should be fine.

Thoughts?

Ludo’.