gvsafronov/futriix
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Cleanup ZADD_* flags (#8559)

Browse Source 
Have a clear separation between in and out flags

Other changes:

delete dead code in RM_ZsetIncrby: if zsetAdd returned error (happens only if
the result of the operation is NAN or if score is NAN) we return immediately so
there is no way that zsetAdd succeeded and returned NAN in the out-flags
This commit is contained in:
guybe7 2021-03-10 15:09:43 +01:00 committed by GitHub
parent 7778f1b4b0
commit 61a73de64d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 67 additions and 78 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

37
src/module.c
View File

@ -2516,19 +2516,19 @@ RedisModuleString *RM_ListPop(RedisModuleKey *key, int where) {
* so that we have everything decoupled. */ * so that we have everything decoupled. */
int moduleZsetAddFlagsToCoreFlags(int flags) { int moduleZsetAddFlagsToCoreFlags(int flags) {
int retflags = 0; int retflags = 0;
if (flags & REDISMODULE_ZADD_XX) retflags |= ZADD_XX; if (flags & REDISMODULE_ZADD_XX) retflags |= ZADD_IN_XX;
if (flags & REDISMODULE_ZADD_NX) retflags |= ZADD_NX; if (flags & REDISMODULE_ZADD_NX) retflags |= ZADD_IN_NX;
if (flags & REDISMODULE_ZADD_GT) retflags |= ZADD_GT; if (flags & REDISMODULE_ZADD_GT) retflags |= ZADD_IN_GT;
if (flags & REDISMODULE_ZADD_LT) retflags |= ZADD_LT; if (flags & REDISMODULE_ZADD_LT) retflags |= ZADD_IN_LT;
return retflags; return retflags;
} }
/* See previous function comment. */ /* See previous function comment. */
int moduleZsetAddFlagsFromCoreFlags(int flags) { int moduleZsetAddFlagsFromCoreFlags(int flags) {
int retflags = 0; int retflags = 0;
if (flags & ZADD_ADDED) retflags |= REDISMODULE_ZADD_ADDED; if (flags & ZADD_OUT_ADDED) retflags |= REDISMODULE_ZADD_ADDED;
if (flags & ZADD_UPDATED) retflags |= REDISMODULE_ZADD_UPDATED; if (flags & ZADD_OUT_UPDATED) retflags |= REDISMODULE_ZADD_UPDATED;
if (flags & ZADD_NOP) retflags |= REDISMODULE_ZADD_NOP; if (flags & ZADD_OUT_NOP) retflags |= REDISMODULE_ZADD_NOP;
return retflags; return retflags;
} }
@ -2565,16 +2565,16 @@ int moduleZsetAddFlagsFromCoreFlags(int flags) {
* * 'score' double value is not a number (NaN). * * 'score' double value is not a number (NaN).
*/ */
int RM_ZsetAdd(RedisModuleKey *key, double score, RedisModuleString *ele, int *flagsptr) { int RM_ZsetAdd(RedisModuleKey *key, double score, RedisModuleString *ele, int *flagsptr) {
int flags = 0; int in_flags = 0, out_flags = 0;
if (!(key->mode & REDISMODULE_WRITE)) return REDISMODULE_ERR; if (!(key->mode & REDISMODULE_WRITE)) return REDISMODULE_ERR;
if (key->value && key->value->type != OBJ_ZSET) return REDISMODULE_ERR; if (key->value && key->value->type != OBJ_ZSET) return REDISMODULE_ERR;
if (key->value == NULL) moduleCreateEmptyKey(key,REDISMODULE_KEYTYPE_ZSET); if (key->value == NULL) moduleCreateEmptyKey(key,REDISMODULE_KEYTYPE_ZSET);
if (flagsptr) flags = moduleZsetAddFlagsToCoreFlags(*flagsptr); if (flagsptr) in_flags = moduleZsetAddFlagsToCoreFlags(*flagsptr);
if (zsetAdd(key->value,score,ele->ptr,&flags,NULL) == 0) { if (zsetAdd(key->value,score,ele->ptr,in_flags,&out_flags,NULL) == 0) {
if (flagsptr) *flagsptr = 0; if (flagsptr) *flagsptr = 0;
return REDISMODULE_ERR; return REDISMODULE_ERR;
} }
if (flagsptr) *flagsptr = moduleZsetAddFlagsFromCoreFlags(flags); if (flagsptr) *flagsptr = moduleZsetAddFlagsFromCoreFlags(out_flags);
return REDISMODULE_OK; return REDISMODULE_OK;
} }
@ -2592,22 +2592,17 @@ int RM_ZsetAdd(RedisModuleKey *key, double score, RedisModuleString *ele, int *f
* with the new score of the element after the increment, if no error * with the new score of the element after the increment, if no error
* is returned. */ * is returned. */
int RM_ZsetIncrby(RedisModuleKey *key, double score, RedisModuleString *ele, int *flagsptr, double *newscore) { int RM_ZsetIncrby(RedisModuleKey *key, double score, RedisModuleString *ele, int *flagsptr, double *newscore) {
int flags = 0; int in_flags = 0, out_flags = 0;
if (!(key->mode & REDISMODULE_WRITE)) return REDISMODULE_ERR; if (!(key->mode & REDISMODULE_WRITE)) return REDISMODULE_ERR;
if (key->value && key->value->type != OBJ_ZSET) return REDISMODULE_ERR; if (key->value && key->value->type != OBJ_ZSET) return REDISMODULE_ERR;
if (key->value == NULL) moduleCreateEmptyKey(key,REDISMODULE_KEYTYPE_ZSET); if (key->value == NULL) moduleCreateEmptyKey(key,REDISMODULE_KEYTYPE_ZSET);
if (flagsptr) flags = moduleZsetAddFlagsToCoreFlags(*flagsptr); if (flagsptr) in_flags = moduleZsetAddFlagsToCoreFlags(*flagsptr);
flags |= ZADD_INCR; in_flags |= ZADD_IN_INCR;
if (zsetAdd(key->value,score,ele->ptr,&flags,newscore) == 0) { if (zsetAdd(key->value,score,ele->ptr,in_flags,&out_flags,newscore) == 0) {
if (flagsptr) *flagsptr = 0; if (flagsptr) *flagsptr = 0;
return REDISMODULE_ERR; return REDISMODULE_ERR;
} }
/* zsetAdd() may signal back that the resulting score is not a number. */ if (flagsptr) *flagsptr = moduleZsetAddFlagsFromCoreFlags(out_flags);
if (flagsptr && (*flagsptr & ZADD_NAN)) {
*flagsptr = 0;
return REDISMODULE_ERR;
}
if (flagsptr) *flagsptr = moduleZsetAddFlagsFromCoreFlags(flags);
return REDISMODULE_OK; return REDISMODULE_OK;
} }

25
src/server.h
View File

@ -2111,21 +2111,18 @@ void ACLUpdateDefaultUserPassword(sds password);
/* Sorted sets data type */ /* Sorted sets data type */
/* Input flags. */ /* Input flags. */
#define ZADD_NONE 0 #define ZADD_IN_NONE 0
#define ZADD_INCR (1<<0) /* Increment the score instead of setting it. */ #define ZADD_IN_INCR (1<<0) /* Increment the score instead of setting it. */
#define ZADD_NX (1<<1) /* Don't touch elements not already existing. */ #define ZADD_IN_NX (1<<1) /* Don't touch elements not already existing. */
#define ZADD_XX (1<<2) /* Only touch elements already existing. */ #define ZADD_IN_XX (1<<2) /* Only touch elements already existing. */
#define ZADD_GT (1<<7) /* Only update existing when new scores are higher. */ #define ZADD_IN_GT (1<<3) /* Only update existing when new scores are higher. */
#define ZADD_LT (1<<8) /* Only update existing when new scores are lower. */ #define ZADD_IN_LT (1<<4) /* Only update existing when new scores are lower. */
/* Output flags. */ /* Output flags. */
#define ZADD_NOP (1<<3) /* Operation not performed because of conditionals.*/ #define ZADD_OUT_NOP (1<<0) /* Operation not performed because of conditionals.*/
#define ZADD_NAN (1<<4) /* Only touch elements already existing. */ #define ZADD_OUT_NAN (1<<1) /* Only touch elements already existing. */
#define ZADD_ADDED (1<<5) /* The element was new and was added. */ #define ZADD_OUT_ADDED (1<<2) /* The element was new and was added. */
#define ZADD_UPDATED (1<<6) /* The element already existed, score updated. */ #define ZADD_OUT_UPDATED (1<<3) /* The element already existed, score updated. */
/* Flags only used by the ZADD command but not by zsetAdd() API: */
#define ZADD_CH (1<<16) /* Return num of elements added or updated. */
/* Struct to hold an inclusive/exclusive range spec by score comparison. */ /* Struct to hold an inclusive/exclusive range spec by score comparison. */
typedef struct { typedef struct {
@ -2156,7 +2153,7 @@ void zsetConvert(robj *zobj, int encoding);
void zsetConvertToZiplistIfNeeded(robj *zobj, size_t maxelelen); void zsetConvertToZiplistIfNeeded(robj *zobj, size_t maxelelen);
int zsetScore(robj *zobj, sds member, double *score); int zsetScore(robj *zobj, sds member, double *score);
unsigned long zslGetRank(zskiplist *zsl, double score, sds o); unsigned long zslGetRank(zskiplist *zsl, double score, sds o);
int zsetAdd(robj *zobj, double score, sds ele, int *flags, double *newscore); int zsetAdd(robj *zobj, double score, sds ele, int in_flags, int *out_flags, double *newscore);
long zsetRank(robj *zobj, sds ele, int reverse); long zsetRank(robj *zobj, sds ele, int reverse);
int zsetDel(robj *zobj, sds ele); int zsetDel(robj *zobj, sds ele);
robj *zsetDup(robj *o); robj *zsetDup(robj *o);

83
src/t_zset.c
View File

@ -1279,9 +1279,7 @@ int zsetScore(robj *zobj, sds member, double *score) {
/* Add a new element or update the score of an existing element in a sorted /* Add a new element or update the score of an existing element in a sorted
* set, regardless of its encoding. * set, regardless of its encoding.
* *
* The set of flags change the command behavior. They are passed with an integer * The set of flags change the command behavior.
* pointer since the function will clear the flags and populate them with
* other flags to indicate different conditions.
* *
* The input flags are the following: * The input flags are the following:
* *
@ -1323,19 +1321,19 @@ int zsetScore(robj *zobj, sds member, double *score) {
* *
* The function does not take ownership of the 'ele' SDS string, but copies * The function does not take ownership of the 'ele' SDS string, but copies
* it if needed. */ * it if needed. */
int zsetAdd(robj *zobj, double score, sds ele, int *flags, double *newscore) { int zsetAdd(robj *zobj, double score, sds ele, int in_flags, int *out_flags, double *newscore) {
/* Turn options into simple to check vars. */ /* Turn options into simple to check vars. */
int incr = (*flags & ZADD_INCR) != 0; int incr = (in_flags & ZADD_IN_INCR) != 0;
int nx = (*flags & ZADD_NX) != 0; int nx = (in_flags & ZADD_IN_NX) != 0;
int xx = (*flags & ZADD_XX) != 0; int xx = (in_flags & ZADD_IN_XX) != 0;
int gt = (*flags & ZADD_GT) != 0; int gt = (in_flags & ZADD_IN_GT) != 0;
int lt = (*flags & ZADD_LT) != 0; int lt = (in_flags & ZADD_IN_LT) != 0;
*flags = 0; /* We'll return our response flags. */ *out_flags = 0; /* We'll return our response flags. */
double curscore; double curscore;
/* NaN as input is an error regardless of all the other parameters. */ /* NaN as input is an error regardless of all the other parameters. */
if (isnan(score)) { if (isnan(score)) {
*flags = ZADD_NAN; *out_flags = ZADD_OUT_NAN;
return 0; return 0;
} }
@ -1346,7 +1344,7 @@ int zsetAdd(robj *zobj, double score, sds ele, int *flags, double *newscore) {
if ((eptr = zzlFind(zobj->ptr,ele,&curscore)) != NULL) { if ((eptr = zzlFind(zobj->ptr,ele,&curscore)) != NULL) {
/* NX? Return, same element already exists. */ /* NX? Return, same element already exists. */
if (nx) { if (nx) {
*flags |= ZADD_NOP; *out_flags |= ZADD_OUT_NOP;
return 1; return 1;
} }
@ -1354,7 +1352,7 @@ int zsetAdd(robj *zobj, double score, sds ele, int *flags, double *newscore) {
if (incr) { if (incr) {
score += curscore; score += curscore;
if (isnan(score)) { if (isnan(score)) {
*flags |= ZADD_NAN; *out_flags |= ZADD_OUT_NAN;
return 0; return 0;
} }
if (newscore) *newscore = score; if (newscore) *newscore = score;
@ -1369,7 +1367,7 @@ int zsetAdd(robj *zobj, double score, sds ele, int *flags, double *newscore) {
{ {
zobj->ptr = zzlDelete(zobj->ptr,eptr); zobj->ptr = zzlDelete(zobj->ptr,eptr);
zobj->ptr = zzlInsert(zobj->ptr,ele,score); zobj->ptr = zzlInsert(zobj->ptr,ele,score);
*flags |= ZADD_UPDATED; *out_flags |= ZADD_OUT_UPDATED;
} }
return 1; return 1;
} else if (!xx) { } else if (!xx) {
@ -1380,10 +1378,10 @@ int zsetAdd(robj *zobj, double score, sds ele, int *flags, double *newscore) {
sdslen(ele) > server.zset_max_ziplist_value) sdslen(ele) > server.zset_max_ziplist_value)
zsetConvert(zobj,OBJ_ENCODING_SKIPLIST); zsetConvert(zobj,OBJ_ENCODING_SKIPLIST);
if (newscore) *newscore = score; if (newscore) *newscore = score;
*flags |= ZADD_ADDED; *out_flags |= ZADD_OUT_ADDED;
return 1; return 1;
} else { } else {
*flags |= ZADD_NOP; *out_flags |= ZADD_OUT_NOP;
return 1; return 1;
} }
} else if (zobj->encoding == OBJ_ENCODING_SKIPLIST) { } else if (zobj->encoding == OBJ_ENCODING_SKIPLIST) {
@ -1395,7 +1393,7 @@ int zsetAdd(robj *zobj, double score, sds ele, int *flags, double *newscore) {
if (de != NULL) { if (de != NULL) {
/* NX? Return, same element already exists. */ /* NX? Return, same element already exists. */
if (nx) { if (nx) {
*flags |= ZADD_NOP; *out_flags |= ZADD_OUT_NOP;
return 1; return 1;
} }
curscore = *(double*)dictGetVal(de); curscore = *(double*)dictGetVal(de);
@ -1404,7 +1402,7 @@ int zsetAdd(robj *zobj, double score, sds ele, int *flags, double *newscore) {
if (incr) { if (incr) {
score += curscore; score += curscore;
if (isnan(score)) { if (isnan(score)) {
*flags |= ZADD_NAN; *out_flags |= ZADD_OUT_NAN;
return 0; return 0;
} }
if (newscore) *newscore = score; if (newscore) *newscore = score;
@ -1422,18 +1420,18 @@ int zsetAdd(robj *zobj, double score, sds ele, int *flags, double *newscore) {
* the hash table representing the sorted set, so we just * the hash table representing the sorted set, so we just
* update the score. */ * update the score. */
dictGetVal(de) = &znode->score; /* Update score ptr. */ dictGetVal(de) = &znode->score; /* Update score ptr. */
*flags |= ZADD_UPDATED; *out_flags |= ZADD_OUT_UPDATED;
} }
return 1; return 1;
} else if (!xx) { } else if (!xx) {
ele = sdsdup(ele); ele = sdsdup(ele);
znode = zslInsert(zs->zsl,score,ele); znode = zslInsert(zs->zsl,score,ele);
serverAssert(dictAdd(zs->dict,ele,&znode->score) == DICT_OK); serverAssert(dictAdd(zs->dict,ele,&znode->score) == DICT_OK);
*flags |= ZADD_ADDED; *out_flags |= ZADD_OUT_ADDED;
if (newscore) *newscore = score; if (newscore) *newscore = score;
return 1; return 1;
} else { } else {
*flags |= ZADD_NOP; *out_flags |= ZADD_OUT_NOP;
return 1; return 1;
} }
} else { } else {
@ -1712,7 +1710,7 @@ void zaddGenericCommand(client *c, int flags) {
robj *zobj; robj *zobj;
sds ele; sds ele;
double score = 0, *scores = NULL; double score = 0, *scores = NULL;
int j, elements; int j, elements, ch = 0;
int scoreidx = 0; int scoreidx = 0;
/* The following vars are used in order to track what the command actually /* The following vars are used in order to track what the command actually
* did during the execution, to reply to the client and to trigger the * did during the execution, to reply to the client and to trigger the
@ -1727,23 +1725,22 @@ void zaddGenericCommand(client *c, int flags) {
scoreidx = 2; scoreidx = 2;
while(scoreidx < c->argc) { while(scoreidx < c->argc) {
char *opt = c->argv[scoreidx]->ptr; char *opt = c->argv[scoreidx]->ptr;
if (!strcasecmp(opt,"nx")) flags |= ZADD_NX; if (!strcasecmp(opt,"nx")) flags |= ZADD_IN_NX;
else if (!strcasecmp(opt,"xx")) flags |= ZADD_XX; else if (!strcasecmp(opt,"xx")) flags |= ZADD_IN_XX;
else if (!strcasecmp(opt,"ch")) flags |= ZADD_CH; else if (!strcasecmp(opt,"ch")) ch = 1; /* Return num of elements added or updated. */
else if (!strcasecmp(opt,"incr")) flags |= ZADD_INCR; else if (!strcasecmp(opt,"incr")) flags |= ZADD_IN_INCR;
else if (!strcasecmp(opt,"gt")) flags |= ZADD_GT; else if (!strcasecmp(opt,"gt")) flags |= ZADD_IN_GT;
else if (!strcasecmp(opt,"lt")) flags |= ZADD_LT; else if (!strcasecmp(opt,"lt")) flags |= ZADD_IN_LT;
else break; else break;
scoreidx++; scoreidx++;
} }
/* Turn options into simple to check vars. */ /* Turn options into simple to check vars. */
int incr = (flags & ZADD_INCR) != 0; int incr = (flags & ZADD_IN_INCR) != 0;
int nx = (flags & ZADD_NX) != 0; int nx = (flags & ZADD_IN_NX) != 0;
int xx = (flags & ZADD_XX) != 0; int xx = (flags & ZADD_IN_XX) != 0;
int ch = (flags & ZADD_CH) != 0; int gt = (flags & ZADD_IN_GT) != 0;
int gt = (flags & ZADD_GT) != 0; int lt = (flags & ZADD_IN_LT) != 0;
int lt = (flags & ZADD_LT) != 0;
/* After the options, we expect to have an even number of args, since /* After the options, we expect to have an even number of args, since
* we expect any number of score-element pairs. */ * we expect any number of score-element pairs. */
@ -1801,17 +1798,17 @@ void zaddGenericCommand(client *c, int flags) {
for (j = 0; j < elements; j++) { for (j = 0; j < elements; j++) {
double newscore; double newscore;
score = scores[j]; score = scores[j];
int retflags = flags; int retflags = 0;
ele = c->argv[scoreidx+1+j*2]->ptr; ele = c->argv[scoreidx+1+j*2]->ptr;
int retval = zsetAdd(zobj, score, ele, &retflags, &newscore); int retval = zsetAdd(zobj, score, ele, flags, &retflags, &newscore);
if (retval == 0) { if (retval == 0) {
addReplyError(c,nanerr); addReplyError(c,nanerr);
goto cleanup; goto cleanup;
} }
if (retflags & ZADD_ADDED) added++; if (retflags & ZADD_OUT_ADDED) added++;
if (retflags & ZADD_UPDATED) updated++; if (retflags & ZADD_OUT_UPDATED) updated++;
if (!(retflags & ZADD_NOP)) processed++; if (!(retflags & ZADD_OUT_NOP)) processed++;
score = newscore; score = newscore;
} }
server.dirty += (added+updated); server.dirty += (added+updated);
@ -1836,11 +1833,11 @@ cleanup:
} }
void zaddCommand(client *c) { void zaddCommand(client *c) {
zaddGenericCommand(c,ZADD_NONE); zaddGenericCommand(c,ZADD_IN_NONE);
} }
void zincrbyCommand(client *c) { void zincrbyCommand(client *c) {
zaddGenericCommand(c,ZADD_INCR); zaddGenericCommand(c,ZADD_IN_INCR);
} }
void zremCommand(client *c) { void zremCommand(client *c) {
@ -2941,7 +2938,7 @@ static void zrangeResultEmitCBufferForStore(zrange_result_handler *handler,
double newscore; double newscore;
int retflags = 0; int retflags = 0;
sds ele = sdsnewlen(value, value_length_in_bytes); sds ele = sdsnewlen(value, value_length_in_bytes);
int retval = zsetAdd(handler->dstobj, score, ele, &retflags, &newscore); int retval = zsetAdd(handler->dstobj, score, ele, ZADD_IN_NONE, &retflags, &newscore);
sdsfree(ele); sdsfree(ele);
serverAssert(retval); serverAssert(retval);
} }
@ -2952,7 +2949,7 @@ static void zrangeResultEmitLongLongForStore(zrange_result_handler *handler,
double newscore; double newscore;
int retflags = 0; int retflags = 0;
sds ele = sdsfromlonglong(value); sds ele = sdsfromlonglong(value);
int retval = zsetAdd(handler->dstobj, score, ele, &retflags, &newscore); int retval = zsetAdd(handler->dstobj, score, ele, ZADD_IN_NONE, &retflags, &newscore);
sdsfree(ele); sdsfree(ele);
serverAssert(retval); serverAssert(retval);
} }