| Message ID | 20200623163649.32444-1-mail@cbaines.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#42023] database: register-items: reduce transaction scope. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
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’.
diff --git a/guix/store/database.scm b/guix/store/database.scm index a38e4d7e52..767335bc0f 100644 --- a/guix/store/database.scm +++ b/guix/store/database.scm @@ -441,24 +441,25 @@ in the database; #f means \"now\". Write a progress report to LOG-PORT." (when reset-timestamps? (reset-timestamps real-file-name)) (let-values (((hash nar-size) (nar-sha256 real-file-name))) - (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) + (call-with-retrying-transaction db + (lambda () + (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))) (when deduplicate? (deduplicate real-file-name hash #:store store-dir))))) - (call-with-retrying-transaction db - (lambda () - (let* ((prefix (format #f "registering ~a items" (length items))) - (progress (progress-reporter/bar (length items) - prefix log-port))) - (call-with-progress-reporter progress - (lambda (report) - (for-each (lambda (item) - (register db item) - (report)) - items))))))) + (let* ((prefix (format #f "registering ~a items" (length items))) + (progress (progress-reporter/bar (length items) + prefix log-port))) + (call-with-progress-reporter progress + (lambda (report) + (for-each (lambda (item) + (register db item) + (report)) + items)))))