[Merged by Bors] - sql: add copy-based migrations and VACUUM INTO #6085

ivan4th · 2024-06-27T22:18:51Z

Motivation

Currently, when vacuuming is required after database migration (according to the configured db-vacuum-state value), simple SQL VACUUM command is used. According to the description in the SQLite docs, VACUUM requires as much as twice the size of the original database file in free disk space, as first it makes a copy of the database in the temporary directory, and then it copies the vacuumed database back to the original database utilizing WAL file which grows to the size of the database itself before changes are committed.
According to the remark in the SQLite source

** Only 1x temporary space and only 1x writes would be required if
** the copy of step (3) were replaced by deleting the original database
** and renaming the transient database as the original.  But that will
** not work if other processes are attached to the original database.
** And a power loss in between deleting the original and renaming the
** transient would cause the database file to appear to be deleted
** following reboot.

In case of go-spacemesh, we don't need concurrent access to the database from multiple processes, so we can optimize the vacuuming step to use only 1x space of the original database for vacuuming, given that VACUUM INTO makes a vacuumed copy of the database with no space requirements besides the size of the copy. Moreover, we can use the temporary copy of the database to run migrations faster as we can use PRAGMA journal_mode=OFF and PRAGMA synchronous=OFF for the temporary database safely, just dropping it if something goes wrong during migration.

On top of requiring 2x space, normal VACUUM (without INTO) has another problem: very slow last step in which the original database is replaced. Let's compare VACUUM with VACUUM INTO on a Mac M3 Max laptop:

$ ls -lh /tmp/state.sql*
total 164511960
-rw-r--r--  1 ivan4th  staff    78G Jun 28 00:06 state.sql
-rw-r--r--  1 ivan4th  staff    32K Jun 28 01:32 state.sql-shm
-rw-r--r--  1 ivan4th  staff     0B Jun 28 00:06 state.sql-wal

$ time sqlite3 ~/rmme/sm-data/state.sql vacuum
real    67m48.211s
user    57m13.518s
sys     4m41.778s

$ time sqlite3 ~/rmme/sm-data/state.sql "vacuum into '/tmp/state.sql'"
real    2m27.813s
user    0m43.039s
sys     0m56.265s

As it can be seen, VACUUM INTO is about 27 times faster on this particular machine.

Another problem described in #6069 is that on Linux systems, $TMP directory is utilized during normal VACUUM, and in case if it's a RAM disk the vacuuming process may run out of space.

Description

The process below applies to both state and local database.

When no vacuum is required, the migration process is unchanged, with migrations done in-place and each one wrapped in a transaction.

When vacuuming is needed according to db-vacuum-state, the following steps are performed. Marker files which are DBNAME_done are used to indicate that the temporary database is complete and synced and can be used to replace the source database. The temporary database is named DBNAME_migrate.

The database is opened, so that it is locked and any concurrent go-spacemesh processes will fail to open it at the same time.
A temporary copy of the database is created in the go-spacemesh data directory by means of VACUUM INTO. As it can be seen from above "benchmark", VACUUM INTO is relatively fast; also, this way we may hold the lock on the database by simply having it open during the copying. In addition to that, if the source database became bloated somehow due to unused (free) pages, the copy will require less disk space.
The migrations are run on the temporary copy with PRAGMA journal_mode=OFF and PRAGMA synchronous=OFF.
The temporary database is synced to disk after setting PRAGMA journal_mode=WAL and PRAGMA synchronous=NORMAL.
The marker file is created and synced to disk.
The temporary database is copied over the source database by means of VACUUM INTO.
The marker file and the temporary database are deleted.

Additionally, before opening a database no matter what its schema version is, the following steps are performed:

If the temporary database exists along with the "..._done" marker file, this means that a previous migration was interrupted when replacing the source database, or immediately preceding that; in this case, the temporary database is opened, copied over the source database by means of VACUUM INTO, after which the marker file and the temporary database are deleted.
If the temporary database exists but there's no marker file, this means that previous migration has failed and in this case the temporary database is deleted.

In addition to the above, this PR adds logging of parts of migration SQL statements (first lines) when debug logging is set for stateDb in the logging section of the config. This makes it easier to see what parts of a migration are taking long time to execute.

Closes #6069

Test Plan

Tested on a mainnet node.

Use consistent package/folder structure for local.sql and state.sql databases. When a new database is created, use schema script instead of running all the migrations. Also, check that there's no schema drift by diffing database schema against the schema script after running migrations.

Also, add a migration that removes stray tables from quicksynced DBs

sql/database.go

ivan4th · 2024-08-22T11:29:45Z

bors merge

spacemesh-bors · 2024-08-22T11:29:50Z

👎 Rejected by code reviews

