[PATCH 0/6] Prepare the database code for use in the daemon

  • Done
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Christopher Baines
  • reepca
Owner
unassigned
Submitted by
Christopher Baines
Severity
normal
C
C
Christopher Baines wrote on 20 Feb 20:20 +0100
(address . guix-patches@gnu.org)
87h6i3lyph.fsf@cbaines.net
These changes prepare the database module for use in a Guile
implementation of the guix-daemon.

There's quite a few procedures that need adding, but these changes just
adapt the existing functionality in to something that can be built on.


Christopher Baines (6):
store: database: Remove call-with-savepoint and associated code.
store: database: Remove with-statement and associated code.
store: database: Inline SQL to where it's used.
store: database: Stop finalizing prepared statements.
store: database: Refactor sqlite-register.
store: database: Rename a couple of procedures.

.dir-locals.el | 3 -
guix/store/database.scm | 273 +++++++++++++++++-----------------------
2 files changed, 112 insertions(+), 164 deletions(-)


base-commit: f4af19b037826cad90bbcfe400ad864f028cc7d8
--
2.41.0
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmXU/gpfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9Xewow//ZcJ4ZCY790jv/n5viw4ucJgi/FFr98JY
dHQg0ygLvbtVAaTFROFg55+s8R4iZSPf5UKEYaxS4X8L5s7vSeN5CZirR9cEwGdh
LNyIzyvjqsUNxKvc1olI/36DRzduS2PIbW5nYBniO2eJ9RKp/3T8rTI+MBccT03N
giHSSM0Q9I/MHbmXzGIFDuoQvP+9+4sUwSVvUfk2gXMp4ZEud0oL4LKP/Jq4AJkI
fUqhQbiVhCHQOLR1okMhlFM1M5rSyOT5e+hFOWOVMw/jbl5Yy0YQ6mtCCsSjZi6g
UY2HOqnfx/uSUNJdfBcQoag6FBZ9nqSm3Zl7QTa2NGLs0TCEK22GzHhY5Fz/NC9b
Hx6Q9zMCC4K1FBhM+1lbNn/gEfNABf3MzQeTdUfqQSVey0Xr7EmGJ3hT6dBcEBNO
YPtDoTIAfyerNLG/OdHy/pdefZET/Rsh96A/02MZe5ySK5eBtrLC6jL/b7iSnGbg
UhKm9n1uV2AsNglExYE/LkdSLFcHdnzkDfwcD/+T45FHp+lyMadeYvpQEyYeLQ8r
mupawMZzcwySmfgYLJzDuRFLZkQW8P2w+TFKdOxiW6q2vgg6Ot7ZLbxLuccTW4n1
K455I8AOl3SBl10zVpM4Pc3/lBeRVqW3gX0riSLGYjYhPF6uu2UPSU16Ymu9ZAMJ
H//8GBe5NM4=
=rx2e
-----END PGP SIGNATURE-----

C
C
Christopher Baines wrote on 20 Feb 20:39 +0100
[PATCH 4/6] store: database: Stop finalizing prepared statements.
(address . 69292@debbugs.gnu.org)
c4bf41b5c91623ee93285f35516265cfdb0ab401.1708457946.git.mail@cbaines.net
Especially since we're asking for these to be cached.

Management of prepared statements isn't trivial, since you don't want to keep
them forever as this can lead to poor query performance, but I don't think
that finalizing them immediately is the right solution.

Change-Id: I61706b4d09d771835bb8f074b8f6a6ee871f5e2d

* guix/store/database.scm (sqlite-step-and-reset): New procedure.
(last-insert-row, path-id, update-or-insert, add-references): Don't finalize
prepared statements.

Change-Id: I2a2c6deb43935d67df9e43000a5105343d72b3e6
---
guix/store/database.scm | 40 ++++++++++++++++++----------------------
1 file changed, 18 insertions(+), 22 deletions(-)

