* Cuirass news
@ 2018-01-24 22:12 Ludovic Courtès
2018-01-25 10:55 ` Mathieu Othacehe
` (4 more replies)
0 siblings, 5 replies; 42+ messages in thread
From: Ludovic Courtès @ 2018-01-24 22:12 UTC (permalink / raw)
To: guix-devel
Hello Guix!
Over the last few days, out of frustration ;-), I hacked Cuirass to
improve several things:
• Logging is improved: useful events are logged, including build
started/succeeded/failed (using a variant of what I proposed in the
Guix ‘wip-ui’ branch). This makes it much easier to understand
what’s going on!
• Concurrency: the evaluate/build cycle would previously happen
sequentially. This was bad for latency (it could take a while
before we would pull from the repo) and for resource usage (while
evaluating all the build machines would be idle). Now there’s some
concurrency, with a switch to Fibers (yay!), which means that these
activities can be interleaved, as well as HTTP request processing.
• Build status: the starttime/stoptime of a ‘Build’ entry in the
database is now accurate since we set them once the build has
actually started/stopped. This means the database is updated as
soon as a build finishes, rather than when the whole
‘build-derivations’ RPC has returned. We can also distinguish
between started and scheduled builds now.
• HTTP API: as a corollary, /api/latestbuilds now really returns the
latest builds. There’s a new /api/queue that returns pending
builds—builds that have a database entry with ‘status’ < 0.
• Restarting unfinished builds: it’s common, especially when testing,
to interrupt Cuirass, leaving a number of builds unfinished or not
even started. Now Cuirass restarts those upon startup.
And! This brings a whole set of new bugs that I’m hunting notably on
berlin (which may thus lag behind…). Overall I think it’ll make Cuirass
easier to work with and more “introspectable”.
Feedback welcome!
Ludo’.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: Cuirass news 2018-01-24 22:12 Cuirass news Ludovic Courtès @ 2018-01-25 10:55 ` Mathieu Othacehe 2018-01-25 10:59 ` Mathieu Othacehe 2018-01-25 21:06 ` Ricardo Wurmus ` (3 subsequent siblings) 4 siblings, 1 reply; 42+ messages in thread From: Mathieu Othacehe @ 2018-01-25 10:55 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel Hey Ludo, > And! This brings a whole set of new bugs that I’m hunting notably on > berlin (which may thus lag behind…). Overall I think it’ll make Cuirass > easier to work with and more “introspectable”. I went through your recent Cuirass commits and it seems awesome. I'll try yo update Cuirass on my build server and give you some feedback. Thanks, Mathieu ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-25 10:55 ` Mathieu Othacehe @ 2018-01-25 10:59 ` Mathieu Othacehe 2018-01-25 13:09 ` Ludovic Courtès 0 siblings, 1 reply; 42+ messages in thread From: Mathieu Othacehe @ 2018-01-25 10:59 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel BTW, when I try to access berlin's Cuirass HTTP API, I have the following error: --8<---------------cut here---------------start------------->8--- curl -i -H "Accept: application/json" http://berlin.guixsd.org/api/latestbuilds?nr=10 HTTP/1.1 502 Bad Gateway Server: nginx/1.12.2 Date: Thu, 25 Jan 2018 12:55:20 GMT Content-Type: text/html Content-Length: 173 Connection: keep-alive <html> <head><title>502 Bad Gateway</title></head> <body bgcolor="white"> <center><h1>502 Bad Gateway</h1></center> <hr><center>nginx/1.12.2</center> </body> </html> --8<---------------cut here---------------end--------------->8--- Mathieu ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-25 10:59 ` Mathieu Othacehe @ 2018-01-25 13:09 ` Ludovic Courtès 2018-01-26 14:30 ` Danny Milosavljevic 0 siblings, 1 reply; 42+ messages in thread From: Ludovic Courtès @ 2018-01-25 13:09 UTC (permalink / raw) To: Mathieu Othacehe; +Cc: guix-devel Mathieu Othacehe <m.othacehe@gmail.com> skribis: > BTW, when I try to access berlin's Cuirass HTTP API, I have the following > error: Yeah, a few things are still brittle and I’m starting/stopping Cuirass on berlin quite frequently. Don’t upgrade your server yet. :-) Ludo’. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-25 13:09 ` Ludovic Courtès @ 2018-01-26 14:30 ` Danny Milosavljevic 2018-01-27 16:01 ` Ludovic Courtès 0 siblings, 1 reply; 42+ messages in thread From: Danny Milosavljevic @ 2018-01-26 14:30 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel [-- Attachment #1: Type: text/plain, Size: 415 bytes --] Hi Ludo, I saw that (cuirass database) has some problems with sql injection. I defused it a little, see attached patch. The idea is that sqlite-exec uses sqlite-bind to pass arguments rather than formatting them on its own. While we are at it, we can also reuse prepared statements (using the sqltext as key to find the right one). I also monitor sqlite accesses now - maybe that's overkill (see "with-mutex"). [-- Attachment #2: 0001-database-Make-sqlite-exec-reuse-the-prepared-stateme.patch --] [-- Type: text/x-patch, Size: 11757 bytes --] From b8fdd9c4e3a11f11c8d948ee07b2003fa4981f81 Mon Sep 17 00:00:00 2001 From: Danny Milosavljevic <dannym@scratchpost.org> Date: Fri, 26 Jan 2018 15:16:04 +0100 Subject: [PATCH] database: Make 'sqlite-exec' reuse the prepared statement. Tags: patch * src/cuirass/database.scm (%sqlite-exec): Delete variable. (<db>): New variable. (%wrap-db): New variable. (%sqlite-prepare): New variable. (%sqlite-bind-args): New variable. (%sqlite-fetch-all): New variable. (sqlite-exec): Modify. (db-init): Use %wrap-db. (db-open): Use %wrap-db. (db-close): Modify. (db-add-specification): Adjust for prepared statement parameters. (db-get-specifications): Adjust for prepared statement parameters. (db-add-derivation): Adjust for prepared statement parameters. (db-get-derivation): Adjust for prepared statement parameters. (db-add-evaluation): Adjust for prepared statement parameters. (db-add-build): Adjust for prepared statement parameters. (db-update-build-status!): Adjust for prepared statement parameters. (db-get-build): Adjust for prepared statement parameters. (db-get-builds): Adjust for prepared statement parameters. (db-get-stamp): Adjust for prepared statement parameters. (db-add-stamp): Adjust for prepared statement parameters. --- src/cuirass/database.scm | 125 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 86 insertions(+), 39 deletions(-) diff --git a/src/cuirass/database.scm b/src/cuirass/database.scm index f1d0118..2c923ec 100644 --- a/src/cuirass/database.scm +++ b/src/cuirass/database.scm @@ -24,8 +24,11 @@ #:use-module (ice-9 match) #:use-module (ice-9 format) #:use-module (ice-9 rdelim) + #:use-module (ice-9 threads) #:use-module (srfi srfi-1) + #:use-module (srfi srfi-9) #:use-module (srfi srfi-19) + #:use-module (srfi srfi-69) #:use-module (sqlite3) #:export (;; Procedures. assq-refs @@ -53,28 +56,68 @@ ;; Macros. with-database)) -(define (%sqlite-exec db sql) - (let* ((stmt (sqlite-prepare db sql)) - (res (let loop ((res '())) - (let ((row (sqlite-step stmt))) - (if (not row) - (reverse! res) - (loop (cons row res))))))) - (sqlite-finalize stmt) - res)) +(define-record-type <db> + (db native-db lock stmts) + db? + (native-db db-native-db) + (lock db-lock) + (stmts db-stmts)) + +(define (%wrap-db native-db) + (db native-db (make-mutex) (make-weak-key-hash-table))) + +(define (%sqlite-prepare db sqlsym sqltext) + (with-mutex (db-lock db) + (let ((stmt (sqlite-prepare (db-native-db db) sqltext))) + (hashq-set! (db-stmts db) sqlsym stmt) + stmt))) + +(define (%sqlite-bind-args stmt args) + (let ((argsi (zip (iota (length args)) args))) + (for-each (match-lambda ((i arg) + (sqlite-bind stmt (1+ i) arg))) + argsi))) + +(define (%sqlite-fetch-all stmt) + (let loop ((res '())) + (let ((row (sqlite-step stmt))) + (if (not row) + (begin + (sqlite-reset stmt) + (reverse! res)) + (loop (cons row res)))))) (define-syntax sqlite-exec - ;; Note: Making it a macro so -Wformat can do its job. (lambda (s) - "Wrap 'sqlite-prepare', 'sqlite-step', and 'sqlite-finalize'. Send to given -SQL statement to DB. FMT and ARGS are passed to 'format'." (syntax-case s () - ((_ db fmt args ...) - #'(%sqlite-exec db (format #f fmt args ...))) - (id - (identifier? #'id) - #'(lambda (db fmt . args) - (%sqlite-exec db (apply format #f fmt args))))))) + ((_ db sqltext arg ...) (string? (syntax->datum #'sqltext)) + #`(let* ((sqlsym (quote #,(datum->syntax #'here (string->symbol (string-trim (syntax->datum #'sqltext)))))) + (stmt (or (hashq-ref (db-stmts db) sqlsym) + (%sqlite-prepare db sqlsym sqltext)))) + (with-mutex (db-lock db) + (%sqlite-bind-args stmt (list arg ...)) + (%sqlite-fetch-all stmt)))) + ((_ db sqltext) (string? (syntax->datum #'sqltext)) + #`(let* ((sqlsym (quote #,(datum->syntax #'here (string->symbol (string-trim (syntax->datum #'sqltext)))))) + (stmt (or (hashq-ref (db-stmts db) sqlsym) + (%sqlite-prepare db sqlsym sqltext)))) + (with-mutex (db-lock db) + (%sqlite-fetch-all stmt)))) + ((_ db sqltext arg ...) + #`(with-mutex (db-lock db) + (let ((stmt (sqlite-prepare (db-native-db db) sqltext))) + (%sqlite-bind-args stmt (list arg ...)) + (let ((result (%sqlite-fetch-all stmt))) + (sqlite-finalize stmt) + result)))) + (id (identifier? #'id) + #'(lambda (db sqltext . args) + (with-mutex (db-lock db) + (let ((stmt (sqlite-prepare (db-native-db db) sqltext))) + (%sqlite-bind-args stmt args) + (let ((result (%sqlite-fetch-all stmt))) + (sqlite-finalize stmt) + result)))))))) (define %package-database ;; Define to the database file name of this package. @@ -106,8 +149,8 @@ database object." (when (file-exists? db-name) (format (current-error-port) "Removing leftover database ~a~%" db-name) (delete-file db-name)) - (let ((db (sqlite-open db-name (logior SQLITE_OPEN_CREATE - SQLITE_OPEN_READWRITE)))) + (let ((db (%wrap-db (sqlite-open db-name (logior SQLITE_OPEN_CREATE + SQLITE_OPEN_READWRITE))))) (for-each (lambda (sql) (sqlite-exec db sql)) (read-sql-file schema)) db)) @@ -116,12 +159,12 @@ database object." "Open database to store or read jobs and builds informations. Return a database object." (if (file-exists? db) - (sqlite-open db SQLITE_OPEN_READWRITE) + (%wrap-db (sqlite-open db SQLITE_OPEN_READWRITE)) (db-init db))) (define (db-close db) "Close database object DB." - (sqlite-close db)) + (sqlite-close (db-native-db db))) (define* (assq-refs alst keys #:optional default-value) (map (lambda (key) (or (assq-ref alst key) default-value)) @@ -136,9 +179,13 @@ database object." (apply sqlite-exec db "\ INSERT OR IGNORE INTO Specifications (repo_name, url, load_path, file, \ proc, arguments, branch, tag, revision, no_compile_p) \ - VALUES ('~A', '~A', '~A', '~A', '~S', '~S', '~A', '~A', '~A', ~A);" + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?);" (append - (assq-refs spec '(#:name #:url #:load-path #:file #:proc #:arguments)) + (assq-refs spec '(#:name #:url #:load-path #:file)) + (map symbol->string (assq-refs spec '(#:proc))) + (map (lambda (e) + (format #f "~A" e)) + (assq-refs spec '(#:arguments))) (assq-refs spec '(#:branch #:tag #:commit) "NULL") (list (if (assq-ref spec #:no-compile?) "1" "0")))) (last-insert-rowid db)) @@ -167,7 +214,7 @@ INSERT OR IGNORE INTO Specifications (repo_name, url, load_path, file, \ "Store a derivation result in database DB and return its ID." (sqlite-exec db "\ INSERT OR IGNORE INTO Derivations (derivation, job_name, system, nix_name, evaluation)\ - VALUES ('~A', '~A', '~A', '~A', '~A');" + VALUES (?, ?, ?, ?, ?);" (assq-ref job #:derivation) (assq-ref job #:job-name) (assq-ref job #:system) @@ -176,11 +223,11 @@ INSERT OR IGNORE INTO Derivations (derivation, job_name, system, nix_name, evalu (define (db-get-derivation db id) "Retrieve a job in database DB which corresponds to ID." - (car (sqlite-exec db "SELECT * FROM Derivations WHERE derivation='~A';" id))) + (car (sqlite-exec db "SELECT * FROM Derivations WHERE derivation=?;" id))) (define (db-add-evaluation db eval) (sqlite-exec db "\ -INSERT INTO Evaluations (specification, revision) VALUES ('~A', '~A');" +INSERT INTO Evaluations (specification, revision) VALUES (?, ?);" (assq-ref eval #:specification) (assq-ref eval #:revision)) (last-insert-rowid db)) @@ -227,7 +274,7 @@ in the OUTPUTS table." (let* ((build-exec (sqlite-exec db "\ INSERT INTO Builds (derivation, evaluation, log, status, timestamp, starttime, stoptime)\ - VALUES ('~A', '~A', '~A', '~A', '~A', '~A', '~A');" + VALUES (?, ?, ?, ?, ?, ?, ?);" (assq-ref build #:derivation) (assq-ref build #:eval-id) (assq-ref build #:log) @@ -241,7 +288,7 @@ INSERT INTO Builds (derivation, evaluation, log, status, timestamp, starttime, s (match output ((name . path) (sqlite-exec db "\ -INSERT INTO Outputs (build, name, path) VALUES ('~A', '~A', '~A');" +INSERT INTO Outputs (build, name, path) VALUES (?, ?, ?);" build-id name path)))) (assq-ref build #:outputs)) build-id)) @@ -254,17 +301,17 @@ log file for DRV." (time-second (current-time time-utc))) (if (= status (build-status started)) - (sqlite-exec db "UPDATE Builds SET starttime='~A', status='~A' \ -WHERE derivation='~A';" + (sqlite-exec db "UPDATE Builds SET starttime=?, status=? \ +WHERE derivation=?;" now status drv) - (sqlite-exec db "UPDATE Builds SET stoptime='~A', \ -status='~A'~@[, log='~A'~] WHERE derivation='~A';" - now status log-file drv))) + (if log-file + (sqlite-exec db "UPDATE Builds SET stoptime=?, status=?, log=? WHERE derivation=?;" now status log-file drv) + (sqlite-exec db "UPDATE Builds SET stoptime=?, status=? WHERE derivation=?;" now status drv)))) (define (db-get-outputs db build-id) "Retrieve the OUTPUTS of the build identified by BUILD-ID in DB database." (let loop ((rows - (sqlite-exec db "SELECT name, path FROM Outputs WHERE build='~A';" + (sqlite-exec db "SELECT name, path FROM Outputs WHERE build=?;" build-id)) (outputs '())) (match rows @@ -305,7 +352,7 @@ INNER JOIN Specifications ON Evaluations.specification = Specifications.repo_nam (define (db-get-build db id) "Retrieve a build in database DB which corresponds to ID." (let ((res (sqlite-exec db (string-append db-build-request - " WHERE Builds.id='~A';") id))) + " WHERE Builds.id=?;") id))) (match res ((build) (db-format-build db build)) @@ -385,7 +432,7 @@ FILTERS is an assoc list which possible keys are 'project | 'jobset | 'job | (define (db-get-stamp db spec) "Return a stamp corresponding to specification SPEC in database DB." - (let ((res (sqlite-exec db "SELECT * FROM Stamps WHERE specification='~A';" + (let ((res (sqlite-exec db "SELECT * FROM Stamps WHERE specification=?;" (assq-ref spec #:name)))) (match res (() "") @@ -395,10 +442,10 @@ FILTERS is an assoc list which possible keys are 'project | 'jobset | 'job | "Associate stamp COMMIT to specification SPEC in database DB." (if (string-null? (db-get-stamp db spec)) (sqlite-exec db "\ -INSERT INTO Stamps (specification, stamp) VALUES ('~A', '~A');" +INSERT INTO Stamps (specification, stamp) VALUES (?, ?);" (assq-ref spec #:name) commit) (sqlite-exec db "\ -UPDATE Stamps SET stamp='~A' WHERE specification='~A';" +UPDATE Stamps SET stamp=? WHERE specification=?;" commit (assq-ref spec #:name)))) ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-26 14:30 ` Danny Milosavljevic @ 2018-01-27 16:01 ` Ludovic Courtès 2018-01-27 17:18 ` Danny Milosavljevic 0 siblings, 1 reply; 42+ messages in thread From: Ludovic Courtès @ 2018-01-27 16:01 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel Hi Danny, Danny Milosavljevic <dannym@scratchpost.org> skribis: > I saw that (cuirass database) has some problems with sql injection. > I defused it a little, see attached patch. Yes, I was unhappy with that, glad you fixed it. :-) > While we are at it, we can also reuse prepared statements (using the > sqltext as key to find the right one). Indeed, good idea! > I also monitor sqlite accesses now - maybe that's overkill (see "with-mutex"). We don’t need mutexes: a given <db> is only ever used from one thread at a time. Sometimes we have several threads accessing the database, but they do so through different handlers, which SQLite handles correctly. Some comments: > From b8fdd9c4e3a11f11c8d948ee07b2003fa4981f81 Mon Sep 17 00:00:00 2001 > From: Danny Milosavljevic <dannym@scratchpost.org> > Date: Fri, 26 Jan 2018 15:16:04 +0100 > Subject: [PATCH] database: Make 'sqlite-exec' reuse the prepared statement. > Tags: patch > > * src/cuirass/database.scm (%sqlite-exec): Delete variable. > (<db>): New variable. > (%wrap-db): New variable. > (%sqlite-prepare): New variable. > (%sqlite-bind-args): New variable. > (%sqlite-fetch-all): New variable. > (sqlite-exec): Modify. > (db-init): Use %wrap-db. > (db-open): Use %wrap-db. > (db-close): Modify. > (db-add-specification): Adjust for prepared statement parameters. > (db-get-specifications): Adjust for prepared statement parameters. > (db-add-derivation): Adjust for prepared statement parameters. > (db-get-derivation): Adjust for prepared statement parameters. > (db-add-evaluation): Adjust for prepared statement parameters. > (db-add-build): Adjust for prepared statement parameters. > (db-update-build-status!): Adjust for prepared statement parameters. > (db-get-build): Adjust for prepared statement parameters. > (db-get-builds): Adjust for prepared statement parameters. > (db-get-stamp): Adjust for prepared statement parameters. > (db-add-stamp): Adjust for prepared statement parameters. [...] > +(define (%wrap-db native-db) > + (db native-db (make-mutex) (make-weak-key-hash-table))) > + > +(define (%sqlite-prepare db sqlsym sqltext) > + (with-mutex (db-lock db) > + (let ((stmt (sqlite-prepare (db-native-db db) sqltext))) > + (hashq-set! (db-stmts db) sqlsym stmt) > + stmt))) I’m not sure what ‘sqlsym’ is. Apparently it’s a symbol derived from the SQL statement, right? I don’t think it’s necessary. Instead you can simply make that hash table a regular (non-weak) hash table that maps strings (SQL text) to prepared statements. You’d also need to use ‘hash-set!’ and ‘hash-ref’ instead of ‘hashq-set!’ and ‘hash-ref’ since strings should be compared with ‘equal?’, not ‘eq?’. However, could the hash table grow indefinitely if there are always new statements prepared? > +(define (%sqlite-bind-args stmt args) > + (let ((argsi (zip (iota (length args)) args))) > + (for-each (match-lambda ((i arg) > + (sqlite-bind stmt (1+ i) arg))) > + argsi))) You can make it (note the indentation of ‘match-lambda’): (for-each (match-lambda ((i arg) (sqlite-bind stmt (1+ i) arg))) (iota (length args)) args) > (define-syntax sqlite-exec > - ;; Note: Making it a macro so -Wformat can do its job. > (lambda (s) > - "Wrap 'sqlite-prepare', 'sqlite-step', and 'sqlite-finalize'. Send to given > -SQL statement to DB. FMT and ARGS are passed to 'format'." > (syntax-case s () > - ((_ db fmt args ...) > - #'(%sqlite-exec db (format #f fmt args ...))) > - (id > - (identifier? #'id) > - #'(lambda (db fmt . args) > - (%sqlite-exec db (apply format #f fmt args))))))) > + ((_ db sqltext arg ...) (string? (syntax->datum #'sqltext)) > + #`(let* ((sqlsym (quote #,(datum->syntax #'here (string->symbol (string-trim (syntax->datum #'sqltext)))))) > + (stmt (or (hashq-ref (db-stmts db) sqlsym) > + (%sqlite-prepare db sqlsym sqltext)))) > + (with-mutex (db-lock db) > + (%sqlite-bind-args stmt (list arg ...)) > + (%sqlite-fetch-all stmt)))) I think we can turn ‘sqlite-exec’ back into a procedure. The only reason to make it a macro was to have -Wformat support, as noted in the comment. Otherwise LGTM. Could you prepare an updated patch to address these and to remove the mutex? Thank you! Ludo’. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-27 16:01 ` Ludovic Courtès @ 2018-01-27 17:18 ` Danny Milosavljevic 2018-01-27 19:12 ` Danny Milosavljevic ` (2 more replies) 0 siblings, 3 replies; 42+ messages in thread From: Danny Milosavljevic @ 2018-01-27 17:18 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel Hi Ludo, > Yes, I was unhappy with that, glad you fixed it. :-) I've now forked guile-sqlite3 and put the stuff that makes sense for a low-level binding there (not sqlite-exec - that would still be in guix-cuirass). These are: - stmts hash-table is now in guile-sqlite3's <db>. - sqlite-finalize adapted to update <db>'s stmts list - sqlite-finalize* variant which doesn't update <db>'s stmts list (because the updating by value is slow) - sqlite-prepare* macro which stores and reuses existing <stmt>s. - sqlite-bind-args convenience function which just binds all the parameters in sequence. - SQLITE_CONSTRAINT and SQLITE_CONSTRAINT_PRIMARYKEY, moved from guix-cuirass https://notabug.org/dannym/guile-sqlite3 If that's OK I'll replace the reference in guix-master, or we could do a pull request to the civodul repository. diff --git a/gnu/packages/guile.scm b/gnu/packages/guile.scm index 50b7170ee..069009336 100644 --- a/gnu/packages/guile.scm +++ b/gnu/packages/guile.scm @@ -1114,7 +1114,7 @@ Guile's foreign function interface.") (deprecated-package "guile2.2-gdbm-ffi" guile-gdbm-ffi)) (define-public guile-sqlite3 - (let ((commit "3ac2ee190937a236eb374230606bdce0cfac2992")) + (let ((commit "12348a0f58821e10ec571c8595064fc31964a307")) (package (name "guile-sqlite3") (version (string-append "0.0-1." (string-take commit 7))) @@ -1130,7 +1130,7 @@ Guile's foreign function interface.") (commit commit))) (sha256 (base32 - "011yy4435c2z6cmyfcjdailj6gsnl9cv84kp971nijskpk09lbk3")) + "1qisy7pf3amfzipkk38b7jylzdvl6951wkn6bazs8i2kzc90y48f")) (file-name (string-append name "-" version "-checkout")) (modules '((guix build utils))) (snippet > We don’t need mutexes: a given <db> is only ever used from one thread at > a time. Oh okay, then we can even disable the mutexes on the sqlite side (there's a NOMUTEX flag on sqlite-open). > I’m not sure what ‘sqlsym’ is. Apparently it’s a symbol derived from > the SQL statement, right? I don’t think it’s necessary. Yes, the idea is to prevent it from having to do string comparison/hashing to find an existing stmt. It's interned at macro-expansion time - so at runtime it shouldn't traverse the sql text after the stmt is created. > Instead you can simply make that hash table a regular (non-weak) hash > table that maps strings (SQL text) to prepared statements. You’d also > need to use ‘hash-set!’ and ‘hash-ref’ instead of ‘hashq-set!’ and > ‘hash-ref’ since strings should be compared with ‘equal?’, not ‘eq?’. That's what I tried to avoid, having it compare 80 character strings every time I want to reuse a prepared statement. :-) What I wanted more is something like a new thread-local variable being declared every time a new SQL statement is used, otherwise the existing variable being used. In the analogy, the variable would be named like the SQL text and reused if the text is the same. It's like manually having (let ((select-*-from-a (stmt "select * from a")) (select-*-from-b (stmt "select * from b"))) ... your program) > However, could the hash table grow indefinitely if there are always new > statements prepared? That's why the macro has a special case only for string literals. If it's a non-literal string, it will not hash it and it will not reuse the prepared statement either. > (for-each (match-lambda > ((i arg) > (sqlite-bind stmt (1+ i) arg))) > (iota (length args)) > args) Nice! > I think we can turn ‘sqlite-exec’ back into a procedure. The only > reason to make it a macro was to have -Wformat support, as noted in the > comment. The reason is now different ;-) > Could you prepare an updated patch to address these and to remove the > mutex? Removed mutex. If possible, let's put guile-sqlite3 stuff there - otherwise there will be xtra records around (I've pushed those to my fork already). Corresponding changes for guix-cuirass, starting from commit 4558d1c86914e2427fc99afbe00c28cb716dbd3d: diff --git a/src/cuirass/database.scm b/src/cuirass/database.scm index 5ca3ad3..b3b1674 100644 --- a/src/cuirass/database.scm +++ b/src/cuirass/database.scm @@ -46,35 +46,38 @@ db-get-builds read-sql-file read-quoted-string - sqlite-exec + sqlite-exec ; for tests only ;; Parameters. %package-database %package-schema-file ;; Macros. with-database)) -(define (%sqlite-exec db sql) - (let* ((stmt (sqlite-prepare db sql)) - (res (let loop ((res '())) - (let ((row (sqlite-step stmt))) - (if (not row) - (reverse! res) - (loop (cons row res))))))) - (sqlite-finalize stmt) - res)) +(define (sqlite-fetch-all stmt) + (reverse! (sqlite-fold cons '() stmt))) (define-syntax sqlite-exec - ;; Note: Making it a macro so -Wformat can do its job. (lambda (s) - "Wrap 'sqlite-prepare', 'sqlite-step', and 'sqlite-finalize'. Send to given -SQL statement to DB. FMT and ARGS are passed to 'format'." (syntax-case s () - ((_ db fmt args ...) - #'(%sqlite-exec db (format #f fmt args ...))) - (id - (identifier? #'id) - #'(lambda (db fmt . args) - (%sqlite-exec db (apply format #f fmt args))))))) + ((_ db sqltext arg ...) (string? (syntax->datum #'sqltext)) + #`(let* ((stmt (sqlite-prepare* db sqltext arg ...))) + (sqlite-fetch-all stmt))) + ((_ db sqltext) (string? (syntax->datum #'sqltext)) + #`(let* ((stmt (sqlite-prepare* db sqltext))) + (sqlite-fetch-all stmt))) + ((_ db sqltext arg ...) + #`(let ((stmt (sqlite-prepare db sqltext))) + (sqlite-bind-args stmt (list arg ...)) + (let ((result (sqlite-fetch-all stmt))) + (sqlite-finalize* stmt) + result))) + (id (identifier? #'id) + #'(lambda (db sqltext . args) + (let ((stmt (sqlite-prepare db sqltext))) + (sqlite-bind-args stmt args) + (let ((result (sqlite-fetch-all stmt))) + (sqlite-finalize* stmt) + result))))))) (define %package-database ;; Define to the database file name of this package. @@ -144,9 +147,11 @@ database object." (apply sqlite-exec db "\ INSERT OR IGNORE INTO Specifications (repo_name, url, load_path, file, \ proc, arguments, branch, tag, revision, no_compile_p) \ - VALUES ('~A', '~A', '~A', '~A', '~S', '~S', '~A', '~A', '~A', ~A);" + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?);" (append - (assq-refs spec '(#:name #:url #:load-path #:file #:proc #:arguments)) + (assq-refs spec '(#:name #:url #:load-path #:file)) + (map symbol->string (assq-refs spec '(#:proc))) + (map object->string (assq-refs spec '(#:arguments))) (assq-refs spec '(#:branch #:tag #:commit) "NULL") (list (if (assq-ref spec #:no-compile?) "1" "0")))) (last-insert-rowid db)) @@ -175,20 +180,21 @@ INSERT OR IGNORE INTO Specifications (repo_name, url, load_path, file, \ "Store a derivation result in database DB and return its ID." (sqlite-exec db "\ INSERT OR IGNORE INTO Derivations (derivation, job_name, system, nix_name, evaluation)\ - VALUES ('~A', '~A', '~A', '~A', '~A');" + VALUES (?, ?, ?, ?, ?);" (assq-ref job #:derivation) (assq-ref job #:job-name) (assq-ref job #:system) (assq-ref job #:nix-name) - (assq-ref job #:eval-id))) + (assq-ref job #:eval-id)) + (last-insert-rowid db)) (define (db-get-derivation db id) "Retrieve a job in database DB which corresponds to ID." - (car (sqlite-exec db "SELECT * FROM Derivations WHERE derivation='~A';" id))) + (car (sqlite-exec db "SELECT * FROM Derivations WHERE derivation=?;" id))) (define (db-add-evaluation db eval) (sqlite-exec db "\ -INSERT INTO Evaluations (specification, revision) VALUES ('~A', '~A');" +INSERT INTO Evaluations (specification, revision) VALUES (?, ?);" (assq-ref eval #:specification) (assq-ref eval #:revision)) (last-insert-rowid db)) @@ -235,7 +241,7 @@ in the OUTPUTS table." (let* ((build-exec (sqlite-exec db "\ INSERT INTO Builds (derivation, evaluation, log, status, timestamp, starttime, stoptime)\ - VALUES ('~A', '~A', '~A', '~A', '~A', '~A', '~A');" + VALUES (?, ?, ?, ?, ?, ?, ?);" (assq-ref build #:derivation) (assq-ref build #:eval-id) (assq-ref build #:log) @@ -249,7 +255,7 @@ INSERT INTO Builds (derivation, evaluation, log, status, timestamp, starttime, s (match output ((name . path) (sqlite-exec db "\ -INSERT INTO Outputs (build, name, path) VALUES ('~A', '~A', '~A');" +INSERT INTO Outputs (build, name, path) VALUES (?, ?, ?);" build-id name path)))) (assq-ref build #:outputs)) build-id)) @@ -262,17 +268,17 @@ log file for DRV." (time-second (current-time time-utc))) (if (= status (build-status started)) - (sqlite-exec db "UPDATE Builds SET starttime='~A', status='~A' \ -WHERE derivation='~A';" + (sqlite-exec db "UPDATE Builds SET starttime=?, status=? \ +WHERE derivation=?;" now status drv) - (sqlite-exec db "UPDATE Builds SET stoptime='~A', \ -status='~A'~@[, log='~A'~] WHERE derivation='~A';" - now status log-file drv))) + (if log-file + (sqlite-exec db "UPDATE Builds SET stoptime=?, status=?, log=? WHERE derivation=?;" now status log-file drv) + (sqlite-exec db "UPDATE Builds SET stoptime=?, status=? WHERE derivation=?;" now status drv)))) (define (db-get-outputs db build-id) "Retrieve the OUTPUTS of the build identified by BUILD-ID in DB database." (let loop ((rows - (sqlite-exec db "SELECT name, path FROM Outputs WHERE build='~A';" + (sqlite-exec db "SELECT name, path FROM Outputs WHERE build=?;" build-id)) (outputs '())) (match rows @@ -313,7 +319,7 @@ INNER JOIN Specifications ON Evaluations.specification = Specifications.repo_nam (define (db-get-build db id) "Retrieve a build in database DB which corresponds to ID." (let ((res (sqlite-exec db (string-append db-build-request - " WHERE Builds.id='~A';") id))) + " WHERE Builds.id=?;") id))) (match res ((build) (db-format-build db build)) @@ -393,7 +399,7 @@ FILTERS is an assoc list which possible keys are 'project | 'jobset | 'job | (define (db-get-stamp db spec) "Return a stamp corresponding to specification SPEC in database DB." - (let ((res (sqlite-exec db "SELECT * FROM Stamps WHERE specification='~A';" + (let ((res (sqlite-exec db "SELECT * FROM Stamps WHERE specification=?;" (assq-ref spec #:name)))) (match res (() "") @@ -403,10 +409,10 @@ FILTERS is an assoc list which possible keys are 'project | 'jobset | 'job | "Associate stamp COMMIT to specification SPEC in database DB." (if (string-null? (db-get-stamp db spec)) (sqlite-exec db "\ -INSERT INTO Stamps (specification, stamp) VALUES ('~A', '~A');" +INSERT INTO Stamps (specification, stamp) VALUES (?, ?);" (assq-ref spec #:name) commit) (sqlite-exec db "\ -UPDATE Stamps SET stamp='~A' WHERE specification='~A';" +UPDATE Stamps SET stamp=? WHERE specification=?;" commit (assq-ref spec #:name)))) ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-27 17:18 ` Danny Milosavljevic @ 2018-01-27 19:12 ` Danny Milosavljevic 2018-01-28 21:47 ` Ludovic Courtès 2018-02-08 13:37 ` Ludovic Courtès 2 siblings, 0 replies; 42+ messages in thread From: Danny Milosavljevic @ 2018-01-27 19:12 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel > I've now forked guile-sqlite3 and put the stuff that makes sense for a > low-level binding there (not sqlite-exec - that would still be in guix-cuirass). I've created a pull request https://notabug.org/civodul/guile-sqlite3/pulls/2 now. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-27 17:18 ` Danny Milosavljevic 2018-01-27 19:12 ` Danny Milosavljevic @ 2018-01-28 21:47 ` Ludovic Courtès 2018-01-28 22:23 ` Danny Milosavljevic 2018-01-29 9:57 ` Andy Wingo 2018-02-08 13:37 ` Ludovic Courtès 2 siblings, 2 replies; 42+ messages in thread From: Ludovic Courtès @ 2018-01-28 21:47 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel Hi Danny, Danny Milosavljevic <dannym@scratchpost.org> skribis: > I've now forked guile-sqlite3 and put the stuff that makes sense for a > low-level binding there (not sqlite-exec - that would still be in guix-cuirass). > > These are: > - stmts hash-table is now in guile-sqlite3's <db>. > - sqlite-finalize adapted to update <db>'s stmts list > - sqlite-finalize* variant which doesn't update <db>'s stmts list > (because the updating by value is slow) > - sqlite-prepare* macro which stores and reuses existing <stmt>s. > - sqlite-bind-args convenience function which just binds all the parameters > in sequence. > - SQLITE_CONSTRAINT and SQLITE_CONSTRAINT_PRIMARYKEY, moved from guix-cuirass > > https://notabug.org/dannym/guile-sqlite3 Good you’re giving it some love. :-) I’ve already applied two commits in civodul/guile-sqlite3. I think the statement cache requires some more work though (see below). > If that's OK I'll replace the reference in guix-master, or we could do a > pull request to the civodul repository. I think we should stick to a single repo for guile-sqlite3, though it prolly shouldn’t be called “civodul/”. Perhaps you can create guile-sqlite3/guile-sqlite3 and add the two of us plus Andy there for a start? >> I’m not sure what ‘sqlsym’ is. Apparently it’s a symbol derived from >> the SQL statement, right? I don’t think it’s necessary. > > Yes, the idea is to prevent it from having to do string comparison/hashing > to find an existing stmt. It's interned at macro-expansion time - so at > runtime it shouldn't traverse the sql text after the stmt is created. > >> Instead you can simply make that hash table a regular (non-weak) hash >> table that maps strings (SQL text) to prepared statements. You’d also >> need to use ‘hash-set!’ and ‘hash-ref’ instead of ‘hashq-set!’ and >> ‘hash-ref’ since strings should be compared with ‘equal?’, not ‘eq?’. > > That's what I tried to avoid, having it compare 80 character strings > every time I want to reuse a prepared statement. :-) > > What I wanted more is something like a new thread-local variable being > declared every time a new SQL statement is used, otherwise the existing > variable being used. The problem is that interned symbols are potentially not GC’d (though I think with Guile 2.2 and its weak sets they may be subject to GC.) The other issue is that we’d still be caching potentially a lot more than needed. For instance, we don’t know whether a statement is a one-off statement (used only once, for instance because it’s derived from user parameters passed through the HTTP API or something), or if it’s a statement that’s regularly used. Excessive caching could be a real issue for long-running processes like cuirass. Thinking more about it, I’m inclined to not try to be smart and instead let users explicitly ask for caching when they want to. WDYT? Thanks, Ludo’. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-28 21:47 ` Ludovic Courtès @ 2018-01-28 22:23 ` Danny Milosavljevic 2018-01-29 9:57 ` Andy Wingo 1 sibling, 0 replies; 42+ messages in thread From: Danny Milosavljevic @ 2018-01-28 22:23 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel Hi Ludo, On Sun, 28 Jan 2018 22:47:34 +0100 ludo@gnu.org (Ludovic Courtès) wrote: > I’ve already applied two commits in civodul/guile-sqlite3. I think the > statement cache requires some more work though (see below). Nice. > > If that's OK I'll replace the reference in guix-master, or we could do a > > pull request to the civodul repository. > > I think we should stick to a single repo for guile-sqlite3, though it > prolly shouldn’t be called “civodul/”. Perhaps you can create > guile-sqlite3/guile-sqlite3 and add the two of us plus Andy there for a > start? Hmm, should I create a new user account on notabug for that? > The problem is that interned symbols are potentially not GC’d (though I > think with Guile 2.2 and its weak sets they may be subject to GC.) Too bad. If that's the case it's not as good as it could be. > The other issue is that we’d still be caching potentially a lot more > than needed. For instance, we don’t know whether a statement is a > one-off statement (used only once, for instance because it’s derived > from user parameters passed through the HTTP API or something), Well, that shouldn't happen because it would allow SQL injection. I usually write filters like (:parameter IS NULL OR (foo = :parameter)) so that the SQL text doesn't change. On the other hand, if only the parameter values change it will reuse the same statement. > Thinking more about it, I’m inclined to not try to be smart and instead > let users explicitly ask for caching when they want to. Whatever else, I think it would be good to actually use prepared statements for what they are for ;-) That they prevent sql injection is a nice bonus but what they are supposed to do is prevent the database engine from having to parse text and build a query plan every time someone changes one bit somewhere. So I'm still for "caching" those. I've left sqlite-exec inside guix-cuirass rather than guile-sqlite3 so that the user of the guile-sqlite3 can decide caching, among other things. We could also have two macros, sqlite-exec-reuse and sqlite-exec (right now it decides that implicitly by whether the SQL text is a string literal or not - I think in practise that's what cuirass will always do). The stuff that's in dannym guile-sqlite3 has no new effect if you don't use sqlite-prepare* (note star) - so it should be quite conservative already. sqlite-prepare* is written in a way that it always caches - I purposefully left the two other cases with non-literal strings out. So you can't sqlite-prepare* with a non-literal string. And maybe also remove the arg bindings from in there. Then the new API would be: sqlite-prepare for new prepared statements, sqlite-prepare* for caching prepared statements. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-28 21:47 ` Ludovic Courtès 2018-01-28 22:23 ` Danny Milosavljevic @ 2018-01-29 9:57 ` Andy Wingo 1 sibling, 0 replies; 42+ messages in thread From: Andy Wingo @ 2018-01-29 9:57 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel On Sun 28 Jan 2018 22:47, ludo@gnu.org (Ludovic Courtès) writes: > interned symbols are potentially not GC’d (though I think with Guile > 2.2 and its weak sets they may be subject to GC.) Symbols are collectable. They are collectable in 2.0 as well. Depending on the context, it may be worth considering using non-symbols if all you need is a unique object -- some freshly allocated object will do. That's for when you know that X is just an identifier of some kind (you don't have to dispatch on its type, etc). Andy ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-27 17:18 ` Danny Milosavljevic 2018-01-27 19:12 ` Danny Milosavljevic 2018-01-28 21:47 ` Ludovic Courtès @ 2018-02-08 13:37 ` Ludovic Courtès 2018-02-08 16:29 ` Danny Milosavljevic 2 siblings, 1 reply; 42+ messages in thread From: Ludovic Courtès @ 2018-02-08 13:37 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel Heya Danny, Danny Milosavljevic <dannym@scratchpost.org> skribis: > If possible, let's put guile-sqlite3 stuff there - otherwise there will be > xtra records around (I've pushed those to my fork already). > > Corresponding changes for guix-cuirass, starting from > commit 4558d1c86914e2427fc99afbe00c28cb716dbd3d: Following our guile-sqlite3 hack session at the workshop, I adjusted your Cuirass patch and committed it: https://git.savannah.gnu.org/cgit/guix/guix-cuirass.git/commit/?id=eb01f46987a583f0bce94de230d749b1d8f16b99 https://git.savannah.gnu.org/cgit/guix/guix-cuirass.git/commit/?id=53fcecd25f3ab8dd1b394c4c75fa509fe51fd24d We’re making progress! :-) Thanks again for your help on this! Ludo’. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-08 13:37 ` Ludovic Courtès @ 2018-02-08 16:29 ` Danny Milosavljevic 2018-02-08 22:21 ` Ludovic Courtès 0 siblings, 1 reply; 42+ messages in thread From: Danny Milosavljevic @ 2018-02-08 16:29 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel Hi Ludo, On Thu, 08 Feb 2018 14:37:58 +0100 ludo@gnu.org (Ludovic Courtès) wrote: > We’re making progress! :-) Nice! I'm still checking a few loose ends but I think we're pretty okay now from a security standpoint - except for db-get-builds, which I'm amending right now. Also, I'd like to get the number of distinct SQL statements down, so I'll propose another patch on guix-patches to do that. Also, I think sqlite-exec shouldn't call sqlite-finalize most of the time - otherwise the cached statement will be lost :P I've also reintroduced sqlite-bind-args in a nicer version, please pull: https://notabug.org/civodul/guile-sqlite3/pulls/3 . ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-08 16:29 ` Danny Milosavljevic @ 2018-02-08 22:21 ` Ludovic Courtès 2018-02-08 23:05 ` Danny Milosavljevic 0 siblings, 1 reply; 42+ messages in thread From: Ludovic Courtès @ 2018-02-08 22:21 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel [-- Attachment #1: Type: text/plain, Size: 1800 bytes --] Hi Danny, Danny Milosavljevic <dannym@scratchpost.org> skribis: > On Thu, 08 Feb 2018 14:37:58 +0100 > ludo@gnu.org (Ludovic Courtès) wrote: >> We’re making progress! :-) > > Nice! I'm still checking a few loose ends but I think we're pretty okay now > from a security standpoint - except for db-get-builds, which I'm amending > right now. Oh sorry, I think I did the same thing as you were sending this message: https://git.savannah.gnu.org/cgit/guix/guix-cuirass.git/commit/?id=8c7c93922bbe0513ff4c4ff3a6e554e3a72635b6 WDYT? > Also, I'd like to get the number of distinct SQL statements down, so I'll > propose another patch on guix-patches to do that. Excellent. > Also, I think sqlite-exec shouldn't call sqlite-finalize most of the time - > otherwise the cached statement will be lost :P Indeed! Should we change ‘sqlite-finalize’ to a noop when called on a cached statement? (Otherwise users would have to keep track of whether or not a statement is cached.) Besides, on the big database on berlin, the initial: (db-get-builds db '((status pending))) call takes a lot of time and memory. I guess we’re doing something wrong, but I’m not sure what. The same query in the ‘sqlite3’ CLI is snappy and does not consume much memory. One of the things we’re doing wrong is that ‘Outputs’ table: each ‘db-format-build’ call triggers a lookup in that table. We should instead probably simply store output lists in the ‘Derivations’ table. Thoughts? Which also means we should have schema versioning and a way to upgrade… > I've also reintroduced sqlite-bind-args in a nicer version, please pull: > https://notabug.org/civodul/guile-sqlite3/pulls/3 . It is OK with you to write it like this: [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Type: text/x-patch, Size: 958 bytes --] diff --git a/sqlite3.scm b/sqlite3.scm index 156c461..fa96bdb 100644 --- a/sqlite3.scm +++ b/sqlite3.scm @@ -38,6 +38,7 @@ sqlite-prepare* sqlite-prepare sqlite-bind + sqlite-bind-arguments sqlite-column-names sqlite-step sqlite-fold @@ -390,6 +391,21 @@ (error "unexpected value" val))) (check-error (stmt->db stmt)))))) +(define (sqlite-bind-arguments stmt . args) + "Bind STMT parameters, one after another, to ARGS. +Also bind named parameters to the respective ones." + (let loop ((i 1) + (args args)) + (match args + (() + #f) + (((? keyword? kw) value . rest) + (sqlite-bind stmt (keyword->symbol kw) value) + (loop i rest)) + ((arg . rest) + (sqlite-bind stmt i arg) + (loop (+ 1 i) rest))))) + (define sqlite-column-count (let ((column-count (pointer->procedure [-- Attachment #3: Type: text/plain, Size: 90 bytes --] ? At some point we’ll also need a real test suite in guile-sqlite3… Ludo’. ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-08 22:21 ` Ludovic Courtès @ 2018-02-08 23:05 ` Danny Milosavljevic 2018-02-09 6:17 ` Gábor Boskovits 2018-02-09 9:41 ` Ludovic Courtès 0 siblings, 2 replies; 42+ messages in thread From: Danny Milosavljevic @ 2018-02-08 23:05 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel Hi Ludo, On Thu, 08 Feb 2018 23:21:58 +0100 ludo@gnu.org (Ludovic Courtès) wrote: > > from a security standpoint - except for db-get-builds, which I'm amending > > right now. > > Oh sorry, I think I did the same thing as you were sending this message: > > https://git.savannah.gnu.org/cgit/guix/guix-cuirass.git/commit/?id=8c7c93922bbe0513ff4c4ff3a6e554e3a72635b6 > WDYT? I'd prefer not to have so many different SQL statements, we get a combinatorial explosion if we aren't careful (whether we cache or not, the relational database management system is going to hate us anyway when we do that). But I guess there are not that many yet. If we are fine in not being able to search for literal NULL we can use NULL as "anything" marker and have a static WHERE statement (this way is customary). Also, I've asked on the sqlite mailing list - ORDER BY cannot support "?", so those are unavoidable (also, we can't usefully do the ORDER BY ourselves by sorting the result - because of the LIMIT clause) Anyway as long as we are under 10000 statements it should be fine :P > Indeed! Should we change ‘sqlite-finalize’ to a noop when called on a > cached statement? (Otherwise users would have to keep track of whether > or not a statement is cached.) Hmm maybe that's a good way. But its a little magic. If you are not finalizing the statement, it will be reused anyway the next time you use the same SQL text. The user just shouldn't call finalize - which sounds simple enough for him not to do. I think having sqlite-exec detect literal SQL text is a nice middle way. If the SQL text is a literal it means it's right there in the source code and is probably not going to change - how would it? Otherwise err on the side of caution and finalize the statement - it's a little slower but safer that way. I think that would pretty much only mean db-get-builds. Do you think that's too much magic? Or more than the other way? I wonder... I think that if we do this magic we do it right there in the cuirass database.scm module and it's never going to move into guile-sqlite3 :) On the other hand, if we special-cached sqlite-finalize, we'd have to provide sqlite-finalize* or something that does the freeing anyway... > Besides, on the big database on berlin, the initial: > > (db-get-builds db '((status pending))) > > call takes a lot of time and memory. I guess we’re doing something > wrong, but I’m not sure what. The same query in the ‘sqlite3’ CLI is > snappy and does not consume much memory. WTF. I'll have a look. > One of the things we’re doing wrong is that ‘Outputs’ table: each > ‘db-format-build’ call triggers a lookup in that table. We should > instead probably simply store output lists in the ‘Derivations’ table. Definitely. That's one of the things we should inline into db-get-builds. Relational databases are good at joins, let's not to their work for them. > Which also means we should have schema versioning and a way to upgrade… Yeah. > > I've also reintroduced sqlite-bind-args in a nicer version, please pull: > > https://notabug.org/civodul/guile-sqlite3/pulls/3 . > > It is OK with you to write it like this: Yes, looks good! Thanks! ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-08 23:05 ` Danny Milosavljevic @ 2018-02-09 6:17 ` Gábor Boskovits 2018-02-09 9:41 ` Ludovic Courtès 1 sibling, 0 replies; 42+ messages in thread From: Gábor Boskovits @ 2018-02-09 6:17 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel [-- Attachment #1: Type: text/plain, Size: 4026 bytes --] 2018-02-09 0:05 GMT+01:00 Danny Milosavljevic <dannym@scratchpost.org>: > Hi Ludo, > > On Thu, 08 Feb 2018 23:21:58 +0100 > ludo@gnu.org (Ludovic Courtès) wrote: > > > > from a security standpoint - except for db-get-builds, which I'm > amending > > > right now. > > > > Oh sorry, I think I did the same thing as you were sending this message: > > > > https://git.savannah.gnu.org/cgit/guix/guix-cuirass.git/commit/?id= > 8c7c93922bbe0513ff4c4ff3a6e554e3a72635b6 > > > WDYT? > > I'd prefer not to have so many different SQL statements, we get a > combinatorial explosion if we aren't careful (whether we cache or not, > the relational database management system is going to hate us anyway > when we do that). > > But I guess there are not that many yet. > > If we are fine in not being able to search for literal NULL we can use > NULL as > "anything" marker and have a static WHERE statement (this way is > customary). > > Also, I've asked on the sqlite mailing list - ORDER BY cannot support "?", > so > those are unavoidable (also, we can't usefully do the ORDER BY ourselves > by sorting the result - because of the LIMIT clause) > > Anyway as long as we are under 10000 statements it should be fine :P > > > Indeed! Should we change ‘sqlite-finalize’ to a noop when called on a > > cached statement? (Otherwise users would have to keep track of whether > > or not a statement is cached.) > > Hmm maybe that's a good way. But its a little magic. > > If you are not finalizing the statement, it will be reused anyway the next > time > you use the same SQL text. The user just shouldn't call finalize - which > sounds > simple enough for him not to do. > > I think having sqlite-exec detect literal SQL text is a nice middle way. > > If the SQL text is a literal it means it's right there in the source code > and is probably not going to change - how would it? > > Otherwise err on the side of caution and finalize the statement - it's > a little slower but safer that way. I think that would pretty much only > mean db-get-builds. > > Do you think that's too much magic? Or more than the other way? I > wonder... > > I think that if we do this magic we do it right there in the cuirass > database.scm > module and it's never going to move into guile-sqlite3 :) > > On the other hand, if we special-cached sqlite-finalize, we'd have to > provide > sqlite-finalize* or something that does the freeing anyway... > > > Besides, on the big database on berlin, the initial: > > > > (db-get-builds db '((status pending))) > > > > call takes a lot of time and memory. I guess we’re doing something > > wrong, but I’m not sure what. The same query in the ‘sqlite3’ CLI is > > snappy and does not consume much memory. > > WTF. I'll have a look. > > > One of the things we’re doing wrong is that ‘Outputs’ table: each > > ‘db-format-build’ call triggers a lookup in that table. We should > > instead probably simply store output lists in the ‘Derivations’ table. > > Definitely. That's one of the things we should inline into db-get-builds. > Relational databases are good at joins, let's not to their work for them. > > > Which also means we should have schema versioning and a way to upgrade… > > Yeah. > > I've used this: http://sqitch.org/ for a few projects, if you see it fits the bill I can help to get this working. I liked it, because it supports different databases, it was a big plus for me. It is also nice to be able to declare dependencies of changes. If you have other preferred method for this, the I would like to hear about that. I use these kind of things regulary in my work, it would be nice to get to know new methods. Thanks. > > > I've also reintroduced sqlite-bind-args in a nicer version, please > pull: > > > https://notabug.org/civodul/guile-sqlite3/pulls/3 . > > > > It is OK with you to write it like this: > > Yes, looks good! Thanks! > > [-- Attachment #2: Type: text/html, Size: 5438 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-08 23:05 ` Danny Milosavljevic 2018-02-09 6:17 ` Gábor Boskovits @ 2018-02-09 9:41 ` Ludovic Courtès 2018-02-09 11:29 ` Danny Milosavljevic 1 sibling, 1 reply; 42+ messages in thread From: Ludovic Courtès @ 2018-02-09 9:41 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel Hello, Danny Milosavljevic <dannym@scratchpost.org> skribis: > On Thu, 08 Feb 2018 23:21:58 +0100 > ludo@gnu.org (Ludovic Courtès) wrote: > >> > from a security standpoint - except for db-get-builds, which I'm amending >> > right now. >> >> Oh sorry, I think I did the same thing as you were sending this message: >> >> https://git.savannah.gnu.org/cgit/guix/guix-cuirass.git/commit/?id=8c7c93922bbe0513ff4c4ff3a6e554e3a72635b6 > >> WDYT? > > I'd prefer not to have so many different SQL statements, we get a > combinatorial explosion if we aren't careful (whether we cache or not, > the relational database management system is going to hate us anyway > when we do that). > > But I guess there are not that many yet. > > If we are fine in not being able to search for literal NULL we can use NULL as > "anything" marker and have a static WHERE statement (this way is customary). > > Also, I've asked on the sqlite mailing list - ORDER BY cannot support "?", so > those are unavoidable (also, we can't usefully do the ORDER BY ourselves > by sorting the result - because of the LIMIT clause) > > Anyway as long as we are under 10000 statements it should be fine :P Yes. Also, in practice, everyone’s going to make the same /api/* requests (because there are only two clients, the Emacs and the Web UI, and they typically always do the same requests), which in turn means we’ll always get the same ‘db-get-builds’ call, possibly with just a different limit, but it’s still the same statement. So I think we should be fine. >> Indeed! Should we change ‘sqlite-finalize’ to a noop when called on a >> cached statement? (Otherwise users would have to keep track of whether >> or not a statement is cached.) > > Hmm maybe that's a good way. But its a little magic. Yes, but I think we have no other option: now that caching is built into sqlite3.scm, it has to be properly handled by all of that module. For the user, it should be a simple matter of choosing #:cache? #t or #:cache? #f, and then (sqlite3) should just DTRT. Otherwise we’d have to remove caching from (sqlite3) altogether, IMO. WDYT? >> Besides, on the big database on berlin, the initial: >> >> (db-get-builds db '((status pending))) >> >> call takes a lot of time and memory. I guess we’re doing something >> wrong, but I’m not sure what. The same query in the ‘sqlite3’ CLI is >> snappy and does not consume much memory. > > WTF. I'll have a look. Great. :-) For the record, this is the GC profile I get (take it with a grain of salt: it tells us what part of the code allocates, not what the live objects are): --8<---------------cut here---------------start------------->8--- scheme@(guile-user)> (define lst (gcprof (lambda () (db-get-builds db '((status pending)))))) % cumulative self time seconds seconds procedure 33.82 468.32 468.32 bytevector->pointer 22.38 1361.15 309.97 cuirass/database.scm:347:0:db-format-build 11.19 154.98 154.98 hashq-set! 6.33 87.60 87.60 make-bytevector 4.62 64.01 64.01 utf8->string 3.89 1051.19 53.91 cuirass/database.scm:324:0:db-get-outputs 2.92 40.43 40.43 apply-smob/1 2.68 37.06 37.06 dereference-pointer 2.43 33.69 33.69 anon #x28e3088 1.46 1010.76 20.22 cuirass/database.scm:55:0:%sqlite-exec 1.46 20.22 20.22 srfi/srfi-1.scm:269:0:iota 1.22 47.17 16.85 ice-9/boot-9.scm:789:2:catch 1.22 16.85 16.85 ice-9/boot-9.scm:777:2:throw 1.22 16.85 16.85 string->utf8 0.97 13.48 13.48 ice-9/boot-9.scm:701:2:make-prompt-tag 0.73 1384.74 10.11 cuirass/database.scm:375:0:db-get-builds 0.73 10.11 10.11 hash-set! 0.49 6.74 6.74 cons 0.24 3.37 3.37 pointer->bytevector 0.00 4462655.56 0.00 /gnu/store/cxxyk9bdas4n7m6zlhdhnm7ixxkw3b0b-profile/share/guile/site/2.2/sqlite3.scm:510:2:lp 0.00 815.34 0.00 /gnu/store/cxxyk9bdas4n7m6zlhdhnm7ixxkw3b0b-profile/share/guile/site/2.2/sqlite3.scm:311:0:sqlite-prepare 0.00 805.24 0.00 /gnu/store/cxxyk9bdas4n7m6zlhdhnm7ixxkw3b0b-profile/share/guile/site/2.2/sqlite3.scm:286:4 0.00 101.08 0.00 /gnu/store/cxxyk9bdas4n7m6zlhdhnm7ixxkw3b0b-profile/share/guile/site/2.2/sqlite3.scm:474:0:sqlite-row 0.00 47.17 0.00 /gnu/store/cxxyk9bdas4n7m6zlhdhnm7ixxkw3b0b-profile/share/guile/site/2.2/sqlite3.scm:223:2:sqlite-remove-statement! 0.00 47.17 0.00 /gnu/store/cxxyk9bdas4n7m6zlhdhnm7ixxkw3b0b-profile/share/guile/site/2.2/sqlite3.scm:241:4 0.00 37.06 0.00 /gnu/store/cxxyk9bdas4n7m6zlhdhnm7ixxkw3b0b-profile/share/guile/site/2.2/sqlite3.scm:444:4 0.00 16.85 0.00 /gnu/store/cxxyk9bdas4n7m6zlhdhnm7ixxkw3b0b-profile/share/guile/site/2.2/sqlite3.scm:227:22 0.00 16.85 0.00 hash-for-each --- Sample count: 411 Total time: 1384.735520913 seconds (1331.66193132 seconds in GC) --8<---------------cut here---------------end--------------->8--- This query takes ~5 seconds with the CLI (which is still way too much): --8<---------------cut here---------------start------------->8--- sqlite> select count(*) from builds inner join Derivations ON Builds.derivation = Derivations.derivation and Builds.evaluation = Derivations.evaluation ...> INNER JOIN Evaluations ON Derivations.evaluation = Evaluations.id ...> INNER JOIN Specifications ON Evaluations.specification = Specifications.repo_name; 2156003 --8<---------------cut here---------------end--------------->8--- >> One of the things we’re doing wrong is that ‘Outputs’ table: each >> ‘db-format-build’ call triggers a lookup in that table. We should >> instead probably simply store output lists in the ‘Derivations’ table. > > Definitely. That's one of the things we should inline into db-get-builds. > Relational databases are good at joins, let's not to their work for them. Right. >> > I've also reintroduced sqlite-bind-args in a nicer version, please pull: >> > https://notabug.org/civodul/guile-sqlite3/pulls/3 . >> >> It is OK with you to write it like this: > > Yes, looks good! Thanks! Pushed! Thanks, Ludo’. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-09 9:41 ` Ludovic Courtès @ 2018-02-09 11:29 ` Danny Milosavljevic 2018-02-09 16:53 ` Ludovic Courtès 0 siblings, 1 reply; 42+ messages in thread From: Danny Milosavljevic @ 2018-02-09 11:29 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel Hi Ludo, On Fri, 09 Feb 2018 10:41:13 +0100 ludo@gnu.org (Ludovic Courtès) wrote: > Yes. Also, in practice, everyone’s going to make the same /api/* > requests (because there are only two clients, the Emacs and the Web UI, > and they typically always do the same requests), which in turn means > we’ll always get the same ‘db-get-builds’ call, possibly with just a > different limit, but it’s still the same statement. > So I think we should be fine. Right. > >> Indeed! Should we change ‘sqlite-finalize’ to a noop when called on a > >> cached statement? (Otherwise users would have to keep track of whether > >> or not a statement is cached.) > > > > Hmm maybe that's a good way. But its a little magic. > > Yes, but I think we have no other option: now that caching is built into > sqlite3.scm, it has to be properly handled by all of that module. For > the user, it should be a simple matter of choosing #:cache? #t > or #:cache? #f, and then (sqlite3) should just DTRT. Yeah, but then let's add sqlite-uncache or something that can be used to remove it from the cache after all. And make sqlite-finalize a noop if it's cached. Sounds good. So a savvy user could do sqlite-uncache and then sqlite-finalize and it would be gone. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-09 11:29 ` Danny Milosavljevic @ 2018-02-09 16:53 ` Ludovic Courtès 2018-02-09 17:06 ` Danny Milosavljevic 0 siblings, 1 reply; 42+ messages in thread From: Ludovic Courtès @ 2018-02-09 16:53 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel [-- Attachment #1: Type: text/plain, Size: 919 bytes --] Heya, Danny Milosavljevic <dannym@scratchpost.org> skribis: > On Fri, 09 Feb 2018 10:41:13 +0100 > ludo@gnu.org (Ludovic Courtès) wrote: [...] >> >> Indeed! Should we change ‘sqlite-finalize’ to a noop when called on a >> >> cached statement? (Otherwise users would have to keep track of whether >> >> or not a statement is cached.) >> > >> > Hmm maybe that's a good way. But its a little magic. >> >> Yes, but I think we have no other option: now that caching is built into >> sqlite3.scm, it has to be properly handled by all of that module. For >> the user, it should be a simple matter of choosing #:cache? #t >> or #:cache? #f, and then (sqlite3) should just DTRT. > > Yeah, but then let's add sqlite-uncache or something that can be used to > remove it from the cache after all. And make sqlite-finalize a noop if > it's cached. Sounds good. What about this patch: [-- Attachment #2: Type: text/x-patch, Size: 2868 bytes --] diff --git a/sqlite3.scm b/sqlite3.scm index fa96bdb..e8d2bf8 100644 --- a/sqlite3.scm +++ b/sqlite3.scm @@ -1,5 +1,6 @@ ;; Guile-SQLite3 ;; Copyright (C) 2010, 2014 Andy Wingo <wingo at pobox dot com> +;; Copyright (C) 2018 Ludovic Courtès <ludo@gnu.org> ;; This library is free software; you can redistribute it and/or modify ;; it under the terms of the GNU Lesser General Public License as @@ -114,6 +115,14 @@ (open? db-open? set-db-open?!) (stmts db-stmts)) +(define-record-type <sqlite-stmt> + (make-stmt pointer live? reset? cached?) + stmt? + (pointer stmt-pointer) + (live? stmt-live? set-stmt-live?!) + (reset? stmt-reset? set-stmt-reset?!) + (cached? stmt-cached? set-stmt-cached?!)) + (define sqlite-errmsg (let ((f (pointer->procedure '* @@ -145,11 +154,17 @@ (dynamic-func "sqlite3_close" libsqlite3) (list '*)))) (lambda (db) - (if (db-open? db) - (begin - (let ((p (db-pointer db))) - (set-db-open?! db #f) - (f p))))))) + (when (db-open? db) + ;; Finalize cached statements. + (hash-for-each (lambda (sql stmt) + (set-stmt-cached?! stmt #f) + (sqlite-finalize stmt)) + (db-stmts db)) + (hash-clear! (db-stmts db)) + + (let ((p (db-pointer db))) + (set-db-open?! db #f) + (f p)))))) (define db-guardian (make-guardian)) (define (pump-db-guardian) @@ -208,18 +223,11 @@ (ele (db-pointer db) onoff)))) +\f ;;; ;;; SQL statements ;;; -(define-record-type <sqlite-stmt> - (make-stmt pointer live? reset? cached?) - stmt? - (pointer stmt-pointer) - (live? stmt-live? set-stmt-live?!) - (reset? stmt-reset? set-stmt-reset?!) - (cached? stmt-cached?)) - (define sqlite-remove-statement! (lambda (db stmt) (when (stmt-cached? stmt) @@ -240,11 +248,15 @@ (dynamic-func "sqlite3_finalize" libsqlite3) (list '*)))) (lambda (stmt) - (if (stmt-live? stmt) - (let ((p (stmt-pointer stmt))) - (sqlite-remove-statement! (stmt->db stmt) stmt) - (set-stmt-live?! stmt #f) - (f p)))))) + ;; Note: When STMT is cached, this is a no-op. This ensures caching + ;; actually works while still separating concerns: users can turn + ;; caching on and off without having to change the rest of their code. + (when (and (stmt-live? stmt) + (not (stmt-cached? stmt))) + (let ((p (stmt-pointer stmt))) + (sqlite-remove-statement! (stmt->db stmt) stmt) + (set-stmt-live?! stmt #f) + (f p)))))) (define *stmt-map* (make-weak-key-hash-table)) (define (stmt->db stmt) [-- Attachment #3: Type: text/plain, Size: 253 bytes --] ? I’d reluctant to ‘sqlite-uncache!’ though. I would think that if users need more sophisticated caching, they can always implement it in their application. I wouldn’t want us to try to be too smart here. WDYT? Thanks, Ludo’. ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-09 16:53 ` Ludovic Courtès @ 2018-02-09 17:06 ` Danny Milosavljevic 2018-02-10 11:18 ` Ludovic Courtès 0 siblings, 1 reply; 42+ messages in thread From: Danny Milosavljevic @ 2018-02-09 17:06 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel Hi Ludo, the patch LGTM! >I’d reluctant to ‘sqlite-uncache!’ though. I was thinking if you are in the REPL and actually want it to forget the statement which you cached before, it would be good to be able to do that. I wonder what I was thinking that was good for, though. User can always just close the database connection. Nevermind! :) ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-09 17:06 ` Danny Milosavljevic @ 2018-02-10 11:18 ` Ludovic Courtès 2018-02-13 9:12 ` Danny Milosavljevic 0 siblings, 1 reply; 42+ messages in thread From: Ludovic Courtès @ 2018-02-10 11:18 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel Hi Danny, Danny Milosavljevic <dannym@scratchpost.org> skribis: > the patch LGTM! Pushed! >>I’d reluctant to ‘sqlite-uncache!’ though. > > I was thinking if you are in the REPL and actually want it to forget the > statement which you cached before, it would be good to be able to do that. > I wonder what I was thinking that was good for, though. > > User can always just close the database connection. Yeah. :-) Now I have another problem: with <https://git.savannah.gnu.org/cgit/guix/guix-cuirass.git/commit/?id=c47dfdf82b4be62501a7932eaec4c124566a1829> HTTP requests are now processed concurrently for real. But that brings its own set of problems, in particular the fact that <sqlite-db> access is not thread-safe, as discussed earlier. Thinking about it, it wouldn’t matter that HTTP requests are processed sequentially if database queries run really fast. I’m not sure if we can achieve it. WDYT? Thanks, Ludo’. PS: I’ll be away for the next few days. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-10 11:18 ` Ludovic Courtès @ 2018-02-13 9:12 ` Danny Milosavljevic 2018-02-14 13:43 ` Ludovic Courtès 0 siblings, 1 reply; 42+ messages in thread From: Danny Milosavljevic @ 2018-02-13 9:12 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel Hi Ludo, On Sat, 10 Feb 2018 12:18:56 +0100 ludo@gnu.org (Ludovic Courtès) wrote: > Thinking about it, it wouldn’t matter that HTTP requests are processed > sequentially if database queries run really fast. I’m not sure if we > can achieve it. WDYT? That depends on how fast. But it should be possible to optimize the actual query (using indices, lookups are O(log N)). Also, if it's the same query as before, it usually will be really fast as most of the pointers are still where they were before. Sqlite3 already automatically created indices for all the primary keys. There's also https://www.sqlite.org/pragma.html#pragma_optimize if we need it. We can always try it with serialized database access and use a connection pool should it get too slow later. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-13 9:12 ` Danny Milosavljevic @ 2018-02-14 13:43 ` Ludovic Courtès 2018-02-14 23:17 ` Ludovic Courtès 2018-02-14 23:21 ` Ludovic Courtès 0 siblings, 2 replies; 42+ messages in thread From: Ludovic Courtès @ 2018-02-14 13:43 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel Hello, Danny Milosavljevic <dannym@scratchpost.org> skribis: > On Sat, 10 Feb 2018 12:18:56 +0100 > ludo@gnu.org (Ludovic Courtès) wrote: > >> Thinking about it, it wouldn’t matter that HTTP requests are processed >> sequentially if database queries run really fast. I’m not sure if we >> can achieve it. WDYT? > > That depends on how fast. But it should be possible to optimize the actual > query (using indices, lookups are O(log N)). Also, if it's the same > query as before, it usually will be really fast as most of the pointers > are still where they were before. > > Sqlite3 already automatically created indices for all the primary keys. > > There's also https://www.sqlite.org/pragma.html#pragma_optimize if we need it. > > We can always try it with serialized database access and use a connection > pool should it get too slow later. My point is /latestbuilds and /queue already take several seconds on the database that we have on berlin, which is quite big. So we have a problem already. I tried this: --8<---------------cut here---------------start------------->8--- $ sudo sqlite3 /var/run/cuirass/cuirass.db Password: SQLite version 3.19.3 2017-06-08 14:26:16 Enter ".help" for usage hints. sqlite> select count(*) from builds where status < 0; 636635 sqlite> select count(*) from builds inner join Derivations ON Builds.derivation = Derivations.derivation and Builds.evaluation = Derivations.evaluation ...> INNER JOIN Evaluations ON Derivations.evaluation = Evaluations.id ...> INNER JOIN Specifications ON Evaluations.specification = Specifications.repo_name; 2156003 sqlite> pragma optimize; sqlite> --8<---------------cut here---------------end--------------->8--- … but that doesn’t seem to have any effect, presumably because sqlite3 already optimized whatever it could. Thoughts? Ludo’. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-14 13:43 ` Ludovic Courtès @ 2018-02-14 23:17 ` Ludovic Courtès 2018-02-19 15:35 ` Danny Milosavljevic 2018-02-14 23:21 ` Ludovic Courtès 1 sibling, 1 reply; 42+ messages in thread From: Ludovic Courtès @ 2018-02-14 23:17 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel ludo@gnu.org (Ludovic Courtès) skribis: > My point is /latestbuilds and /queue already take several seconds on the > database that we have on berlin, which is quite big. So we have a > problem already. Turns out creating the relevant indexes helps a lot, unsurprisingly I guess: https://git.savannah.gnu.org/cgit/guix/guix-cuirass.git/commit/?id=db27955ad3fae260ee2aa4bace8dd6a4115d338c I also discovered SQLite’s “EXPLAIN QUERY PLAN”, which is pretty cool: https://www.sqlite.org/lang_explain.html With that /latestbuilds still usually takes a bit more than 2 seconds. This could be due to the repeated ‘Outputs’ queries via ‘db-get-outputs’. Ludo’. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-14 23:17 ` Ludovic Courtès @ 2018-02-19 15:35 ` Danny Milosavljevic 2018-02-19 15:35 ` [PATCH] database: Simplify 'db-get-builds' Danny Milosavljevic ` (2 more replies) 0 siblings, 3 replies; 42+ messages in thread From: Danny Milosavljevic @ 2018-02-19 15:35 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel Hi Ludo, On Thu, 15 Feb 2018 00:17:55 +0100 ludo@gnu.org (Ludovic Courtès) wrote: > Turns out creating the relevant indexes helps a lot, unsurprisingly I > guess: > > https://git.savannah.gnu.org/cgit/guix/guix-cuirass.git/commit/?id=db27955ad3fae260ee2aa4bace8dd6a4115d338c Yup. It's a tradeoff because now the DBMS has to update the indices every time you insert or update, but reading is now faster. I guess we query more than we build so it's fine. > I also discovered SQLite’s “EXPLAIN QUERY PLAN”, which is pretty cool: > > https://www.sqlite.org/lang_explain.html Yup :) > With that /latestbuilds still usually takes a bit more than 2 seconds. > This could be due to the repeated ‘Outputs’ queries via ‘db-get-outputs’. Oh yeah, I wanted to optimize that. It won't be that nice but it will be faster. First, I'll send a patch simplifying db-get-builds while retaining the current functionality. ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] database: Simplify 'db-get-builds'. 2018-02-19 15:35 ` Danny Milosavljevic @ 2018-02-19 15:35 ` Danny Milosavljevic 2018-02-19 17:52 ` [PATCH] database: db-get-builds: Inline output selection Danny Milosavljevic 2018-02-19 22:08 ` Cuirass news Danny Milosavljevic 2 siblings, 0 replies; 42+ messages in thread From: Danny Milosavljevic @ 2018-02-19 15:35 UTC (permalink / raw) To: guix-devel, ludo * src/cuirass/database.scm (db-get-builds): Modify. (db-get-build): Modify. --- src/cuirass/database.scm | 165 ++++++++++++++++------------------------------- 1 file changed, 55 insertions(+), 110 deletions(-) diff --git a/src/cuirass/database.scm b/src/cuirass/database.scm index dd3e5a2..5a4631f 100644 --- a/src/cuirass/database.scm +++ b/src/cuirass/database.scm @@ -26,6 +26,7 @@ #:use-module (ice-9 rdelim) #:use-module (srfi srfi-1) #:use-module (srfi srfi-19) + #:use-module (srfi srfi-26) #:use-module (sqlite3) #:export (;; Procedures. db-init @@ -347,15 +348,6 @@ log file for DRV." (cons `(,name . ((#:path . ,path))) outputs)))))) -(define db-build-request "\ -SELECT Builds.id, Builds.timestamp, Builds.starttime, Builds.stoptime, Builds.log, Builds.status, Builds.derivation,\ -Derivations.job_name, Derivations.system, Derivations.nix_name,\ -Specifications.repo_name, Specifications.branch \ -FROM Builds \ -INNER JOIN Derivations ON Builds.derivation = Derivations.derivation and Builds.evaluation = Derivations.evaluation \ -INNER JOIN Evaluations ON Derivations.evaluation = Evaluations.id \ -INNER JOIN Specifications ON Evaluations.specification = Specifications.repo_name") - (define (db-format-build db build) (match build (#(id timestamp starttime stoptime log status derivation job-name system @@ -374,112 +366,65 @@ INNER JOIN Specifications ON Evaluations.specification = Specifications.repo_nam (#:outputs . ,(db-get-outputs db id)) (#:branch . ,branch))))) -(define (db-get-build db id) - "Retrieve a build in database DB which corresponds to ID." - (let ((res (sqlite-exec db (string-append db-build-request - " WHERE Builds.id=") - id ";"))) - (match res - ((build) - (db-format-build db build)) - (() #f)))) - (define (db-get-builds db filters) "Retrieve all builds in database DB which are matched by given FILTERS. FILTERS is an assoc list which possible keys are 'project | 'jobset | 'job | 'system | 'nr | 'order | 'status." - (define (clauses->query+arguments clauses) - ;; Given CLAUSES, return two values: a SQL query string, and a list of - ;; arguments to bind. Each element of CLAUSES must be either a string, or - ;; a (SQL ARGUMENT) tuple, where SQL is a query fragment and ARGUMENT is - ;; the argument to be bound for that fragment. - (let loop ((clauses clauses) - (query '()) - (arguments '())) - (match clauses - (() - (values (string-concatenate-reverse query) - (reverse arguments))) - (((? string? clause) . rest) - (loop rest - (cons clause query) - arguments)) - ((((? string? clause) argument) . rest) - (loop rest - (cons clause query) - (cons argument arguments)))))) - - (define (where-clauses filters) - (match (filter-map (match-lambda - (('project project) - (list "Specifications.repo_name=?" project)) - (('jobset jobset) - (list "Specifications.branch=?" jobset)) - (('job job) - (list "Derivations.job_name=?" job)) - (('system system) - (list "Derivations.system=?" system)) - (('status 'done) - "Builds.status >= 0") - (('status 'pending) - "Builds.status < 0") - (_ #f)) - filters) - (() - '("")) - ((clause) - (list "WHERE " clause)) - ((clause0 rest ...) - (cons* "WHERE " clause0 - (fold-right (lambda (clause result) - `(" AND " ,clause ,@result)) - '() - rest))))) - - (define (order-clause filters) - (or (any (match-lambda - (('order 'build-id) - "ORDER BY Builds.id ASC") - (('order 'decreasing-build-id) - "ORDER BY Builds.id DESC") - (('order 'finish-time) - "ORDER BY Builds.stoptime DESC") - (('order 'start-time) - "ORDER BY Builds.start DESC") - (('order 'submission-time) - "ORDER BY Builds.timestamp DESC") - (('order 'status+submission-time) - ;; With this order, builds in 'running' state (-1) appear - ;; before those in 'scheduled' state (-2). - "ORDER BY Builds.status DESC, Builds.timestamp DESC") - (_ #f)) - filters) - "ORDER BY Builds.id DESC")) ;default order - - (define (limit-clause filters) - (or (any (match-lambda - (('nr number) - (list "LIMIT ?" number)) - (_ #f)) - filters) - "")) - - (call-with-values - (lambda () - (clauses->query+arguments (append (list db-build-request " ") - (where-clauses filters) '(" ") - (list (order-clause filters) " ") - (list (limit-clause filters) " ")))) - (lambda (sql arguments) - (let loop ((rows (apply %sqlite-exec db sql arguments)) - (outputs '())) - (match rows - (() - (reverse outputs)) - ((row . rest) - (loop rest - (cons (db-format-build db row) outputs)))))))) + ;; XXX Change caller and remove + (define (assqx-ref filters key) + (if (null? filters) + #f + (match (car filters) + ((xkey xvalue) (if (eq? key xkey) + xvalue + (assqx-ref (cdr filters) key)))))) + (let* ((order (if (eq? (assqx-ref filters 'order) 'build-id) + "ASC" + "DESC")) + (order-column-name + (match (assqx-ref filters 'order) + (('order 'build-id) "Builds.id") + (('order 'decreasing-build-id) "Builds.id") + (('order 'finish-time) "Builds.stoptime") + (('order 'start-time) "Builds.starttime") + (('order 'submission-time) "Builds.timestamp") + (_ "Builds.id"))) + (stmt-text (format #f "\ +SELECT Builds.id, Builds.timestamp, Builds.starttime, Builds.stoptime, Builds.log, Builds.status, Builds.derivation,\ +Derivations.job_name, Derivations.system, Derivations.nix_name,\ +Specifications.repo_name, Specifications.branch \ +FROM Builds \ +INNER JOIN Derivations ON Builds.derivation = Derivations.derivation AND Builds.evaluation = Derivations.evaluation \ +INNER JOIN Evaluations ON Derivations.evaluation = Evaluations.id \ +INNER JOIN Specifications ON Evaluations.specification = Specifications.repo_name \ +WHERE (:id IS NULL OR (:id = Builds.id)) \ +OR (:project IS NULL OR (:project = Specifications.repo_name)) \ +OR (:jobset IS NULL OR (:jobset = Specifications.branch)) \ +OR (:job IS NULL OR (:job = Derivations.job_name)) \ +OR (:system IS NULL OR (:system = Derivations.system)) \ +OR (:status IS NULL OR (:status = 'done' AND Builds.status >= 0) OR (:status = 'pending' AND Builds.status < 0)) \ +ORDER BY ~a ~a LIMIT :nr;" order-column-name order)) + (stmt (sqlite-prepare db stmt-text #:cache? #t))) + (sqlite-bind-arguments stmt #:id (assqx-ref filters 'id) + #:project (assqx-ref filters 'project) + #:jobset (assqx-ref filters 'jobset) + #:job (assqx-ref filters 'job) + #:system (assqx-ref filters 'system) + #:status (and=> (assqx-ref filters 'status) + object->string) + #:nr (match (assqx-ref filters 'nr) + (#f -1) + (x x))) + (map (cut db-format-build db <>) + (sqlite-fold-right cons '() stmt)))) + +(define (db-get-build db id) + "Retrieve a build in database DB which corresponds to ID." + (match (db-get-builds db '(('id id))) + ((build) + build) + (() #f))) (define (db-get-stamp db spec) "Return a stamp corresponding to specification SPEC in database DB." ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH] database: db-get-builds: Inline output selection. 2018-02-19 15:35 ` Danny Milosavljevic 2018-02-19 15:35 ` [PATCH] database: Simplify 'db-get-builds' Danny Milosavljevic @ 2018-02-19 17:52 ` Danny Milosavljevic 2018-02-19 22:08 ` Cuirass news Danny Milosavljevic 2 siblings, 0 replies; 42+ messages in thread From: Danny Milosavljevic @ 2018-02-19 17:52 UTC (permalink / raw) To: guix-devel * src/cuirass/database.scm (db-get-builds): Inline output selection. --- src/cuirass/database.scm | 87 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 83 insertions(+), 4 deletions(-) diff --git a/src/cuirass/database.scm b/src/cuirass/database.scm index 5a4631f..f19ee03 100644 --- a/src/cuirass/database.scm +++ b/src/cuirass/database.scm @@ -379,6 +379,85 @@ FILTERS is an assoc list which possible keys are 'project | 'jobset | 'job | ((xkey xvalue) (if (eq? key xkey) xvalue (assqx-ref (cdr filters) key)))))) + + (define (format-output name path) + `(,name . ((#:path . ,path)))) + + (define (cons-output name path rest) + "If NAME and PATH are both not #f, cons them to REST. +Otherwise return REST unchanged." + (if (and (not name) (not path)) + rest + (cons (format-output name path) rest))) + + (define (collect-outputs repeated-builds-id repeated-row outputs rows) + "Given rows somewhat like +1 'a 'b 2 'x +^ 'c 'd 2 'x +| ^^^^^ ^^^^ +| group ++++- group headers +| detail ++------------ group id + +return rows somewhat like + +1 2 'x '((a b) (c d)) + +. + +As a special case, if the group detail is #f #f, ignore it. +This is made specifically to support LEFT JOINs. + +Assumes that iff group id stays the same the group headers stay the same." + (define (finish-group) + (match repeated-row + (#(timestamp starttime stoptime log status derivation job-name system + nix-name repo-name branch) + `((#:id . ,repeated-builds-id) + (#:timestamp . ,timestamp) + (#:starttime . ,starttime) + (#:stoptime . ,stoptime) + (#:log . ,log) + (#:status . ,status) + (#:derivation . ,derivation) + (#:job-name . ,job-name) + (#:system . ,system) + (#:nix-name . ,nix-name) + (#:repo-name . ,repo-name) + (#:outputs . ,outputs) + (#:branch . ,branch))))) + + (define (same-group? builds-id) + (= builds-id repeated-builds-id)) + + (match rows + (() (list (finish-group))) + ((#((? same-group? x-builds-id) x-output-name x-output-path ...) . rest) + ;; Accumulate group members of current group. + (let ((outputs (cons-output x-output-name x-output-path outputs))) + (collect-outputs repeated-builds-id repeated-row outputs rest))) + ((#(x-builds-id x-output-name x-output-path timestamp starttime stoptime log + status derivation job-name system nix-name repo-name branch) . rest) + (cons ;; Finish current group. + (finish-group) + ;; Start new group. + (let ((outputs (cons-output x-output-name x-output-path '()))) + (let ((x-repeated-row (vector timestamp starttime stoptime + log status derivation job-name system + nix-name repo-name branch))) + (collect-outputs x-builds-id x-repeated-row outputs rest))))))) + + (define (group-outputs rows) + (match rows + (() '()) + ((#(x-builds-id x-output-name x-output-path timestamp starttime stoptime + log status derivation job-name system + nix-name repo-name branch) . rest) + (let ((x-repeated-row (vector timestamp starttime stoptime + log status derivation job-name system + nix-name repo-name branch))) + (collect-outputs x-builds-id x-repeated-row '() rows))))) + (let* ((order (if (eq? (assqx-ref filters 'order) 'build-id) "ASC" "DESC")) @@ -391,20 +470,21 @@ FILTERS is an assoc list which possible keys are 'project | 'jobset | 'job | (('order 'submission-time) "Builds.timestamp") (_ "Builds.id"))) (stmt-text (format #f "\ -SELECT Builds.id, Builds.timestamp, Builds.starttime, Builds.stoptime, Builds.log, Builds.status, Builds.derivation,\ +SELECT Builds.id, Outputs.name, Outputs.path, Builds.timestamp, Builds.starttime, Builds.stoptime, Builds.log, Builds.status, Builds.derivation,\ Derivations.job_name, Derivations.system, Derivations.nix_name,\ Specifications.repo_name, Specifications.branch \ FROM Builds \ INNER JOIN Derivations ON Builds.derivation = Derivations.derivation AND Builds.evaluation = Derivations.evaluation \ INNER JOIN Evaluations ON Derivations.evaluation = Evaluations.id \ INNER JOIN Specifications ON Evaluations.specification = Specifications.repo_name \ +LEFT JOIN Outputs ON Outputs.build = Builds.id \ WHERE (:id IS NULL OR (:id = Builds.id)) \ OR (:project IS NULL OR (:project = Specifications.repo_name)) \ OR (:jobset IS NULL OR (:jobset = Specifications.branch)) \ OR (:job IS NULL OR (:job = Derivations.job_name)) \ OR (:system IS NULL OR (:system = Derivations.system)) \ OR (:status IS NULL OR (:status = 'done' AND Builds.status >= 0) OR (:status = 'pending' AND Builds.status < 0)) \ -ORDER BY ~a ~a LIMIT :nr;" order-column-name order)) +ORDER BY ~a ~a, Builds.id ASC LIMIT :nr;" order-column-name order)) (stmt (sqlite-prepare db stmt-text #:cache? #t))) (sqlite-bind-arguments stmt #:id (assqx-ref filters 'id) #:project (assqx-ref filters 'project) @@ -416,8 +496,7 @@ ORDER BY ~a ~a LIMIT :nr;" order-column-name order)) #:nr (match (assqx-ref filters 'nr) (#f -1) (x x))) - (map (cut db-format-build db <>) - (sqlite-fold-right cons '() stmt)))) + (group-outputs (sqlite-fold-right cons '() stmt)))) (define (db-get-build db id) "Retrieve a build in database DB which corresponds to ID." ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-19 15:35 ` Danny Milosavljevic 2018-02-19 15:35 ` [PATCH] database: Simplify 'db-get-builds' Danny Milosavljevic 2018-02-19 17:52 ` [PATCH] database: db-get-builds: Inline output selection Danny Milosavljevic @ 2018-02-19 22:08 ` Danny Milosavljevic 2018-03-02 13:21 ` Ludovic Courtès 2 siblings, 1 reply; 42+ messages in thread From: Danny Milosavljevic @ 2018-02-19 22:08 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel Pushed both. 2 of the unit tests fail with the mysterious error message: unexpected value #<procedure 2798160 at ice-9/boot-9.scm:1048:2 _> What's more, when I copy the contents of the test-assert to right before the test-assert, the problem vanishes and all tests succeed... ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-19 22:08 ` Cuirass news Danny Milosavljevic @ 2018-03-02 13:21 ` Ludovic Courtès 2018-03-02 22:06 ` Ludovic Courtès 2018-03-02 23:29 ` Danny Milosavljevic 0 siblings, 2 replies; 42+ messages in thread From: Ludovic Courtès @ 2018-03-02 13:21 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel Danny Milosavljevic <dannym@scratchpost.org> skribis: > Pushed both. Awesome! Inlining output fetching does not quite have the performance impact I was hoping for. For example, /api/queue remains quite slow (sometimes a tenth of a second but often several seconds.) The problem might be elsewhere though, perhaps some Fiber scheduling issue. > 2 of the unit tests fail with the mysterious error message: > > unexpected value #<procedure 2798160 at ice-9/boot-9.scm:1048:2 _> I fixed that in dbea9790d30ff1c2e80585e9441d45ed0b740e30. Please don’t push when tests fail. Thanks! Ludo’. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-03-02 13:21 ` Ludovic Courtès @ 2018-03-02 22:06 ` Ludovic Courtès 2018-03-02 23:29 ` Danny Milosavljevic 1 sibling, 0 replies; 42+ messages in thread From: Ludovic Courtès @ 2018-03-02 22:06 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel ludo@gnu.org (Ludovic Courtès) skribis: > Danny Milosavljevic <dannym@scratchpost.org> skribis: > >> Pushed both. > > Awesome! > > Inlining output fetching does not quite have the performance impact I > was hoping for. For example, /api/queue remains quite slow (sometimes a > tenth of a second but often several seconds.) The problem might be > elsewhere though, perhaps some Fiber scheduling issue. The slowness must come from elsewhere because cuirass.log on berlin shows that queries take between 0.02 and 0.4 seconds (according to “builds request took …” messages). Ludo’. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-03-02 13:21 ` Ludovic Courtès 2018-03-02 22:06 ` Ludovic Courtès @ 2018-03-02 23:29 ` Danny Milosavljevic 1 sibling, 0 replies; 42+ messages in thread From: Danny Milosavljevic @ 2018-03-02 23:29 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel [-- Attachment #1: Type: text/plain, Size: 509 bytes --] > Inlining output fetching does not quite have the performance impact I > was hoping for. For example, /api/queue remains quite slow (sometimes a > tenth of a second but often several seconds.) The problem might be > elsewhere though, perhaps some Fiber scheduling issue. Hmm, we could examine the query plan and see what's up with it. I don't think this specific SELECT can be made any faster anymore by rewriting the SELECT - but maybe by adding indices or reordering data (index statistics). [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-02-14 13:43 ` Ludovic Courtès 2018-02-14 23:17 ` Ludovic Courtès @ 2018-02-14 23:21 ` Ludovic Courtès 1 sibling, 0 replies; 42+ messages in thread From: Ludovic Courtès @ 2018-02-14 23:21 UTC (permalink / raw) To: guix-devel Hi again! I’ve updated the ‘cuirass’ package to correspond to the current Fibers-based code. If you’re running a Cuirass instance, please give it a try and report any issues you encounter. /var/log/cuirass.log now provides details as to what’s happening, and the HTTP API also gives a pretty accurate view of CI activity. That should help troubleshoot things. Ludo’. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-24 22:12 Cuirass news Ludovic Courtès 2018-01-25 10:55 ` Mathieu Othacehe @ 2018-01-25 21:06 ` Ricardo Wurmus 2018-01-26 11:12 ` Ludovic Courtès 2018-01-25 22:28 ` Danny Milosavljevic ` (2 subsequent siblings) 4 siblings, 1 reply; 42+ messages in thread From: Ricardo Wurmus @ 2018-01-25 21:06 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel Hi Ludo, > Over the last few days, out of frustration ;-), I hacked Cuirass to > improve several things: Oh yeah! That’s great. > • Logging is improved: useful events are logged, including build > started/succeeded/failed (using a variant of what I proposed in the > Guix ‘wip-ui’ branch). This makes it much easier to understand > what’s going on! Finally! Better logging alone would be a reason to celebrate :) IIRC the wip-ui branch parsed the “@”-prefixed messages of the daemon. I didn’t find this in your commits to Cuirass, though. > • Restarting unfinished builds: it’s common, especially when testing, > to interrupt Cuirass, leaving a number of builds unfinished or not > even started. Now Cuirass restarts those upon startup. Also very useful. Does this mean Cuirass resumes work more quickly now whereas previously it would have to compute the full evaluation after a restart? I wonder about commit 49a341866afabe64c8ac3b8d93c64d2b6b20895d: you’re chunking the number of derivations because guix-daemon doesn’t perform well when it is asked to build lots of derivations at once. Is it possible to parse, lock, and run individual derivations in the daemon when presented with lots of them, or is there a good reason why each of these phases is executed for all derivations? > And! This brings a whole set of new bugs that I’m hunting notably on > berlin (which may thus lag behind…). I see that there are a bunch of spawn-fiber invocations with “with-database” bodies. Maybe I remember this wrong, but I thought sqlite doesn’t support concurrent database access. > Overall I think it’ll make Cuirass > easier to work with and more “introspectable”. I think so too. Thanks again. -- Ricardo GPG: BCA6 89B6 3655 3801 C3C6 2150 197A 5888 235F ACAC https://elephly.net ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-25 21:06 ` Ricardo Wurmus @ 2018-01-26 11:12 ` Ludovic Courtès 0 siblings, 0 replies; 42+ messages in thread From: Ludovic Courtès @ 2018-01-26 11:12 UTC (permalink / raw) To: Ricardo Wurmus; +Cc: guix-devel Hello! Ricardo Wurmus <rekado@elephly.net> skribis: >> • Logging is improved: useful events are logged, including build >> started/succeeded/failed (using a variant of what I proposed in the >> Guix ‘wip-ui’ branch). This makes it much easier to understand >> what’s going on! > > Finally! Better logging alone would be a reason to celebrate :) > > IIRC the wip-ui branch parsed the “@”-prefixed messages of the daemon. > I didn’t find this in your commits to Cuirass, though. This is done in 1f701262e1a4a706a341b820796ba31954e1be11. It sometimes misparses these lines or misses some of them because of interleaving, but I think that’s fixable. >> • Restarting unfinished builds: it’s common, especially when testing, >> to interrupt Cuirass, leaving a number of builds unfinished or not >> even started. Now Cuirass restarts those upon startup. > > Also very useful. Does this mean Cuirass resumes work more quickly now > whereas previously it would have to compute the full evaluation after a > restart? Yes, it restarts builds upfront without having to perform an evaluation. > I wonder about commit 49a341866afabe64c8ac3b8d93c64d2b6b20895d: you’re > chunking the number of derivations because guix-daemon doesn’t perform > well when it is asked to build lots of derivations at once. Is it > possible to parse, lock, and run individual derivations in the daemon > when presented with lots of them, or is there a good reason why each of > these phases is executed for all derivations? The daemon-side implementation of ‘build-derivations’ always does that parse/lock/run/monitor sequence, not just when it is passed many derivations. It’s OK when ‘build-derivations’ is passed a reasonable number of derivations, because the parse/lock/run part will take a few seconds at most, so the monitor part (reading the stdout/stderr of the build processes started in the ‘run’ part) happens soon enough. The problem is when there are too many of them: first the parse/lock part takes a lot of time, and then the real problem is that the daemon spawns lots of build processes and starts reading their stdout/stderr long after. A proper fix would be for guix-daemon to perform these activities concurrently… but there’s no Fibers for C++. :-) >> And! This brings a whole set of new bugs that I’m hunting notably on >> berlin (which may thus lag behind…). > > I see that there are a bunch of spawn-fiber invocations with > “with-database” bodies. Maybe I remember this wrong, but I thought > sqlite doesn’t support concurrent database access. I thought that too, but it seems to work, so I thought that maybe it’s OK if those accesses come from the same process. We should definitely check, though! Ludo’. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-24 22:12 Cuirass news Ludovic Courtès 2018-01-25 10:55 ` Mathieu Othacehe 2018-01-25 21:06 ` Ricardo Wurmus @ 2018-01-25 22:28 ` Danny Milosavljevic 2018-01-26 10:47 ` Ludovic Courtès 2018-01-26 0:46 ` Cuirass news Danny Milosavljevic 2018-01-26 17:55 ` Jan Nieuwenhuizen 4 siblings, 1 reply; 42+ messages in thread From: Danny Milosavljevic @ 2018-01-25 22:28 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel Hmmm... is it down right now? I've written a HTML frontend and I always get 504 or timeout from https://berlin.guixsd.org/api/latestbuilds?nr=20 or similar. Sorry if I'm too impatient :-) ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-25 22:28 ` Danny Milosavljevic @ 2018-01-26 10:47 ` Ludovic Courtès 2018-01-28 12:33 ` Cuirass frontend Danny Milosavljevic 0 siblings, 1 reply; 42+ messages in thread From: Ludovic Courtès @ 2018-01-26 10:47 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel Danny Milosavljevic <dannym@scratchpost.org> skribis: > Hmmm... is it down right now? I've written a HTML frontend and I always get 504 or timeout from > > https://berlin.guixsd.org/api/latestbuilds?nr=20 > > or similar. > > Sorry if I'm too impatient :-) I wondered whose browser was polling this URL, now I know. :-) Like I wrote I’m still debugging various issues so this is mostly down for now. Ludo’. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Cuirass frontend 2018-01-26 10:47 ` Ludovic Courtès @ 2018-01-28 12:33 ` Danny Milosavljevic 2018-01-29 17:42 ` Ludovic Courtès 0 siblings, 1 reply; 42+ messages in thread From: Danny Milosavljevic @ 2018-01-28 12:33 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel [-- Attachment #1: Type: text/plain, Size: 884 bytes --] First version of Cuirass status frontend is attached (renamed from "index.html"). It displays, for a given project and jobset, a list of the latest finished builds and the latest queued builds. It also updates this list from time to time (every minute or so). For some reason, Cuirass only returns one project and jobset for the "jobsets" request, so the HTML SELECTs will never contain more than one each :P I can't find a way to get Cuirass to return a list of failed builds so that's not supported. To test it, you need to (1) put it somewhere on https://berlin.guixsd.org/ in order not to be "cross" (in this case, "URLPREFIX" can be simplified), or (2) make Cuirass output special HTTP headers that specify the location from where the scripting request is okay, or (3) disable the cross-site scripting protection in icecat (Add-on "CORS Everywhere"). Requires Javascript. [-- Attachment #2: a.html --] [-- Type: text/html, Size: 21665 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass frontend 2018-01-28 12:33 ` Cuirass frontend Danny Milosavljevic @ 2018-01-29 17:42 ` Ludovic Courtès 0 siblings, 0 replies; 42+ messages in thread From: Ludovic Courtès @ 2018-01-29 17:42 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel Heya, Danny Milosavljevic <dannym@scratchpost.org> skribis: > First version of Cuirass status frontend is attached (renamed from "index.html"). > > It displays, for a given project and jobset, a list of the latest finished builds > and the latest queued builds. It also updates this list from time to time (every > minute or so). Awesome! I committed it in the guix-maintenace.git repo on your behalf and put it on-line at <https://berlin.guixsd.org/status/> (warning: the Cuirass instance on berlin is still in flux so expect it to break occasionally!). > For some reason, Cuirass only returns one project and jobset for the "jobsets" > request, so the HTML SELECTs will never contain more than one each :P Uh uh. That’s a great start, thanks a lot! Eventually we can probably add it to Cuirass proper so that it serves it by default. Thanks! Ludo’. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-24 22:12 Cuirass news Ludovic Courtès ` (2 preceding siblings ...) 2018-01-25 22:28 ` Danny Milosavljevic @ 2018-01-26 0:46 ` Danny Milosavljevic 2018-01-27 17:27 ` Danny Milosavljevic 2018-01-26 17:55 ` Jan Nieuwenhuizen 4 siblings, 1 reply; 42+ messages in thread From: Danny Milosavljevic @ 2018-01-26 0:46 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel Might want this: $ git diff diff --git a/build-aux/guix.scm b/build-aux/guix.scm index c2f6cdb..4075806 100644 --- a/build-aux/guix.scm +++ b/build-aux/guix.scm @@ -81,6 +81,7 @@ "guile-json" "guile-sqlite3" "guile-git" + "guile-fibers" "guix"))) (native-inputs (map spec+package-list Also, guix package -f build-aux/guix.scm 's tests hang... ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-26 0:46 ` Cuirass news Danny Milosavljevic @ 2018-01-27 17:27 ` Danny Milosavljevic 2018-01-28 21:48 ` Ludovic Courtès 0 siblings, 1 reply; 42+ messages in thread From: Danny Milosavljevic @ 2018-01-27 17:27 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel ... and a fix to an example: Note: I'm incapable of making the pass-through of the fetch-repository result through (call-with-output-fdes 1 "/dev/null" (lambda () (fetch-repository spec)) work, thus disabled it. diff --git a/examples/guix-track-git.scm b/examples/guix-track-git.scm index e3531f0..f867c08 100644 --- a/examples/guix-track-git.scm +++ b/examples/guix-track-git.scm @@ -190,20 +190,22 @@ valid." (move->fdes port fdes) result))) -(define* (package->git-tracked pkg #:key (branch "master") commit url) +(define* (package->git-tracked store pkg #:key (branch "master") commit url) (let* ((source (package-source pkg)) (uri (origin-uri source))) - (if (not branch) pkg - (let* ((spec (package->spec pkg #:branch branch #:commit commit #:url url)) - (commit (call-with-output-fdes 1 "/dev/null" - (lambda () (fetch-repository spec)))) - (url (or url (git-reference-url uri))) - (git-dir (string-append (%package-cachedir) "/" (url->file-name url))) - (hash (bytevector->nix-base32-string (file-hash git-dir))) - (source (origin (uri (git-reference (url url) (commit commit))) - (method git-fetch) - (sha256 (base32 hash))))) - (set-fields pkg ((package-source) source)))))) + (if (not branch) + pkg + (let* ((spec (package->spec pkg #:branch branch #:commit commit #:url url))) + (let-values (((checkout commit) + (fetch-repository store spec))) + (let* ((url (or url (git-reference-url uri))) + ; maybe (string-append (%package-cachedir) "/" (url->file-name url)) + (git-dir checkout) + (hash (bytevector->nix-base32-string (file-hash git-dir))) + (source (origin (uri (git-reference (url url) (commit commit))) + (method git-fetch) + (sha256 (base32 hash))))) + (set-fields pkg ((package-source) source)))))))) \f ;;; @@ -215,7 +217,7 @@ valid." (pkg (specification->package name)) (branch (or (assoc-ref arguments 'branch) "master")) (url (assoc-ref arguments 'url)) - (pkg.git (package->git-tracked pkg #:branch branch #:url url)) + (pkg.git (package->git-tracked store pkg #:branch branch #:url url)) (system (or (assoc-ref arguments 'system) "x86_64-linux"))) (parameterize ((%graft? #f)) (list (package-job store (job-name pkg) pkg.git system))))) ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-27 17:27 ` Danny Milosavljevic @ 2018-01-28 21:48 ` Ludovic Courtès 0 siblings, 0 replies; 42+ messages in thread From: Ludovic Courtès @ 2018-01-28 21:48 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: guix-devel Danny Milosavljevic <dannym@scratchpost.org> skribis: > ... and a fix to an example: > > Note: I'm incapable of making the pass-through of the fetch-repository result > through > > (call-with-output-fdes 1 "/dev/null" > (lambda () (fetch-repository spec)) > > work, thus disabled it. > > diff --git a/examples/guix-track-git.scm b/examples/guix-track-git.scm Actually I’ve never tried this example, but if it fixes stuff please push! Thanks, Ludo’. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: Cuirass news 2018-01-24 22:12 Cuirass news Ludovic Courtès ` (3 preceding siblings ...) 2018-01-26 0:46 ` Cuirass news Danny Milosavljevic @ 2018-01-26 17:55 ` Jan Nieuwenhuizen 4 siblings, 0 replies; 42+ messages in thread From: Jan Nieuwenhuizen @ 2018-01-26 17:55 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guix-devel Ludovic Courtès writes: > Over the last few days, out of frustration ;-), I hacked Cuirass to > improve several things: That's *very* good news, the improvement bit I mean :-). Thanks!!! > Feedback welcome! I have been preparing to deploy cuirass but it still runs as a prototype; got my mind on other things atm. The things you fixed were all troublesome. I'll let you know once I get down to deploying it. Greetings, janneke -- Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2018-03-02 23:30 UTC | newest] Thread overview: 42+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-01-24 22:12 Cuirass news Ludovic Courtès 2018-01-25 10:55 ` Mathieu Othacehe 2018-01-25 10:59 ` Mathieu Othacehe 2018-01-25 13:09 ` Ludovic Courtès 2018-01-26 14:30 ` Danny Milosavljevic 2018-01-27 16:01 ` Ludovic Courtès 2018-01-27 17:18 ` Danny Milosavljevic 2018-01-27 19:12 ` Danny Milosavljevic 2018-01-28 21:47 ` Ludovic Courtès 2018-01-28 22:23 ` Danny Milosavljevic 2018-01-29 9:57 ` Andy Wingo 2018-02-08 13:37 ` Ludovic Courtès 2018-02-08 16:29 ` Danny Milosavljevic 2018-02-08 22:21 ` Ludovic Courtès 2018-02-08 23:05 ` Danny Milosavljevic 2018-02-09 6:17 ` Gábor Boskovits 2018-02-09 9:41 ` Ludovic Courtès 2018-02-09 11:29 ` Danny Milosavljevic 2018-02-09 16:53 ` Ludovic Courtès 2018-02-09 17:06 ` Danny Milosavljevic 2018-02-10 11:18 ` Ludovic Courtès 2018-02-13 9:12 ` Danny Milosavljevic 2018-02-14 13:43 ` Ludovic Courtès 2018-02-14 23:17 ` Ludovic Courtès 2018-02-19 15:35 ` Danny Milosavljevic 2018-02-19 15:35 ` [PATCH] database: Simplify 'db-get-builds' Danny Milosavljevic 2018-02-19 17:52 ` [PATCH] database: db-get-builds: Inline output selection Danny Milosavljevic 2018-02-19 22:08 ` Cuirass news Danny Milosavljevic 2018-03-02 13:21 ` Ludovic Courtès 2018-03-02 22:06 ` Ludovic Courtès 2018-03-02 23:29 ` Danny Milosavljevic 2018-02-14 23:21 ` Ludovic Courtès 2018-01-25 21:06 ` Ricardo Wurmus 2018-01-26 11:12 ` Ludovic Courtès 2018-01-25 22:28 ` Danny Milosavljevic 2018-01-26 10:47 ` Ludovic Courtès 2018-01-28 12:33 ` Cuirass frontend Danny Milosavljevic 2018-01-29 17:42 ` Ludovic Courtès 2018-01-26 0:46 ` Cuirass news Danny Milosavljevic 2018-01-27 17:27 ` Danny Milosavljevic 2018-01-28 21:48 ` Ludovic Courtès 2018-01-26 17:55 ` Jan Nieuwenhuizen
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/guix.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).