Set Command with IFEQ Support

[sarthakaggarwal97]

This PR allows the Valkey users to perform conditional updates where the SET command is completed if the given comparison-value matches the key’s current value.

Syntax:

SET key value IFEQ comparison-value

Behavior:

If the values match, the SET completes as expected. If they do not match, the command returns a (nil), except if the GET argument is also given (see below).

Behavior with Additional Flags:

1. SET key value IFEQ comparison-value GET returns the existing value, regardless of whether it matches comparison-value or not. The conditional set operation is performed if the given comparison value matches the existing value. To check if the SET succeeded, the caller needs to check if the returned string matches the comparison-value.
2. SET key value IFEQ comparison-value XX is a syntax error.
3. SET key value IFEQ comparison-value NX is a syntax error.

Closes: #1215

[hpatro]

Would be nice to think through the behavior with other flags/parameters of SET command https://valkey.io/commands/set/

NX -- Only set the key if it does not already exist - should we error in this case if CAS is provided?
XX -- Only set the key if it already exists - This becomes redundant to use I believe with CAS.
GET -- Return the old string stored at key, or nil if key did not exist. An error is returned and SET aborted if the value stored at key is not a string. - I think we need to support this parameter in some form. The scenario which comes to my mind is when CAS fails and a user doesn't need to send a GET command again to find out the value stored in the engine. However, we need to think about how to differentiate between success scenario vs failure scenario.

Also, we need to document them.

[codecov]

Codecov Report

Attention: Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.83%. Comparing base (4f61034) to head (73a7463).
Report is 4 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/t_string.c	96.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1324      +/-   ##
============================================
- Coverage     70.85%   70.83%   -0.02%     
============================================
  Files           118      118              
  Lines         63591    63611      +20     
============================================
+ Hits          45058    45062       +4     
- Misses        18533    18549      +16     

Files with missing lines	Coverage Δ
src/commands.def	100.00% <ø> (ø)
src/t_string.c	96.68% <96.66%> (-0.07%)	⬇️

... and 10 files with indirect coverage changes

[hpatro]

@sarthakaggarwal97 Please avoid force pushing. force push removes the reviewer's history in Github and one needs to look at the entire change again.

[sarthakaggarwal97]

noted @hpatro, will avoid it.

[hpatro]

Thanks @sarthakaggarwal97 for the PR! Looks pretty good. Could you also document the behavior in the top comment. Will be easier for others to review and we can finalize it.

[hpatro]

LGTM overall. The behavior currently is:

1. if value matches, set the new value.
2. If existing value is of different type, return error WRONGTYPE Operation against a key holding the wrong kind of value
3. if existing value is a mismatch, return nil. (want us to finalize on this).

[zuiderkwast]

Let's make IFEQ mutually exclusive with NX and XX. It doesn't make sense to combine them so let's make it a syntax error now. (If we don't, we can't fix it in the future without a breaking change.)

SET key value [ NX | XX | IFEQ comparison-value ] [ other args... ]

In the JSON file, putting them in the same "oneOf" block, this is also used for the website and man pages where the syntax is rendered.

Regarding the combining IFEQ with the GET parameter will be difficult to use, but not impossible with the behaviour you described. If SET replied with the 'comparison-value', it means the SET has succeeded. I think it's logical, even if it will probably be very rarely used.

[zuiderkwast]

This looks pretty good. Just don't want to allow IFEQ combined with NX and XX.

[zuiderkwast]

Great, LGTM now.

Just a few nits. Not a blocker for merging.

[zuiderkwast]

@madolson @hpatro @enjoy-binbin Let's finalize the exact behavior with the various flags. This is the currently implemented logic:

• IFEQ is mutually exclusive with NX and XX. They can't be combined.
• IFEQ without GET: On success +OK and on mismatch (nil) is returned. This matches the behavior of XX and NX.
• IFEQ with GET: In this case, the old value is returned regardless of whether the comparison succeeded.

Please confirm ( 👍 ) or protest ( 😱 ).

[hpatro]

• IFEQ without GET: On success +OK and on mismatch (nil) is returned. This matches the behavior of XX and NX.
• IFEQ with GET: In this case, the old value is returned regardless of whether the comparison succeeded.

What I like about this is, it's opt in. If the user actually wants the old value only then they get it. And, if the stored value is large, they could choose to not get it back.

[madolson]

We discussed this a bit in the weekly meeting, a callout was that we want to make sure that clients add support for this.

The most significant callout was that there is some uncertainty now if this is the right API. The ask was that we should spend more time to figure out if this is the best way to solve the original use cases, so if we could create some examples in the documentation page about what problem this solves and why it's better than LUA or multi, we will follow up. @hpatro and @sarthakaggarwal97 let me know if you have any questions.

[zuiderkwast]

Client changes?

That's only if the client does some detailed wrapping of each and every command argument, right? Is that really considered good practice? I would like to call it an anti-pattern.

No changes are needed for any of the clients I've been involved in: libvalkey (hiredis-fork), valkey-cli and ered (an Erlang client). A command line is just represented as a list of strings and called like client.call("SET", "foo", "bar", "IFEQ", "baz"). Why do some clients do it differently? It just seems very unpractical.

[soloestoy]

• IFEQ with GET: In this case, the old value is returned regardless of whether the comparison succeeded.

When using GET parameters, the result of SET IFEQ is ambiguous, and we cannot intuitively determine whether the command successfully modified the data. There is a similar issue with NX and XX. I have submitted an issue #1415 to discuss this problem.

[zuiderkwast]

why it's better than LUA or multi

For simple use cases, it's simpler than Lua and WATCH + MULTI. It's also faster. (With WATCH + MULTI you need two round trips to the server.)

[madolson]

That's only if the client does some detailed wrapping of each and every command argument, right? Is that really considered good practice? I would like to call it an anti-pattern.

I mean, I'll pin it for a future discussion, but all of the GLIDE clients have fully typed inputs and outputs so that it's easier for developers to see what the output will look like instead of having to infer it.

[PingXie]

but all of the GLIDE clients have fully typed inputs

I think @soloestoy's question in #1415 is about semantics, i.e., nil could mean both failure and success. The type of the return value for set however is still string with this change.