Skip to content

Commit 83e4314

Browse files
HywanHywan
committed
fix(sqlite): Fix a UNIQUE constraint violation with Update::RemoveItem.
Imagine we have the following events: | event_id | room_id | chunk_id | position | |----------|---------|----------|----------| | $ev0 | !r0 | 42 | 0 | | $ev1 | !r0 | 42 | 1 | | $ev2 | !r0 | 42 | 2 | | $ev3 | !r0 | 42 | 3 | | $ev4 | !r0 | 42 | 4 | `$ev2` has been removed, then we end up in this state: | event_id | room_id | chunk_id | position | |----------|---------|----------|----------| | $ev0 | !r0 | 42 | 0 | | $ev1 | !r0 | 42 | 1 | | | | | | <- no more `$ev2` | $ev3 | !r0 | 42 | 3 | | $ev4 | !r0 | 42 | 4 | We need to shift the `position` of `$ev3` and `$ev4` to `position - 1`, like so: | event_id | room_id | chunk_id | position | |----------|---------|----------|----------| | $ev0 | !r0 | 42 | 0 | | $ev1 | !r0 | 42 | 1 | | $ev3 | !r0 | 42 | 2 | | $ev4 | !r0 | 42 | 3 | Usually, it boils down to run the following query: ```sql UPDATE event_chunks SET position = position - 1 WHERE position > 2 AND … ``` Okay. But `UPDATE` runs on rows in no particular order. It means that it can update `$ev4` before `$ev3` for example. What happens in this particular case? The `position` of `$ev4` becomes `3`, however `$ev3` already has `position = 3`. Because there is a `UNIQUE` constraint on `(room_id, chunk_id, position)`, it will result in a constraint violation. There is **no way** to control the execution order of `UPDATE` in SQLite. To persuade yourself, try: ```sql UPDATE event_chunks SET position = position - 1 FROM ( SELECT event_id FROM event_chunks WHERE position > 2 AND … ORDER BY position ASC ) as ordered WHERE event_chunks.event_id = ordered.event_id ``` It will fail the same way. Thus, we have 2 solutions: 1. Remove the `UNIQUE` constraint, 2. Be creative. The `UNIQUE` constraint is a safe belt. Normally, we have `event_cache::Deduplicator` that is responsible to ensure there is no duplicated event. However, relying on this is “fragile” in the sense it can contain bugs. Relying on the `UNIQUE` constraint from SQLite is more robust. It's “braces and belt” as we say here. So. We need to be creative. Many solutions exist. Amongst the most popular, we see _dropping and re-creating the index_, which is no-go for us, it's too expensive. I (@Hywan) have adopted the following one: - Do `position = position - 1` but in the negative space, so `position = -(position - 1)`. A position cannot be negative; we are sure it is unique! - Once all candidate rows are updated, do `position = -position` to move back to the positive space. 'told you it's gonna be creative. This solution is a hack, **but** it is a small number of operations, and we can keep the `UNIQUE` constraint in place. This patch updates the `test_linked_chunk_remove_item` to handle 6 events. On _my_ system, with _my_ SQLite version, it triggers the `UNIQUE` constraint violation without the bug fix.
1 parent c726bc5 commit 83e4314

File tree

1 file changed

+125
-11
lines changed

1 file changed

+125
-11
lines changed

‎crates/matrix-sdk-sqlite/src/event_cache_store.rs

