Support fine-grained database schema migrations by edolstra · Pull Request #11668 · NixOS/nix

edolstra · 2024-10-10T15:00:17Z

Motivation

Backward-compatible schema changes (e.g. those that add tables or nullable columns) now no longer need a change to the global schema file (/nix/var/nix/db/schema). Thus, old Nix versions can continue to access the database.

This is especially useful for schema changes required by experimental features. In particular, it replaces the ad-hoc handling of the schema changes for CA derivations (i.e. the file /nix/var/nix/db/ca-schema).

Schema versions 8 and 10 could have been handled by this mechanism in a backward-compatible way as well.

Context

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Mic92 · 2024-10-15T20:42:01Z

Does this also help with the deadlock that are seeing when enabling ca derivations? Currently it requires a reboot of the machine because existing running nix processes will not allow the schema migration from happening as they hold the global lock file.

edolstra · 2024-10-16T15:36:02Z

@Mic92 It should help with the deadlock since it doesn't hold the global lock anymore while doing SQLite schema migrations. (Maybe it should, but AFAIK SQLite doesn't need it.)

Mic92 · 2024-10-16T17:03:59Z

Great that's my main road block to deploy this by default on a larger amount of machines

nixos-discourse · 2024-10-24T09:02:13Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-10-16-nix-team-meeting-minutes-187/54835/1

Backward-compatible schema changes (e.g. those that add tables or nullable columns) now no longer need a change to the global schema file (/nix/var/nix/db/schema). Thus, old Nix versions can continue to access the database. This is especially useful for schema changes required by experimental features. In particular, it replaces the ad-hoc handling of the schema changes for CA derivations (i.e. the file /nix/var/nix/db/ca-schema). Schema versions 8 and 10 could have been handled by this mechanism in a backward-compatible way as well.

bryanhonof

To me, this looks good. Haven't tested it out though, also not sure how one might test this.

Mic92 · 2024-11-06T05:52:02Z

To me, this looks good. Haven't tested it out though, also not sure how one might test this.

On a system without ca-derivations, enable ca-derivations and see if they work.

Ericson2314 · 2024-11-18T01:54:53Z

I don't yet fully understand the full lifecycle here.

For example:

What does one do if they need to make a "breaking" change to the CA schema while its experimental? It is non-breaking from the non-experimental status quo, but breaking from the last CA change.
What does one do if CA derivations is merged (not experimental) and then we make some sort of cross-cutting breaking change?

I think what this change is doing is grafting on a powerset of feature names in top of the last breaking migration, but I don't think that is sufficient to make this sustainable.

Mic92 · 2024-11-19T15:46:45Z

I don't yet fully understand the full lifecycle here.

For example:
1. What does one do if they need to make a "breaking" change to the CA schema while its experimental? It is non-breaking from the non-experimental status quo, but breaking from the last CA change.

2. What does one do if CA derivations is merged (not experimental) and then we make some sort of cross-cutting breaking change?
I think what this change is doing is grafting on a powerset of feature names in top of the last breaking migration, but I don't think that is sufficient to make this sustainable.

We should not break the schema, experimental or not. We would append a migration after 20220326-ca-derivations if we need to update it. And when we remove the experimental feature status we will remove the if statement and apply this migration unconditionally. If we drop the feature, we would just ignore those extra columns that some nix installations might have in their database. Of course we need to make sure that we don't have different experimental features that introduce conflicting schemas, but we need to make sure this doesn't happen in any case.

Ericson2314 · 2024-11-19T19:19:09Z

@Mic92 Agreed. my understanding is this fine-grained approach assumes each named migration is independent of any other. It works for 2^n schemas freely generated by n non-conflicted migrations, but breaks down for when the partial order is not such a free semi-lattice (power set).

Mic92 · 2024-11-20T18:42:31Z

@Mic92 Agreed. my understanding is this fine-grained approach assumes each named migration is independent of any other. It works for 2^n schemas freely generated by n non-conflicted migrations, but breaks down for when the partial order is not such a free semi-lattice (power set).

