Skip to content

Commit

Permalink
Revert "fix(event cache store): start RemoveChunk transactions in wri…
Browse files Browse the repository at this point in the history 
…te mode"

This reverts commit 5b8c751.
  • Loading branch information
@bnjbvr
bnjbvr committed Dec 12, 2024
1 parent 5b8c751 commit 167219f
Showing 1 changed file with 19 additions and 76 deletions.
95 changes: 19 additions & 76 deletions crates/matrix-sdk-sqlite/src/event_cache_store.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -441,44 +441,22 @@ impl EventCacheStore for SqliteEventCacheStore {

trace!(%room_id, "removing chunk @ {chunk_id}");

// Note: previously, we used a single select query to retrieve the
// previous/next values before doing two UPDATE. Unfortunately, this
// means that a read-only transaction may be upgraded to a write
// transaction, which fails if there's another write transaction
// happening elsewhere.
//
// To work-around this, we include the select statement in the update
// statement, so the transaction immediately starts in write mode.
// Find chunk to delete.
let (previous, next): (Option<usize>, Option<usize>) = txn.query_row(
"SELECT previous, next FROM linked_chunks WHERE id = ? AND room_id = ?",
(chunk_id, &hashed_room_id),
|row| Ok((row.get(0)?, row.get(1)?))
)?;

// Replace its previous' next to its own next.
//
// The inner SELECT retrieves the current chunk's next, and it's stored
// as cur.next.

txn.execute(r#"
UPDATE linked_chunks AS lc
SET next = cur.next
FROM
(SELECT id, next
FROM linked_chunks
WHERE id = ? AND room_id = ?
)
AS cur
WHERE lc.next = cur.id AND lc.room_id = ?
"#, (chunk_id, &hashed_room_id, &hashed_room_id))?;
if let Some(previous) = previous {
txn.execute("UPDATE linked_chunks SET next = ? WHERE id = ? AND room_id = ?", (next, previous, &hashed_room_id))?;
}

// Replace its next' previous to its own previous.
txn.execute(r#"
UPDATE linked_chunks AS lc
SET previous = cur.previous
FROM
(SELECT id, previous
FROM linked_chunks
WHERE id = ? AND room_id = ?
)
AS cur
WHERE lc.previous = cur.id AND lc.room_id = ?
"#, (chunk_id, &hashed_room_id, &hashed_room_id))?;
if let Some(next) = next {
txn.execute("UPDATE linked_chunks SET previous = ? WHERE id = ? AND room_id = ?", (previous, next, &hashed_room_id))?;
}

// Now delete it, and let cascading delete corresponding entries in the
// other data tables.
Expand Down Expand Up @@ -801,7 +779,7 @@ mod tests {
use tempfile::{tempdir, TempDir};

use super::SqliteEventCacheStore;
use crate::{event_cache_store::keys, utils::SqliteAsyncConnExt};
use crate::utils::SqliteAsyncConnExt;

static TMP_DIR: Lazy<TempDir> = Lazy::new(|| tempdir().unwrap());
static NUM: AtomicU32 = AtomicU32::new(0);
Expand Down Expand Up @@ -993,12 +971,11 @@ mod tests {
async fn test_linked_chunk_remove_chunk() {
let store = get_event_cache_store().await.expect("creating cache store failed");

let room_id = &*DEFAULT_TEST_ROOM_ID;
let room_id2 = room_id!("!a:b.c");
let room_id = &DEFAULT_TEST_ROOM_ID;

store
.handle_linked_chunk_updates(
&room_id,
room_id,
vec![
Update::NewGapChunk {
previous: None,
Expand All @@ -1024,36 +1001,7 @@ mod tests {
.await
.unwrap();

// Add some updates to another room using the same IDs, to make sure the query
// only affects a single room.
store
.handle_linked_chunk_updates(
room_id2,
vec![
Update::NewGapChunk {
previous: None,
new: ChunkIdentifier::new(42),
next: None,
gap: Gap { prev_token: "raclette".to_owned() },
},
Update::NewGapChunk {
previous: Some(ChunkIdentifier::new(42)),
new: ChunkIdentifier::new(43),
next: None,
gap: Gap { prev_token: "fondue".to_owned() },
},
Update::NewGapChunk {
previous: Some(ChunkIdentifier::new(43)),
new: ChunkIdentifier::new(44),
next: None,
gap: Gap { prev_token: "tartiflette".to_owned() },
},
],
)
.await
.unwrap();

let mut chunks = store.reload_linked_chunk(&room_id).await.unwrap();
let mut chunks = store.reload_linked_chunk(room_id).await.unwrap();

assert_eq!(chunks.len(), 2);

Expand All @@ -1075,17 +1023,15 @@ mod tests {
});

// Check that cascading worked. Yes, sqlite, I doubt you.
let store2 = store.clone();
let gaps = store
.acquire()
.await
.unwrap()
.with_transaction(move |txn| -> rusqlite::Result<_> {
.with_transaction(|txn| -> rusqlite::Result<_> {
let mut gaps = Vec::new();
let hashed_room_id = store2.encode_key(keys::LINKED_CHUNKS, *room_id);
for data in txn
.prepare("SELECT chunk_id FROM gaps WHERE room_id = ? ORDER BY chunk_id")?
.query_map((&hashed_room_id,), |row| row.get::<_, u64>(0))?
.prepare("SELECT chunk_id FROM gaps ORDER BY chunk_id")?
.query_map((), |row| row.get::<_, u64>(0))?
{
gaps.push(data?);
}
Expand All @@ -1095,9 +1041,6 @@ mod tests {
.unwrap();

assert_eq!(gaps, vec![42, 44]);

// Sanity-check: chunks from the second room haven't been touched.
assert_eq!(store.reload_linked_chunk(room_id2).await.unwrap().len(), 3);
}

#[async_test]
Expand Down

0 comments on commit 167219f

Please sign in to comment.