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.

Behavior

SET <key> <value> IFEQ <comparison-value>

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.84%. Comparing base (105509c) to head (07eaf76).

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.83%   70.84%   +0.01%     
============================================
  Files           118      118              
  Lines         63549    63569      +20     
============================================
+ Hits          45013    45038      +25     
+ Misses        18536    18531       -5     

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

... and 11 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]

@valkey-io/core-team On failure of compare/set instead of nil value we should return an error with old value in it. Otherwise, a client would need to perform another GET operation.

[enjoy-binbin]

return an error with old value seems odd to me. i think in normal cases, a client is unlikely get an error in set IFEQ, they hold the old value in somewhere, and if the old value is unvalid, this mean client should abort the set, this usually means the client should abort the entire business logic. In this case, client should GET the new value as needed and usually they don't really need the new value, they juse want the result of whether the SET succeeded or not.

[hpatro]

@enjoy-binbin I understand if the value is updated from a DB to Valkey as a cache, the client won't benefit much from the value present.

However, if I think Valkey being used as a datastore and value is updated by multiple clients based on the value stored (let say increment old value by 1). for this case, they would need to know the previous value.

Let's hear other devs opinion on this and resolve this.

[madolson]

I like the idea of returning value if the IFEQ fails, not as an error, just as a string. That way you can also check to see if someone else did the same work you did (and silently succeed) or retry you work again.

[zuiderkwast]

With the current implementation of SET key value IFEQ compare GET in this PR, the old value is returned regardless of whether it's matching the compare string or not. This can be used for the use case @madolson described.

Without GET, I don't think we can return the old value on mismatch, because of this corner case: When the old value is the string "OK", if the client library returns "OK" as a string (i.e. if it doesn't distinguish between bulk string and simple string), the user can't tell if "OK" means that the SET succeeded or if just returned the old value which was the string "OK".

[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).

[enjoy-binbin]

can this happen? i see in the above, we already do this getGenericCommand thing, this will handle all these error path i guess?

    if (flags & OBJ_SET_GET) {
        if (getGenericCommand(c) == C_ERR) return;
    }

supporting GET and IFEQ look odd to me. GET will get the old value, isn't the value passed in by IFEQ is the same old value?

[sarthakaggarwal97]

thanks @enjoy-binbin for the review.

can this happen?

We currently just want to support Check and Set if the existing value is string. The check for OBJ_SET_GET flag prevents us to add additional line of response, when get is supposed to already add response before.

supporting GET and IFEQ look odd to me

I feel it's okay to support GET with IFEQ, in case the user gives an incorrect existing value, and GET can help retrieve it. It would also solve the discussion around if IFEQ should return the value incase the input doesn't matches the existing value (adding GET flag can always do that now).

@madolson @hpatro what do you guys think 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.