Set Command with IFEQ Support #1324
+149
−34
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -67,15 +67,16 @@ static int checkStringLength(client *c, long long size, long long append) { | |||||
| * If abort_reply is NULL, "$-1" is used. */ | ||||||
|
|
||||||
| #define OBJ_NO_FLAGS 0 | ||||||
| #define OBJ_SET_NX (1 << 0) /* Set if key not exists. */ | ||||||
| #define OBJ_SET_XX (1 << 1) /* Set if key exists. */ | ||||||
| #define OBJ_EX (1 << 2) /* Set if time in seconds is given */ | ||||||
| #define OBJ_PX (1 << 3) /* Set if time in ms in given */ | ||||||
| #define OBJ_KEEPTTL (1 << 4) /* Set and keep the ttl */ | ||||||
| #define OBJ_SET_GET (1 << 5) /* Set if want to get key before set */ | ||||||
| #define OBJ_EXAT (1 << 6) /* Set if timestamp in second is given */ | ||||||
| #define OBJ_PXAT (1 << 7) /* Set if timestamp in ms is given */ | ||||||
| #define OBJ_PERSIST (1 << 8) /* Set if we need to remove the ttl */ | ||||||
| #define OBJ_SET_IFEQ (1 << 9) /* Set if we need compare and set */ | ||||||
|
|
||||||
| /* Forward declaration */ | ||||||
| static int getExpireMillisecondsOrReply(client *c, robj *expire, int flags, int unit, long long *milliseconds); | ||||||
|
|
@@ -87,7 +88,8 @@ void setGenericCommand(client *c, | |||||
| robj *expire, | ||||||
| int unit, | ||||||
| robj *ok_reply, | ||||||
| robj *abort_reply, | ||||||
| robj *comparison) { | ||||||
| long long milliseconds = 0; /* initialized to avoid any harmness warning */ | ||||||
| int found = 0; | ||||||
| int setkey_flags = 0; | ||||||
|
|
@@ -100,7 +102,27 @@ void setGenericCommand(client *c, | |||||
| if (getGenericCommand(c) == C_ERR) return; | ||||||
| } | ||||||
|
|
||||||
| robj *existing_value = lookupKeyWrite(c->db, key); | ||||||
| found = existing_value != NULL; | ||||||
|
|
||||||
| /* Handle the IFEQ conditional check */ | ||||||
| if (flags & OBJ_SET_IFEQ && found) { | ||||||
| if (!(flags & OBJ_SET_GET) && checkType(c, existing_value, OBJ_STRING)) { | ||||||
|
There was a problem hiding this comment. can this happen? i see in the above, we already do this getGenericCommand thing, this will handle all these error path i guess? 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? There was a problem hiding this comment. thanks @enjoy-binbin for the review.
We currently just want to support Check and Set if the existing value is string. The check for
I feel it's okay to support |
||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| if (compareStringObjects(existing_value, comparison) != 0) { | ||||||
| if (!(flags & OBJ_SET_GET)) { | ||||||
| addReply(c, abort_reply ? abort_reply : shared.null[c->resp]); | ||||||
| } | ||||||
| return; | ||||||
| } | ||||||
| } else if (flags & OBJ_SET_IFEQ && !found) { | ||||||
| if (!(flags & OBJ_SET_GET)) { | ||||||
| addReply(c, abort_reply ? abort_reply : shared.null[c->resp]); | ||||||
| } | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| if ((flags & OBJ_SET_NX && found) || (flags & OBJ_SET_XX && !found)) { | ||||||
| if (!(flags & OBJ_SET_GET)) { | ||||||
|
|
@@ -208,7 +230,7 @@ static int getExpireMillisecondsOrReply(client *c, robj *expire, int flags, int | |||||
| * string arguments used in SET and GET command. | ||||||
| * | ||||||
| * Get specific commands - PERSIST/DEL | ||||||
| * Set specific commands - XX/NX/GET/IFEQ | ||||||
| * Common commands - EX/EXAT/PX/PXAT/KEEPTTL | ||||||
| * | ||||||
| * Function takes pointers to client, flags, unit, pointer to pointer of expire obj if needed | ||||||
|
|
@@ -219,7 +241,7 @@ static int getExpireMillisecondsOrReply(client *c, robj *expire, int flags, int | |||||
| * Input flags are updated upon parsing the arguments. Unit and expire are updated if there are any | ||||||
| * EX/EXAT/PX/PXAT arguments. Unit is updated to millisecond if PX/PXAT is set. | ||||||
| */ | ||||||
| int parseExtendedStringArgumentsOrReply(client *c, int *flags, int *unit, robj **expire, robj **compare_val, int command_type) { | ||||||
| int j = command_type == COMMAND_GET ? 2 : 3; | ||||||
| for (; j < c->argc; j++) { | ||||||
| char *opt = c->argv[j]->ptr; | ||||||
|
|
@@ -295,7 +317,17 @@ int parseExtendedStringArgumentsOrReply(client *c, int *flags, int *unit, robj * | |||||
| *unit = UNIT_MILLISECONDS; | ||||||
| *expire = next; | ||||||
| j++; | ||||||
| } else if ((opt[0] == 'i' || opt[0] == 'I') && | ||||||
| (opt[1] == 'f' || opt[1] == 'F') && | ||||||
| (opt[2] == 'e' || opt[2] == 'E') && | ||||||
| (opt[3] == 'q' || opt[3] == 'Q') && opt[4] == '\0' && | ||||||
| next && (command_type == COMMAND_SET)) | ||||||
| { | ||||||
| *flags |= OBJ_SET_IFEQ; | ||||||
| *compare_val = next; | ||||||
| j++; | ||||||
| } | ||||||
| else { | ||||||
| addReplyErrorObject(c,shared.syntaxerr); | ||||||
| return C_ERR; | ||||||
| } | ||||||
|
|
@@ -305,33 +337,34 @@ int parseExtendedStringArgumentsOrReply(client *c, int *flags, int *unit, robj * | |||||
| } | ||||||
|
|
||||||
| /* SET key value [NX] [XX] [KEEPTTL] [GET] [EX <seconds>] [PX <milliseconds>] | ||||||
| * [EXAT <seconds-timestamp>][PXAT <milliseconds-timestamp>] [IFEQ <comparison-value>] */ | ||||||
|
Comment on lines
339
to
+340
There was a problem hiding this comment. Something like that is more in line with the actual syntax. On website and man pages we use lowercase for syntax variables. No |
||||||
| void setCommand(client *c) { | ||||||
| robj *expire = NULL; | ||||||
| robj *comparison = NULL; | ||||||
| int unit = UNIT_SECONDS; | ||||||
| int flags = OBJ_NO_FLAGS; | ||||||
|
|
||||||
| if (parseExtendedStringArgumentsOrReply(c, &flags, &unit, &expire, &comparison, COMMAND_SET) != C_OK) { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| c->argv[2] = tryObjectEncoding(c->argv[2]); | ||||||
| setGenericCommand(c, flags, c->argv[1], c->argv[2], expire, unit, NULL, NULL, comparison); | ||||||
| } | ||||||
|
|
||||||
| void setnxCommand(client *c) { | ||||||
| c->argv[2] = tryObjectEncoding(c->argv[2]); | ||||||
| setGenericCommand(c, OBJ_SET_NX, c->argv[1], c->argv[2], NULL, 0, shared.cone, shared.czero, NULL); | ||||||
| } | ||||||
|
|
||||||
| void setexCommand(client *c) { | ||||||
| c->argv[3] = tryObjectEncoding(c->argv[3]); | ||||||
| setGenericCommand(c, OBJ_EX, c->argv[1], c->argv[3], c->argv[2], UNIT_SECONDS, NULL, NULL, NULL); | ||||||
| } | ||||||
|
|
||||||
| void psetexCommand(client *c) { | ||||||
| c->argv[3] = tryObjectEncoding(c->argv[3]); | ||||||
| setGenericCommand(c, OBJ_PX, c->argv[1], c->argv[3], c->argv[2], UNIT_MILLISECONDS, NULL, NULL, NULL); | ||||||
| } | ||||||
|
|
||||||
| int getGenericCommand(client *c) { | ||||||
|
|
@@ -374,10 +407,11 @@ void getCommand(client *c) { | |||||
| */ | ||||||
| void getexCommand(client *c) { | ||||||
| robj *expire = NULL; | ||||||
| robj *comparison = NULL; | ||||||
| int unit = UNIT_SECONDS; | ||||||
| int flags = OBJ_NO_FLAGS; | ||||||
|
|
||||||
| if (parseExtendedStringArgumentsOrReply(c, &flags, &unit, &expire, &comparison, COMMAND_GET) != C_OK) { | ||||||
|
There was a problem hiding this comment. IFEQ is not allowed with COMMAND_GET, so I think we can just pass NULL for the comparison argument. It will not be used. Makes sense?
Suggested change
|
||||||
| return; | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -68,18 +68,18 @@ | |||||
| "ZRANGE k 1 2 WITHSCORES " "[BYSCORE|BYLEX] [REV] [LIMIT offset count]" | ||||||
|
|
||||||
| # Optional one-of args with parameters: SET key value [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL] | ||||||
| "SET key value " "[IFEQ comparison-value] [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]" | ||||||
| "SET key value EX" "[IFEQ comparison-value] [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]" | ||||||
|
There was a problem hiding this comment. All of these...
Suggested change
|
||||||
| "SET key value EX " "seconds [IFEQ comparison-value] [NX|XX] [GET]" | ||||||
| "SET key value EX 23 " "[IFEQ comparison-value] [NX|XX] [GET]" | ||||||
| "SET key value EXAT" "[IFEQ comparison-value] [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]" | ||||||
| "SET key value EXAT " "unix-time-seconds [IFEQ comparison-value] [NX|XX] [GET]" | ||||||
| "SET key value PX" "[IFEQ comparison-value] [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]" | ||||||
| "SET key value PX " "milliseconds [IFEQ comparison-value] [NX|XX] [GET]" | ||||||
| "SET key value PXAT" "[IFEQ comparison-value] [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]" | ||||||
| "SET key value PXAT " "unix-time-milliseconds [IFEQ comparison-value] [NX|XX] [GET]" | ||||||
| "SET key value KEEPTTL " "[IFEQ comparison-value] [NX|XX] [GET]" | ||||||
| "SET key value XX " "[IFEQ comparison-value] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]" | ||||||
|
|
||||||
| # If an input word can't be matched, stop hinting. | ||||||
| "SET key value FOOBAR " "" | ||||||
|
|
||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -582,6 +582,67 @@ if {[string match {*jemalloc*} [s mem_allocator]]} { | |
| set err1 | ||
| } {*WRONGTYPE*} | ||
|
|
||
| test "SET with IFEQ conditional" { | ||
sarthakaggarwal97 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| r del foo | ||
|
|
||
| r set foo "initial_value" | ||
|
|
||
| assert_equal {OK} [r set foo "new_value" ifeq "initial_value"] | ||
| assert_equal "new_value" [r get foo] | ||
|
|
||
| assert_equal {} [r set foo "should_not_set" ifeq "wrong_value"] | ||
|
There was a problem hiding this comment. @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. There was a problem hiding this comment. 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. There was a problem hiding this comment. @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. There was a problem hiding this comment. 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. |
||
| assert_equal "new_value" [r get foo] | ||
| } | ||
|
|
||
| test "SET with IFEQ conditional - non-string current value" { | ||
| r del foo | ||
|
|
||
| r sadd foo "some_set_value" | ||
| assert_error {WRONGTYPE Operation against a key holding the wrong kind of value} {r set foo "new_value" ifeq "some_set_value"} | ||
| } | ||
|
|
||
|
|
||
| test "SET with IFEQ conditional - with get" { | ||
| r del foo | ||
|
|
||
| assert_equal {} [r set foo "new_value" ifeq "initial_value" get] | ||
|
|
||
| r set foo "initial_value" | ||
|
|
||
| assert_equal "initial_value" [r set foo "new_value" ifeq "initial_value" get] | ||
| assert_equal "new_value" [r get foo] | ||
| } | ||
|
|
||
| test "SET with IFEQ conditional - non string current value with get" { | ||
| r del foo | ||
|
|
||
| r sadd foo "some_set_value" | ||
|
|
||
| assert_error {WRONGTYPE Operation against a key holding the wrong kind of value} {r set foo "new_value" ifeq "initial_value" get} | ||
| } | ||
|
|
||
| test "SET with IFEQ conditional - with xx" { | ||
| r del foo | ||
|
|
||
| assert_equal {} [r set foo "new_value" ifeq "initial_value" xx] | ||
|
|
||
| r set foo "initial_value" | ||
|
|
||
| assert_equal {OK} [r set foo "new_value" ifeq "initial_value" xx] | ||
| assert_equal "new_value" [r get foo] | ||
| } | ||
|
|
||
| test "SET with IFEQ conditional - with nx" { | ||
| r del foo | ||
|
|
||
| assert_equal {} [r set foo "new_value" ifeq "initial_value" nx] | ||
|
|
||
| r set foo "initial_value" | ||
|
|
||
| assert_equal {} [r set foo "new_value" ifeq "initial_value" nx] | ||
| assert_equal "initial_value" [r get foo] | ||
| } | ||
|
|
||
| test {Extended SET EX option} { | ||
| r del foo | ||
| r set foo bar ex 10 | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our next version is
8.1.0.