GNU bug report logs - #69292
[PATCH 0/6] Prepare the database code for use in the daemon

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 69292 in the body.
You can then email your comments to 69292 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: guix-patches <at> gnu.org
Subject: [PATCH 0/6] Prepare the database code for use in the daemon
Date: Tue, 20 Feb 2024 19:20:45 +0000

[Message part 1 (text/plain, inline)]

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

[signature.asc (application/pgp-signature, inline)]

Message #8 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 69292 <at> debbugs.gnu.org
Subject: [PATCH 4/6] store: database: Stop finalizing prepared statements.
Date: Tue, 20 Feb 2024 19:39:04 +0000

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(-)

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

Message #11 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 69292 <at> debbugs.gnu.org
Subject: [PATCH 5/6] store: database: Refactor sqlite-register.
Date: Tue, 20 Feb 2024 19:39:05 +0000

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(-)

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

Message #14 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 69292 <at> debbugs.gnu.org
Subject: [PATCH 2/6] store: database: Remove with-statement and associated
 code.
Date: Tue, 20 Feb 2024 19:39:02 +0000

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(-)

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

Message #17 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 69292 <at> debbugs.gnu.org
Subject: [PATCH 1/6] store: database: Remove call-with-savepoint and
 associated code.
Date: Tue, 20 Feb 2024 19:39:01 +0000

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(-)

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

Message #20 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 69292 <at> debbugs.gnu.org
Subject: [PATCH 3/6] store: database: Inline SQL to where it's used.
Date: Tue, 20 Feb 2024 19:39:03 +0000

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(-)

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

Message #23 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 69292 <at> debbugs.gnu.org
Subject: [PATCH 6/6] store: database: Rename a couple of procedures.
Date: Tue, 20 Feb 2024 19:39:06 +0000

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(-)

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

Message #26 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: reepca <at> russelstein.xyz
To: Christopher Baines <mail <at> cbaines.net>
Cc: 69292 <at> debbugs.gnu.org
Subject: Re: [PATCH 4/6] store: database: Stop finalizing prepared statements.
Date: Thu, 22 Feb 2024 06:39:37 -0600

[Message part 1 (text/plain, inline)]

Christopher Baines <mail <at> 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.

- reepca

[signature.asc (application/pgp-signature, inline)]

Message #29 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 Caleb Ristvedt <reepca <at> russelstein.xyz>, 69292 <at> debbugs.gnu.org,
 Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#69292] [PATCH 2/6] store: database: Remove with-statement
 and associated code.
Date: Fri, 23 Feb 2024 17:35:03 +0100

Christopher Baines <mail <at> cbaines.net> skribis:

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

Message #32 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 69292 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#69292] [PATCH 5/6] store: database: Refactor sqlite-register.
Date: Fri, 23 Feb 2024 17:33:13 +0100

Christopher Baines <mail <at> cbaines.net> skribis:

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

Message #35 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 69292 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#69292] [PATCH 1/6] store: database: Remove
 call-with-savepoint and associated code.
Date: Fri, 23 Feb 2024 17:31:20 +0100

Christopher Baines <mail <at> cbaines.net> skribis:

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

Message #38 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 69292 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#69292] [PATCH 6/6] store: database: Rename a couple of
 procedures.
Date: Fri, 23 Feb 2024 17:43:11 +0100

Christopher Baines <mail <at> 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.

Ludo’.

Message #41 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 69292 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#69292] [PATCH 3/6] store: database: Inline SQL to where
 it's used.
Date: Fri, 23 Feb 2024 17:40:29 +0100

Christopher Baines <mail <at> cbaines.net> skribis:

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

>    (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:

--8<---------------cut here---------------start------------->8---
(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 ...))
--8<---------------cut here---------------end--------------->8---

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

Message #44 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Reepca Russelstein <reepca <at> russelstein.xyz>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Tobias Geerinckx-Rice <me <at> tobias.gr>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Christopher Baines <mail <at> cbaines.net>, Ricardo Wurmus <rekado <at> elephly.net>,
 69292 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#69292] [PATCH 2/6] store: database: Remove with-statement
 and associated code.
Date: Fri, 23 Feb 2024 12:32:03 -0600

[Message part 1 (text/plain, inline)]

>> 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#7 with this
much more magnificent Cc list.

>> 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
https://www.sqlite.org/c3ref/close.html it would just hold off on
actually closing the database until all statements have been finalized.
So I guess that works.

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

[signature.asc (application/pgp-signature, inline)]

Message #47 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: reepca <at> russelstein.xyz
Cc: 69292 <at> debbugs.gnu.org
Subject: Re: [PATCH 4/6] store: database: Stop finalizing prepared statements.
Date: Mon, 26 Feb 2024 10:50:08 +0000

[Message part 1 (text/plain, inline)]

reepca <at> russelstein.xyz writes:

> Christopher Baines <mail <at> 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.

[signature.asc (application/pgp-signature, inline)]

Message #50 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 69292 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#69292] [PATCH 6/6] store: database: Rename a couple of
 procedures.
Date: Mon, 26 Feb 2024 11:03:04 +0000

[Message part 1 (text/plain, inline)]

Ludovic Courtès <ludo <at> gnu.org> writes:

> Christopher Baines <mail <at> 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.

[signature.asc (application/pgp-signature, inline)]

Message #53 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 69292 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#69292] [PATCH 6/6] store: database: Rename a couple of
 procedures.
Date: Tue, 27 Feb 2024 10:24:25 +0100

Christopher Baines <mail <at> cbaines.net> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:
>
>> Christopher Baines <mail <at> 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’.

Message #56 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Reepca Russelstein <reepca <at> russelstein.xyz>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Christopher Baines <mail <at> cbaines.net>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 69292 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#69292] [PATCH 2/6] store: database: Remove with-statement
 and associated code.
Date: Tue, 05 Mar 2024 12:05:38 +0100

Hi,

Reepca Russelstein <reepca <at> russelstein.xyz> skribis:

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

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

Message #59 received at 69292 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 69292 <at> debbugs.gnu.org
Subject: Re: [bug#69292] [PATCH 6/6] store: database: Rename a couple of
 procedures.
Date: Wed, 03 Apr 2024 18:35:19 +0100

[Message part 1 (text/plain, inline)]

Ludovic Courtès <ludo <at> gnu.org> writes:

> Christopher Baines <mail <at> cbaines.net> skribis:
>
>> Ludovic Courtès <ludo <at> gnu.org> writes:
>>
>>> Christopher Baines <mail <at> 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.

[signature.asc (application/pgp-signature, inline)]

Message #64 received at 69292-done <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 69292-done <at> debbugs.gnu.org
Subject: Re: [bug#69292] [PATCH 0/6] Prepare the database code for use in
 the daemon
Date: Wed, 03 Apr 2024 18:36:04 +0100

[Message part 1 (text/plain, inline)]

Christopher Baines <mail <at> cbaines.net> writes:

> 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

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.