This differential implements methods to create and close connection to SQLite database. It also disables WAL mode on web to prevent issues described here: https://linear.app/comm/issue/ENG-6413/error-while-processing-replace-thread-operation-disk-io-error#comment-b6ef8d83
Details
Details
- Reviewers
kamil michal - Commits
- rCOMM9130b5df4182: Prepare for new connection handling
Future differentials will heavily use connection handling so testing it directly is redundant. To test WAL disabling we must actually use the whole stack:
- Build WASM and run web app without this stack.
- Apply this stack, re-build WASM and run web app again. Ensure not to clear site data or not to run tests in incognito mode! Otherwise you will only test WAL disabling upon fresh db creation but not the migration.
- Ensure refreshing doesn't cause log-out/loss of data.
Execute test plan for native app as well since WAL is also disabled there.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 869–874 ↗ | (On Diff #35793) | not happy to see more #ifndefs... but probably there isn't anything better right now Also, I don't think this is enough, you're planning to land this stack without any migration or logging out users? set_up_database is called when db_version == 0 so when creating DB from scratch, for users already using DB with created schema when you land this stack probably persistence will stop working as described here, because WAL mode is persistent ( point 3.3 in docs). We probably need some sort of migration for the web to disable this. |
| 1060 ↗ | (On Diff #35793) | maybe reverse this condition and early return to reduce indentation? |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.h | ||
| 23 ↗ | (On Diff #35793) | While having some constants like sqliteFilePath or sqlcipherEncryptionKeySize static makes some sense, I have the impression that having a connection itself as static seems not natural. Given that this class is only used with thread_local I think it should be safe to just have a normal field. Not saying it is wrong, just wondering about the reasoning here |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.h | ||
|---|---|---|
| 23 ↗ | (On Diff #35793) | In fact this differential we are not introducing static connection. We have always had static connection. If you take a look at getStorage function in creates storage_t object as static. This object keeps connection to the database internally. If you feel strongly about making connection non-static I am open to that but we should have some sort of analysis since static connection proved to be working correctly (ORM ws working correctly) so we need to be sure that we won't introduce a regression. |
Comment Actions
- Reduce indentation in getConnection method.
- Implement web-specific migration to disable WAL journalling.
- Disable WAL journalling after succesfull restore.
Comment Actions
I updated this diff to disable WAL on web. If you @ashoat or @atul meant to disable WAL across all platforms just let me know and I will change the behaviour.
I'm not sure if @atul's suggestion in this comment was meant to be web-only or across the whole codebase.
Personally, I don't feel strongly. Are there any risks in having a different approach on web vs. native?
Comment Actions
Personally, I don't feel strongly. Are there any risks in having a different approach on web vs. native?
I don't think there are. In D10709 I ensured that WAL is disabled after restoring from native compaction.
Comment Actions
I don’t think there’s much advantage on any platform to using WAL given our use of SQLite. I think ROLLBACK mode is perfectly fine and it would be good to keep things consistent throughout.
(typed this out earlier, but looks like I didn’t submit)
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 480 ↗ | (On Diff #36375) | it feels more like enable_ rollback_journal_mode but not feel strongly - and not sure why we need _web suffix |