Toggle diff (86 lines)
diff --git a/guix/store/database.scm b/guix/store/database.scm
index 7e3a2873ce..8d8b7346e0 100644
--- a/guix/store/database.scm
+++ b/guix/store/database.scm
@@ -167,16 +167,19 @@ (define-syntax with-database
((_ file db exp ...)
(call-with-database file (lambda (db) exp ...)))))
+(define (sqlite-step-and-reset statement)
+ (let ((val (sqlite-step statement)))
+ (sqlite-reset statement)
+ val))
+
(define (last-insert-row-id db)
;; XXX: (sqlite3) currently lacks bindings for 'sqlite3_last_insert_rowid'.
;; Work around that.
- (let* ((stmt (sqlite-prepare db "SELECT last_insert_rowid();"
- #:cache? #t))
- (result (sqlite-fold cons '() stmt)))
- (sqlite-finalize stmt)
- (match result
- ((#(id)) id)
- (_ #f))))
+ (let ((stmt (sqlite-prepare db
+ "SELECT last_insert_rowid();"
+ #:cache? #t)))
+ (vector-ref (sqlite-step-and-reset stmt)
+ 0)))
(define* (path-id db path)
"If PATH exists in the 'ValidPaths' table, return its numerical
@@ -187,11 +190,9 @@ (define* (path-id db path)
SELECT id FROM ValidPaths WHERE path = :path"
#:cache? #t)))
(sqlite-bind-arguments stmt #:path path)
- (let ((result (sqlite-fold cons '() stmt)))
- (sqlite-finalize stmt)
- (match result
- ((#(id) . _) id)
- (_ #f)))))
+ (match (sqlite-step-and-reset stmt)
+ (#(id) id)
+ (#f #f))))
(define-inlinable (assert-integer proc in-range? key number)
(unless (integer? number)
@@ -228,9 +229,8 @@ (define* (update-or-insert db #:key path deriver hash nar-size time)
(sqlite-bind-arguments stmt #:id id
#:deriver deriver
#:hash hash #:size nar-size #:time time)
- (sqlite-fold cons '() stmt)
- (sqlite-finalize stmt)
- (last-insert-row-id db))
+ (sqlite-step-and-reset stmt)
+ id)
(let ((stmt (sqlite-prepare
db
"
@@ -240,8 +240,7 @@ (define* (update-or-insert db #:key path deriver hash nar-size time)
(sqlite-bind-arguments stmt
#:path path #:deriver deriver
#:hash hash #:size nar-size #:time time)
- (sqlite-fold cons '() stmt) ;execute it
- (sqlite-finalize stmt)
+ (sqlite-step-and-reset stmt)
(last-insert-row-id db)))))
(define (add-references db referrer references)
@@ -254,13 +253,10 @@ (define (add-references db referrer references)
VALUES (:referrer, :reference)"
#:cache? #t)))
(for-each (lambda (reference)
- (sqlite-reset stmt)
(sqlite-bind-arguments stmt #:referrer referrer
#:reference reference)
- (sqlite-fold cons '() stmt) ;execute it
- (last-insert-row-id db))
- references)
- (sqlite-finalize stmt)))
+ (sqlite-step-and-reset stmt))
+ references)))
(define (timestamp)
"Return a timestamp, either the current time of SOURCE_DATE_EPOCH."
--
2.41.0
C
C
Christopher Baines wrote on 20 Feb 20:39 +0100
[PATCH 5/6] store: database: Refactor sqlite-register.
(address . 69292@debbugs.gnu.org)
11b24242dd71349c09486341ed0d01b55f5f8f9a.1708457946.git.mail@cbaines.net
The update-or-insert procedure name was unhelpfully generic, and these changes
should improve the code readability.

* guix/store/database.scm (update-or-insert): Remove procedure and inline
functionality in to sqlite-register.

Change-Id: Ifab0cdb7972d095460cc1f79b8b2f0e9b958059c
---
guix/store/database.scm | 132 ++++++++++++++++++++--------------------
1 file changed, 66 insertions(+), 66 deletions(-)

Toggle diff (159 lines)
diff --git a/guix/store/database.scm b/guix/store/database.scm
index 8d8b7346e0..0b570eabcd 100644
--- a/guix/store/database.scm
+++ b/guix/store/database.scm
@@ -194,54 +194,15 @@ (define* (path-id db path)
(#(id) id)
(#f #f))))
-(define-inlinable (assert-integer proc in-range? key number)
- (unless (integer? number)
- (throw 'wrong-type-arg proc
- "Wrong type argument ~A: ~S" (list key number)
- (list number)))
- (unless (in-range? number)
- (throw 'out-of-range proc
- "Integer ~A out of range: ~S" (list key number)
- (list number))))
-
-(define* (update-or-insert db #:key path deriver hash nar-size time)
- "The classic update-if-exists and insert-if-doesn't feature that sqlite
-doesn't exactly have... they've got something close, but it involves deleting
-and re-inserting instead of updating, which causes problems with foreign keys,
-of course. Returns the row id of the row that was modified or inserted."
-
- ;; Make sure NAR-SIZE is valid.
- (assert-integer "update-or-insert" positive? #:nar-size nar-size)
- (assert-integer "update-or-insert" (cut >= <> 0) #:time time)
-
- (let ((id (path-id db path)))
- (if id
- (let ((stmt (sqlite-prepare
- db
- "
-UPDATE ValidPaths
-SET hash = :hash,
- registrationTime = :time,
- deriver = :deriver,
- narSize = :size
-WHERE id = :id"
- #:cache? #t)))
- (sqlite-bind-arguments stmt #:id id
- #:deriver deriver
- #:hash hash #:size nar-size #:time time)
- (sqlite-step-and-reset stmt)
- id)
- (let ((stmt (sqlite-prepare
- db
- "
-INSERT INTO ValidPaths (path, hash, registrationTime, deriver, narSize)
-VALUES (:path, :hash, :time, :deriver, :size)"
- #:cache? #t)))
- (sqlite-bind-arguments stmt
- #:path path #:deriver deriver
- #:hash hash #:size nar-size #:time time)
- (sqlite-step-and-reset stmt)
- (last-insert-row-id db)))))
+(define (timestamp)
+ "Return a timestamp, either the current time of SOURCE_DATE_EPOCH."
+ (match (getenv "SOURCE_DATE_EPOCH")
+ (#f
+ (current-time time-utc))
+ ((= string->number seconds)
+ (if seconds
+ (make-time time-utc 0 seconds)
+ (current-time time-utc)))))
(define (add-references db referrer references)
"REFERRER is the id of the referring store item, REFERENCES is a list
@@ -258,15 +219,15 @@ (define (add-references db referrer references)
(sqlite-step-and-reset stmt))
references)))
-(define (timestamp)
- "Return a timestamp, either the current time of SOURCE_DATE_EPOCH."
- (match (getenv "SOURCE_DATE_EPOCH")
- (#f
- (current-time time-utc))
- ((= string->number seconds)
- (if seconds
- (make-time time-utc 0 seconds)
- (current-time time-utc)))))
+(define-inlinable (assert-integer proc in-range? key number)
+ (unless (integer? number)
+ (throw 'wrong-type-arg proc
+ "Wrong type argument ~A: ~S" (list key number)
+ (list number)))
+ (unless (in-range? number)
+ (throw 'out-of-range proc
+ "Integer ~A out of range: ~S" (list key number)
+ (list number))))
(define* (sqlite-register db #:key path (references '())
deriver hash nar-size
@@ -279,15 +240,54 @@ (define* (sqlite-register db #:key path (references '())
the database or #f, meaning \"right now\".
Every store item in REFERENCES must already be registered."
- (let ((id (update-or-insert db #:path path
- #:deriver deriver
- #:hash hash
- #:nar-size nar-size
- #:time (time-second time))))
- ;; Call 'path-id' on each of REFERENCES. This ensures we get a
- ;; "non-NULL constraint" failure if one of REFERENCES is unregistered.
- (add-references db id
- (map (cut path-id db <>) references))))
+
+ (define registration-time
+ (time-second time))
+
+ ;; Make sure NAR-SIZE is valid.
+ (assert-integer "sqlite-register" positive? #:nar-size nar-size)
+ (assert-integer "sqlite-register" (cut >= <> 0) #:time registration-time)
+
+ (define id
+ (let ((existing-id (path-id db path)))
+ (if existing-id
+ (let ((stmt (sqlite-prepare
+ db
+ "
+UPDATE ValidPaths
+SET hash = :hash,
+ registrationTime = :time,
+ deriver = :deriver,
+ narSize = :size
+WHERE id = :id"
+ #:cache? #t)))
+ (sqlite-bind-arguments stmt
+ #:id existing-id
+ #:deriver deriver
+ #:hash hash
+ #:size nar-size
+ #:time registration-time)
+ (sqlite-step-and-reset stmt)
+ existing-id)
+ (let ((stmt (sqlite-prepare
+ db
+ "
+INSERT INTO ValidPaths (path, hash, registrationTime, deriver, narSize)
+VALUES (:path, :hash, :time, :deriver, :size)"
+ #:cache? #t)))
+ (sqlite-bind-arguments stmt
+ #:path path
+ #:deriver deriver
+ #:hash hash
+ #:size nar-size
+ #:time registration-time)
+ (sqlite-step-and-reset stmt)
+ (last-insert-row-id db)))))
+
+ ;; Call 'path-id' on each of REFERENCES. This ensures we get a
+ ;; "non-NULL constraint" failure if one of REFERENCES is unregistered.
+ (add-references db id
+ (map (cut path-id db <>) references)))
;;;
--
2.41.0
C
C
Christopher Baines wrote on 20 Feb 20:39 +0100
[PATCH 2/6] store: database: Remove with-statement and associated code.
(address . 69292@debbugs.gnu.org)
b7360abb08559073653effea98a99332fc1f8071.1708457946.git.mail@cbaines.net
I think using dynamic-wind to finalize all statements is the wrong
approach. Firstly it would be good to allow reseting statements rather than
finalizing them. Then for the problem of handling errors, the approach I've
settled on in the build coordinator is to close the database connection, since
that'll trigger guile-sqlite3 to finalize all the cached statements.

This reverts commit 5d6e2255286e591def122ec2f4a3cbda497fea21.

* .dir-locals.el (scheme-mode): Remove with-statement.
* guix/store/database.scm (call-with-statement): Remove procedure.
(with-statement): Remove syntax rule.
(call-with-transaction, last-insert-row-id, path-id, update-or-insert,
add-references): Don't use with-statement.

Change-Id: I2fd976b3f12ec8105cc56350933a953cf53647e8
---
.dir-locals.el | 1 -
guix/store/database.scm | 62 ++++++++++++++++++-----------------------
2 files changed, 27 insertions(+), 36 deletions(-)

Toggle diff (142 lines)
diff --git a/.dir-locals.el b/.dir-locals.el
index f135eb69a5..2d1a03c313 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -131,7 +131,6 @@
(eval . (put 'with-database 'scheme-indent-function 2))
(eval . (put 'call-with-database 'scheme-indent-function 1))
(eval . (put 'call-with-transaction 'scheme-indent-function 1))
- (eval . (put 'with-statement 'scheme-indent-function 3))
(eval . (put 'call-with-retrying-transaction 'scheme-indent-function 1))
(eval . (put 'call-with-container 'scheme-indent-function 1))
diff --git a/guix/store/database.scm b/guix/store/database.scm
index 3093fd816a..de72b79860 100644
--- a/guix/store/database.scm
+++ b/guix/store/database.scm
@@ -130,25 +130,22 @@ (define* (call-with-transaction db proc #:key restartable?)
the transaction, otherwise commit the transaction after it finishes.
RESTARTABLE? may be set to a non-#f value when it is safe to run PROC multiple
times. This may reduce contention for the database somewhat."
- (define (exec sql)
- (with-statement db sql stmt
- (sqlite-fold cons '() stmt)))
;; We might use begin immediate here so that if we need to retry, we figure
;; that out immediately rather than because some SQLITE_BUSY exception gets
;; thrown partway through PROC - in which case the part already executed
;; (which may contain side-effects!) might have to be executed again for
;; every retry.
- (exec (if restartable? "begin;" "begin immediate;"))
+ (sqlite-exec db (if restartable? "begin;" "begin immediate;"))
(catch #t
(lambda ()
(let-values ((result (proc)))
- (exec "commit;")
+ (sqlite-exec db "commit;")
(apply values result)))
(lambda args
;; The roll back may or may not have occurred automatically when the
;; error was generated. If it has occurred, this does nothing but signal
;; an error. If it hasn't occurred, this needs to be done.
- (false-if-exception (exec "rollback;"))
+ (false-if-exception (sqlite-exec db "rollback;"))
(apply throw args))))
(define* (call-with-retrying-transaction db proc #:key restartable?)
@@ -170,26 +167,14 @@ (define-syntax with-database
((_ file db exp ...)
(call-with-database file (lambda (db) exp ...)))))
-(define (call-with-statement db sql proc)
- (let ((stmt (sqlite-prepare db sql #:cache? #t)))
- (dynamic-wind
- (const #t)
- (lambda ()
- (proc stmt))
- (lambda ()
- (sqlite-finalize stmt)))))
-
-(define-syntax-rule (with-statement db sql stmt exp ...)
- "Run EXP... with STMT bound to a prepared statement corresponding to the sql
-string SQL for DB."
- (call-with-statement db sql
- (lambda (stmt) exp ...)))
-
(define (last-insert-row-id db)
;; XXX: (sqlite3) currently lacks bindings for 'sqlite3_last_insert_rowid'.
;; Work around that.
- (with-statement db "SELECT last_insert_rowid();" stmt
- (match (sqlite-fold cons '() stmt)
+ (let* ((stmt (sqlite-prepare db "SELECT last_insert_rowid();"
+ #:cache? #t))
+ (result (sqlite-fold cons '() stmt)))
+ (sqlite-finalize stmt)
+ (match result
((#(id)) id)
(_ #f))))
@@ -199,11 +184,13 @@ (define path-id-sql
(define* (path-id db path)
"If PATH exists in the 'ValidPaths' table, return its numerical
identifier. Otherwise, return #f."
- (with-statement db path-id-sql stmt
+ (let ((stmt (sqlite-prepare db path-id-sql #:cache? #t)))
(sqlite-bind-arguments stmt #:path path)
- (match (sqlite-fold cons '() stmt)
- ((#(id) . _) id)
- (_ #f))))
+ (let ((result (sqlite-fold cons '() stmt)))
+ (sqlite-finalize stmt)
+ (match result
+ ((#(id) . _) id)
+ (_ #f)))))
(define update-sql
"UPDATE ValidPaths SET hash = :hash, registrationTime = :time, deriver =
@@ -235,17 +222,20 @@ (define* (update-or-insert db #:key path deriver hash nar-size time)
(let ((id (path-id db path)))
(if id
- (with-statement db update-sql stmt
+ (let ((stmt (sqlite-prepare db update-sql #:cache? #t)))
(sqlite-bind-arguments stmt #:id id
#:deriver deriver
#:hash hash #:size nar-size #:time time)
- (sqlite-fold cons '() stmt))
- (with-statement db insert-sql stmt
+ (sqlite-fold cons '() stmt)
+ (sqlite-finalize stmt)
+ (last-insert-row-id db))
+ (let ((stmt (sqlite-prepare db insert-sql #:cache? #t)))
(sqlite-bind-arguments stmt
#:path path #:deriver deriver
#:hash hash #:size nar-size #:time time)
- (sqlite-fold cons '() stmt)))
- (last-insert-row-id db)))
+ (sqlite-fold cons '() stmt) ;execute it
+ (sqlite-finalize stmt)
+ (last-insert-row-id db)))))
(define add-reference-sql
"INSERT OR REPLACE INTO Refs (referrer, reference) VALUES (:referrer, :reference);")
@@ -253,13 +243,15 @@ (define add-reference-sql
(define (add-references db referrer references)
"REFERRER is the id of the referring store item, REFERENCES is a list
ids of items referred to."
- (with-statement db add-reference-sql stmt
+ (let ((stmt (sqlite-prepare db add-reference-sql #:cache? #t)))
(for-each (lambda (reference)
(sqlite-reset stmt)
(sqlite-bind-arguments stmt #:referrer referrer
#:reference reference)
- (sqlite-fold cons '() stmt))
- references)))
+ (sqlite-fold cons '() stmt) ;execute it
+ (last-insert-row-id db))
+ references)
+ (sqlite-finalize stmt)))
(define (timestamp)
"Return a timestamp, either the current time of SOURCE_DATE_EPOCH."
--
2.41.0
C
C
Christopher Baines wrote on 20 Feb 20:39 +0100
[PATCH 1/6] store: database: Remove call-with-savepoint and associated code.
(address . 69292@debbugs.gnu.org)
4b6a268daab5e0b307dff2229d551a47c9fe1ebc.1708457946.git.mail@cbaines.net
While care does need to be taken with making updates or inserts to the
ValidPaths table, I think that trying to ensure this within update-or-insert
is the wrong approach. Instead, when working with the store database, only one
connection should be used to make changes to the database and those changes
should happen in transactions that ideally begin immediately.

This reverts commit 37545de4a3bf59611c184b31506fe9a16abe4c8b.

* .dir-locals.el (scheme-mode): Remove entries for call-with-savepoint and
call-with-retrying-savepoint.
* guix/store/database.scm (call-with-savepoint, call-with-retrying-savepoint):
Remove procedures.
(update-or-insert): Remove use of call-with-savepoint.

Change-Id: I2f986e8623d8235a90c40d5f219c1292c1ab157b
---
.dir-locals.el | 2 --
guix/store/database.scm | 75 +++++++----------------------------------
2 files changed, 13 insertions(+), 64 deletions(-)

Toggle diff (115 lines)
diff --git a/.dir-locals.el b/.dir-locals.el
index d18e6ba760..f135eb69a5 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -133,8 +133,6 @@
(eval . (put 'call-with-transaction 'scheme-indent-function 1))
(eval . (put 'with-statement 'scheme-indent-function 3))
(eval . (put 'call-with-retrying-transaction 'scheme-indent-function 1))
- (eval . (put 'call-with-savepoint 'scheme-indent-function 1))
- (eval . (put 'call-with-retrying-savepoint 'scheme-indent-function 1))
(eval . (put 'call-with-container 'scheme-indent-function 1))
(eval . (put 'container-excursion 'scheme-indent-function 1))
diff --git a/guix/store/database.scm b/guix/store/database.scm
index 2968f13492..3093fd816a 100644
--- a/guix/store/database.scm
+++ b/guix/store/database.scm
@@ -151,39 +151,11 @@ (define* (call-with-transaction db proc #:key restartable?)
(false-if-exception (exec "rollback;"))
(apply throw args))))
-(define* (call-with-savepoint db proc
- #:optional (savepoint-name "SomeSavepoint"))
- "Call PROC after creating a savepoint named SAVEPOINT-NAME. If PROC exits
-abnormally, rollback to that savepoint. In all cases, remove the savepoint
-prior to returning."
- (define (exec sql)
- (with-statement db sql stmt
- (sqlite-fold cons '() stmt)))
-
- (dynamic-wind
- (lambda ()
- (exec (string-append "SAVEPOINT " savepoint-name ";")))
- (lambda ()
- (catch #t
- proc
- (lambda args
- (exec (string-append "ROLLBACK TO " savepoint-name ";"))
- (apply throw args))))
- (lambda ()
- (exec (string-append "RELEASE " savepoint-name ";")))))
-
(define* (call-with-retrying-transaction db proc #:key restartable?)
(call-with-SQLITE_BUSY-retrying
(lambda ()
(call-with-transaction db proc #:restartable? restartable?))))
-(define* (call-with-retrying-savepoint db proc
- #:optional (savepoint-name
- "SomeSavepoint"))
- (call-with-SQLITE_BUSY-retrying
- (lambda ()
- (call-with-savepoint db proc savepoint-name))))
-
(define %default-database-file
;; Default location of the store database.
(string-append %store-database-directory "/db.sqlite"))
@@ -261,40 +233,19 @@ (define* (update-or-insert db #:key path deriver hash nar-size time)
(assert-integer "update-or-insert" positive? #:nar-size nar-size)
(assert-integer "update-or-insert" (cut >= <> 0) #:time time)
- ;; It's important that querying the path-id and the insert/update operation
- ;; take place in the same transaction, as otherwise some other
- ;; process/thread/fiber could register the same path between when we check
- ;; whether it's already registered and when we register it, resulting in
- ;; duplicate paths (which, due to a 'unique' constraint, would cause an
- ;; exception to be thrown). With the default journaling mode this will
- ;; prevent writes from occurring during that sensitive time, but with WAL
- ;; mode it will instead arrange to return SQLITE_BUSY when a write occurs
- ;; between the start of a read transaction and its upgrading to a write
- ;; transaction (see https://sqlite.org/rescode.html#busy_snapshot).
- ;; Experimentally, it seems this SQLITE_BUSY will ignore a busy_timeout and
- ;; immediately return (makes sense, since waiting won't change anything).
-
- ;; Note that when that kind of SQLITE_BUSY error is returned, it will keep
- ;; being returned every time we try to upgrade the same outermost
- ;; transaction to a write transaction. So when retrying, we have to restart
- ;; the *outermost* write transaction. We can't inherently tell whether
- ;; we're the outermost write transaction, so we leave the retry-handling to
- ;; the caller.
- (call-with-savepoint db
- (lambda ()
- (let ((id (path-id db path)))
- (if id
- (with-statement db update-sql stmt
- (sqlite-bind-arguments stmt #:id id
- #:deriver deriver
- #:hash hash #:size nar-size #:time time)
- (sqlite-fold cons '() stmt))
- (with-statement db insert-sql stmt
- (sqlite-bind-arguments stmt
- #:path path #:deriver deriver
- #:hash hash #:size nar-size #:time time)
- (sqlite-fold cons '() stmt)))
- (last-insert-row-id db)))))
+ (let ((id (path-id db path)))
+ (if id
+ (with-statement db update-sql stmt
+ (sqlite-bind-arguments stmt #:id id
+ #:deriver deriver
+ #:hash hash #:size nar-size #:time time)
+ (sqlite-fold cons '() stmt))
+ (with-statement db insert-sql stmt
+ (sqlite-bind-arguments stmt
+ #:path path #:deriver deriver
+ #:hash hash #:size nar-size #:time time)
+ (sqlite-fold cons '() stmt)))
+ (last-insert-row-id db)))
(define add-reference-sql
"INSERT OR REPLACE INTO Refs (referrer, reference) VALUES (:referrer, :reference);")

base-commit: f4af19b037826cad90bbcfe400ad864f028cc7d8
--
2.41.0
C
C
Christopher Baines wrote on 20 Feb 20:39 +0100
[PATCH 3/6] store: database: Inline SQL to where it's used.
(address . 69292@debbugs.gnu.org)
5abcabc4cb65ea63db7c4f046700116e4882b287.1708457946.git.mail@cbaines.net
This makes the code easier to read, as you don't have to keep jumping between
the two places.

* guix/store/database.scm (path-id-sql, update-sql, insert-sql,
add-reference-sql): Remove variables.
(path-id, update-or-insert, add-references): Include SQL.

Change-Id: I53b4ab973be8d0cd10a0f35ba25972f1c9680353
---
guix/store/database.scm | 45 ++++++++++++++++++++++++-----------------
1 file changed, 27 insertions(+), 18 deletions(-)

Toggle diff (91 lines)
diff --git a/guix/store/database.scm b/guix/store/database.scm
index de72b79860..7e3a2873ce 100644
--- a/guix/store/database.scm
+++ b/guix/store/database.scm
@@ -178,13 +178,14 @@ (define (last-insert-row-id db)
((#(id)) id)
(_ #f))))
-(define path-id-sql
- "SELECT id FROM ValidPaths WHERE path = :path")
-
(define* (path-id db path)
"If PATH exists in the 'ValidPaths' table, return its numerical
identifier. Otherwise, return #f."
- (let ((stmt (sqlite-prepare db path-id-sql #:cache? #t)))
+ (let ((stmt (sqlite-prepare
+ db
+ "
+SELECT id FROM ValidPaths WHERE path = :path"
+ #:cache? #t)))
(sqlite-bind-arguments stmt #:path path)
(let ((result (sqlite-fold cons '() stmt)))
(sqlite-finalize stmt)
@@ -192,14 +193,6 @@ (define* (path-id db path)
((#(id) . _) id)
(_ #f)))))
-(define update-sql
- "UPDATE ValidPaths SET hash = :hash, registrationTime = :time, deriver =
-:deriver, narSize = :size WHERE id = :id")
-
-(define insert-sql
- "INSERT INTO ValidPaths (path, hash, registrationTime, deriver, narSize)
-VALUES (:path, :hash, :time, :deriver, :size)")
-
(define-inlinable (assert-integer proc in-range? key number)
(unless (integer? number)
(throw 'wrong-type-arg proc
@@ -222,14 +215,28 @@ (define* (update-or-insert db #:key path deriver hash nar-size time)
(let ((id (path-id db path)))
(if id
- (let ((stmt (sqlite-prepare db update-sql #:cache? #t)))
+ (let ((stmt (sqlite-prepare
+ db
+ "
+UPDATE ValidPaths
+SET hash = :hash,
+ registrationTime = :time,
+ deriver = :deriver,
+ narSize = :size
+WHERE id = :id"
+ #:cache? #t)))
(sqlite-bind-arguments stmt #:id id
#:deriver deriver
#:hash hash #:size nar-size #:time time)
(sqlite-fold cons '() stmt)
(sqlite-finalize stmt)
(last-insert-row-id db))
- (let ((stmt (sqlite-prepare db insert-sql #:cache? #t)))
+ (let ((stmt (sqlite-prepare
+ db
+ "
+INSERT INTO ValidPaths (path, hash, registrationTime, deriver, narSize)
+VALUES (:path, :hash, :time, :deriver, :size)"
+ #:cache? #t)))
(sqlite-bind-arguments stmt
#:path path #:deriver deriver
#:hash hash #:size nar-size #:time time)
@@ -237,13 +244,15 @@ (define* (update-or-insert db #:key path deriver hash nar-size time)
(sqlite-finalize stmt)
(last-insert-row-id db)))))
-(define add-reference-sql
- "INSERT OR REPLACE INTO Refs (referrer, reference) VALUES (:referrer, :reference);")
-
(define (add-references db referrer references)
"REFERRER is the id of the referring store item, REFERENCES is a list
ids of items referred to."
- (let ((stmt (sqlite-prepare db add-reference-sql #:cache? #t)))
+ (let ((stmt (sqlite-prepare
+ db
+ "
+INSERT OR REPLACE INTO Refs (referrer, reference)
+VALUES (:referrer, :reference)"
+ #:cache? #t)))
(for-each (lambda (reference)
(sqlite-reset stmt)
(sqlite-bind-arguments stmt #:referrer referrer
--
2.41.0
C
C
Christopher Baines wrote on 20 Feb 20:39 +0100
[PATCH 6/6] store: database: Rename a couple of procedures.
(address . 69292@debbugs.gnu.org)
12f872394c84039fa553e50381d040de24f24d3e.1708457946.git.mail@cbaines.net
These names should be more descriptive.

* guix/store/database.scm (path-id): Rename to select-valid-path-id.
(sqlite-register): Rename to register-valid-path.
(register-items): Update accordingly.

Change-Id: I6d4a14d4cde9d71ab34d6ffdbfbfde51b2c0e1db
---
guix/store/database.scm | 41 ++++++++++++++++++++++-------------------
1 file changed, 22 insertions(+), 19 deletions(-)

Toggle diff (92 lines)
diff --git a/guix/store/database.scm b/guix/store/database.scm
index 0b570eabcd..0190696ad5 100644
--- a/guix/store/database.scm
+++ b/guix/store/database.scm
@@ -40,8 +40,10 @@ (define-module (guix store database)
store-database-file
call-with-database
with-database
- path-id
- sqlite-register
+
+ select-valid-path-id
+
+ register-valid-path
register-items
%epoch
reset-timestamps
@@ -181,9 +183,9 @@ (define (last-insert-row-id db)
(vector-ref (sqlite-step-and-reset stmt)
0)))
-(define* (path-id db path)
- "If PATH exists in the 'ValidPaths' table, return its numerical
-identifier. Otherwise, return #f."
+(define (select-valid-path-id db path)
+ "If PATH exists in the 'ValidPaths' table, return its numerical identifier.
+Otherwise, return #f."
(let ((stmt (sqlite-prepare
db
"
@@ -229,9 +231,9 @@ (define-inlinable (assert-integer proc in-range? key number)
"Integer ~A out of range: ~S" (list key number)
(list number))))
-(define* (sqlite-register db #:key path (references '())
- deriver hash nar-size
- (time (timestamp)))
+(define* (register-valid-path db #:key path (references '())
+ deriver hash nar-size
+ (time (timestamp)))
"Registers this stuff in DB. PATH is the store item to register and
REFERENCES is the list of store items PATH refers to; DERIVER is the '.drv'
that produced PATH, HASH is the base16-encoded Nix sha256 hash of
@@ -249,7 +251,7 @@ (define* (sqlite-register db #:key path (references '())
(assert-integer "sqlite-register" (cut >= <> 0) #:time registration-time)
(define id
- (let ((existing-id (path-id db path)))
+ (let ((existing-id (select-valid-path-id db path)))
(if existing-id
(let ((stmt (sqlite-prepare
db
@@ -287,7 +289,8 @@ (define* (sqlite-register db #:key path (references '())
;; Call 'path-id' on each of REFERENCES. This ensures we get a
;; "non-NULL constraint" failure if one of REFERENCES is unregistered.
(add-references db id
- (map (cut path-id db <>) references)))
+ (map (cut select-valid-path-id db <>) references)))
+
;;;
@@ -364,18 +367,18 @@ (define* (register-items db items
;; When TO-REGISTER is already registered, skip it. This makes a
;; significant differences when 'register-closures' is called
;; consecutively for overlapping closures such as 'system' and 'bootcfg'.
- (unless (path-id db to-register)
+ (unless (select-valid-path-id db to-register)
(let-values (((hash nar-size) (nar-sha256 real-file-name)))
(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))))))
+ (register-valid-path 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))))))
(let* ((prefix (format #f "registering ~a items" (length items)))
(progress (progress-reporter/bar (length items)
--
2.41.0
R
R
reepca wrote on 22 Feb 13:39 +0100
Re: [PATCH 4/6] store: database: Stop finalizing prepared statements.
(name . Christopher Baines)(address . mail@cbaines.net)(address . 69292@debbugs.gnu.org)
87y1bcr7ue.fsf@a.a
Christopher Baines <mail@cbaines.net> writes:

Toggle quote (6 lines)
> Especially since we're asking for these to be cached.
>
> Management of prepared statements isn't trivial, since you don't want to keep
> them forever as this can lead to poor query performance, but I don't think
> that finalizing them immediately is the right solution.

guile-sqlite3 arranges for cached statements to only be reset, not
finalized, when sqlite-finalize is called on them (see
The idea behind this admittedly-unintuitive behavior is that it allows
for the caching behavior of a statement to be decided independently of
the code that actually uses it: if it's been decided elsewhere that a
prepared statement is worth keeping around, it will reuse it, but if it
hasn't, it will still properly clean up what it created.

Perhaps reusing the name 'sqlite-finalize' to make that behavior
transparent wasn't the best choice in the long run.

I hope that makes the way it was written a bit less baffling.

- reepca
-----BEGIN PGP SIGNATURE-----

iQFLBAEBCAA1FiEEdNapMPRLm4SepVYGwWaqSV9/GJwFAmXXQIkXHHJlZXBjYUBy
dXNzZWxzdGVpbi54eXoACgkQwWaqSV9/GJxKiAf/fsuX+zHXnKGCOIzBXAbWsW9H
cf6cY/5FQK/55CUQhqJ8f4+xsM8ldIgXMcNs1Nwx2gvHCAiyUOnDFrpg6zJI7Iw3
oQnQoa62L+3YtbXM60kuk3cgd/1u0Bi8euNJFdA2CdkUlHt2OkbJ0UlK6he+LCH8
BaUh7hvKxBa1gYixB8P8G4+7B8mLnlb8eFeMMGWi8cO19wPwHgr/m54qWr6KKhzl
+rTamrOa3Q9Ju1Jm38Qo0b6gpX9agQ9bPrhOKGmxURgjmU274JayHg+Fuwoxz9TJ
ydo781BYQpvuvFuiUMgonOBsEIBT5jJE5RQhubuCzTC2CvKrZ7MSHQ2vdwFbzA==
=mr/M
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 23 Feb 17:35 +0100
Re: [bug#69292] [PATCH 2/6] store: database: Remove with-statement and associated code.
(name . Christopher Baines)(address . mail@cbaines.net)
878r3bf8aw.fsf@gnu.org
Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (16 lines)
> I think using dynamic-wind to finalize all statements is the wrong
> approach. Firstly it would be good to allow reseting statements rather than
> finalizing them. Then for the problem of handling errors, the approach I've
> settled on in the build coordinator is to close the database connection, since
> that'll trigger guile-sqlite3 to finalize all the cached statements.
>
> This reverts commit 5d6e2255286e591def122ec2f4a3cbda497fea21.
>
> * .dir-locals.el (scheme-mode): Remove with-statement.
> * guix/store/database.scm (call-with-statement): Remove procedure.
> (with-statement): Remove syntax rule.
> (call-with-transaction, last-insert-row-id, path-id, update-or-insert,
> add-references): Don't use with-statement.
>
> Change-Id: I2fd976b3f12ec8105cc56350933a953cf53647e8

I’m all for removing ‘dynamic-wind’, we’ll have to do it to make it
usable in a fiberized context anyway.

I’ll let reepca comment.

Ludo’.
L
L
Ludovic Courtès wrote on 23 Feb 17:33 +0100
Re: [bug#69292] [PATCH 5/6] store: database: Refactor sqlite-register.
(name . Christopher Baines)(address . mail@cbaines.net)
87cysnf8dy.fsf@gnu.org
Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (8 lines)
> The update-or-insert procedure name was unhelpfully generic, and these changes
> should improve the code readability.
>
> * guix/store/database.scm (update-or-insert): Remove procedure and inline
> functionality in to sqlite-register.
>
> Change-Id: Ifab0cdb7972d095460cc1f79b8b2f0e9b958059c

LGTM.
L
L
Ludovic Courtès wrote on 23 Feb 17:31 +0100
Re: [bug#69292] [PATCH 1/6] store: database: Remove call-with-savepoint and associated code.
(name . Christopher Baines)(address . mail@cbaines.net)
87h6hzf8h3.fsf@gnu.org
Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (16 lines)
> While care does need to be taken with making updates or inserts to the
> ValidPaths table, I think that trying to ensure this within update-or-insert
> is the wrong approach. Instead, when working with the store database, only one
> connection should be used to make changes to the database and those changes
> should happen in transactions that ideally begin immediately.
>
> This reverts commit 37545de4a3bf59611c184b31506fe9a16abe4c8b.
>
> * .dir-locals.el (scheme-mode): Remove entries for call-with-savepoint and
> call-with-retrying-savepoint.
> * guix/store/database.scm (call-with-savepoint, call-with-retrying-savepoint):
> Remove procedures.
> (update-or-insert): Remove use of call-with-savepoint.
>
> Change-Id: I2f986e8623d8235a90c40d5f219c1292c1ab157b

Okay, I trust you on this; we’ll have to make sure to actually start
transactions at the top level.

(BTW, make sure at least “make check TESTS=tests/store-database.scm”
passes for changes to this module.)
L
L
Ludovic Courtès wrote on 23 Feb 17:43 +0100
Re: [bug#69292] [PATCH 6/6] store: database: Rename a couple of procedures.
(name . Christopher Baines)(address . mail@cbaines.net)
87zfvrdtcw.fsf@gnu.org
Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (8 lines)
> These names should be more descriptive.
>
> * guix/store/database.scm (path-id): Rename to select-valid-path-id.
> (sqlite-register): Rename to register-valid-path.
> (register-items): Update accordingly.
>
> Change-Id: I6d4a14d4cde9d71ab34d6ffdbfbfde51b2c0e1db

OK for ‘register-valid-path’.

For ‘path-id’ my preference would be ‘valid-path-id’ or keeping
‘path-id’; ‘select’ looks odd as nothing’s being “selected” here in the
non-SQL sense of the word.

Ludo’.
L
L
Ludovic Courtès wrote on 23 Feb 17:40 +0100
Re: [bug#69292] [PATCH 3/6] store: database: Inline SQL to where it's used.
(name . Christopher Baines)(address . mail@cbaines.net)
874jdzf81u.fsf@gnu.org
Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (9 lines)
> This makes the code easier to read, as you don't have to keep jumping between
> the two places.
>
> * guix/store/database.scm (path-id-sql, update-sql, insert-sql,
> add-reference-sql): Remove variables.
> (path-id, update-or-insert, add-references): Include SQL.
>
> Change-Id: I53b4ab973be8d0cd10a0f35ba25972f1c9680353

LGTM.

Toggle quote (14 lines)
> (let ((id (path-id db path)))
> (if id
> - (let ((stmt (sqlite-prepare db update-sql #:cache? #t)))
> + (let ((stmt (sqlite-prepare
> + db
> + "
> +UPDATE ValidPaths
> +SET hash = :hash,
> + registrationTime = :time,
> + deriver = :deriver,
> + narSize = :size
> +WHERE id = :id"
> + #:cache? #t)))

I think we can make it a bit more dense (3 or 4 lines in total for the
statement). :-)

In the future, we should probably add a macro to sqlite3 like that one
from Cuirass:

Toggle snippet (14 lines)
(define-syntax-rule (exec-query/bind db query args ...)
"Execute the specific QUERY with the given ARGS. Uses of 'exec-query/bind'
typically look like this:

(exec-query/bind db \"SELECT * FROM Foo WHERE x = \" x \"AND Y=\" y \";\")

References to variables 'x' and 'y' here are replaced by $1 and $2 in the
SQL query.

This ensures that (1) SQL injection is impossible, and (2) the number of
parameters matches the number of arguments to bind."
(%exec-query/bind db () "" query args ...))

That makes things slightly more readable IMO since you don’t end up with
two separate calls, one to prepare the statement and the other one to
bind its arguments.

Ludo’.
R
R
Reepca Russelstein wrote on 23 Feb 19:32 +0100
Re: [bug#69292] [PATCH 2/6] store: database: Remove with-statement and associated code.
(name . Ludovic Courtès)(address . ludo@gnu.org)
87ttlzqbfg.fsf@russelstein.xyz
Toggle quote (4 lines)
>> I think using dynamic-wind to finalize all statements is the wrong
>> approach. Firstly it would be good to allow reseting statements rather than
>> finalizing them.

Let me once again mention https://issues.guix.gnu.org/69292#7with this
much more magnificent Cc list.

Toggle quote (4 lines)
>> Then for the problem of handling errors, the approach I've
>> settled on in the build coordinator is to close the database connection, since
>> that'll trigger guile-sqlite3 to finalize all the cached statements.

And in the event that a statement is *not* cached, it will hang around
until the gc next pumps the statement guardian, at which point it will
do... whatever happens when a statement is finalized after the database
connection it was created with has already been closed, I guess. I
don't know if that's a problem or not. On further investigation, it
appears that sqlite_close would return SQLITE_BUSY, but guile-sqlite3's
sqlite-close doesn't throw any exceptions, and according to
actually closing the database until all statements have been finalized.
So I guess that works.

Toggle quote (16 lines)
>>
>> This reverts commit 5d6e2255286e591def122ec2f4a3cbda497fea21.
>>
>> * .dir-locals.el (scheme-mode): Remove with-statement.
>> * guix/store/database.scm (call-with-statement): Remove procedure.
>> (with-statement): Remove syntax rule.
>> (call-with-transaction, last-insert-row-id, path-id, update-or-insert,
>> add-references): Don't use with-statement.
>>
>> Change-Id: I2fd976b3f12ec8105cc56350933a953cf53647e8
>
> I’m all for removing ‘dynamic-wind’, we’ll have to do it to make it
> usable in a fiberized context anyway.
>
> I’ll let reepca comment.

What is the proper fibers-friendly replacement for dynamic-wind, anyway,
that is "like dynamic-wind, except when suspending a fiber"? It feels
like the current interaction between dynamic-wind and fibers is more of
an accident of how the implementation works; I don't suppose fibers could
export a transparent replacement, like how it already exports a
replacement for 'sleep'? Or perhaps the underlying issue is that we
keep using 'dynamic-wind' in situations where it only makes sense to
enter or exit the dynamic extent once? Is it time to bring
'unwind-protect' back into style? I see that fibers now has a
dynamic-wind*, should that be preferred?

I don't have a strong opinion on these changes, I just want to make sure
we're all aware of how guile-sqlite3's sqlite-finalize acts with cached
statements.

- reepca
-----BEGIN PGP SIGNATURE-----

iQFLBAEBCAA1FiEEdNapMPRLm4SepVYGwWaqSV9/GJwFAmXY5KMXHHJlZXBjYUBy
dXNzZWxzdGVpbi54eXoACgkQwWaqSV9/GJwZ5gf9G5/VPsqO4Y/RkSuL6Qz2RdNy
kLNh3TiARpSTcEh5I3KB9pPyztQu5w0QYwp91VrdQbLOtzUXwzgQQlRgmhrtruwH
cOwOgYqHZOjyJwMRz0QQLs0rzNr5rsfNFVDBjLViBg3nZ5InRXJ7xWZo6s9thsoz
exmMvTVuDRlDaA9PNTU1N/GKaLkq/LTgpw4mQ/PCrGLd/noNHGfyLoIbhyi9w1ok
Iew6wYUbioBwGlj6/rLz6hPqt7irSKtZ/WpuWH+CaA7bhH+t0GQ18FTEWvErf9zW
xkCjNYuodAXlEzbqoExWkAvRPayQ0c3+xy+YAbs3vqC0SHFMPWMa/x+/PG1e4A==
=LhLQ
-----END PGP SIGNATURE-----

C
C
Christopher Baines wrote on 26 Feb 11:50 +0100
Re: [PATCH 4/6] store: database: Stop finalizing prepared statements.
(address . reepca@russelstein.xyz)(address . 69292@debbugs.gnu.org)
875xyb8pl9.fsf@cbaines.net
reepca@russelstein.xyz writes:

Toggle quote (22 lines)
> Christopher Baines <mail@cbaines.net> writes:
>
>> Especially since we're asking for these to be cached.
>>
>> Management of prepared statements isn't trivial, since you don't want to keep
>> them forever as this can lead to poor query performance, but I don't think
>> that finalizing them immediately is the right solution.
>
> guile-sqlite3 arranges for cached statements to only be reset, not
> finalized, when sqlite-finalize is called on them (see
> https://notabug.org/guile-sqlite3/guile-sqlite3/src/master/sqlite3.scm.in#L283).
> The idea behind this admittedly-unintuitive behavior is that it allows
> for the caching behavior of a statement to be decided independently of
> the code that actually uses it: if it's been decided elsewhere that a
> prepared statement is worth keeping around, it will reuse it, but if it
> hasn't, it will still properly clean up what it created.
>
> Perhaps reusing the name 'sqlite-finalize' to make that behavior
> transparent wasn't the best choice in the long run.
>
> I hope that makes the way it was written a bit less baffling.

Right, this is something I hadn't realised. I don't think this causes
any problems for how I'm using sqlite-finalize though. Thanks for
pointing this out.
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmXcbXJfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9XeQNg//SopGtll99qYYhHg3iBVFT49HcJ9CATDz
lVa93cxK8gIBnRMVSHo35EFGW2fzpIpCnB100KZMScyoRtfhSwFTUTCHh8EGYYng
YA1W7du4b85IbNw4zipMeHd9WZBsyNedyTwtftj3++Ib1egWRHutDBWgUA1AED6M
1IYwWmTKzlbMBnmUn++xmKiinKYg7xDgNW2UWeuWIU6hoIli8CmfsXizQQcohFq7
RMz2LgverA50NMe26IJadFCmsLelxF1wLCz7xbfxFgU5TXB3VOcR9NKuFixhvFV0
zgcYNn6NG+4OQNXewqx3scYxWOEzdDkPzYQr+QATCgKokQ7/LyUG5QPGwPt0ymhF
0uj3oEYTTHLGghxgI8di5EvDT7WBEx4KteBUhgchjj1ir1JPszTahmEk+oiSwCMi
SZ6nvac3WcP/2rBDAQ3BkvgRS/SBF11O65lgUs80DA2e8lU5EgS5Xx5UFb5gy0uv
pXe1Acseu0fcjZ4xHPuLxzgsMDhEK5DAMrKpW9rgYFjsJBVlZjhE8rhh45DiYlnX
E5eVxEQfQ16rIeJHlXFrrMJkC2YZrXrLKMy0c3GeVAPYNqsoi/my9vawCl637N8s
Kl0Eiv70pOnL7XrKfgw3ipOQ4skzy5r4kYZjwZjeZ8BBrT4dWs3u0H1LKeiOdvwX
IUn8H6Uu5u8=
=S9g+
-----END PGP SIGNATURE-----

C
C
Christopher Baines wrote on 26 Feb 12:03 +0100
Re: [bug#69292] [PATCH 6/6] store: database: Rename a couple of procedures.
(name . Ludovic Courtès)(address . ludo@gnu.org)
871q8z8p0n.fsf@cbaines.net
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (16 lines)
> Christopher Baines <mail@cbaines.net> skribis:
>
>> These names should be more descriptive.
>>
>> * guix/store/database.scm (path-id): Rename to select-valid-path-id.
>> (sqlite-register): Rename to register-valid-path.
>> (register-items): Update accordingly.
>>
>> Change-Id: I6d4a14d4cde9d71ab34d6ffdbfbfde51b2c0e1db
>
> OK for ‘register-valid-path’.
>
> For ‘path-id’ my preference would be ‘valid-path-id’ or keeping
> ‘path-id’; ‘select’ looks odd as nothing’s being “selected” here in the
> non-SQL sense of the word.

The main thing I'm trying to do here is make the procedures that
directly interact with the database stand out (similar to the ! suffix
convention). Maybe the procedure taking the db as an argument is enough,
but to me path-id/valid-path-id look too much like record accessors.
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmXccFhfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9XeODA/+P0cbrvuITjA3nD1phTqlKCrJQqQX+i+9
mJ7viwHgbsn0+xoxNRBVkqNW9v5QOGc8PdLoSwbCO6n9i0f/sPD44NVwDQGNyZjO
S0vWBPx3GSoqGkpoSN2GlHqCVK5/tQ2XeP2qoFIszkPYHo53BMPUDTfg1+jj465J
goLQxSiPIi9ZOtXHlToAfRQPH2Cw/bGqOOocJlhN99M943uwYOpwkB4G2utZjmui
rXjneDW0uVKpawo1RisFV9QD6wHSXzJbUImJ+WSsdyUIGTBVp0goafyYyhRkbbsU
j7RsyCow8ONPmVsQG+nax8YnCjDIrNhimEdCSjKc73bvUyC66HYIUZsy9AzLaXtm
5eIxht2nbaRkjxCioId6gH8TTcEAbvy9Nnd0YAWRlTpUgou4Sg1bpr3wGgJH+FlS
YvlCl4zJ5JTapt6kxPqmePZ8BfmIW8HFvqzKFfsrH40Dk2/9SM66icDktFNUMLV4
xtGlAri/42KNrxb8DtljrICYHsYFzDp/En49it5Rl+pCoiihOWO2TjOHBEuFZcRb
1zqsA0OX/0Wh0EzkNo9sg6IfE/mtN8dfuHxDlbOSUjOqHaioIe1RIQZWjAhLLkiV
DhdTCAnSyfQnkU8TSLauVFEZ0byqr0jWASQutYJTjtKFlRcycM+sChegEVgGEVJO
H0hw4IVP0rI=
=7zfH
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 27 Feb 10:24 +0100
(name . Christopher Baines)(address . mail@cbaines.net)
871q8yl0om.fsf@gnu.org
Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (23 lines)
> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Christopher Baines <mail@cbaines.net> skribis:
>>
>>> These names should be more descriptive.
>>>
>>> * guix/store/database.scm (path-id): Rename to select-valid-path-id.
>>> (sqlite-register): Rename to register-valid-path.
>>> (register-items): Update accordingly.
>>>
>>> Change-Id: I6d4a14d4cde9d71ab34d6ffdbfbfde51b2c0e1db
>>
>> OK for ‘register-valid-path’.
>>
>> For ‘path-id’ my preference would be ‘valid-path-id’ or keeping
>> ‘path-id’; ‘select’ looks odd as nothing’s being “selected” here in the
>> non-SQL sense of the word.
>
> The main thing I'm trying to do here is make the procedures that
> directly interact with the database stand out (similar to the ! suffix
> convention). Maybe the procedure taking the db as an argument is enough,
> but to me path-id/valid-path-id look too much like record accessors.

I’d say that having an explicit ‘db’ argument is enough (similar to
procedures that take a ‘store’ argument).

Ludo’.
L
L
Ludovic Courtès wrote on 5 Mar 12:05 +0100
Re: [bug#69292] [PATCH 2/6] store: database: Remove with-statement and associated code.
(name . Reepca Russelstein)(address . reepca@russelstein.xyz)
87h6hlc519.fsf@gnu.org
Hi,

Reepca Russelstein <reepca@russelstein.xyz> skribis:

Toggle quote (16 lines)
>> I’m all for removing ‘dynamic-wind’, we’ll have to do it to make it
>> usable in a fiberized context anyway.
>>
>> I’ll let reepca comment.
>
> What is the proper fibers-friendly replacement for dynamic-wind, anyway,
> that is "like dynamic-wind, except when suspending a fiber"? It feels
> like the current interaction between dynamic-wind and fibers is more of
> an accident of how the implementation works; I don't suppose fibers could
> export a transparent replacement, like how it already exports a
> replacement for 'sleep'? Or perhaps the underlying issue is that we
> keep using 'dynamic-wind' in situations where it only makes sense to
> enter or exit the dynamic extent once? Is it time to bring
> 'unwind-protect' back into style? I see that fibers now has a
> dynamic-wind*, should that be preferred?

I’ve come to think that ‘dynamic-wind’ is a problematic abstraction.
What we really want is to perform an effect after a normal exit or when
an exception is thrown. The latter case is actually best handled with
‘with-exception-handler’ (this is what I did for ‘with-store’; see
commit 8ed597f4a261fe188de82cd1f5daed83dba948eb).

(In Cuirass I added ‘unwind-protect’ at one point but in the end it was
not so nice and I eventually removed it.)

Toggle quote (4 lines)
> I don't have a strong opinion on these changes, I just want to make sure
> we're all aware of how guile-sqlite3's sqlite-finalize acts with cached
> statements.

Agreed.

Thank you for chiming in!

Ludo’.
C
C
Christopher Baines wrote on 3 Apr 19:35 +0200
Re: [bug#69292] [PATCH 6/6] store: database: Rename a couple of procedures.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 69292@debbugs.gnu.org)
877chejqnc.fsf@cbaines.net
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (28 lines)
> Christopher Baines <mail@cbaines.net> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>
>>> Christopher Baines <mail@cbaines.net> skribis:
>>>
>>>> These names should be more descriptive.
>>>>
>>>> * guix/store/database.scm (path-id): Rename to select-valid-path-id.
>>>> (sqlite-register): Rename to register-valid-path.
>>>> (register-items): Update accordingly.
>>>>
>>>> Change-Id: I6d4a14d4cde9d71ab34d6ffdbfbfde51b2c0e1db
>>>
>>> OK for ‘register-valid-path’.
>>>
>>> For ‘path-id’ my preference would be ‘valid-path-id’ or keeping
>>> ‘path-id’; ‘select’ looks odd as nothing’s being “selected” here in the
>>> non-SQL sense of the word.
>>
>> The main thing I'm trying to do here is make the procedures that
>> directly interact with the database stand out (similar to the ! suffix
>> convention). Maybe the procedure taking the db as an argument is enough,
>> but to me path-id/valid-path-id look too much like record accessors.
>
> I’d say that having an explicit ‘db’ argument is enough (similar to
> procedures that take a ‘store’ argument).

I've changed it to valid-path-id now.
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmYNk1dfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9XesYA/9Gh9ZjhoTsFV3RktzVH2+VpeuxXu9cv/7
5WtGRkJFttYs8OQMSAQN/4s/06HbT3Rs3YIhFYRADcRUc5q8Sq9/+p+dIFrqFOeZ
4hCa91zmP36AuYR2TmhyEG5ATi34ptNgo7sieDnEAAeDwa42MAZw1NKIH2gXKmmI
DjuTXTTDJirr98m36X3mvaZzbKNL8+/j6LjYJ17+umXqq+LvVsDE4NmT26H6aGKD
4GYtB52oRiaOYswclZjVjzk6Qg4dedc8GGBTuRp0Nq30v5JageG71iLma0TlpAcN
v/ZISEikh1UNM5cpRbhQnU9gUF+ZgCvGoZYKltCx8GJJ24uRKhjFcup95Ppgv26+
ukM3Gcf0a0q+GIwxJld8gENFr1o94epRJKglV3EuzM1Yu+yzD1OEeEc8zDvctF5j
SmMjgJO+Dpgmy41OFFpaSkcgSZr+s4mGLAcAWjyDHeWypKXQU0LcX6nvfl32mhNu
7BoQPn1HshkOCEpctjF3ZrO3lM/f/8qLjbhnQmRLjliZmjhQkvZza4dluPwV795W
ukCMQh1tfaSopidEB6N0y/WtX/R/bJ6XUSh/46MG6eGAYxIHcvfaK4DMvX7liD6L
mfjY1N6eTFAXDwhmL+Rbx1IZ7mwDhUFyxwj7hczwnsD7FL4n0RXVlhmzd/PeTN5U
8V6LgltayjI=
=c+j+
-----END PGP SIGNATURE-----

C
C
Christopher Baines wrote on 3 Apr 19:36 +0200
Re: [bug#69292] [PATCH 0/6] Prepare the database code for use in the daemon
(address . 69292-done@debbugs.gnu.org)
87y19uic1n.fsf@cbaines.net
Christopher Baines <mail@cbaines.net> writes:

Toggle quote (19 lines)
> These changes prepare the database module for use in a Guile
> implementation of the guix-daemon.
>
> There's quite a few procedures that need adding, but these changes just
> adapt the existing functionality in to something that can be built on.
>
>
> Christopher Baines (6):
> store: database: Remove call-with-savepoint and associated code.
> store: database: Remove with-statement and associated code.
> store: database: Inline SQL to where it's used.
> store: database: Stop finalizing prepared statements.
> store: database: Refactor sqlite-register.
> store: database: Rename a couple of procedures.
>
> .dir-locals.el | 3 -
> guix/store/database.scm | 273 +++++++++++++++++-----------------------
> 2 files changed, 112 insertions(+), 164 deletions(-)

I've pushed these to master as c9cd16c630ccba655b93ff32fd9a99570b4f5373.

Chris
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmYNk4RfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9XdY8g//VKUlDMUdMmRLfOl0gtQLKnKhooNXS+dG
sYS4G9WNIUapvQ0Umgp60zvrGJaAZGIR60Uk4dv1I1rGRCkUUhpNIq2bwIJC3qlN
OwqNcyrV/7RnWx65ZP/FHgMjk83aKURdA2CQxo/dQVWz1wQoPm/EOwr3uBuklcrd
UxoSQh7v9a1eyAZmeu/46OszpbfCDADlrbwM+ZZElM92pQ6Oqe3N4Z0PkhrhonL3
wn6/hBb+bi0sPps0M+GfFYelTvGFZnEjcJHt6SNGKe235sbUPg6a1UPMZa60M2x8
cJtSmggGtbWzfP7DM0xIrEizoGD1jp/yoxW43zS2LPy+6iap5WFErc+N1XJLk2Sm
WUW+HJbPDYTvrKYqotcEjujs66/2IRc9THivQoTKaiUshuLox/LFAsySzpHTmTEL
lde6IgEnmNW+zLX5j8c9JTcrD39sW0pXCs6fsY1DwgF6AggZLFxEU+130x5fYaGc
1AdrykTeJsVY6W/404stkVwIkqo3qVdaKxiaKOkbMOyoiUj23ing2NqkT9Mh3LE2
25t5gvmZQ2/7vzu7rIoez0PDF6RPS0c4veXk2MguBZFuD1gpIhk8wlZFRwU/qSVc
udvM/uCozv6Ve7va07iTsBAsSeo9Zs1afWvXmtQpkSAK3Db2r6jkklzJeHW59JEf
ieKYlPAHQHo=
=rYh8
-----END PGP SIGNATURE-----

Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 69292@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 69292
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch