-
Notifications
You must be signed in to change notification settings - Fork 4
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat(backend): Remove
WITHOUT ROWID
from SQLite migrations (#1349)
### Motivation Issue that prompted this was unusually slow inserts when loading large amounts of data into SQLite (using wa-sqlite and IndexedDB). ### Investigation After some investigation, we found that the `WITHOUT ROWID` table specification we add to all SQLite tables makes SQLite store data as a B-Tree rather than a B*-Tree, which causes significant slowdowns for large rows (significant w.r.t to the page size), and also causes the incremental blob I/O mechanism to not work. See https://www.sqlite.org/withoutrowid.html With disabled FKs downstream, simply moving from `WITHOUT ROWID` to ordinary tables with rowids the initial sync in Linearlite for 5k issues (and 25k comments) was reduced from 30 sec to 6-7sec This explanation also matches my observations that cutting down the bodies of the comments/issues to a few characters seemed to significantly speed everything up, as it would make the rows smaller and easily fit within pages and thus the inefficiency of accessing long blobs that comes with regular B-trees doesn't come up I've further confirmed that it's unrelated to WASM boundaries etc by creating a temporary table, inserting data there, and then copying it to the actual table with `INSERT ... SELECT ...` so the data transfer is all done within WASM and SQLite and it's still slow - which makes sense with the above. I've also removed all triggers to ensure that it's not a non-disabled trigger that's causing the slowdown. Additionally, in the Linearlite experiments, moving to having rowids results in a smaller storage footprint despite having to store an additional column of rowids and an index (21mb vs 26mb), and looking at how wa-sqlite stores stuff in IndexedDB, since each entry is a 4kb page, when using `WITHOUT ROWID` a lot of pages are mostly empty (since blobs are not stored sequentially as they are in B*-trees) so it ends up with ~6k pages whereas with rowids it ends up with ~4k pages and sampling the pages I don't seem to find empty blocks of data. ### Requirements If using ordinary tables in SQLite, there are 3 things to consider: 1. It is not necessary to specify a primary key 2. Primary keys are not also checked for being `NOT NULL` by default 3. The oplog needs to ignore the added `rowid` column from using ordinary tables and replication should work seamlessly For (1), the migration proxy will reject any schema that does not specify a primary key as electrification requires it, so even if technically an SQLite schema without a primary key is valid it will not be accepted by the migrations proxy. For (2), the SQLite migrations are generated by translating PG schemas, and that translation will always enforce that primary keys are also `NOT NULL`, so the resulting SQLite migration will always have both a `PRIMARY KEY` constraint along with a `NOT NULL` constraint. For (3), the oplog triggers only capture the actual columns of the specified table and do not include the `rowid`, so the replication is unaffected by this change, at least from what I could tell and test. ### Consequences This update means that any new SQLite migrations generated will not have different table definitions (without `WITHOUT ROWID` in them), but that isn't too much of an issue. I've checked that replication works fine between clients using `WITHOUT ROWID` in their tables and ones without, and clients are ephemeral either way so I wouldn't mark this as a breaking change. That being said, for users to see the performance benefits of this change they _will_ need to regenerate their migrations/clients. ### Notes For the full performance benefit measured, the foreign key checks also need to be disabled when applying transactions in the client, which currently can be done with an optional config key but is not enabled by default. I think we need to consider enabling it by default before releasing a patch.
- Loading branch information
Showing
15 changed files
with
125 additions
and
128 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@core/electric": patch | ||
--- | ||
|
||
Remove `WITHOUT ROWID` specification on SQLite migrations for improved performance. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 changes: 3 additions & 3 deletions
6
clients/typescript/test/support/migrations/20230123_170646_833_test_schema/satellite.sql
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.