Lines changed: 125 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -630,16 +630,120 @@ impl EventCacheStore for SqliteEventCacheStore {
630630
// Remove the entry in the chunk table.
631631
txn.execute("DELETE FROM event_chunks WHERE room_id = ? AND chunk_id = ? AND position = ?", (&hashed_room_id, chunk_id, index))?;
632632

633-
// Decrement the index of each item after the one we're going to remove.
633+
// Decrement the index of each item after the one we are
634+
// going to remove.
635+
//
636+
// Imagine we have the following events:
637+
//
638+
// | event_id | room_id | chunk_id | position |
639+
// |----------|---------|----------|----------|
640+
// | $ev0 | !r0 | 42 | 0 |
641+
// | $ev1 | !r0 | 42 | 1 |
642+
// | $ev2 | !r0 | 42 | 2 |
643+
// | $ev3 | !r0 | 42 | 3 |
644+
// | $ev4 | !r0 | 42 | 4 |
645+
//
646+
// `$ev2` has been removed, then we end up in this
647+
// state:
648+
//
649+
// | event_id | room_id | chunk_id | position |
650+
// |----------|---------|----------|----------|
651+
// | $ev0 | !r0 | 42 | 0 |
652+
// | $ev1 | !r0 | 42 | 1 |
653+
// | | | | | <- no more `$ev2`
654+
// | $ev3 | !r0 | 42 | 3 |
655+
// | $ev4 | !r0 | 42 | 4 |
656+
//
657+
// We need to shift the `position` of `$ev3` and `$ev4`
658+
// to `position - 1`, like so:
659+
//
660+
// | event_id | room_id | chunk_id | position |
661+
// |----------|---------|----------|----------|
662+
// | $ev0 | !r0 | 42 | 0 |
663+
// | $ev1 | !r0 | 42 | 1 |
664+
// | $ev3 | !r0 | 42 | 2 |
665+
// | $ev4 | !r0 | 42 | 3 |
666+
//
667+
// Usually, it boils down to run the following query:
668+
//
669+
// ```sql
670+
// UPDATE event_chunks
671+
// SET position = position - 1
672+
// WHERE position > 2 AND …
673+
// ```
674+
//
675+
// Okay. But `UPDATE` runs on rows in no particular
676+
// order. It means that it can update `$ev4` before
677+
// `$ev3` for example. What happens in this particular
678+
// case? The `position` of `$ev4` becomes `3`, however
679+
// `$ev3` already has `position = 3`. Because there
680+
// is a `UNIQUE` constraint on `(room_id, chunk_id,
681+
// position)`, it will result in a constraint violation.
682+
//
683+
// There is **no way** to control the execution order of
684+
// `UPDATE` in SQLite. To persuade yourself, try:
685+
//
686+
// ```sql
687+
// UPDATE event_chunks
688+
// SET position = position - 1
689+
// FROM (
690+
// SELECT event_id
691+
// FROM event_chunks
692+
// WHERE position > 2 AND …
693+
// ORDER BY position ASC
694+
// ) as ordered
695+
// WHERE event_chunks.event_id = ordered.event_id
696+
// ```
697+
//
698+
// It will fail the same way.
699+
//
700+
// Thus, we have 2 solutions:
701+
//
702+
// 1. Remove the `UNIQUE` constraint,
703+
// 2. Be creative.
704+
//
705+
// The `UNIQUE` constraint is a safe belt. Normally, we
706+
// have `event_cache::Deduplicator` that is responsible
707+
// to ensure there is no duplicated event. However,
708+
// relying on this is “fragile” in the sense it can
709+
// contain bugs. Relying on the `UNIQUE` constraint from
710+
// SQLite is more robust. It's “braces and belt” as we
711+
// say here.
712+
//
713+
// So. We need to be creative.
714+
//
715+
// Many solutions exist. Amongst the most popular, we
716+
// see _dropping and re-creating the index_, which is
717+
// no-go for us, it's too expensive. I (@hywan) have
718+
// adopted the following one:
719+
//
720+
// - Do `position = position - 1` but in the negative
721+
// space, so `position = -(position - 1)`. A position
722+
// cannot be negative; we are sure it is unique!
723+
// - Once all candidate rows are updated, do `position =
724+
// -position` to move back to the positive space.
725+
//
726+
// 'told you it's gonna be creative.
727+
//
728+
// This solution is a hack, **but** it is a small
729+
// number of operations, and we can keep the `UNIQUE`
730+
// constraint in place.
634731
txn.execute(
635732
r#"
636733
UPDATE event_chunks
637-
SET position = position - 1
734+
SET position = -(position - 1)
638735
WHERE room_id = ? AND chunk_id = ? AND position > ?
639736
"#,
640737
(&hashed_room_id, chunk_id, index)
641738
)?;
642-
739+
txn.execute(
740+
r#"
741+
UPDATE event_chunks
742+
SET position = -position
743+
WHERE position < 0 AND room_id = ? AND chunk_id = ?
744+
"#,
745+
(&hashed_room_id, chunk_id)
746+
)?;
643747
}
644748

645749
Update::DetachLastItems { at } => {
@@ -1905,11 +2009,17 @@ mod tests {
19052009
Update::PushItems {
19062010
at: Position::new(ChunkIdentifier::new(42), 0),
19072011
items: vec![
1908-
make_test_event(room_id, "hello"),
1909-
make_test_event(room_id, "world"),
2012+
make_test_event(room_id, "one"),
2013+
make_test_event(room_id, "two"),
2014+
make_test_event(room_id, "three"),
2015+
make_test_event(room_id, "four"),
2016+
make_test_event(room_id, "five"),
2017+
make_test_event(room_id, "six"),
19102018
],
19112019
},
1912-
Update::RemoveItem { at: Position::new(ChunkIdentifier::new(42), 0) },
2020+
Update::RemoveItem {
2021+
at: Position::new(ChunkIdentifier::new(42), 2), /* "three" */
2022+
},
19132023
],
19142024
)
19152025
.await
@@ -1924,25 +2034,29 @@ mod tests {
19242034
assert_eq!(c.previous, None);
19252035
assert_eq!(c.next, None);
19262036
assert_matches!(c.content, ChunkContent::Items(events) => {
1927-
assert_eq!(events.len(), 1);
1928-
check_test_event(&events[0], "world");
2037+
assert_eq!(events.len(), 5);
2038+
check_test_event(&events[0], "one");
2039+
check_test_event(&events[1], "two");
2040+
check_test_event(&events[2], "four");
2041+
check_test_event(&events[3], "five");
2042+
check_test_event(&events[4], "six");
19292043
});
19302044

1931-
// Make sure the position has been updated for the remaining event.
2045+
// Make sure the position have been updated for the remaining events.
19322046
let num_rows: u64 = store
19332047
.acquire()
19342048
.await
19352049
.unwrap()
19362050
.with_transaction(move |txn| {
19372051
txn.query_row(
1938-
"SELECT COUNT(*) FROM event_chunks WHERE chunk_id = 42 AND room_id = ? AND position = 0",
2052+
"SELECT COUNT(*) FROM event_chunks WHERE chunk_id = 42 AND room_id = ? AND position IN (2, 3, 4)",
19392053
(room_id.as_bytes(),),
19402054
|row| row.get(0),
19412055
)
19422056
})
19432057
.await
19442058
.unwrap();
1945-
assert_eq!(num_rows, 1);
2059+
assert_eq!(num_rows, 3);
19462060
}
19472061

19482062
#[async_test]

0 commit comments

Comments
 (0)