Add HSETEX command and expand HSETNX to support multi fields #9058

enjoy-binbin · 2021-06-08T15:37:02Z

We have HSET to support multiple fields, but it can not extend
additional parameters such as NX.
We have HSETNX support NX, but it does not support multiple fields.

We want to implement such as HMSETNX which support multiple fields and
also supports NX flag.

Improving HSETNX is easy, but we want to do it with a more generic HMSET
with flags, it is common pattern. So a new command come up.

Syntax for the new HSETEX command:
HSETEX key [NX|XX] [CH] FIELDS field value [field value ...]

Also we decide to extend HSETNX to support multiple fiedls since it is
trivial to implement and more convenient for some users.

So the new syntax for HSETNX command:
HSETNX key field value [field value ...]

This one inspired from #542 (comment)

Some usage examples

127.0.0.1:6379> hsetex key [NX|XX] [CH] FIELDS field value [field value ...]
[root@binblog redis]# src/redis-cli
127.0.0.1:6379> flushdb
OK
127.0.0.1:6379> hsetex hash nx fields f1 v1 f2 v2
(integer) 2
127.0.0.1:6379> hsetex hash xx fields f1 v1 f2 v2
(integer) 0
127.0.0.1:6379> hsetex hash xx ch fields f1 v1 f2 v2
(integer) 2
127.0.0.1:6379> hsetex hash xx ch fields f1 v1 f2 v2 f3 v3
(integer) 2
127.0.0.1:6379> hsetnx hash f1 v1 f4 v4
(integer) 1

oranagra

i haven't done a very detailed review since i see there's some additional cleanup needed around the in_flags and out_flags (also indicated by the fact it is still marked as draft).

i think we should either model it more like the flags in zsetAdd, and properly expose the flags in server.h.
or we keep this a private implementation detail of t_hash.c (and keep the api used by module.c unchanged).

if we have CH flag, we don't need an UPDATED out flag, or if we have an out flag, we don't need the CH input flag (we can do the counting outside).

i suppose it'll be easier to follow and do any future updates if it's modeled after the zsets one.
let me know if there's something i'm missing, some reason why we can't do that.

other than that, this PR obviously needs some tests, (including propagation tests).

oranagra · 2021-09-12T10:30:14Z

src/help.h

+    { "HSETEX",
+    "key [NX|XX] [CH] FIELDS field value [field value ...]",
+    "for test. Will update from doc",
+    5,
+    "2.0.0" },


this file is auto generated from redis-doc repo, no need to edit

oranagra · 2021-09-12T10:33:56Z

src/server.c