Well assuming we don't go crazy with experimental features, we should be able to test all combinations of migrations.

Ericson2314 · 2024-11-20T20:49:59Z

src/libstore/local-store.cc

+
+    std::set<std::string> schemaMigrations;
+
+    {


Suggested change

{

// Load all the names of the migrations we have already done

{

Ericson2314 · 2024-11-20T20:52:29Z

src/libstore/local-store.cc

+            schemaMigrations.insert(useQuerySchemaMigrations.getStr(0));
+    }
+
+    auto doUpgrade = [&](const std::string & migrationName, const std::string & stmt)


Suggested change

auto doUpgrade = [&](const std::string & migrationName, const std::string & stmt)

// Do a migration if it is not already done.

auto doUpgrade = [&](const std::string & migrationName, const std::string & stmt)

Ericson2314 · 2024-11-20T20:53:17Z

src/libstore/local-store.cc

-static int getSchema(Path schemaPath)
-{
-    int curSchema = 0;
-    if (pathExists(schemaPath)) {
-        auto s = readFile(schemaPath);
-        auto n = string2Int<int>(s);
-        if (!n)
-            throw Error("'%1%' is corrupt", schemaPath);
-        curSchema = *n;
-    }
-    return curSchema;
-}


N.B. this was just inlined.

Ericson2314

Whoops I was thinking this was more advanced than it was (but less advanced than it need to be).

I see right now it's just memoization of schemas, and the logic of what to migrate (including dependencies) is out of scope. That's fine!

nixos-discourse · 2024-11-27T20:00:14Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-11-20-nix-team-meeting-minutes-196/56688/1

edolstra requested a review from Ericson2314 as a code owner October 10, 2024 15:00

github-actions bot added the store Issues and pull requests concerning the Nix store label Oct 10, 2024

edolstra force-pushed the schema-migrations branch from 3865587 to 79b1708 Compare October 10, 2024 15:08

edolstra assigned Ericson2314 Oct 16, 2024

edolstra mentioned this pull request Oct 25, 2024

Store path provenance tracking #11749

Draft

edolstra force-pushed the schema-migrations branch 2 times, most recently from 5e27a7c to 517e5da Compare October 25, 2024 12:55

edolstra force-pushed the schema-migrations branch from 517e5da to 27ea437 Compare October 25, 2024 16:51

bryanhonof approved these changes Nov 4, 2024

View reviewed changes

Ericson2314 reviewed Nov 20, 2024

View reviewed changes

Ericson2314 approved these changes Nov 20, 2024

View reviewed changes

Ericson2314 merged commit 82f6fba into NixOS:master Nov 20, 2024

edolstra deleted the schema-migrations branch November 21, 2024 10:32

	{
	// Load all the names of the migrations we have already done
	{

	auto doUpgrade = [&](const std::string & migrationName, const std::string & stmt)
	// Do a migration if it is not already done.
	auto doUpgrade = [&](const std::string & migrationName, const std::string & stmt)

Uh oh!

Conversation

edolstra commented Oct 10, 2024

Motivation

Context

Priorities and Process

Uh oh!

Mic92 commented Oct 15, 2024

Uh oh!

edolstra commented Oct 16, 2024

Uh oh!

Mic92 commented Oct 16, 2024

Uh oh!

nixos-discourse commented Oct 24, 2024

Uh oh!

bryanhonof left a comment

Choose a reason for hiding this comment

Uh oh!

Mic92 commented Nov 6, 2024

Uh oh!

Ericson2314 commented Nov 18, 2024

Uh oh!

Mic92 commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mic92 commented Nov 20, 2024

Uh oh!

Ericson2314 Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

Ericson2314 left a comment

Choose a reason for hiding this comment

Uh oh!

nixos-discourse commented Nov 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Mic92 commented Nov 19, 2024 •

edited

Loading

Ericson2314 commented Nov 19, 2024 •

edited

Loading

Ericson2314 Nov 20, 2024 •

edited

Loading