Revert SQLite WAL journal change #6320
Conversation
|
@friedbunny, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @bleege and @1ec5 to be potential reviewers. |
| @@ -49,7 +49,8 @@ void OfflineDatabase::ensureSchema() { | |||
| case 1: break; // cache-only database; ok to delete | |||
| case 2: migrateToVersion3(); // fall through | |||
| case 3: migrateToVersion4(); // fall through | |||
There was a problem hiding this comment.
Thinking out loud: we could remove OfflineDatabase::migrateToVersion4, and have case 3 fall through to migrateToVersion5() without doing anything itself. But then we'd have two types of databases in the wild: those that have been switched temporarily to WAL and back, and those that haven't. There probably isn't any interesting difference between those two types, but why introduce the variable?
OTOH, what if switching to WAL, even temporarily, can cause an issue in some corner case? Then we would be better off avoiding the migration through v4 when starting with v3.
I don't see a strong argument either way, so I'm fine with keeping this as is here.
There was a problem hiding this comment.
Would it be feasible to smoketest a collection of test fixture databases that go through different subsets of these migration paths?
There was a problem hiding this comment.
Yes. In fact, we should require that every new migration be accompanied by the addition of a test case that tests migrating from the prior version. In local development, migrations are a rarely-exercised code path.
There was a problem hiding this comment.
OTOH, what if switching to WAL, even temporarily, can cause an issue in some corner case? Then we would be better off avoiding the migration through v4 when starting with v3.
That’s a great point — I’d lean towards removing migrateToVersion4 and eliminating any possibility of journal weirdness for the vast majority of users.
There was a problem hiding this comment.
I’ll look at adding migration tests.
There was a problem hiding this comment.
I'm fine with that too.
Can you create MigrateFromV3Schema and MigrateFromV4Schema testcases, using checked-in database fixtures generated by upgrading the existing test/fixtures/offline_database/v2.db fixture to v3 and then v4 prior to this PR?
|
|
Android CI failures are unrelated to these changes, but I’ll wait for those fixes to land before rebasing/pushing for another chance at the 💚. Otherwise, this is ready for review. |
| } | ||
|
|
||
| EXPECT_EQ(5, databaseUserVersion("test/fixtures/offline_database/v5.db")); | ||
| EXPECT_LT(databasePageCount("test/fixtures/offline_database/v5.db"), |
There was a problem hiding this comment.
✂️ this assertion (and in MigrateFromV4Schema too). It's specific to the v2 ⇢ v3 migration (checking that it runs VACUUM). If it passes for other migrations, it's coincidental.
friedbunny
force-pushed
the
fb-revert-WAL
branch
from
317ad5c to
0bdd968
Compare
Reverts #5796, which changed the SQLite database’s journal mode to Write-Ahead Logging (WAL). This was a performance increase, but brought instability to Android (#6193).
journal_mode = DELETEandsynchronous = FULLare the system defaults, but they’re now also set explicitly so that there’s no potential confusion between the default schema and the v5 migration.Fixes #6193 and obviates #6319.
/cc @jfirebaugh @zugaldia