Repository navigation
Conversation
| for old in old_ids { | ||
| if !current_heads.contains(old) { | ||
| return Err(StoreError::CasConflict(format!( | ||
| "write race detected on stale old_op_head_ids: head {} no longer active in repo {}", |
There was a problem hiding this comment.
I had a bit of a confusion due to the naming... 'append_row' makes me think that this is purely an additive operation, and 'old_ids' seemed like it was meant for write race detection. Maybe ids_to_remove, and id_to_add would have made this a bit clearer?
There was a problem hiding this comment.
fixed. changed the name to update_op_heads_append_new_remove_old
changed variable names to ids_to_remove and id_to_add (singular)
bfccf98 to
0226ba2
Compare
| async fn update_op_heads( | ||
| // Updates op heads by removing `ids_to_remove` (if present) and appending `id_to_add`. | ||
| // Validates that `ids_to_remove` are currently active to prevent stale client overwrites. | ||
| async fn update_op_heads_append_new_remove_old( |
There was a problem hiding this comment.
Instead of two separate update_op_heads_append_new_remove_old and update_op_heads_compare_and_swap methods, I think you can keep a single update_op_heads:
async fn update_op_heads(
&self,
repo_id: String,
ids_to_remove: &[Vec<u8>],
ids_to_add: &[Vec<u8>],
expected_op_heads: Option<&[Vec<u8>]>,
) -> StoreResult<Vec<Vec<u8>>>;
This would first read the current_opheads. If expected_op_heads is set and is different from current_opheads it returns an error. Otherwise it applies new_opheads = current_opheads - ids_to_remove + ids_to_add. It should fail if new_opheads is empty. Otherwise it updates opheads = new_opheads. The whole thing needs to be atomic.
There was a problem hiding this comment.
Doing it this way makes it possible to update op heads to some arbitrary set of new opheads, and it can easily be used by the reconciliation logic.
There was a problem hiding this comment.
I would prefer to have 2 different function, both atomic transactions. I think the functionality will be the exact same, and I would prefer to have the reconcile vs normal update call the separate database function, rather than call the same one with different arguments, and have it resolve inside with an if statement.
I agree that changing to ids_to_add: &[Vec<u8>], from id_to_add: Vec<u8> is better, I will make the change.
There was a problem hiding this comment.
hmm.. I thought jj's upstream OpHeadsStore trait allowed passing any number of operations to add, but that is not the case; it allows exactly 1 operation to add: https://github.com/jj-vcs/jj/blob/e9f48b3e4c1c5e816fbbfbfd06c154a6a96b0215/lib/src/op_heads_store.rs#L60
It is ok to keep id_to_add: Vec<u8>. Sorry for the backtracking.
There was a problem hiding this comment.
Ok sounds, good. I will leave it as is. no changes made.
Remove the update_op_heads function for the SqliteStore backend. Previously, this sqlite store method to update the op heads did not have any functionality for atomic operations, and had no functionality for op head reconciliation. The previous update_op_heads function simply updated or appended the op heads to the database row. If another concurrent write to the db happened during that time, there either would be undefined behavior, or the op head would have been overwritten, causing data loss. We add 2 new functions in place of this, both using Sqlite transactions, which are atomic read-write operations. The update_op_heads_append_new_remove_old function is invoked only when another op head is to be added, simply removing the old op head and inserting the new one, appending to another row if there are concurrent op heads, it does NOT perform a merge. The compare_and_swap function is invoked in the path of a reconciliation operation. It takes an input of an array of expected op heads and a new reconciled op head (which is calculated by the jj-lib resolve_op_heads function). If the values found in the database during the transaction are not what are expected (meaning that another process has updated the op heads in that time) the transaction fails and throws an Abort CasConflict error. Happy path and failure path tests are added to verify that the compare_and_swap and update_op_heads_append_new_remove_old functions succeed, as well as a failure path test to verify that the CasConflict error is thrown on a conflict.
0226ba2 to
513337c
Compare
store: add sqlite transactions needed for op head reconciliation
Remove the update_op_heads function for the SqliteStore backend. Previously, this sqlite store method to update the op heads did not have any functionality for atomic operations, and had no functionality for op head reconciliation. The previous update_op_heads function simply updated or appended the op heads to the database row. If another concurrent write to the db happened during that time, there either would be undefined behavior, or the op head would have been overwritten, causing data loss.
We add 2 new functions in place of this, both using Sqlite transactions, which are atomic read-write operations. The update_op_heads_append_row function is invoked only when another op head is to be added, simply removing the old op head and inserting the new one, appending to another row if there are concurrent op heads, it does NOT perform a merge. The compare_and_swap function is invoked in the path of a reconciliation operation. It takes an input of an array of expected op heads and a new reconciled op head (which is calculated by the jj-lib resolve_op_heads function). If the values found in the database during the transaction are not what are expected (meaning that another process has updated the op heads in that time) the transaction fails and throws an Abort CasConflict error.
Happy path and failure path tests are added to verify that the compare_and_swap and append_row functions succeed, as well as a failure path test to verify that the CasConflict error is thrown on a conflict.