Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(*)!: Adds cas operation and fixes keys cursor #46

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

thomastaylor312
Copy link
Collaborator

This PR incorporates some feedback received on the current draft interface. First off, this changes the cursor for listing keys to be a string instead of a number in order to support more systems.

The second changes adds back in a CAS operation as multiple people had been asking for it. See the discussion in #44 for more context.

Because these are breaking changes and there are multiple draft implementations out there, I bumped this to draft2. Ideally we should be able to move to an RC soon with these changes added in

Closes #44
Closes #45

@thomastaylor312
Copy link
Collaborator Author

Marked this as a draft until we've finished any conversation in #44.

Also cc @kate-goldenring for visibility

wit/atomic.wit Outdated Show resolved Hide resolved
This PR incorporates some feedback received on the current draft interface.
First off, this changes the cursor for listing keys to be a string instead
of a number in order to support more systems.

The second changes adds back in a CAS operation as multiple people had
been asking for it. See the discussion in WebAssembly#44 for more context.

Because these are breaking changes and there are multiple draft
implementations out there, I bumped this to draft2. Ideally we should be
able to move to an RC soon with these changes added in

Closes WebAssembly#44
Closes WebAssembly#45

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
@thomastaylor312
Copy link
Collaborator Author

I did a little noodling with @devigned and came up with something much cleaner. Curious what people think

When checking this operation across stores, I noticed that everything was
using signed numbers so you can increment or decrement as needed

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
@thomastaylor312
Copy link
Collaborator Author

Also, one last change after looking at the increment functionality across databases. The number and the delta should be signed integers so the counter can be decremented as needed

@kate-goldenring
Copy link
Contributor

With Spin, right now, we are only focused on implementing the store interface, so these additions are not anything we have to iterate on and by design LGTM.

Comment on lines +25 to +32
resource cas {
/// Construct a new CAS operation. Implementors can map the underlying functionality
/// (transactions, versions, etc) as desired.
new: static func(bucket: borrow<bucket>, key: string) -> result<cas, error>;
/// Get the current value of the key (if it exists). This allows for avoiding reads if all
/// that is needed to ensure the atomicity of the operation
current: func() -> result<option<list<u8>>, error>;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious to hear your motivations / rational behind the cas resource design? My feeling for that when I first saw it was complicated and more unintuitive than a simple operation like compare-and-swap: func(key: string, old: option<list<u8>>, new: option<list<u8>>) -> result<bool, error>;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gives a lot more flexibility for how implementors can solve this. As seen in the survey of keyvalue stores I did, the most common things were transactions or revision_id-based options, along with some direct comparing and swapping. Representing this as a resource allows implementations to keep track of state for the operation. So for each of the 3 aforementioned types of operation, this is what is would roughly look like:

  • Transactions: An implementation can start a transaction and link it to a resource. You can still fetch the current value if you wish to do a comparison, but otherwise can use the guarantee of the transaction to ensure that you aren't clobbering a write
  • Revision IDs: Implementations can fetch the current revision of the key along with the value (or lazily load the value if current is called) and keep that revision ID around as its state. Then when the value is swapped it can use the revision ID
  • Direct compare and swap: The implementation wouldn't actually have any state but could fetch the value and do the comparison on the back end. As mentioned, this was by far the least used option among all the stores I surveyed

Forcing everyone to do a direct compare and swap means they have to a) fetch the value and b) send the other value. This could be much less efficient, especially if the values are large. It basically forces everything to implement it using a much less efficient way (that is much more vulnerable to the ABA problem you mentioned below) even though it is the least common option. The other options at least reduce the possibility of or eliminate the ABA problem as well, addressing your other comment)


/// Perform the swap on a CAS operation. This consumes the CAS handle and returns an error if
/// the CAS operation failed.
swap: func(cas: cas, value: list<u8>) -> result<_, cas-error>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A common problem for the implementation of compare-and-swap operation is that it suffers from the ABA problem. I am hoping to see documentation of this API to guide the host implementors on the expectation. Just mentioning "this API might suffer from the ABA problem" is fine to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The cursor in list-keys should probably be a string, not a u64 Test-and-set and other atomic operations?
4 participants