fasmat · 2024-08-22T11:34:25Z

bors merge

spacemesh-bors · 2024-08-22T11:34:29Z

👎 Rejected by code reviews

fasmat · 2024-08-22T11:35:49Z

I think @acud needs to give approval for bors accepting the merge

acud

LGTM, minor nitpicks as usual :)

sql/database_test.go

acud · 2024-08-20T13:45:07Z

sql/schema.go

+	}
+
+	if _, err := db.Exec("PRAGMA synchronous=FULL", nil, nil); err != nil {
+		return fmt.Errorf("PRAGMA journal_mode=OFF: %w", err)


the error annotation here is wrong, maybe we can just instrument like pragma full: %w? otherwise we're using errors to be log messages

acud · 2024-08-20T13:45:19Z

sql/schema.go

+
+	// This should trigger file sync
+	if err := s.setVersion(db, v); err != nil {
+		return err


instrumenting the error would be nice

acud · 2024-08-22T13:38:22Z

sql/database.go

+
+	if config.exclusive {
+		if err := db.startExclusive(); err != nil {
+			return nil, fmt.Errorf("error switching to exclusive mode: %w", err)


nit: the error could be reduced, like start exclusive: %w

acud · 2024-08-22T13:40:09Z

sql/database.go

+			return nil, fmt.Errorf("PRAGMA journal_mode=OFF: %w", err)
+		}
+		if _, err := db.Exec("PRAGMA synchronous=OFF", nil, nil); err != nil {
+			return nil, fmt.Errorf("PRAGMA journal_mode=OFF: %w", err)


wrong error

acud · 2024-08-22T13:51:36Z

sql/database.go

+	// EXCLUSIVE mode, a shared lock is obtained and held. The first time the
+	// database is written, an exclusive lock is obtained and held.
+	if _, err := exec(conn, "PRAGMA locking_mode=EXCLUSIVE", nil, nil); err != nil {
+		db.pool.Put(conn)


these are copied around and can just go into a defer statement after L636

acud · 2024-08-22T13:55:59Z

sql/database.go

+func (db *sqliteDatabase) copyMigrateDB(config *conf) (finalDB *sqliteDatabase, err error) {
+	dbPath, migratedPath, err := dbMigrationPaths(config.uri)
+	if err != nil {
+		return nil, err


would be nice to have error instrumentation here

acud · 2024-08-22T13:56:32Z

sql/database.go

@@ -415,6 +720,13 @@ func (db *sqliteDatabase) WithTxImmediate(
 // Note that Exec will block until database is closed or statement has finished.
 // If application needs to control statement execution lifetime use one of the transaction.
 func (db *sqliteDatabase) Exec(query string, encoder Encoder, decoder Decoder) (int, error) {
+	if err := db.runInterceptors(query); err != nil {
+		return 0, err


would be nice to have error instrumentation here

acud · 2024-08-22T13:57:10Z

sql/database.go

+func handleIncompleteCopyMigration(config *conf) error {
+	dbPath, migratedPath, err := dbMigrationPaths(config.uri)
+	if err != nil {
+		return err


would be nice to have error instrumentation here

ivan4th · 2024-08-22T15:14:17Z

bors merge

------- ## Motivation Currently, when vacuuming is required after database migration (according to the configured `db-vacuum-state` value), simple SQL `VACUUM` command is used. According to the [description](https://www.sqlite.org/lang_vacuum.html#how_vacuum_works) in the SQLite docs, `VACUUM` requires as much as twice the size of the original database file in free disk space, as first it makes a copy of the database in the temporary directory, and then it copies the vacuumed database back to the original database utilizing WAL file which grows to the size of the database itself before changes are committed. According to the [remark in the SQLite source](https://github.com/sqlite/sqlite/blob/105c20648e1b05839fd0638686b95f2e3998abcb/src/vacuum.c#L97-L103) ``` ** Only 1x temporary space and only 1x writes would be required if ** the copy of step (3) were replaced by deleting the original database ** and renaming the transient database as the original. But that will ** not work if other processes are attached to the original database. ** And a power loss in between deleting the original and renaming the ** transient would cause the database file to appear to be deleted ** following reboot. ``` In case of go-spacemesh, we don't need concurrent access to the database from multiple processes, so we can optimize the vacuuming step to use only 1x space of the original database for vacuuming, given that `VACUUM INTO` makes a vacuumed copy of the database with no space requirements besides the size of the copy. Moreover, we can use the temporary copy of the database to run migrations faster as we can use `PRAGMA journal_mode=OFF` and `PRAGMA synchronous=OFF` for the temporary database safely, just dropping it if something goes wrong during migration. On top of requiring 2x space, normal `VACUUM` (without `INTO`) has another problem: very slow last step in which the original database is replaced. Let's compare `VACUUM` with `VACUUM INTO` on a Mac M3 Max laptop: ```console $ ls -lh /tmp/state.sql* total 164511960 -rw-r--r-- 1 ivan4th staff 78G Jun 28 00:06 state.sql -rw-r--r-- 1 ivan4th staff 32K Jun 28 01:32 state.sql-shm -rw-r--r-- 1 ivan4th staff 0B Jun 28 00:06 state.sql-wal $ time sqlite3 ~/rmme/sm-data/state.sql vacuum real 67m48.211s user 57m13.518s sys 4m41.778s $ time sqlite3 ~/rmme/sm-data/state.sql "vacuum into '/tmp/state.sql'" real 2m27.813s user 0m43.039s sys 0m56.265s ``` As it can be seen, `VACUUM INTO` is about 27 times faster on this particular machine. Another problem described in #6069 is that on Linux systems, `$TMP` directory is utilized during normal `VACUUM`, and in case if it's a RAM disk the vacuuming process may run out of space.

spacemesh-bors · 2024-08-22T16:22:29Z

Build failed:

systest-status

ivan4th · 2024-08-22T16:30:50Z

Systests timed out, trying again

ivan4th · 2024-08-22T16:30:58Z

bors try

ivan4th · 2024-08-22T16:31:04Z

bors cancel

spacemesh-bors · 2024-08-22T16:32:06Z

try

Build failed:

systest-status

ivan4th · 2024-08-22T17:26:21Z

bors merge

------- ## Motivation Currently, when vacuuming is required after database migration (according to the configured `db-vacuum-state` value), simple SQL `VACUUM` command is used. According to the [description](https://www.sqlite.org/lang_vacuum.html#how_vacuum_works) in the SQLite docs, `VACUUM` requires as much as twice the size of the original database file in free disk space, as first it makes a copy of the database in the temporary directory, and then it copies the vacuumed database back to the original database utilizing WAL file which grows to the size of the database itself before changes are committed. According to the [remark in the SQLite source](https://github.com/sqlite/sqlite/blob/105c20648e1b05839fd0638686b95f2e3998abcb/src/vacuum.c#L97-L103) ``` ** Only 1x temporary space and only 1x writes would be required if ** the copy of step (3) were replaced by deleting the original database ** and renaming the transient database as the original. But that will ** not work if other processes are attached to the original database. ** And a power loss in between deleting the original and renaming the ** transient would cause the database file to appear to be deleted ** following reboot. ``` In case of go-spacemesh, we don't need concurrent access to the database from multiple processes, so we can optimize the vacuuming step to use only 1x space of the original database for vacuuming, given that `VACUUM INTO` makes a vacuumed copy of the database with no space requirements besides the size of the copy. Moreover, we can use the temporary copy of the database to run migrations faster as we can use `PRAGMA journal_mode=OFF` and `PRAGMA synchronous=OFF` for the temporary database safely, just dropping it if something goes wrong during migration. On top of requiring 2x space, normal `VACUUM` (without `INTO`) has another problem: very slow last step in which the original database is replaced. Let's compare `VACUUM` with `VACUUM INTO` on a Mac M3 Max laptop: ```console $ ls -lh /tmp/state.sql* total 164511960 -rw-r--r-- 1 ivan4th staff 78G Jun 28 00:06 state.sql -rw-r--r-- 1 ivan4th staff 32K Jun 28 01:32 state.sql-shm -rw-r--r-- 1 ivan4th staff 0B Jun 28 00:06 state.sql-wal $ time sqlite3 ~/rmme/sm-data/state.sql vacuum real 67m48.211s user 57m13.518s sys 4m41.778s $ time sqlite3 ~/rmme/sm-data/state.sql "vacuum into '/tmp/state.sql'" real 2m27.813s user 0m43.039s sys 0m56.265s ``` As it can be seen, `VACUUM INTO` is about 27 times faster on this particular machine. Another problem described in #6069 is that on Linux systems, `$TMP` directory is utilized during normal `VACUUM`, and in case if it's a RAM disk the vacuuming process may run out of space.

spacemesh-bors · 2024-08-22T18:12:43Z

Pull request successfully merged into develop.

Build succeeded:

## Motivation We need to vacuum the DB after migrations, and with #6085 merged, we need to use VACUUM INTO based migration for recent DB schema updates

ivan4th added 30 commits June 1, 2024 05:38

sql: fixup: malsync and rewards

6a53628

sql: add migration / schema drift related tests

5db7e18

sql: make schema drift table ignore regexp configurable

c2938b2

sql: refactor localsql / statesql tests

ec3397e

Update CHANGELOG.md

1af47bb

config: fix db presets for db schema drift detection

7bb2b1b

sql: fix review comments

bc98240

sql: add database schema handling docs

895f50f

Merge branch 'develop' into feature/schema-snapshot

6f33999

config: fix build errors

6257174

Fix README.md

aada7c6

Fix go.mod / go.sum

dbea047

sql: use go:generate for database schema files

6bf114a

sql: make schema drift fatal by default

bd62a8e

sql: remove db-ignore-schema-rx config option

c9d1a6c

Also, add a migration that removes stray tables from quicksynced DBs

sql: avoid cyclic dependencies in future coded migrations

beccddd

Merge branch 'develop' into feature/schema-snapshot

8552a96

activation: fix test

2e41e1a

sql: fix tests

dd2ed3e

sql: fix query cache handling

13e8c14

sql: update mocks

fbf2880

sql: fix QueryCache related mocks

6dbc902

node: don't print error twice upon failure

09e36bb

sql: fix schema drift on Windows

9dc3f0c

sql: update docs on schema handling

8bbeeb5

sql, malsync: fix handling of context cancelation

30dcf71

sql: split Schema.Migrate() method

8506114

sql: another fix for Windows newlines in the schema

feb39fe

merge-nodes: fix test naming

77b46d1

ivan4th added 2 commits August 22, 2024 02:39

sql: improve error handling during migrations

3a3bd94

sql: hold exclusive database locks during copy-based migration

f462433

fasmat reviewed Aug 22, 2024

View reviewed changes

sql/database.go Outdated Show resolved Hide resolved

fasmat approved these changes Aug 22, 2024

View reviewed changes

sql: further simplify error handling

3119a63

fasmat approved these changes Aug 22, 2024

View reviewed changes

Remove unneeded errors.Join

11527ab

acud approved these changes Aug 22, 2024

View reviewed changes

Address comments

64f4d38

ivan4th mentioned this pull request Aug 22, 2024

[Merged by Bors] - Use VACUUM when migrating from DB versions below 21 #6283

Closed

spacemesh-bors bot added a commit that referenced this pull request Aug 22, 2024

Try #6085:

c92f168

spacemesh-bors bot changed the title ~~sql: add copy-based migrations and VACUUM INTO~~ [Merged by Bors] - sql: add copy-based migrations and VACUUM INTO Aug 22, 2024

spacemesh-bors bot closed this Aug 22, 2024

spacemesh-bors bot deleted the feature/vacuum-into branch August 22, 2024 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - sql: add copy-based migrations and VACUUM INTO #6085

[Merged by Bors] - sql: add copy-based migrations and VACUUM INTO #6085

ivan4th commented Jun 27, 2024

ivan4th commented Aug 22, 2024

spacemesh-bors bot commented Aug 22, 2024

fasmat commented Aug 22, 2024

spacemesh-bors bot commented Aug 22, 2024

fasmat commented Aug 22, 2024

acud left a comment

acud Aug 20, 2024

ivan4th Aug 22, 2024

acud Aug 20, 2024

ivan4th Aug 22, 2024

acud Aug 22, 2024

ivan4th Aug 22, 2024

acud Aug 22, 2024

ivan4th Aug 22, 2024

acud Aug 22, 2024

ivan4th Aug 22, 2024

acud Aug 22, 2024

ivan4th Aug 22, 2024

acud Aug 22, 2024

ivan4th Aug 22, 2024

acud Aug 22, 2024

ivan4th Aug 22, 2024

ivan4th commented Aug 22, 2024

spacemesh-bors bot commented Aug 22, 2024

ivan4th commented Aug 22, 2024

ivan4th commented Aug 22, 2024

ivan4th commented Aug 22, 2024

spacemesh-bors bot commented Aug 22, 2024

ivan4th commented Aug 22, 2024

spacemesh-bors bot commented Aug 22, 2024

[Merged by Bors] - sql: add copy-based migrations and VACUUM INTO #6085

[Merged by Bors] - sql: add copy-based migrations and VACUUM INTO #6085

Conversation

ivan4th commented Jun 27, 2024

Motivation

Description

Test Plan

ivan4th commented Aug 22, 2024

spacemesh-bors bot commented Aug 22, 2024

fasmat commented Aug 22, 2024

spacemesh-bors bot commented Aug 22, 2024

fasmat commented Aug 22, 2024

acud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan4th commented Aug 22, 2024

spacemesh-bors bot commented Aug 22, 2024

ivan4th commented Aug 22, 2024

ivan4th commented Aug 22, 2024

ivan4th commented Aug 22, 2024

spacemesh-bors bot commented Aug 22, 2024

try

ivan4th commented Aug 22, 2024

spacemesh-bors bot commented Aug 22, 2024