tapdb: backup sqlite db before running database migration #968
Conversation
ffranr
force-pushed
the
backup-sqlite-db-before-migration
branch
2 times, most recently
from
feecc5c to
53a1a97
Compare
tapdb/sqlite.go
Outdated
| @@ -140,7 +144,41 @@ func NewSqliteStore(cfg *SqliteConfig) (*SqliteStore, error) { | |||
| // Now that the database is open, populate the database with our set of | |||
| // schemas based on our embedded in-memory file system. | |||
| if !cfg.SkipMigrations { | |||
| if err := s.ExecuteMigrations(TargetLatest); err != nil { | |||
| backupAndMigrate := func(mig *migrate.Migrate) error { | |||
There was a problem hiding this comment.
Should we keep the mechanism with the MigrationTarget? So we can do a backup while also being able to target a specific version? Would be handy for unit tests.
There was a problem hiding this comment.
Hard for a skip backup flag to reach that point.
ffranr
force-pushed
the
backup-sqlite-db-before-migration
branch
2 times, most recently
from
d4f8354 to
ad71ab8
Compare
|
I think the unit test is failing because the backup copies the file on disk but the backup procedure doesn't force sqlite to dump the db state to that file. It might be wise to run the command |
ffranr
force-pushed
the
backup-sqlite-db-before-migration
branch
2 times, most recently
from
8d63551 to
c2938e8
Compare
ffranr
force-pushed
the
backup-sqlite-db-before-migration
branch
2 times, most recently
from
fdcaf2a to
14c2a63
Compare
|
I think this PR should be ready for reviews and merge now. I got rid of the big downside risk I was worried about (naively copying the database file). We now execute a I've also added a unit test which checks the backup. |
| timestamp := time.Now().UnixNano() | ||
|
|
||
| // Add the timestamp to the backup name. | ||
| backupFullFilePath := fmt.Sprintf( |
There was a problem hiding this comment.
From https://sqlite.org/lang_vacuum.html
'The file named by the INTO clause must not previously exist, or else it must be an empty file, or the VACUUM INTO command will fail with an error.'
So we should check that the path isn't non-empty before we try the backup, and make an empty file there before the actual VACUUM.
There was a problem hiding this comment.
I read that as: if a file does exist, it must be empty, otherwise it will fail with an error.
If a file does not exist at the backup path at all, then VACUUM INTO will just create one. I don't think we need to add an empty file as a sort of placeholder. And I think we can skip checking for an existing file because of the nanosecond timestamp included in the file name.
There was a problem hiding this comment.
I read it the same way, should have tagged it with a nit. As in, the timestamp should indeed protect us from a file conflict.
Was suggesting the placeholder as a way to catch some errors before we call VACUUM vs. have SQLite err and possibly return less useful information on err.
Non-blocking.
There was a problem hiding this comment.
Was suggesting the placeholder as a way to catch some errors before we call VACUUM vs. have SQLite err and possibly return less useful information on err.
I'm not sure what error info that we might catch with a placeholder that we wouldn't have otherwise seen. Do you know of any? I appreciate that this is a nit now. Just curious.
This commit modifies the database migration procedure. Before performing any actual migration, we first compute the current database migration version and the maximum known migration version (the highest version we could migrate to). This information is logged. We then pass this information into the `MigrationTarget` closure, allowing it to decide whether and how to migrate. This enables us to conditionally create a database backup if a migration upgrade is possible.
This commit adds functionality to back up any existing SQLite database file before attempting a database migration. A backup file is only created if the database driver reports a different current and max database versions. This commit also adds a command line flag called `SkipMigrationDbBackup` which can be set to skip database backup creation before migration.
ffranr
force-pushed
the
backup-sqlite-db-before-migration
branch
from
14c2a63 to
538aa43
Compare
This commit adds functionality to back up any existing SQLite database file before attempting a database migration.
The process involves creating a backup file, migrating the database, and removing the backup if no changes occurred to the
database file. This approach avoids unnecessary backups but results in a backup being created and deleted each time tapd starts.