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

[Clément Lassieur]

Hi Ricardo,

Ricardo Wurmus <address@hidden> writes:

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

I removed the 'id' column from the Builds table because it was used as
the primary key, and I wanted the primary key to be 'derivation'.  But
we still need to display the build id in the web interface, and the API
allows to get the builds by id.  We can't use the 'id' column anymore
because AUTOINCREMENT isn't allowed without PRIMARY KEY[1], but we can
use the rowid[2] implicit column instead.

I updated the documentation on my personal branch[3], but I didn't send
it to the ml, because I have sent too many things already ;)

[1]: https://www.sqlite.org/autoinc.html
[2]: https://www.sqlite.org/rowidtable.html
[3]: https://git.lassieur.org/cgit/cuirass.git/

>> 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-build db build)
>>    "Store BUILD in database DB. BUILD eventual outputs are stored
>>  in the OUTPUTS table."

[...]

> This procedure is called when a build is scheduled, isn’t it?

Yes.

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

Indeed, it's just the output paths written in the derivation files.

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

We would need to manually check that the derivation doesn't already
exist.  That would be an extra query so I think it would be less
efficient and it would require more code.  I believe the 'constraint' is
a pretty good mechanism to describe things that shouldn't happen in the
database, I don't know any better way.

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

No, I forgot to remove it.

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

:-) you're welcome!  Thanks for reviewing.

Clément