surrealdb / surrealkv

A low-level, versioned, embedded, ACID-compliant, key-value database for Rust
https://surrealdb.com
Apache License 2.0
293 stars 18 forks source link

Savepoint rollback on an updated key should roll it back to previous version #76

Closed arriqaaq closed 1 month ago

arriqaaq commented 1 month ago

There could be a potential issue here when the same key is overwritten. Let's say we have

    #[tokio::test]
    async fn savepoint_rollback_on_updated_key_should_pass() {
        // Scan set is only considered for SSI.
        let (store, _) = create_store(true);

        let k1 = Bytes::from("k1");
        let value = Bytes::from("test_value1");
        let value2 = Bytes::from("test_value2");

        let mut txn1 = store.begin().unwrap();
        txn1.set(&k1, &value).unwrap();
        txn1.set_savepoint().unwrap();
        txn1.set(&k1, &value2).unwrap();
        txn1.rollback_to_savepoint().unwrap();
        let val = txn1.get(&k1).unwrap();
        assert!(val.is_some());
    }

Because we are deleting the entire key from the snapshot (which does not track the savepoint number), this value is deleted, and could become an issue. The same would apply for scans too because scans also read from the snapshot

arriqaaq commented 1 month ago

One way to solve this is to modify the set function on snapshot

/// Set a key-value pair into the snapshot.
pub fn set(&mut self, key: &VariableSizeKey, value: Bytes) {
    // TODO: need to fix this to avoid cloning the key
    // This happens because the VariableSizeKey transfrom from
    // a &[u8] does not terminate the key with a null byte.
    let key = &key.terminate();
    self.snap.insert(key, value, self.snap.version());
}

to use save points as timestamp during insert than self.snap.version, and then add method on vart (remove_at_ts or something similar) which will not delete the entire key/node from the tree but just the latest value