+    "write use-memory fast @hash",
+    0,NULL,1,1,1,0,0,0},
+
+    {"hsetnx",hsetnxCommand,-4,


did we also decide to modify HSETNX?
i don't think so.
i suppose that's ok since it's trivial to implement, and may be more convenient for some users.
but it's not mentioned in the top comment, please update the top comment.

oranagra · 2021-09-12T10:35:55Z

src/server.h

@@ -2352,7 +2352,7 @@ void hashTypeCurrentObject(hashTypeIterator *hi, int what, unsigned char **vstr,
 sds hashTypeCurrentObjectNewSds(hashTypeIterator *hi, int what);
 robj *hashTypeLookupWriteOrCreate(client *c, robj *key);
 robj *hashTypeGetValueObject(robj *o, sds field);
-int hashTypeSet(robj *o, sds field, sds value, int flags);
+int hashTypeSet(robj *o, sds field, sds value, int flags, int in_flags, int *out_flags);


it's a bit odd to see the prototype and flag args defined in the header, but the flags themselves defined in the C file.
maybe instead the C file should have a hashTypeSetGeneric that takes the flags, and the header file should only expose the old hashTypeSet api which doesn't take any flags.

oranagra · 2021-09-12T10:38:25Z

src/t_hash.c

+#define HSET_IN_NONE 0
+#define HSET_IN_NX (1<<0)       /* Don't touch elements not already existing. */
+#define HSET_IN_XX (1<<1)       /* Only touch elements already existing. */
+#define HSET_IN_CH (1<<2)       /* CH is an abbreviation of changed. */


abbreviation of changed doesn't say what it actually does. ie:

Modify the return value from the number of new elements added, to the total number of elements changed

oranagra · 2021-09-12T11:04:46Z

src/t_hash.c

 */
 #define HASH_SET_TAKE_FIELD (1<<0)
 #define HASH_SET_TAKE_VALUE (1<<1)
 #define HASH_SET_COPY 0
-int hashTypeSet(robj *o, sds field, sds value, int flags) {
+int hashTypeSet(robj *o, sds field, sds value, int flags, int in_flags, int *out_flags) {


it's odd that this function has both flags and in_flags.
why do we need two sets of input flags? i suppose we should combine them into one.
probably either deleting HASH_SET_COPY (it's clear that 0 is the default for no flags), or renaming it to HASH_SET_DEFAULT?

actually, i see this is modeled after zsetAdd, right?
so let's continue on that path, any reason they should not be more similar (only two sets of flags, and use the out_flags instead of a CH in flag)?

oranagra · 2021-09-12T11:30:43Z

src/t_hash.c

    /* HMSET (deprecated) and HSET return value is different. */
    char *cmdname = c->argv[0]->ptr;
    if (cmdname[1] == 's' || cmdname[1] == 'S') {


i think at this point (calling this function from a 3rd place), we should stop using this trick, and pass an argument)
or alternatively, since HMSET was the odd case, maybe do a full string compare to HMSET rather than look at the second letter.

We have HSET to support multiple fields, but it can not extend additional parameters such as NX. We have HSETNX support NX, but it does not support multiple fields. We want to implement such as HMSETNX which support multiple fields and also supports NX flag. Improving HSETNX is easy, but we want to do it with a more generic HMSET with flags, it is common pattern. So a new command come up. Syntax for the new HSETEX command: `HSETEX key [NX|XX] [CH] FIELDS field value [field value ...]` Also we decide to extend HSETNX to support multiple fiedls since it is trivial to implement and more convenient for some users. So the new syntax for HSETNX command: `HSETNX key field value [field value ...]`

enjoy-binbin · 2021-11-09T11:07:01Z

Sorry for taking so long.. I updated the PR based on the previous suggestions.
Because the PR is relatively old, i (we) may forget some details.

Please ack if i do it in the right way.

Also see #542 (comment), regarding the FIELDS, i may need more suggestions

I'm also not sure I'm a fan of the FIELDS argument.

I will add test cases later, but i have some questions about the propagation tests.
Do we only need to verify that the new command can be propagation from the master to the replica and take effect?

oranagra · 2021-11-09T11:19:09Z

haven't reviewed the changes yet.
i'm not clear about the propagation tests question, what is the alternative?
i don't think each and every command needs a propagation test, we mainly do that for commands with special complications (like expiry time, or blocked commands)

enjoy-binbin · 2021-11-09T12:11:56Z

other than that, this PR obviously needs some tests, (including propagation tests).

oh, sorry, I'm just a little confused about the last line of this comment: #9058 (review)

oranagra · 2021-11-09T12:48:35Z

i'm sorry.. looks like i'm inconsistent with myself (2 months apart). skimmed though the code again, i can't recall why i thought that this PR requires propagation tests (i don't see the code dong anything special that would be at risk).
I don't see that HSET and SETNX have any propagation tests.

guybe7 · 2022-11-08T08:01:43Z

for what it's worth, i think the name HSETEX has some expiry connotation, i think i'd prefer using a different name (i even prefer HSET2)

zuiderkwast · 2024-02-19T18:46:42Z

Agree with @guybe that "EX" usually means "expire", so HSETEX is not a very good name.

I think HSETNX should accept multiple fields, since MSET already does that, just like HMSET. There's no reason for HSETNX to be less powerful than it's brothers and sisters.

Since we already have HSETNX, I think we can add HSETXX too for symmetry, and let's make it multi-field. Or maybe we shall call it HSETX. We have LPUSHX and RPUSHX for lists. (XX is usually a flag, not part of a command name.)

CLAassistant · 2024-03-24T23:18:32Z

All committers have signed the CLA.

oranagra added this to Backlog in 7.0 via automation Aug 19, 2021

oranagra moved this from Backlog to In progress in 7.0 Aug 19, 2021

enjoy-binbin force-pushed the generic-hset branch from b98133d to e69d7ec Compare August 19, 2021 13:42

enjoy-binbin marked this pull request as ready for review September 10, 2021 09:52

oranagra linked an issue Sep 12, 2021 that may be closed by this pull request

Support for hmsetnx #542

Open

oranagra reviewed Sep 12, 2021

View reviewed changes

oranagra mentioned this pull request Sep 12, 2021

HSETNX command supports multi fields #8304

Closed

oranagra added the 7.0-not-after-rc1 label Oct 26, 2021

oranagra moved this from In progress to In Review in 7.0 Nov 2, 2021

enjoy-binbin force-pushed the generic-hset branch from e69d7ec to 52439a9 Compare November 9, 2021 10:53

enjoy-binbin changed the title ~~WIP: Support hmsetnx and introduce XX/NX/CH flags for hset~~ Add HSETEX command and expand HSETNX to support multi fields Nov 9, 2021

oranagra moved this from In Review to In progress in 7.0 Dec 7, 2021

oranagra removed the 7.0-not-after-rc1 label Jan 25, 2022

oranagra removed this from In progress in 7.0 Jan 25, 2022

oranagra added this to the Next minor backlog milestone Jan 25, 2022

oranagra added this to backlog in 7.2 <obsolete> Jan 25, 2022

enjoy-binbin mentioned this pull request Feb 19, 2024

Implement HSETXX #13067

Open

enjoy-binbin linked an issue Feb 19, 2024 that may be closed by this pull request

Implement HSETXX #13067

Open

zuiderkwast mentioned this pull request Feb 22, 2024

Implement HSETXX #13083

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HSETEX command and expand HSETNX to support multi fields #9058

Add HSETEX command and expand HSETNX to support multi fields #9058

enjoy-binbin commented Jun 8, 2021 •

edited

oranagra left a comment

oranagra Sep 12, 2021

oranagra Sep 12, 2021

oranagra Sep 12, 2021

oranagra Sep 12, 2021

oranagra Sep 12, 2021

oranagra Sep 12, 2021

enjoy-binbin commented Nov 9, 2021

oranagra commented Nov 9, 2021

enjoy-binbin commented Nov 9, 2021

oranagra commented Nov 9, 2021

guybe7 commented Nov 8, 2022

zuiderkwast commented Feb 19, 2024

CLAassistant commented Mar 24, 2024 •

edited

Add HSETEX command and expand HSETNX to support multi fields #9058

Are you sure you want to change the base?

Add HSETEX command and expand HSETNX to support multi fields #9058

Conversation

enjoy-binbin commented Jun 8, 2021 • edited

oranagra left a comment

Choose a reason for hiding this comment

oranagra Sep 12, 2021

Choose a reason for hiding this comment

oranagra Sep 12, 2021

Choose a reason for hiding this comment

oranagra Sep 12, 2021

Choose a reason for hiding this comment

oranagra Sep 12, 2021

Choose a reason for hiding this comment

oranagra Sep 12, 2021

Choose a reason for hiding this comment

oranagra Sep 12, 2021

Choose a reason for hiding this comment

enjoy-binbin commented Nov 9, 2021

oranagra commented Nov 9, 2021

enjoy-binbin commented Nov 9, 2021

oranagra commented Nov 9, 2021

guybe7 commented Nov 8, 2022

zuiderkwast commented Feb 19, 2024

CLAassistant commented Mar 24, 2024 • edited

enjoy-binbin commented Jun 8, 2021 •

edited

CLAassistant commented Mar 24, 2024 •

edited