fix(event cache store): use immediate transactions in handle_linked_chunk_updates
#4410
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4410 +/- ##
==========================================
+ Coverage 85.27% 85.29% +0.01%
==========================================
Files 282 282
Lines 31209 31219 +10
==========================================
+ Hits 26615 26629 +14
+ Misses 4594 4590 -4 ☔ View full report in Codecov by Sentry. |
bnjbvr
force-pushed
the
bnjbvr/fix-sqlite-transaction-mode
branch
from
693dc8a to
1b79fa6
Compare
bnjbvr
changed the title
handle_linked_chunk_updates
| Ok(()) | ||
| }) | ||
| .await?; | ||
| .await.unwrap()?; |
There was a problem hiding this comment.
Copied from the implementation of with_transaction(); the only valid error this can return is because an underlying spawned task has panick'd, which is a valid reason to panic ourselves.
| // Start the transaction in IMMEDIATE mode since all updates may cause writes, to | ||
| // avoid read transactions upgrading to write mode and causing SQLITE_BUSY errors. | ||
| // See also: https://www.sqlite.org/lang_transaction.html#deferred_immediate_and_exclusive_transactions | ||
| conn.set_transaction_behavior(TransactionBehavior::Immediate); |
There was a problem hiding this comment.
Does the thing go back to the default after we call drop() on the connection?
There was a problem hiding this comment.
I haven't seen any sign of such thing, but since most accesses are write anyways (even reading from the media cache causes a write access to update the last_touch time), I figured it was OK. That being said, it's not very future-proof, and there are also a few read transactions. So I think I can manually set it back to Deferred at the end.
There was a problem hiding this comment.
Yeah, let's set it back, otherwise me might slow down things or run into more database locked errors.
| // Start the transaction in IMMEDIATE mode since all updates may cause writes, to | ||
| // avoid read transactions upgrading to write mode and causing SQLITE_BUSY errors. | ||
| // See also: https://www.sqlite.org/lang_transaction.html#deferred_immediate_and_exclusive_transactions | ||
| conn.set_transaction_behavior(TransactionBehavior::Immediate); |
There was a problem hiding this comment.
Yeah, let's set it back, otherwise me might slow down things or run into more database locked errors.
bnjbvr
force-pushed
the
bnjbvr/fix-sqlite-transaction-mode
branch
from
cd16316 to
39b0f35
Compare
…d chunk updates If a linked chunk update starts with a RemoveChunk update, then the transaction may start with a SELECT query and be considered a read transaction. Soon enough, it will be upgraded into a write transaction, because of the next UPDATE/DELETE operations that happen thereafter. If there's another write transaction already happening, this may result in a SQLITE_BUSY error, according to https://www.sqlite.org/lang_transaction.html#deferred_immediate_and_exclusive_transactions One solution is to always start the transaction in immediate mode. This may also fail with SQLITE_BUSY according to the documentation, but it's unclear whether it will happen in general, since we're using WAL mode too. Let's try it out.
bnjbvr
force-pushed
the
bnjbvr/fix-sqlite-transaction-mode
branch
from
39b0f35 to
0a2d5df
Compare
This is unfortunate, but we're hitting a bad case of a read transaction upgrading to a write transaction, and that can plain fail if there's any other write transaction already happening in the background.
To work around this, we need to start the transaction in write mode, so start with the update statements themselves. This means some work is duplicated (fetching a linked chunk), but this should be fine since there's an index on the pair (chunk id, room id).