feat(sqlite): Run VACUUM operation after removing a room #4651
Conversation
Signed-off-by: Kévin Commaille <[email protected]>
A room can be associated to a lot of data, depending on the number of members in the room. So freeing space on the filesystem should be worth it in some cases. Signed-off-by: Kévin Commaille <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4651 +/- ##
==========================================
+ Coverage 85.68% 85.69% +0.01%
==========================================
Files 292 292
Lines 33570 33574 +4
==========================================
+ Hits 28763 28770 +7
+ Misses 4807 4804 -3 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Kévin Commaille <[email protected]>
| }) | ||
| .await?; | ||
|
|
||
| conn.vacuum().await |
There was a problem hiding this comment.
I'm wondering if we can vacuum as part of the transaction, or if it would lead to more trouble? It's not clear from the sqlite doc…
There was a problem hiding this comment.
No, it's not possible it returns an error.
| #[cfg(test)] | ||
| return Err(error.into()); | ||
| } | ||
| conn.vacuum().await?; |
There was a problem hiding this comment.
Preexisting, but does it make sense to vacuum all the time? If only a single media has been removed, it's probably not worth it — especially as vacuuming requires doubling the size of the database on disk, since it creates a copy of the database while it does it. Unfortunately that means heuristics to decide when to trigger, then, and maybe over a certain number of files / total size on disk, that would be profitable and simple enough to trigger often?
| #[cfg(not(test))] | ||
| use tracing::warn; |
There was a problem hiding this comment.
I'd use tracing::warn! at the single place where it's used instead, to not avoid a conditional use statement (which I personally find ugly).
| // We want to know if there is an error with this step during tests. | ||
| #[cfg(test)] |
There was a problem hiding this comment.
Can we enable it if we've compiled in debug mode too? (that is, with #[cfg(debug_assertions)] too)
| @@ -10,6 +10,8 @@ All notable changes to this project will be documented in this file. | |||
|
|
|||
| - Implement the new method of `EventCacheStoreMedia` for `SqliteEventCacheStore`. | |||
| ([#4603](https://github.com/matrix-org/matrix-rust-sdk/pull/4603)) | |||
| - Defragment the state store after removing a room. | |||
There was a problem hiding this comment.
| - Defragment the state store after removing a room. | |
| - Defragment an sqlite state store after removing a room. |
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
A room can be associated to a lot of data, depending on the number of members in the room.
So freeing space on the filesystem should be worth it in some cases.
An (extreme) example: I have a test account that is in ~60 rooms, a few of those big public rooms, including Matrix HQ. The size of the
matrix-sdk-state.sqlite3file is 542 MB. Using this PR and leaving, then forgetting Matrix HQ brings the DB down to 255 MB.