bug#32190: [PATCH] database: Merge Derivations into Builds table.

[Ricardo Wurmus <rekado@elephly.net>]

Hi Clément,

> Fixes <https://bugs.gnu.org/32190>.

Woo!  Thank you for this patch.

> * src/cuirass/base.scm (evaluate): Don't add jobs to the Derivations table.

I see that you’ve mentioned your changes to “build-packages” in a later
email.

I have two general questions about this: why was the change from “id” to
“rowid” necessary?  And: could you please also update the documentation
so that is reflects the changes?

> diff --git a/src/cuirass/database.scm b/src/cuirass/database.scm
> index b4b1652..7788ac9 100644
> --- a/src/cuirass/database.scm
> +++ b/src/cuirass/database.scm
[…]
>  (define (db-add-evaluation db eval)
>    (sqlite-exec db "\
>  INSERT INTO Evaluations (specification, commits) VALUES ("
> @@ -384,27 +356,39 @@ string."
>  (define (db-add-build db build)
>    "Store BUILD in database DB. BUILD eventual outputs are stored
>  in the OUTPUTS table."
> -  (let* ((build-exec
> -          (sqlite-exec db "\
> -INSERT INTO Builds (derivation, evaluation, log, status, timestamp, starttime, stoptime)\
> -  VALUES ("
> -                       (assq-ref build #:derivation) ", "
> -                       (assq-ref build #:eval-id) ", "
> -                       (assq-ref build #:log) ", "
> -                       (or (assq-ref build #:status)
> -                           (build-status scheduled)) ", "
> -                       (or (assq-ref build #:timestamp) 0) ", "
> -                       (or (assq-ref build #:starttime) 0) ", "
> -                       (or (assq-ref build #:stoptime) 0) ");"))
> -         (build-id (last-insert-rowid db)))
> -    (for-each (lambda (output)
> -                (match output
> -                  ((name . path)
> -                   (sqlite-exec db "\
> -INSERT INTO Outputs (build, name, path) VALUES ("
> -                                build-id ", " name ", " path ");"))))
> -              (assq-ref build #:outputs))
> -    build-id))
> +  (catch 'sqlite-error
> +    (lambda ()
> +      (sqlite-exec db "
> +INSERT INTO Builds (derivation, evaluation, job_name, system, nix_name, log,
> +status, timestamp, starttime, stoptime)
> +VALUES ("
> +                   (assq-ref build #:derivation) ", "
> +                   (assq-ref build #:eval-id) ", "
> +                   (assq-ref build #:job-name) ", "
> +                   (assq-ref build #:system) ", "
> +                   (assq-ref build #:nix-name) ", "
> +                   (assq-ref build #:log) ", "
> +                   (or (assq-ref build #:status)
> +                       (build-status scheduled)) ", "
> +                   (or (assq-ref build #:timestamp) 0) ", "
> +                   (or (assq-ref build #:starttime) 0) ", "
> +                   (or (assq-ref build #:stoptime) 0) ");")
> +      (let ((derivation (assq-ref build #:derivation)))
> +        (for-each (lambda (output)
> +                    (match output
> +                      ((name . path)
> +                       (sqlite-exec db "\
> +INSERT INTO Outputs (derivation, name, path) VALUES ("
> +                                    derivation ", " name ", " path ");"))))
> +                  (assq-ref build #:outputs))
> +        derivation))

This procedure is called when a build is scheduled, isn’t it?  The
docstring says “BUILD eventual outputs are stored in the OUTPUTS table.”
– does this mean the names of the *expected* output directories are
recorded in Outputs?  They don’t exist at this point, right?

> +    (lambda (key who code message . rest)
> +      ;; If we get a unique-constraint-failed error, that means we have
> +      ;; already inserted the same build.  That happens when several jobs
> +      ;; produce the same derivation, and we can ignore it.
> +      (if (= code SQLITE_CONSTRAINT_PRIMARYKEY)
> +          #f
> +          (apply throw key who code rest)))))

Okay.

Can we prevent this from happening in the first place?  I feel a little
uncomfortable about performing an operation that we expect to cause
primary key errors regularly.

> diff --git a/src/schema.sql b/src/schema.sql
> index eb0f7e9..0452495 100644
> --- a/src/schema.sql
> +++ b/src/schema.sql
[…]
> --- Builds are not in a one to one relationship with derivations in order to
> --- keep track of non deterministic compilations.

Is this comment still correct considering that the derivation is now the
primary key of the Builds table?

>  CREATE TABLE Builds (
> -  id            INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT,
> -  derivation    TEXT NOT NULL,
> +  derivation    TEXT NOT NULL PRIMARY KEY,
>    evaluation    INTEGER NOT NULL,
> +  job_name      TEXT NOT NULL,
> +  system        TEXT NOT NULL,
> +  nix_name      TEXT NOT NULL,
>    log           TEXT NOT NULL,
>    status        INTEGER NOT NULL,
>    timestamp     INTEGER NOT NULL,
>    starttime     INTEGER NOT NULL,
>    stoptime      INTEGER NOT NULL,
> -  FOREIGN KEY (derivation) REFERENCES Derivations (derivation),
>    FOREIGN KEY (evaluation) REFERENCES Evaluations (id)
>  );

Thanks again!

--
Ricardo