From 642fecac8be7cb6ee087659e541152d07be152b1 Mon Sep 17 00:00:00 2001 From: "debing.sun" Date: Fri, 7 Jun 2024 14:59:07 +0800 Subject: [PATCH 01/12] Add new hexpired notification for hash field expiration --- src/t_hash.c | 37 +++++++++++++++++++++++-------------- tests/unit/pubsub.tcl | 22 +++++++++++++++++++++- 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/src/t_hash.c b/src/t_hash.c index efb489265a5..fb39578e924 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -771,15 +771,17 @@ GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vs propagateHashFieldDeletion(db, key, field, sdslen(field)); /* If the field is the last one in the hash, then the hash will be deleted */ + robj *keyObj = createStringObject(key, sdslen(key)); if (hashTypeLength(o, 0) == 0) { - robj *keyObj = createStringObject(key, sdslen(key)); notifyKeyspaceEvent(NOTIFY_GENERIC, "del", keyObj, db->id); dbDelete(db,keyObj); - decrRefCount(keyObj); - return GETF_EXPIRED_HASH; + res = GETF_EXPIRED_HASH; + } else { + notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", keyObj, db->id); + res = GETF_EXPIRED; } - - return GETF_EXPIRED; + decrRefCount(keyObj); + return res; } /* Like hashTypeGetValue() but returns a Redis object, which is useful for @@ -1126,7 +1128,8 @@ void hashTypeSetExDone(HashTypeSetEx *ex) { if (ex->c) { server.dirty += ex->fieldDeleted + ex->fieldUpdated; signalModifiedKey(ex->c, ex->db, ex->key); - notifyKeyspaceEvent(NOTIFY_HASH, "hexpire", ex->key, ex->db->id); + notifyKeyspaceEvent(NOTIFY_HASH, ex->fieldDeleted ? "hexpired" : "hexpire", + ex->key, ex->db->id); } if (ex->fieldDeleted && hashTypeLength(ex->hashObj, 0) == 0) { dbDelete(ex->db,ex->key); @@ -1845,10 +1848,11 @@ static ExpireAction hashTypeActiveExpire(eItem _hashObj, void *ctx) { ActiveExpireCtx *activeExpireCtx = (ActiveExpireCtx *) ctx; sds keystr = NULL; ExpireInfo info = {0}; + ExpireAction ret = ACT_STOP_ACTIVE_EXP; /* If no more quota left for this callback, stop */ if (activeExpireCtx->fieldsToExpireQuota == 0) - return ACT_STOP_ACTIVE_EXP; + return ret; if (hashObj->encoding == OBJ_ENCODING_LISTPACK_EX) { info = (ExpireInfo){ @@ -1885,22 +1889,27 @@ static ExpireAction hashTypeActiveExpire(eItem _hashObj, void *ctx) { activeExpireCtx->fieldsToExpireQuota -= info.itemsExpired; /* If hash has no more fields to expire, remove it from HFE DB */ + robj *key = createStringObject(keystr, sdslen(keystr)); if (info.nextExpireTime == EB_EXPIRE_TIME_INVALID) { if (hashTypeLength(hashObj, 0) == 0) { - robj *key = createStringObject(keystr, sdslen(keystr)); dbDelete(activeExpireCtx->db, key); - notifyKeyspaceEvent(NOTIFY_GENERIC,"del",key, activeExpireCtx->db->id); - server.dirty++; - signalModifiedKey(NULL, &server.db[0], key); - decrRefCount(key); + notifyKeyspaceEvent(NOTIFY_GENERIC,"del",key,activeExpireCtx->db->id); + } else { + notifyKeyspaceEvent(NOTIFY_HASH,"hexpired",key,activeExpireCtx->db->id); } - return ACT_REMOVE_EXP_ITEM; + ret = ACT_REMOVE_EXP_ITEM; } else { /* Hash has more fields to expire. Update next expiration time of the hash * and indicate to add it back to global HFE DS */ ebSetMetaExpTime(hashGetExpireMeta(hashObj), info.nextExpireTime); - return ACT_UPDATE_EXP_ITEM; + notifyKeyspaceEvent(NOTIFY_HASH,"hexpired",key,activeExpireCtx->db->id); + ret = ACT_UPDATE_EXP_ITEM; } + + server.dirty++; + signalModifiedKey(NULL, activeExpireCtx->db, key); + decrRefCount(key); + return ret; } /* Return the next/minimum expiry time of the hash-field. This is useful if a diff --git a/tests/unit/pubsub.tcl b/tests/unit/pubsub.tcl index 153ba059c56..242bd06aacf 100644 --- a/tests/unit/pubsub.tcl +++ b/tests/unit/pubsub.tcl @@ -360,12 +360,13 @@ start_server {tags {"pubsub network"}} { r del myhash set rd1 [redis_deferring_client] assert_equal {1} [psubscribe $rd1 *] - r hmset myhash yes 1 no 0 + r hmset myhash yes 1 no 0 f1 1 f2 2 r hincrby myhash yes 10 r hexpire myhash 999999 FIELDS 1 yes r hexpireat myhash [expr {[clock seconds] + 999999}] NX FIELDS 1 no r hpexpire myhash 999999 FIELDS 1 yes r hpersist myhash FIELDS 1 yes + r hpexpire myhash 0 FIELDS 1 yes assert_encoding $type myhash assert_equal "pmessage * __keyspace@${db}__:myhash hset" [$rd1 read] assert_equal "pmessage * __keyspace@${db}__:myhash hincrby" [$rd1 read] @@ -373,6 +374,25 @@ start_server {tags {"pubsub network"}} { assert_equal "pmessage * __keyspace@${db}__:myhash hexpire" [$rd1 read] assert_equal "pmessage * __keyspace@${db}__:myhash hexpire" [$rd1 read] assert_equal "pmessage * __keyspace@${db}__:myhash hpersist" [$rd1 read] + assert_equal "pmessage * __keyspace@${db}__:myhash hexpired" [$rd1 read] + + # Test that we wll get `hexpired` notification when + # a hash fied is removed by active expire. + r hpexpire myhash 10 FIELDS 1 f1 + assert_equal "pmessage * __keyspace@${db}__:myhash hexpire" [$rd1 read] + after 100 ;# Wait for active expire + assert_equal "pmessage * __keyspace@${db}__:myhash hexpired" [$rd1 read] + + # Test that we wll get `hexpired` notification when + # a hash fied is removed by lazy active. + r debug set-active-expire 0 + r hpexpire myhash 10 FIELDS 1 f2 + assert_equal "pmessage * __keyspace@${db}__:myhash hexpire" [$rd1 read] + after 20 + r hstrlen myhash f2 ;# Trigger lazy expire + assert_equal "pmessage * __keyspace@${db}__:myhash hexpired" [$rd1 read] + r debug set-active-expire 1 + $rd1 close } } ;# foreach From 26c9b12048bfcb84f592ff735d5fce1b7a9c1ffc Mon Sep 17 00:00:00 2001 From: "debing.sun" Date: Fri, 7 Jun 2024 15:00:54 +0800 Subject: [PATCH 02/12] Fix spell --- tests/unit/pubsub.tcl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unit/pubsub.tcl b/tests/unit/pubsub.tcl index 242bd06aacf..f362f5aeef8 100644 --- a/tests/unit/pubsub.tcl +++ b/tests/unit/pubsub.tcl @@ -376,15 +376,15 @@ start_server {tags {"pubsub network"}} { assert_equal "pmessage * __keyspace@${db}__:myhash hpersist" [$rd1 read] assert_equal "pmessage * __keyspace@${db}__:myhash hexpired" [$rd1 read] - # Test that we wll get `hexpired` notification when - # a hash fied is removed by active expire. + # Test that we will get `hexpired` notification when + # a hash field is removed by active expire. r hpexpire myhash 10 FIELDS 1 f1 assert_equal "pmessage * __keyspace@${db}__:myhash hexpire" [$rd1 read] after 100 ;# Wait for active expire assert_equal "pmessage * __keyspace@${db}__:myhash hexpired" [$rd1 read] - # Test that we wll get `hexpired` notification when - # a hash fied is removed by lazy active. + # Test that we will get `hexpired` notification when + # a hash field is removed by lazy active. r debug set-active-expire 0 r hpexpire myhash 10 FIELDS 1 f2 assert_equal "pmessage * __keyspace@${db}__:myhash hexpire" [$rd1 read] From 3c8d6df215f203da96f4222a37b339892128a88c Mon Sep 17 00:00:00 2001 From: "debing.sun" Date: Fri, 7 Jun 2024 15:41:56 +0800 Subject: [PATCH 03/12] Add needs:debug for test --- tests/unit/pubsub.tcl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/pubsub.tcl b/tests/unit/pubsub.tcl index f362f5aeef8..9f9f81840c3 100644 --- a/tests/unit/pubsub.tcl +++ b/tests/unit/pubsub.tcl @@ -394,7 +394,7 @@ start_server {tags {"pubsub network"}} { r debug set-active-expire 1 $rd1 close - } + } {0} {needs:debug} } ;# foreach test "Keyspace notifications: stream events test" { From 81e551ec7a1de386eab5c2c81e24c3020f16dbc2 Mon Sep 17 00:00:00 2001 From: "debing.sun" Date: Tue, 11 Jun 2024 11:01:50 +0800 Subject: [PATCH 04/12] Fix CR 1. send "hexpired" notification first and then "del" 2. add signalModifiedKey() in hashTypeGetValue() Co-authored-by: Ozan Tezcan Co-authored-by: Moti Cohen --- src/t_hash.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/t_hash.c b/src/t_hash.c index fb39578e924..2e3d5040a84 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -780,6 +780,7 @@ GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vs notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", keyObj, db->id); res = GETF_EXPIRED; } + signalModifiedKey(NULL, db, keyObj); decrRefCount(keyObj); return res; } @@ -1848,11 +1849,10 @@ static ExpireAction hashTypeActiveExpire(eItem _hashObj, void *ctx) { ActiveExpireCtx *activeExpireCtx = (ActiveExpireCtx *) ctx; sds keystr = NULL; ExpireInfo info = {0}; - ExpireAction ret = ACT_STOP_ACTIVE_EXP; /* If no more quota left for this callback, stop */ if (activeExpireCtx->fieldsToExpireQuota == 0) - return ret; + return ACT_STOP_ACTIVE_EXP; if (hashObj->encoding == OBJ_ENCODING_LISTPACK_EX) { info = (ExpireInfo){ @@ -1889,20 +1889,19 @@ static ExpireAction hashTypeActiveExpire(eItem _hashObj, void *ctx) { activeExpireCtx->fieldsToExpireQuota -= info.itemsExpired; /* If hash has no more fields to expire, remove it from HFE DB */ + ExpireAction ret; robj *key = createStringObject(keystr, sdslen(keystr)); + notifyKeyspaceEvent(NOTIFY_HASH,"hexpired",key,activeExpireCtx->db->id); if (info.nextExpireTime == EB_EXPIRE_TIME_INVALID) { if (hashTypeLength(hashObj, 0) == 0) { dbDelete(activeExpireCtx->db, key); notifyKeyspaceEvent(NOTIFY_GENERIC,"del",key,activeExpireCtx->db->id); - } else { - notifyKeyspaceEvent(NOTIFY_HASH,"hexpired",key,activeExpireCtx->db->id); } ret = ACT_REMOVE_EXP_ITEM; } else { /* Hash has more fields to expire. Update next expiration time of the hash * and indicate to add it back to global HFE DS */ ebSetMetaExpTime(hashGetExpireMeta(hashObj), info.nextExpireTime); - notifyKeyspaceEvent(NOTIFY_HASH,"hexpired",key,activeExpireCtx->db->id); ret = ACT_UPDATE_EXP_ITEM; } From f7875839d4eae42da6841db7f86a00c00ecbff23 Mon Sep 17 00:00:00 2001 From: "debing.sun" Date: Tue, 11 Jun 2024 11:14:02 +0800 Subject: [PATCH 05/12] Merge branch 'unstable' into hexpired_notification --- src/module.c | 77 ++-- src/server.h | 9 +- src/sort.c | 2 +- src/t_hash.c | 605 ++++++++++++-------------- tests/integration/rdb.tcl | 6 +- tests/unit/moduleapi/hash.tcl | 39 ++ tests/unit/moduleapi/scan.tcl | 22 +- tests/unit/type/hash-field-expire.tcl | 14 +- 8 files changed, 400 insertions(+), 374 deletions(-) diff --git a/src/module.c b/src/module.c index 81f72660297..a279d40290a 100644 --- a/src/module.c +++ b/src/module.c @@ -5271,10 +5271,21 @@ int RM_HashSet(RedisModuleKey *key, int flags, ...) { /* Handle XX and NX */ if (flags & (REDISMODULE_HASH_XX|REDISMODULE_HASH_NX)) { - int isHashDeleted; - int exists = hashTypeExists(key->db, key->value, field->ptr, &isHashDeleted); - /* hash-field-expiration is not exposed to modules */ - serverAssert(isHashDeleted == 0); + int hfeFlags = HFE_LAZY_AVOID_HASH_DEL; /* Avoid invalidate the key */ + + /* + * The hash might contain expired fields. If we lazily delete expired + * field and the command was sent with XX flag, the operation could + * fail and leave the hash empty, which the caller might not expect. + * To prevent unexpected behavior, we avoid lazy deletion in this case + * yet let the operation fail. Note that moduleDelKeyIfEmpty() + * below won't delete the hash if it left with single expired key + * because hash counts blindly expired fields as well. + */ + if (flags & REDISMODULE_HASH_XX) + hfeFlags |= HFE_LAZY_AVOID_FIELD_DEL; + + int exists = hashTypeExists(key->db, key->value, field->ptr, hfeFlags, NULL); if (((flags & REDISMODULE_HASH_XX) && !exists) || ((flags & REDISMODULE_HASH_NX) && exists)) { @@ -5357,6 +5368,7 @@ int RM_HashSet(RedisModuleKey *key, int flags, ...) { * RedisModule_FreeString(), or by enabling automatic memory management. */ int RM_HashGet(RedisModuleKey *key, int flags, ...) { + int hfeFlags = HFE_LAZY_AVOID_FIELD_DEL | HFE_LAZY_AVOID_HASH_DEL; va_list ap; if (key->value && key->value->type != OBJ_HASH) return REDISMODULE_ERR; @@ -5378,21 +5390,16 @@ int RM_HashGet(RedisModuleKey *key, int flags, ...) { if (flags & REDISMODULE_HASH_EXISTS) { existsptr = va_arg(ap,int*); if (key->value) { - int isHashDeleted; - *existsptr = hashTypeExists(key->db, key->value, field->ptr, &isHashDeleted); - /* hash-field-expiration is not exposed to modules */ - serverAssert(isHashDeleted == 0); + *existsptr = hashTypeExists(key->db, key->value, field->ptr, hfeFlags, NULL); } else { *existsptr = 0; } } else { - int isHashDeleted; valueptr = va_arg(ap,RedisModuleString**); if (key->value) { - *valueptr = hashTypeGetValueObject(key->db,key->value,field->ptr, &isHashDeleted); + *valueptr = hashTypeGetValueObject(key->db, key->value, field->ptr, + hfeFlags, NULL); - /* Currently hash-field-expiration is not exposed to modules */ - serverAssert(isHashDeleted == 0); if (*valueptr) { robj *decoded = getDecodedObject(*valueptr); decrRefCount(*valueptr); @@ -11080,6 +11087,11 @@ static void moduleScanKeyCallback(void *privdata, const dictEntry *de) { value = NULL; } else if (o->type == OBJ_HASH) { sds val = dictGetVal(de); + + /* If field is expired, then ignore */ + if (hfieldIsExpired(key)) + return; + field = createStringObject(key, hfieldlen(key)); value = createStringObject(val, sdslen(val)); } else if (o->type == OBJ_ZSET) { @@ -11189,9 +11201,8 @@ int RM_ScanKey(RedisModuleKey *key, RedisModuleScanCursor *cursor, RedisModuleSc ret = 0; } else if (o->type == OBJ_ZSET || o->type == OBJ_HASH) { unsigned char *lp, *p; - unsigned char *vstr; - unsigned int vlen; - long long vll; + /* is hash with expiry on fields, then lp tuples are [field][value][expire] */ + int hfe = o->type == OBJ_HASH && o->encoding == OBJ_ENCODING_LISTPACK_EX; if (o->type == OBJ_HASH) lp = hashTypeListpackGetLp(o); @@ -11200,19 +11211,32 @@ int RM_ScanKey(RedisModuleKey *key, RedisModuleScanCursor *cursor, RedisModuleSc p = lpSeek(lp,0); while(p) { - vstr = lpGetValue(p,&vlen,&vll); - robj *field = (vstr != NULL) ? - createStringObject((char*)vstr,vlen) : - createStringObjectFromLongLongWithSds(vll); + long long vllField, vllValue, vllExpire; + unsigned int lenField, lenValue; + unsigned char *pField, *pValue; + + pField = lpGetValue(p,&lenField,&vllField); p = lpNext(lp,p); - vstr = lpGetValue(p,&vlen,&vll); - robj *value = (vstr != NULL) ? - createStringObject((char*)vstr,vlen) : - createStringObjectFromLongLongWithSds(vll); - fn(key, field, value, privdata); + pValue = lpGetValue(p,&lenValue,&vllValue); p = lpNext(lp,p); - if (o->type == OBJ_HASH && o->encoding == OBJ_ENCODING_LISTPACK_EX) - p = lpNext(lp, p); /* Skip expire time */ + + if (hfe) { + serverAssert(lpGetIntegerValue(p, &vllExpire)); + p = lpNext(lp, p); + + /* Skip expired fields */ + if (hashTypeIsExpired(o, vllExpire)) + continue; + } + + robj *value = (pValue != NULL) ? + createStringObject((char*)pValue,lenValue) : + createStringObjectFromLongLongWithSds(vllValue); + + robj *field = (pField != NULL) ? + createStringObject((char*)pField,lenField) : + createStringObjectFromLongLongWithSds(vllField); + fn(key, field, value, privdata); decrRefCount(field); decrRefCount(value); @@ -11225,7 +11249,6 @@ int RM_ScanKey(RedisModuleKey *key, RedisModuleScanCursor *cursor, RedisModuleSc return ret; } - /* -------------------------------------------------------------------------- * ## Module fork API * -------------------------------------------------------------------------- */ diff --git a/src/server.h b/src/server.h index 39975ddb09b..59bad41ab96 100644 --- a/src/server.h +++ b/src/server.h @@ -3191,9 +3191,14 @@ typedef struct dictExpireMetadata { #define HASH_SET_TAKE_VALUE (1<<1) #define HASH_SET_COPY 0 +/* Hash field lazy expiration flags. Used by core hashTypeGetValue() and its callers */ +#define HFE_LAZY_EXPIRE (0) /* Delete expired field, and if last field also the hash */ +#define HFE_LAZY_AVOID_FIELD_DEL (1<<0) /* Avoid deleting expired field */ +#define HFE_LAZY_AVOID_HASH_DEL (1<<1) /* Avoid deleting hash if the field is the last one */ + void hashTypeConvert(robj *o, int enc, ebuckets *hexpires); void hashTypeTryConversion(redisDb *db, robj *subject, robj **argv, int start, int end); -int hashTypeExists(redisDb *db, robj *o, sds key, int *isHashDeleted); +int hashTypeExists(redisDb *db, robj *o, sds key, int hfeFlags, int *isHashDeleted); int hashTypeDelete(robj *o, void *key, int isSdsField); unsigned long hashTypeLength(const robj *o, int subtractExpiredFields); hashTypeIterator *hashTypeInitIterator(robj *subject); @@ -3210,7 +3215,7 @@ void hashTypeCurrentObject(hashTypeIterator *hi, int what, unsigned char **vstr, unsigned int *vlen, long long *vll, uint64_t *expireTime); sds hashTypeCurrentObjectNewSds(hashTypeIterator *hi, int what); hfield hashTypeCurrentObjectNewHfield(hashTypeIterator *hi); -robj *hashTypeGetValueObject(redisDb *db, robj *o, sds field, int *isHashDeleted); +robj *hashTypeGetValueObject(redisDb *db, robj *o, sds field, int hfeFlags, int *isHashDeleted); int hashTypeSet(redisDb *db, robj *o, sds field, sds value, int flags); robj *hashTypeDup(robj *o, sds newkey, uint64_t *minHashExpire); uint64_t hashTypeRemoveFromExpires(ebuckets *hexpires, robj *o); diff --git a/src/sort.c b/src/sort.c index d45c380ac39..2dcea1754f5 100644 --- a/src/sort.c +++ b/src/sort.c @@ -95,7 +95,7 @@ robj *lookupKeyByPattern(redisDb *db, robj *pattern, robj *subst) { /* Retrieve value from hash by the field name. The returned object * is a new object with refcount already incremented. */ int isHashDeleted; - o = hashTypeGetValueObject(db, o, fieldobj->ptr, &isHashDeleted); + o = hashTypeGetValueObject(db, o, fieldobj->ptr, HFE_LAZY_EXPIRE, &isHashDeleted); if (isHashDeleted) goto noobj; diff --git a/src/t_hash.c b/src/t_hash.c index 2e3d5040a84..3db2b6be9e0 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -14,14 +14,24 @@ * update the expiration time of the hash object in global HFE DS. */ #define HASH_NEW_EXPIRE_DIFF_THRESHOLD max(4000, 1<> 2) + typedef enum GetFieldRes { /* common (Used by hashTypeGet* value family) */ - GETF_OK = 0, + GETF_OK = 0, /* The field was found. */ GETF_NOT_FOUND, /* The field was not found. */ - - /* used only by hashTypeGetValue() */ - GETF_EXPIRED, /* Logically expired but not yet deleted. */ + GETF_EXPIRED, /* Logically expired (Might be lazy deleted or not) */ GETF_EXPIRED_HASH, /* Delete hash since retrieved field was expired and * it was the last field in the hash. */ } GetFieldRes; @@ -152,36 +162,26 @@ static inline int isDictWithMetaHFE(dict *d) { } /*----------------------------------------------------------------------------- - * setex* - Set field OR field's expiration + * setex* - Set field's expiration * - * Whereas setting plain fields is rather straightforward, setting expiration - * time to fields might be time-consuming and complex since each update of - * expiration time, not only updates `ebuckets` of corresponding hash, but also - * might update `ebuckets` of global HFE DS. It is required to opt sequence of - * field updates with expirartion for a given hash, such that only once done, - * the global HFE DS will get updated. + * Setting expiration time to fields might be time-consuming and complex since + * each update of expiration time, not only updates `ebuckets` of corresponding + * hash, but also might update `ebuckets` of global HFE DS. It is required to opt + * sequence of field updates with expirartion for a given hash, such that only + * once done, the global HFE DS will get updated. * * To do so, follow the scheme: * 1. Call hashTypeSetExInit() to initialize the HashTypeSetEx struct. * 2. Call hashTypeSetEx() one time or more, for each field/expiration update. * 3. Call hashTypeSetExDone() for notification and update of global HFE. - * - * If expiration is not required, then avoid this API and use instead hashTypeSet() *----------------------------------------------------------------------------*/ /* Returned value of hashTypeSetEx() */ typedef enum SetExRes { - /* Common res from hashTypeSetEx() */ HSETEX_OK = 1, /* Expiration time set/updated as expected */ - - /* If provided HashTypeSetEx struct to hashTypeSetEx() */ HSETEX_NO_FIELD = -2, /* No such hash-field */ HSETEX_NO_CONDITION_MET = 0, /* Specified NX | XX | GT | LT condition not met */ HSETEX_DELETED = 2, /* Field deleted because the specified time is in the past */ - - /* If not provided HashTypeSetEx struct to hashTypeSetEx() (plain HSET) */ - HSET_UPDATE = 4, /* Update of the field without expiration time */ - } SetExRes; /* Used by httlGenericCommand() */ @@ -190,20 +190,6 @@ typedef enum GetExpireTimeRes { HFE_GET_NO_TTL = -1, /* No TTL attached to the field */ } GetExpireTimeRes; -/* on fail return HSETEX_NO_CONDITION_MET */ -typedef enum FieldSetCond { - FIELD_CREATE_OR_OVRWRT = 0, - FIELD_DONT_CREATE = 1, - FIELD_DONT_CREATE2 = 2, /* on fail return HSETEX_NO_FIELD */ - FIELD_DONT_OVRWRT = 3 -} FieldSetCond; - -typedef enum FieldGet { /* TBD */ - FIELD_GET_NONE = 0, - FIELD_GET_NEW = 1, - FIELD_GET_OLD = 2 -} FieldGet; - typedef enum ExpireSetCond { HFE_NX = 1<<0, HFE_XX = 1<<1, @@ -211,16 +197,10 @@ typedef enum ExpireSetCond { HFE_LT = 1<<3 } ExpireSetCond; -typedef struct HashTypeSet { - sds value; - int flags; -} HashTypeSet; - /* Used by hashTypeSetEx() for setting fields or their expiry */ typedef struct HashTypeSetEx { /*** config ***/ - FieldSetCond fieldSetCond; /* [DCF | DOF] */ ExpireSetCond expireSetCond; /* [XX | NX | GT | LT] */ /*** metadata ***/ @@ -239,14 +219,10 @@ typedef struct HashTypeSetEx { const char *cmd; } HashTypeSetEx; -static SetExRes hashTypeSetExListpack(redisDb *db, robj *o, sds field, HashTypeSet *setParams, - uint64_t expireAt, HashTypeSetEx *exParams); - int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cmd, - FieldSetCond fieldSetCond, ExpireSetCond expireSetCond, HashTypeSetEx *ex); + ExpireSetCond expireSetCond, HashTypeSetEx *ex); -SetExRes hashTypeSetEx(redisDb *db, robj *o, sds field, HashTypeSet *setKeyVal, - uint64_t expireAt, HashTypeSetEx *exInfo); +SetExRes hashTypeSetEx(robj *o, sds field, uint64_t expireAt, HashTypeSetEx *exInfo); void hashTypeSetExDone(HashTypeSetEx *e); @@ -712,23 +688,21 @@ GetFieldRes hashTypeGetFromHashTable(robj *o, sds field, sds *value, uint64_t *e /* Higher level function of hashTypeGet*() that returns the hash value * associated with the specified field. + * Arguments: + * hfeFlags - Lookup for HFE_LAZY_* flags * * Returned: - * - GetFieldRes: OK: Return Field's valid value - * NOT_FOUND: Field was not found. - * EXPIRED: Field is expired and Lazy deleted - * EXPIRED_HASH: Returned only if the field is the last one in the - * hash and the hash is deleted. - * - vstr, vlen : if string, ref in either *vstr and *vlen if it's + * GetFieldRes - Result of get operation + * vstr, vlen - if string, ref in either *vstr and *vlen if it's * returned in string form, - * - vll : or stored in *vll if it's returned as a number. + * vll - or stored in *vll if it's returned as a number. * If *vll is populated *vstr is set to NULL, so the caller can * always check the function return by checking the return value * for GETF_OK and checking if vll (or vstr) is NULL. * */ GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vstr, - unsigned int *vlen, long long *vll) { + unsigned int *vlen, long long *vll, int hfeFlags) { uint64_t expiredAt; sds key; GetFieldRes res; @@ -760,7 +734,12 @@ GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vs (expiredAt >= (uint64_t) commandTimeSnapshot()) ) return GETF_OK; - /* Got expired. Extract attached key from LISTPACK_EX/HT */ + /* Field is expired */ + + /* If indicated to avoid deleting expired field */ + if (hfeFlags & HFE_LAZY_AVOID_FIELD_DEL) + return GETF_EXPIRED; + if (o->encoding == OBJ_ENCODING_LISTPACK_EX) key = ((listpackEx *) o->ptr)->key; else @@ -771,14 +750,14 @@ GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vs propagateHashFieldDeletion(db, key, field, sdslen(field)); /* If the field is the last one in the hash, then the hash will be deleted */ + res = GETF_EXPIRED; robj *keyObj = createStringObject(key, sdslen(key)); - if (hashTypeLength(o, 0) == 0) { + notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", keyObj, db->id); + if ((hashTypeLength(o, 0) == 0) && (!(hfeFlags & HFE_LAZY_AVOID_HASH_DEL))) { + robj *keyObj = createStringObject(key, sdslen(key)); notifyKeyspaceEvent(NOTIFY_GENERIC, "del", keyObj, db->id); dbDelete(db,keyObj); res = GETF_EXPIRED_HASH; - } else { - notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", keyObj, db->id); - res = GETF_EXPIRED; } signalModifiedKey(NULL, db, keyObj); decrRefCount(keyObj); @@ -790,24 +769,25 @@ GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vs * The function returns NULL if the field is not found in the hash. Otherwise * a newly allocated string object with the value is returned. * + * hfeFlags - Lookup HFE_LAZY_* flags * isHashDeleted - If attempted to access expired field and it's the last field * in the hash, then the hash will as well be deleted. In this case, * isHashDeleted will be set to 1. */ -robj *hashTypeGetValueObject(redisDb *db, robj *o, sds field, int *isHashDeleted) { +robj *hashTypeGetValueObject(redisDb *db, robj *o, sds field, int hfeFlags, int *isHashDeleted) { unsigned char *vstr; unsigned int vlen; long long vll; - *isHashDeleted = 0; /*default*/ - GetFieldRes res = hashTypeGetValue(db,o,field,&vstr,&vlen,&vll); + if (isHashDeleted) *isHashDeleted = 0; + GetFieldRes res = hashTypeGetValue(db,o,field,&vstr,&vlen,&vll, hfeFlags); if (res == GETF_OK) { if (vstr) return createStringObject((char*)vstr,vlen); else return createStringObjectFromLongLong(vll); } - if (res == GETF_EXPIRED_HASH) + if ((res == GETF_EXPIRED_HASH) && (isHashDeleted)) *isHashDeleted = 1; /* GETF_EXPIRED_HASH, GETF_EXPIRED, GETF_NOT_FOUND */ @@ -817,19 +797,21 @@ robj *hashTypeGetValueObject(redisDb *db, robj *o, sds field, int *isHashDeleted /* Test if the specified field exists in the given hash. If the field is * expired (HFE), then it will be lazy deleted * - * Returns 1 if the field exists, and 0 when it doesn't. - * + * hfeFlags - Lookup HFE_LAZY_* flags * isHashDeleted - If attempted to access expired field and it is the last field * in the hash, then the hash will as well be deleted. In this case, * isHashDeleted will be set to 1. + * + * Returns 1 if the field exists, and 0 when it doesn't. */ -int hashTypeExists(redisDb *db, robj *o, sds field, int *isHashDeleted) { +int hashTypeExists(redisDb *db, robj *o, sds field, int hfeFlags, int *isHashDeleted) { unsigned char *vstr = NULL; unsigned int vlen = UINT_MAX; long long vll = LLONG_MAX; - GetFieldRes res = hashTypeGetValue(db, o, field, &vstr, &vlen, &vll); - *isHashDeleted = (res == GETF_EXPIRED_HASH) ? 1 : 0; + GetFieldRes res = hashTypeGetValue(db, o, field, &vstr, &vlen, &vll, hfeFlags); + if (isHashDeleted) + *isHashDeleted = (res == GETF_EXPIRED_HASH) ? 1 : 0; return (res == GETF_OK) ? 1 : 0; } @@ -842,7 +824,7 @@ int hashTypeExists(redisDb *db, robj *o, sds field, int *isHashDeleted) { * * HASH_SET_TAKE_FIELD -- The SDS field ownership passes to the function. * HASH_SET_TAKE_VALUE -- The SDS value ownership passes to the function. - * HASH_SET_KEEP_FIELD -- keep original field along with TTL if already exists + * HASH_SET_KEEP_TTL -- keep original TTL if field already exists * * When the flags are used the caller does not need to release the passed * SDS string(s). It's up to the function to use the string to create a new @@ -854,204 +836,251 @@ int hashTypeExists(redisDb *db, robj *o, sds field, int *isHashDeleted) { */ #define HASH_SET_TAKE_FIELD (1<<0) #define HASH_SET_TAKE_VALUE (1<<1) -#define HASH_SET_KEEP_FIELD (1<<2) +#define HASH_SET_KEEP_TTL (1<<2) #define HASH_SET_COPY 0 int hashTypeSet(redisDb *db, robj *o, sds field, sds value, int flags) { - HashTypeSet set = {value, flags}; - return (hashTypeSetEx(db, o, field, &set, 0, NULL) == HSET_UPDATE) ? 1 : 0; -} + int update = 0; -SetExRes hashTypeSetExpiry(HashTypeSetEx *ex, sds field, uint64_t expireAt, dictEntry **de) { - dict *ht = ex->hashObj->ptr; - dictEntry *newEntry = NULL, *existingEntry = NULL; + /* Check if the field is too long for listpack, and convert before adding the item. + * This is needed for HINCRBY* case since in other commands this is handled early by + * hashTypeTryConversion, so this check will be a NOP. */ + if (o->encoding == OBJ_ENCODING_LISTPACK || + o->encoding == OBJ_ENCODING_LISTPACK_EX) { + if (sdslen(field) > server.hash_max_listpack_value || sdslen(value) > server.hash_max_listpack_value) + hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); + } - /* New field with expiration metadata */ - hfield hfNew = hfieldNew(field, sdslen(field), 1 /*withExpireMeta*/); + if (o->encoding == OBJ_ENCODING_LISTPACK) { + unsigned char *zl, *fptr, *vptr; - if ((ex->fieldSetCond == FIELD_DONT_CREATE) || (ex->fieldSetCond == FIELD_DONT_CREATE2)) { - if ((existingEntry = dictFind(ht, field)) == NULL) { - hfieldFree(hfNew); - return (ex->fieldSetCond == FIELD_DONT_CREATE) ? - HSETEX_NO_CONDITION_MET : HSETEX_NO_FIELD; + zl = o->ptr; + fptr = lpFirst(zl); + if (fptr != NULL) { + fptr = lpFind(zl, fptr, (unsigned char*)field, sdslen(field), 1); + if (fptr != NULL) { + /* Grab pointer to the value (fptr points to the field) */ + vptr = lpNext(zl, fptr); + serverAssert(vptr != NULL); + + /* Replace value */ + zl = lpReplace(zl, &vptr, (unsigned char*)value, sdslen(value)); + update = 1; + } } - } else { + + if (!update) { + /* Push new field/value pair onto the tail of the listpack */ + zl = lpAppend(zl, (unsigned char*)field, sdslen(field)); + zl = lpAppend(zl, (unsigned char*)value, sdslen(value)); + } + o->ptr = zl; + + /* Check if the listpack needs to be converted to a hash table */ + if (hashTypeLength(o, 0) > server.hash_max_listpack_entries) + hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); + } else if (o->encoding == OBJ_ENCODING_LISTPACK_EX) { + unsigned char *fptr = NULL, *vptr = NULL, *tptr = NULL; + listpackEx *lpt = o->ptr; + long long expireTime = HASH_LP_NO_TTL; + + fptr = lpFirst(lpt->lp); + if (fptr != NULL) { + fptr = lpFind(lpt->lp, fptr, (unsigned char*)field, sdslen(field), 2); + if (fptr != NULL) { + /* Grab pointer to the value (fptr points to the field) */ + vptr = lpNext(lpt->lp, fptr); + serverAssert(vptr != NULL); + + /* Replace value */ + lpt->lp = lpReplace(lpt->lp, &vptr, (unsigned char *) value, sdslen(value)); + update = 1; + + fptr = lpPrev(lpt->lp, vptr); + serverAssert(fptr != NULL); + + tptr = lpNext(lpt->lp, vptr); + serverAssert(tptr && lpGetIntegerValue(tptr, &expireTime)); + + if (flags & HASH_SET_KEEP_TTL) { + /* keep old field along with TTL */ + } else if (expireTime != HASH_LP_NO_TTL) { + /* re-insert field and override TTL */ + listpackExUpdateExpiry(o, field, fptr, vptr, HASH_LP_NO_TTL); + } + } + } + + if (!update) + listpackExAddNew(o, field, sdslen(field), value, sdslen(value), + HASH_LP_NO_TTL); + + /* Check if the listpack needs to be converted to a hash table */ + if (hashTypeLength(o, 0) > server.hash_max_listpack_entries) + hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); + + } else if (o->encoding == OBJ_ENCODING_HT) { + hfield newField = hfieldNew(field, sdslen(field), 0); + dict *ht = o->ptr; + dictEntry *de, *existing; + + /* stored key is different than lookup key */ dictUseStoredKeyApi(ht, 1); - newEntry = dictAddRaw(ht, hfNew, &existingEntry); + de = dictAddRaw(ht, newField, &existing); dictUseStoredKeyApi(ht, 0); - } - if (newEntry) { - *de = newEntry; - - if (ex->expireSetCond & (HFE_XX | HFE_LT | HFE_GT)) { - dictDelete(ht, field); - return HSETEX_NO_CONDITION_MET; + /* If field already exists, then update "field". "Value" will be set afterward */ + if (de == NULL) { + if (flags & HASH_SET_KEEP_TTL) { + /* keep old field along with TTL */ + hfieldFree(newField); + } else { + /* If attached TTL to the old field, then remove it from hash's private ebuckets */ + hfield oldField = dictGetKey(existing); + hfieldPersist(o, oldField); + hfieldFree(oldField); + dictSetKey(ht, existing, newField); + } + sdsfree(dictGetVal(existing)); + update = 1; + de = existing; } - } else { /* field exist */ - *de = existingEntry; - if (ex->fieldSetCond == FIELD_DONT_OVRWRT) { - hfieldFree(hfNew); - return HSETEX_NO_CONDITION_MET; + if (flags & HASH_SET_TAKE_VALUE) { + dictSetVal(ht, de, value); + flags &= ~HASH_SET_TAKE_VALUE; + } else { + dictSetVal(ht, de, sdsdup(value)); } + } else { + serverPanic("Unknown hash encoding"); + } - hfield hfOld = dictGetKey(existingEntry); + /* Free SDS strings we did not referenced elsewhere if the flags + * want this function to be responsible. */ + if (flags & HASH_SET_TAKE_FIELD && field) sdsfree(field); + if (flags & HASH_SET_TAKE_VALUE && value) sdsfree(value); + return update; +} - /* If field doesn't have expiry metadata attached */ - if (!hfieldIsExpireAttached(hfOld)) { +SetExRes hashTypeSetExpiryHT(HashTypeSetEx *exInfo, sds field, uint64_t expireAt) { + dict *ht = exInfo->hashObj->ptr; + dictEntry *existingEntry = NULL; - /* For fields without expiry, LT condition is considered valid */ - if (ex->expireSetCond & (HFE_XX | HFE_GT)) { - hfieldFree(hfNew); - return HSETEX_NO_CONDITION_MET; - } + /* New field with expiration metadata */ + hfield hfNew = hfieldNew(field, sdslen(field), 1 /*withExpireMeta*/); + + if ((existingEntry = dictFind(ht, field)) == NULL) { + hfieldFree(hfNew); + return HSETEX_NO_FIELD; + } - /* Delete old field. Below goanna dictSetKey(..,hfNew) */ - hfieldFree(hfOld); + hfield hfOld = dictGetKey(existingEntry); - } else { /* field has ExpireMeta struct attached */ + /* If field doesn't have expiry metadata attached */ + if (!hfieldIsExpireAttached(hfOld)) { - /* No need for hfNew (Just modify expire-time of existing field) */ + /* For fields without expiry, LT condition is considered valid */ + if (exInfo->expireSetCond & (HFE_XX | HFE_GT)) { hfieldFree(hfNew); + return HSETEX_NO_CONDITION_MET; + } - uint64_t prevExpire = hfieldGetExpireTime(hfOld); + /* Delete old field. Below goanna dictSetKey(..,hfNew) */ + hfieldFree(hfOld); - /* If field has valid expiration time, then check GT|LT|NX */ - if (prevExpire != EB_EXPIRE_TIME_INVALID) { - if (((ex->expireSetCond == HFE_GT) && (prevExpire >= expireAt)) || - ((ex->expireSetCond == HFE_LT) && (prevExpire <= expireAt)) || - (ex->expireSetCond == HFE_NX) ) - return HSETEX_NO_CONDITION_MET; + } else { /* field has ExpireMeta struct attached */ - /* remove old expiry time from hash's private ebuckets */ - dictExpireMetadata *dm = (dictExpireMetadata *) dictMetadata(ht); - ebRemove(&dm->hfe, &hashFieldExpireBucketsType, hfOld); + /* No need for hfNew (Just modify expire-time of existing field) */ + hfieldFree(hfNew); - /* Track of minimum expiration time (only later update global HFE DS) */ - if (ex->minExpireFields > prevExpire) - ex->minExpireFields = prevExpire; + uint64_t prevExpire = hfieldGetExpireTime(hfOld); - } else { - /* field has invalid expiry. No need to ebRemove() */ + /* If field has valid expiration time, then check GT|LT|NX */ + if (prevExpire != EB_EXPIRE_TIME_INVALID) { + if (((exInfo->expireSetCond == HFE_GT) && (prevExpire >= expireAt)) || + ((exInfo->expireSetCond == HFE_LT) && (prevExpire <= expireAt)) || + (exInfo->expireSetCond == HFE_NX) ) + return HSETEX_NO_CONDITION_MET; - /* Check XX|LT|GT */ - if (ex->expireSetCond & (HFE_XX | HFE_GT)) - return HSETEX_NO_CONDITION_MET; - } + /* remove old expiry time from hash's private ebuckets */ + dictExpireMetadata *dm = (dictExpireMetadata *) dictMetadata(ht); + ebRemove(&dm->hfe, &hashFieldExpireBucketsType, hfOld); + + /* Track of minimum expiration time (only later update global HFE DS) */ + if (exInfo->minExpireFields > prevExpire) + exInfo->minExpireFields = prevExpire; - /* Reuse hfOld as hfNew and rewrite its expiry with ebAdd() */ - hfNew = hfOld; + } else { + /* field has invalid expiry. No need to ebRemove() */ + + /* Check XX|LT|GT */ + if (exInfo->expireSetCond & (HFE_XX | HFE_GT)) + return HSETEX_NO_CONDITION_MET; } - dictSetKey(ht, existingEntry, hfNew); + /* Reuse hfOld as hfNew and rewrite its expiry with ebAdd() */ + hfNew = hfOld; } + dictSetKey(ht, existingEntry, hfNew); + + /* if expiration time is in the past */ if (unlikely(checkAlreadyExpired(expireAt))) { - hashTypeDelete(ex->hashObj, field, 1); - ex->fieldDeleted++; + hashTypeDelete(exInfo->hashObj, field, 1); + exInfo->fieldDeleted++; return HSETEX_DELETED; } - if (ex->minExpireFields > expireAt) - ex->minExpireFields = expireAt; + if (exInfo->minExpireFields > expireAt) + exInfo->minExpireFields = expireAt; dictExpireMetadata *dm = (dictExpireMetadata *) dictMetadata(ht); ebAdd(&dm->hfe, &hashFieldExpireBucketsType, hfNew, expireAt); - ex->fieldUpdated++; + exInfo->fieldUpdated++; return HSETEX_OK; } /* - * Set fields OR field's expiration (See also `setex*` comment above) + * Set field expiration * * Take care to call first hashTypeSetExInit() and then call this function. * Finally, call hashTypeSetExDone() to notify and update global HFE DS. - * - * NOTE: this functions is also called during RDB load to set dict-encoded - * fields with and without expiration. */ -SetExRes hashTypeSetEx(redisDb *db, robj *o, sds field, HashTypeSet *setKeyVal, - uint64_t expireAt, HashTypeSetEx *exInfo) +SetExRes hashTypeSetEx(robj *o, sds field, uint64_t expireAt, HashTypeSetEx *exInfo) { - SetExRes res = HSETEX_OK; - int isSetKeyValue = (setKeyVal) ? 1 : 0; - int isSetExpire = (exInfo) ? 1 : 0; - int flags = (setKeyVal) ? setKeyVal->flags : 0; - - /* Check if the field is too long for listpack, and convert before adding the item. - * This is needed for HINCRBY* case since in other commands this is handled early by - * hashTypeTryConversion, so this check will be a NOP. */ - if (o->encoding == OBJ_ENCODING_LISTPACK || - o->encoding == OBJ_ENCODING_LISTPACK_EX) + if (o->encoding == OBJ_ENCODING_LISTPACK_EX) { - if ( (isSetKeyValue) && - (sdslen(field) > server.hash_max_listpack_value || - sdslen(setKeyVal->value) > server.hash_max_listpack_value) ) - { - hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); - } else { - res = hashTypeSetExListpack(db, o, field, setKeyVal, expireAt, exInfo); - goto SetExDone; /*done*/ - } - } + unsigned char *fptr = NULL, *vptr = NULL, *tptr = NULL; - if (o->encoding != OBJ_ENCODING_HT) - serverPanic("Unknown hash encoding"); + listpackEx *lpt = o->ptr; + long long expireTime = HASH_LP_NO_TTL; - /*** now deal with HT ***/ - hfield newField; - dict *ht = o->ptr; - dictEntry *de; + if ((fptr = lpFirst(lpt->lp)) == NULL) + return HSETEX_NO_FIELD; - /* If needed to set the field along with expiry */ - if (isSetExpire) { - res = hashTypeSetExpiry(exInfo, field, expireAt, &de); - if (res != HSETEX_OK) goto SetExDone; - } else { - dictEntry *existing; - /* Cannot leverage HASH_SET_TAKE_FIELD since hfield is not of type sds */ - newField = hfieldNew(field, sdslen(field), 0); + fptr = lpFind(lpt->lp, fptr, (unsigned char*)field, sdslen(field), 2); - /* stored key is different than lookup key */ - dictUseStoredKeyApi(ht, 1); - de = dictAddRaw(ht, newField, &existing); - dictUseStoredKeyApi(ht, 0); + if (!fptr) + return HSETEX_NO_FIELD; - /* If field already exists, then update "field". "Value" will be set afterward */ - if (de == NULL) { - if (flags & HASH_SET_KEEP_FIELD) { - /* Not keep old field along with TTL */ - hfieldFree(newField); - } else { - /* If attached TTL to the old field, then remove it from hash's private ebuckets */ - hfield oldField = dictGetKey(existing); - hfieldPersist(o, oldField); - hfieldFree(oldField); - dictSetKey(ht, existing, newField); - } - sdsfree(dictGetVal(existing)); - res = HSET_UPDATE; - de = existing; - } - } + /* Grab pointer to the value (fptr points to the field) */ + vptr = lpNext(lpt->lp, fptr); + serverAssert(vptr != NULL); - /* If need to set value */ - if (isSetKeyValue) { - if (flags & HASH_SET_TAKE_VALUE) { - dictSetVal(ht, de, setKeyVal->value); - flags &= ~HASH_SET_TAKE_VALUE; - } else { - dictSetVal(ht, de, sdsdup(setKeyVal->value)); - } + tptr = lpNext(lpt->lp, vptr); + serverAssert(tptr && lpGetIntegerValue(tptr, &expireTime)); + + /* update TTL */ + return hashTypeSetExpiryListpack(exInfo, field, fptr, vptr, tptr, expireAt); + } else if (o->encoding == OBJ_ENCODING_HT) { + /* If needed to set the field along with expiry */ + return hashTypeSetExpiryHT(exInfo, field, expireAt); + } else { + serverPanic("Unknown hash encoding"); } -SetExDone: - /* Free SDS strings we did not referenced elsewhere if the flags - * want this function to be responsible. */ - if (flags & HASH_SET_TAKE_FIELD && field) sdsfree(field); - if (flags & HASH_SET_TAKE_VALUE && setKeyVal->value) sdsfree(setKeyVal->value); - return res; + return HSETEX_OK; /* never reach here */ } void initDictExpireMetadata(sds key, robj *o) { @@ -1069,12 +1098,10 @@ void initDictExpireMetadata(sds key, robj *o) { * Don't have to provide client and "cmd". If provided, then notification once * done by function hashTypeSetExDone(). */ -int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cmd, FieldSetCond fieldSetCond, +int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cmd, ExpireSetCond expireSetCond, HashTypeSetEx *ex) { dict *ht = o->ptr; - - ex->fieldSetCond = fieldSetCond; ex->expireSetCond = expireSetCond; ex->minExpire = EB_EXPIRE_TIME_INVALID; ex->c = c; @@ -1126,16 +1153,16 @@ void hashTypeSetExDone(HashTypeSetEx *ex) { /* Notify keyspace event, update dirty count and update global HFE DS */ if (ex->fieldDeleted + ex->fieldUpdated > 0) { - if (ex->c) { - server.dirty += ex->fieldDeleted + ex->fieldUpdated; - signalModifiedKey(ex->c, ex->db, ex->key); - notifyKeyspaceEvent(NOTIFY_HASH, ex->fieldDeleted ? "hexpired" : "hexpire", - ex->key, ex->db->id); - } + server.dirty += ex->fieldDeleted + ex->fieldUpdated; if (ex->fieldDeleted && hashTypeLength(ex->hashObj, 0) == 0) { dbDelete(ex->db,ex->key); - if (ex->c) notifyKeyspaceEvent(NOTIFY_GENERIC,"del",ex->key, ex->db->id); + signalModifiedKey(ex->c, ex->db, ex->key); + notifyKeyspaceEvent(NOTIFY_GENERIC,"del",ex->key, ex->db->id); } else { + signalModifiedKey(ex->c, ex->db, ex->key); + notifyKeyspaceEvent(NOTIFY_HASH, ex->fieldDeleted ? "hexpired" : "hexpire", + ex->key, ex->db->id); + /* If minimum HFE of the hash is smaller than expiration time of the * specified fields in the command as well as it is smaller or equal * than expiration time provided in the command, then the minimum @@ -1165,99 +1192,6 @@ void hashTypeSetExDone(HashTypeSetEx *ex) { } } -/* Check if the field is too long for listpack, and convert before adding the item. - * This is needed for HINCRBY* case since in other commands this is handled early by - * hashTypeTryConversion, so this check will be a NOP. */ -static SetExRes hashTypeSetExListpack(redisDb *db, robj *o, sds field, HashTypeSet *setParams, - uint64_t expireAt, HashTypeSetEx *exParams) -{ - int res = HSETEX_OK; - unsigned char *fptr = NULL, *vptr = NULL, *tptr = NULL; - - if (o->encoding == OBJ_ENCODING_LISTPACK) { - /* If reached here, then no need to set expiration. Otherwise, as precond - * listpack is converted to listpackex by hashTypeSetExInit() */ - - unsigned char *zl = o->ptr; - fptr = lpFirst(zl); - if (fptr != NULL) { - fptr = lpFind(zl, fptr, (unsigned char*)field, sdslen(field), 1); - if (fptr != NULL) { - /* Grab pointer to the value (fptr points to the field) */ - vptr = lpNext(zl, fptr); - serverAssert(vptr != NULL); - res = HSET_UPDATE; - - /* Replace value */ - zl = lpReplace(zl, &vptr, (unsigned char *) setParams->value, sdslen(setParams->value)); - } - } - - if (res != HSET_UPDATE) { - /* Push new field/value pair onto the tail of the listpack */ - zl = lpAppend(zl, (unsigned char*)field, sdslen(field)); - zl = lpAppend(zl, (unsigned char*)setParams->value, sdslen(setParams->value)); - } - o->ptr = zl; - goto out; - } else if (o->encoding == OBJ_ENCODING_LISTPACK_EX) { - listpackEx *lpt = o->ptr; - long long expireTime = HASH_LP_NO_TTL; - - fptr = lpFirst(lpt->lp); - if (fptr != NULL) { - fptr = lpFind(lpt->lp, fptr, (unsigned char*)field, sdslen(field), 2); - if (fptr != NULL) { - /* Grab pointer to the value (fptr points to the field) */ - vptr = lpNext(lpt->lp, fptr); - serverAssert(vptr != NULL); - - if (setParams) { - /* Replace value */ - lpt->lp = lpReplace(lpt->lp, &vptr, - (unsigned char *) setParams->value, - sdslen(setParams->value)); - - fptr = lpPrev(lpt->lp, vptr); - serverAssert(fptr != NULL); - res = HSET_UPDATE; - } - tptr = lpNext(lpt->lp, vptr); - serverAssert(tptr && lpGetIntegerValue(tptr, &expireTime)); - - /* Keep, update or clear TTL */ - if (setParams && setParams->flags & HASH_SET_KEEP_FIELD) { - /* keep old field along with TTL */ - } else if (exParams) { - res = hashTypeSetExpiryListpack(exParams, field, fptr, vptr, tptr, - expireAt); - if (res != HSETEX_OK) - goto out; - } else if (res == HSET_UPDATE && expireTime != HASH_LP_NO_TTL) { - /* Clear TTL */ - listpackExUpdateExpiry(o, field, fptr, vptr, HASH_LP_NO_TTL); - } - } - } - - if (!fptr) { - if (setParams) { - listpackExAddNew(o, field, sdslen(field), - setParams->value, sdslen(setParams->value), - exParams ? expireAt : HASH_LP_NO_TTL); - } else { - res = HSETEX_NO_FIELD; - } - } - } -out: - /* Check if the listpack needs to be converted to a hash table */ - if (hashTypeLength(o, 0) > server.hash_max_listpack_entries) - hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); - - return res; -} - /* Delete an element from a hash. * * Return 1 on deleted and 0 on not found. @@ -2087,7 +2021,7 @@ void hsetnxCommand(client *c) { robj *o; if ((o = hashTypeLookupWriteOrCreate(c,c->argv[1])) == NULL) return; - if (hashTypeExists(c->db, o, c->argv[2]->ptr, &isHashDeleted)) { + if (hashTypeExists(c->db, o, c->argv[2]->ptr, HFE_LAZY_EXPIRE, &isHashDeleted)) { addReply(c, shared.czero); return; } @@ -2145,7 +2079,8 @@ void hincrbyCommand(client *c) { if (getLongLongFromObjectOrReply(c,c->argv[3],&incr,NULL) != C_OK) return; if ((o = hashTypeLookupWriteOrCreate(c,c->argv[1])) == NULL) return; - GetFieldRes res = hashTypeGetValue(c->db,o,c->argv[2]->ptr,&vstr,&vlen,&value); + GetFieldRes res = hashTypeGetValue(c->db,o,c->argv[2]->ptr,&vstr,&vlen,&value, + HFE_LAZY_EXPIRE); if (res == GETF_OK) { if (vstr) { if (string2ll((char*)vstr,vlen,&value) == 0) { @@ -2170,7 +2105,7 @@ void hincrbyCommand(client *c) { } value += incr; new = sdsfromlonglong(value); - hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE | HASH_SET_KEEP_FIELD); + hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL); addReplyLongLong(c,value); signalModifiedKey(c,c->db,c->argv[1]); notifyKeyspaceEvent(NOTIFY_HASH,"hincrby",c->argv[1],c->db->id); @@ -2191,7 +2126,8 @@ void hincrbyfloatCommand(client *c) { return; } if ((o = hashTypeLookupWriteOrCreate(c,c->argv[1])) == NULL) return; - GetFieldRes res = hashTypeGetValue(c->db, o,c->argv[2]->ptr,&vstr,&vlen,&ll); + GetFieldRes res = hashTypeGetValue(c->db, o,c->argv[2]->ptr,&vstr,&vlen,&ll, + HFE_LAZY_EXPIRE); if (res == GETF_OK) { if (vstr) { if (string2ld((char*)vstr,vlen,&value) == 0) { @@ -2219,7 +2155,7 @@ void hincrbyfloatCommand(client *c) { char buf[MAX_LONG_DOUBLE_CHARS]; int len = ld2string(buf,sizeof(buf),value,LD_STR_HUMAN); new = sdsnewlen(buf,len); - hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE | HASH_SET_KEEP_FIELD); + hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL); addReplyBulkCBuffer(c,buf,len); signalModifiedKey(c,c->db,c->argv[1]); notifyKeyspaceEvent(NOTIFY_HASH,"hincrbyfloat",c->argv[1],c->db->id); @@ -2245,7 +2181,8 @@ static GetFieldRes addHashFieldToReply(client *c, robj *o, sds field) { unsigned int vlen = UINT_MAX; long long vll = LLONG_MAX; - GetFieldRes res = hashTypeGetValue(c->db, o, field, &vstr, &vlen, &vll); + GetFieldRes res = hashTypeGetValue(c->db, o, field, &vstr, &vlen, &vll, + HFE_LAZY_EXPIRE); if (res == GETF_OK) { if (vstr) { addReplyBulkCBuffer(c, vstr, vlen); @@ -2338,7 +2275,8 @@ void hstrlenCommand(client *c) { if ((o = lookupKeyReadOrReply(c,c->argv[1],shared.czero)) == NULL || checkType(c,o,OBJ_HASH)) return; - GetFieldRes res = hashTypeGetValue(c->db, o, c->argv[2]->ptr, &vstr, &vlen, &vll); + GetFieldRes res = hashTypeGetValue(c->db, o, c->argv[2]->ptr, &vstr, &vlen, &vll, + HFE_LAZY_EXPIRE); if (res == GETF_NOT_FOUND || res == GETF_EXPIRED || res == GETF_EXPIRED_HASH) { addReply(c, shared.czero); @@ -2429,11 +2367,11 @@ void hgetallCommand(client *c) { void hexistsCommand(client *c) { robj *o; - int isHashDeleted; if ((o = lookupKeyReadOrReply(c,c->argv[1],shared.czero)) == NULL || checkType(c,o,OBJ_HASH)) return; - addReply(c,hashTypeExists(c->db,o,c->argv[2]->ptr,&isHashDeleted) ? shared.cone : shared.czero); + addReply(c,hashTypeExists(c->db,o,c->argv[2]->ptr,HFE_LAZY_EXPIRE, NULL) ? + shared.cone : shared.czero); } void hscanCommand(client *c) { @@ -2855,6 +2793,7 @@ static ExpireMeta *hashGetExpireMeta(const eItem hash) { serverPanic("Unknown encoding: %d", hashObj->encoding); } } + /* HTTL key */ static void httlGenericCommand(client *c, const char *cmd, long long basetime, int unit) { UNUSED(cmd); @@ -2997,7 +2936,7 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime } if (unit == UNIT_SECONDS) { - if (expire > (long long) EB_EXPIRE_TIME_MAX / 1000) { + if (expire > (long long) HFE_MAX_ABS_TIME_MSEC / 1000) { addReplyErrorExpireTime(c); return; } @@ -3005,7 +2944,7 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime } /* Ensure that the final absolute Unix timestamp does not exceed EB_EXPIRE_TIME_MAX. */ - if (expire > (long long) EB_EXPIRE_TIME_MAX - basetime) { + if (expire > (long long) HFE_MAX_ABS_TIME_MSEC - basetime) { addReplyErrorExpireTime(c); return; } @@ -3040,16 +2979,12 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime } HashTypeSetEx exCtx; - hashTypeSetExInit(keyArg, hashObj, c, c->db, cmd, - FIELD_DONT_CREATE2, - expireSetCond, - &exCtx); - + hashTypeSetExInit(keyArg, hashObj, c, c->db, cmd, expireSetCond, &exCtx); addReplyArrayLen(c, numFields); for (int i = 0 ; i < numFields ; i++) { sds field = c->argv[numFieldsAt+i+1]->ptr; - SetExRes res = hashTypeSetEx(c->db, hashObj, field, NULL, expire, &exCtx); + SetExRes res = hashTypeSetEx(hashObj, field, expire, &exCtx); addReplyLongLong(c,res); } hashTypeSetExDone(&exCtx); @@ -3224,5 +3159,9 @@ void hpersistCommand(client *c) { /* Generates a hpersist event if the expiry time associated with any field * has been successfully deleted. */ - if (changed) notifyKeyspaceEvent(NOTIFY_HASH,"hpersist",c->argv[1],c->db->id); + if (changed) { + notifyKeyspaceEvent(NOTIFY_HASH, "hpersist", c->argv[1], c->db->id); + signalModifiedKey(c, c->db, c->argv[1]); + server.dirty++; + } } diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index 9db78168996..f528097f4c6 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -429,8 +429,8 @@ start_server [list overrides [list "dir" $server_path]] { r HMSET key a 1 b 2 c 3 d 4 e 5 # expected to be expired long after restart r HEXPIREAT key 2524600800 FIELDS 1 a - # expected long TTL value (6 bytes) is saved and loaded correctly - r HPEXPIREAT key 188900976391764 FIELDS 1 b + # expected long TTL value (46 bits) is saved and loaded correctly + r HPEXPIREAT key 65755674080852 FIELDS 1 b # expected to be already expired after restart r HPEXPIRE key 80 FIELDS 1 d # expected to be expired soon after restart @@ -443,7 +443,7 @@ start_server [list overrides [list "dir" $server_path]] { wait_done_loading r assert_equal [lsort [r hgetall key]] "1 2 3 a b c" - assert_equal [r hpexpiretime key FIELDS 3 a b c] {2524600800000 188900976391764 -1} + assert_equal [r hpexpiretime key FIELDS 3 a b c] {2524600800000 65755674080852 -1} assert_equal [s rdb_last_load_keys_loaded] 1 # wait until expired_hash_fields equals 2 diff --git a/tests/unit/moduleapi/hash.tcl b/tests/unit/moduleapi/hash.tcl index 116b1c5120f..8cd919b3ab3 100644 --- a/tests/unit/moduleapi/hash.tcl +++ b/tests/unit/moduleapi/hash.tcl @@ -21,6 +21,45 @@ start_server {tags {"modules"}} { r hgetall k } {squirrel ofcourse banana no what nothing something nice} + test {Module hash - set (override) NX expired field successfully} { + r debug set-active-expire 0 + r del H1 H2 + r hash.set H1 "n" f1 v1 + r hpexpire H1 1 FIELDS 1 f1 + r hash.set H2 "n" f1 v1 f2 v2 + r hpexpire H2 1 FIELDS 1 f1 + after 5 + assert_equal 0 [r hash.set H1 "n" f1 xx] + assert_equal "f1 xx" [r hgetall H1] + assert_equal 0 [r hash.set H2 "n" f1 yy] + assert_equal "f1 f2 v2 yy" [lsort [r hgetall H2]] + r debug set-active-expire 1 + } {OK} {needs:debug} + + test {Module hash - set XX of expired field gets failed as expected} { + r debug set-active-expire 0 + r del H1 H2 + r hash.set H1 "n" f1 v1 + r hpexpire H1 1 FIELDS 1 f1 + r hash.set H2 "n" f1 v1 f2 v2 + r hpexpire H2 1 FIELDS 1 f1 + after 5 + + # expected to fail on condition XX. hgetall should return empty list + r hash.set H1 "x" f1 xx + assert_equal "" [lsort [r hgetall H1]] + # But expired field was not lazy deleted + assert_equal 1 [r hlen H1] + + # expected to fail on condition XX. hgetall should return list without expired f1 + r hash.set H2 "x" f1 yy + assert_equal "f2 v2" [lsort [r hgetall H2]] + # But expired field was not lazy deleted + assert_equal 2 [r hlen H2] + + r debug set-active-expire 1 + } {OK} {needs:debug} + test "Unload the module - hash" { assert_equal {OK} [r module unload hash] } diff --git a/tests/unit/moduleapi/scan.tcl b/tests/unit/moduleapi/scan.tcl index 7cf8e60afae..2f012726706 100644 --- a/tests/unit/moduleapi/scan.tcl +++ b/tests/unit/moduleapi/scan.tcl @@ -25,12 +25,16 @@ start_server {tags {"modules"}} { } {{f1 1}} test {Module scan hash listpack with hexpire} { - r hmset hh f1 v1 f2 v2 + r debug set-active-expire 0 + r hmset hh f1 v1 f2 v2 f3 v3 r hexpire hh 100000 fields 1 f1 + r hpexpire hh 1 fields 1 f3 + after 10 assert_range [r httl hh fields 1 f1] 10000 100000 assert_encoding listpackex hh + r debug set-active-expire 1 lsort [r scan.scan_key hh] - } {{f1 v1} {f2 v2}} + } {{f1 v1} {f2 v2}} {needs:debug} test {Module scan hash dict} { r config set hash-max-ziplist-entries 2 @@ -44,10 +48,22 @@ start_server {tags {"modules"}} { r del hh r hmset hh f1 v1 f2 v2 f3 v3 r hexpire hh 100000 fields 1 f2 + r hpexpire hh 5 fields 1 f3 assert_range [r httl hh fields 1 f2] 10000 100000 assert_encoding hashtable hh + after 10 lsort [r scan.scan_key hh] - } {{f1 v1} {f2 v2} {f3 v3}} + } {{f1 v1} {f2 v2}} + + test {Module scan hash with hexpire can return no items} { + r del hh + r debug set-active-expire 0 + r hmset hh f1 v1 f2 v2 f3 v3 + r hpexpire hh 1 fields 3 f1 f2 f3 + after 10 + r debug set-active-expire 1 + lsort [r scan.scan_key hh] + } {} {needs:debug} test {Module scan zset listpack} { r zadd zz 1 f1 2 f2 diff --git a/tests/unit/type/hash-field-expire.tcl b/tests/unit/type/hash-field-expire.tcl index 1c908486184..58ff4cfc95d 100644 --- a/tests/unit/type/hash-field-expire.tcl +++ b/tests/unit/type/hash-field-expire.tcl @@ -208,12 +208,12 @@ start_server {tags {"external:skip needs:debug"}} { assert_error {*Parameter `numFields` is more than number of arguments} {r hpexpire myhash 1000 NX FIELDS 4 f1 f2 f3} } - test "HPEXPIRE - parameter expire-time near limit of 2^48 ($type)" { + test "HPEXPIRE - parameter expire-time near limit of 2^46 ($type)" { r del myhash r hset myhash f1 v1 # below & above - assert_equal [r hpexpire myhash [expr (1<<48) - [clock milliseconds] - 1000 ] FIELDS 1 f1] [list $E_OK] - assert_error {*invalid expire time*} {r hpexpire myhash [expr (1<<48) - [clock milliseconds] + 100 ] FIELDS 1 f1} + assert_equal [r hpexpire myhash [expr (1<<46) - [clock milliseconds] - 1000 ] FIELDS 1 f1] [list $E_OK] + assert_error {*invalid expire time*} {r hpexpire myhash [expr (1<<46) - [clock milliseconds] + 100 ] FIELDS 1 f1} } test "Lazy Expire - fields are lazy deleted ($type)" { @@ -1103,8 +1103,11 @@ start_server {tags {"external:skip needs:debug"}} { r hexpireat h1 [expr [clock seconds]+100] NX FIELDS 1 f1 r hset h2 f2 v2 r hpexpireat h2 [expr [clock seconds]*1000+100000] NX FIELDS 1 f2 - r hset h3 f3 v3 f4 v4 + r hset h3 f3 v3 f4 v4 f5 v5 + # hpersist does nothing here. Verify it is not propagated. + r hpersist h3 FIELDS 1 f5 r hexpire h3 100 FIELDS 3 f3 f4 non_exists_field + r hpersist h3 FIELDS 1 f3 assert_replication_stream $repl { {select *} @@ -1112,8 +1115,9 @@ start_server {tags {"external:skip needs:debug"}} { {hpexpireat h1 * NX FIELDS 1 f1} {hset h2 f2 v2} {hpexpireat h2 * NX FIELDS 1 f2} - {hset h3 f3 v3 f4 v4} + {hset h3 f3 v3 f4 v4 f5 v5} {hpexpireat h3 * FIELDS 3 f3 f4 non_exists_field} + {hpersist h3 FIELDS 1 f3} } close_replication_stream $repl } {} {needs:repl} From f6cfe92c5d784a6144261fc7e74646da6732dc52 Mon Sep 17 00:00:00 2001 From: "debing.sun" Date: Tue, 11 Jun 2024 11:16:41 +0800 Subject: [PATCH 06/12] Send `hexpired` notification before `del` notification --- src/t_hash.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/t_hash.c b/src/t_hash.c index 3db2b6be9e0..6fd639486b0 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -1157,6 +1157,7 @@ void hashTypeSetExDone(HashTypeSetEx *ex) { if (ex->fieldDeleted && hashTypeLength(ex->hashObj, 0) == 0) { dbDelete(ex->db,ex->key); signalModifiedKey(ex->c, ex->db, ex->key); + notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", ex->key, ex->db->id); notifyKeyspaceEvent(NOTIFY_GENERIC,"del",ex->key, ex->db->id); } else { signalModifiedKey(ex->c, ex->db, ex->key); From 658803b670d39d944d0500699ad780b5fb12dee6 Mon Sep 17 00:00:00 2001 From: "debing.sun" Date: Tue, 11 Jun 2024 11:29:05 +0800 Subject: [PATCH 07/12] Fix memory leak --- src/t_hash.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/t_hash.c b/src/t_hash.c index 6fd639486b0..3b5fa83d1dc 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -754,7 +754,6 @@ GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vs robj *keyObj = createStringObject(key, sdslen(key)); notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", keyObj, db->id); if ((hashTypeLength(o, 0) == 0) && (!(hfeFlags & HFE_LAZY_AVOID_HASH_DEL))) { - robj *keyObj = createStringObject(key, sdslen(key)); notifyKeyspaceEvent(NOTIFY_GENERIC, "del", keyObj, db->id); dbDelete(db,keyObj); res = GETF_EXPIRED_HASH; From 85c95f10dc2f3b87a262fce25c9535cac5e8c576 Mon Sep 17 00:00:00 2001 From: "debing.sun" Date: Tue, 11 Jun 2024 16:10:47 +0800 Subject: [PATCH 08/12] Only send one `hexpired`notification for hmgetcommand Co-authored-by: Ozan Tezcan --- src/server.h | 2 ++ src/t_hash.c | 37 ++++++++++++++++++++++--------------- tests/unit/pubsub.tcl | 11 +++++++---- 3 files changed, 31 insertions(+), 19 deletions(-) diff --git a/src/server.h b/src/server.h index 59bad41ab96..cff87e10f07 100644 --- a/src/server.h +++ b/src/server.h @@ -3195,6 +3195,8 @@ typedef struct dictExpireMetadata { #define HFE_LAZY_EXPIRE (0) /* Delete expired field, and if last field also the hash */ #define HFE_LAZY_AVOID_FIELD_DEL (1<<0) /* Avoid deleting expired field */ #define HFE_LAZY_AVOID_HASH_DEL (1<<1) /* Avoid deleting hash if the field is the last one */ +#define HFE_LAZY_NO_NOTIFICATION (1<<2) /* Do not send notification, used when multiple fields + * may expire and only one notification is desired. */ void hashTypeConvert(robj *o, int enc, ebuckets *hexpires); void hashTypeTryConversion(redisDb *db, robj *subject, robj **argv, int start, int end); diff --git a/src/t_hash.c b/src/t_hash.c index 3b5fa83d1dc..c7b2b2f93a9 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -752,9 +752,11 @@ GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vs /* If the field is the last one in the hash, then the hash will be deleted */ res = GETF_EXPIRED; robj *keyObj = createStringObject(key, sdslen(key)); - notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", keyObj, db->id); + if (!(hfeFlags & HFE_LAZY_NO_NOTIFICATION)) + notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", keyObj, db->id); if ((hashTypeLength(o, 0) == 0) && (!(hfeFlags & HFE_LAZY_AVOID_HASH_DEL))) { - notifyKeyspaceEvent(NOTIFY_GENERIC, "del", keyObj, db->id); + if (!(hfeFlags & HFE_LAZY_NO_NOTIFICATION)) + notifyKeyspaceEvent(NOTIFY_GENERIC, "del", keyObj, db->id); dbDelete(db,keyObj); res = GETF_EXPIRED_HASH; } @@ -2171,7 +2173,7 @@ void hincrbyfloatCommand(client *c) { decrRefCount(newobj); } -static GetFieldRes addHashFieldToReply(client *c, robj *o, sds field) { +static GetFieldRes addHashFieldToReply(client *c, robj *o, sds field, int hfeFlags) { if (o == NULL) { addReplyNull(c); return GETF_NOT_FOUND; @@ -2181,8 +2183,7 @@ static GetFieldRes addHashFieldToReply(client *c, robj *o, sds field) { unsigned int vlen = UINT_MAX; long long vll = LLONG_MAX; - GetFieldRes res = hashTypeGetValue(c->db, o, field, &vstr, &vlen, &vll, - HFE_LAZY_EXPIRE); + GetFieldRes res = hashTypeGetValue(c->db, o, field, &vstr, &vlen, &vll, hfeFlags); if (res == GETF_OK) { if (vstr) { addReplyBulkCBuffer(c, vstr, vlen); @@ -2201,13 +2202,14 @@ void hgetCommand(client *c) { if ((o = lookupKeyReadOrReply(c,c->argv[1],shared.null[c->resp])) == NULL || checkType(c,o,OBJ_HASH)) return; - addHashFieldToReply(c, o, c->argv[2]->ptr); + addHashFieldToReply(c, o, c->argv[2]->ptr, HFE_LAZY_EXPIRE); } void hmgetCommand(client *c) { GetFieldRes res = GETF_OK; robj *o; int i; + int expired = 0, deleted = 0;; /* Don't abort when the key cannot be found. Non-existing keys are empty * hashes, where HMGET should respond with a series of null bulks. */ @@ -2216,17 +2218,22 @@ void hmgetCommand(client *c) { addReplyArrayLen(c, c->argc-2); for (i = 2; i < c->argc ; i++) { - - res = addHashFieldToReply(c, o, c->argv[i]->ptr); - - /* If hash got lazy expired since all fields are expired (o is invalid), - * then fill the rest with trivial nulls and return */ - if (res == GETF_EXPIRED_HASH) { - while (++i < c->argc) - addReplyNull(c); - return; + if (!deleted) { + res = addHashFieldToReply(c, o, c->argv[i]->ptr, HFE_LAZY_NO_NOTIFICATION); + expired += (res == GETF_EXPIRED); + deleted += (res == GETF_EXPIRED_HASH); + } else { + /* If hash got lazy expired since all fields are expired (o is invalid), + * then fill the rest with trivial nulls and return */ + addReplyNull(c); } } + + if (expired) { + notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", c->argv[1], c->db->id); + if (deleted) + notifyKeyspaceEvent(NOTIFY_GENERIC, "del", c->argv[1], c->db->id); + } } void hdelCommand(client *c) { diff --git a/tests/unit/pubsub.tcl b/tests/unit/pubsub.tcl index 9f9f81840c3..df62c0a2d75 100644 --- a/tests/unit/pubsub.tcl +++ b/tests/unit/pubsub.tcl @@ -356,7 +356,7 @@ start_server {tags {"pubsub network"}} { foreach {type max_lp_entries} {listpackex 512 hashtable 0} { test "Keyspace notifications: hash events test ($type)" { r config set hash-max-listpack-entries $max_lp_entries - r config set notify-keyspace-events Kh + r config set notify-keyspace-events Khg r del myhash set rd1 [redis_deferring_client] assert_equal {1} [psubscribe $rd1 *] @@ -378,7 +378,7 @@ start_server {tags {"pubsub network"}} { # Test that we will get `hexpired` notification when # a hash field is removed by active expire. - r hpexpire myhash 10 FIELDS 1 f1 + r hpexpire myhash 10 FIELDS 1 no assert_equal "pmessage * __keyspace@${db}__:myhash hexpire" [$rd1 read] after 100 ;# Wait for active expire assert_equal "pmessage * __keyspace@${db}__:myhash hexpired" [$rd1 read] @@ -386,11 +386,14 @@ start_server {tags {"pubsub network"}} { # Test that we will get `hexpired` notification when # a hash field is removed by lazy active. r debug set-active-expire 0 - r hpexpire myhash 10 FIELDS 1 f2 + r hpexpire myhash 10 FIELDS 2 f1 f2 assert_equal "pmessage * __keyspace@${db}__:myhash hexpire" [$rd1 read] after 20 - r hstrlen myhash f2 ;# Trigger lazy expire + r hmget myhash f1 f2 ;# Trigger lazy expire + # We should get only one `hexpired` notification even two fields was expired. assert_equal "pmessage * __keyspace@${db}__:myhash hexpired" [$rd1 read] + # We should get a `del` notificaion after all fields were expired. + assert_equal "pmessage * __keyspace@${db}__:myhash del" [$rd1 read] r debug set-active-expire 1 $rd1 close From b8900ca7aa8980feab79c7c318d4cb5638a249d3 Mon Sep 17 00:00:00 2001 From: "debing.sun" Date: Tue, 11 Jun 2024 16:12:07 +0800 Subject: [PATCH 09/12] Fix spell --- tests/unit/pubsub.tcl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/pubsub.tcl b/tests/unit/pubsub.tcl index df62c0a2d75..b7b6565eff9 100644 --- a/tests/unit/pubsub.tcl +++ b/tests/unit/pubsub.tcl @@ -392,7 +392,7 @@ start_server {tags {"pubsub network"}} { r hmget myhash f1 f2 ;# Trigger lazy expire # We should get only one `hexpired` notification even two fields was expired. assert_equal "pmessage * __keyspace@${db}__:myhash hexpired" [$rd1 read] - # We should get a `del` notificaion after all fields were expired. + # We should get a `del` notification after all fields were expired. assert_equal "pmessage * __keyspace@${db}__:myhash del" [$rd1 read] r debug set-active-expire 1 From a9433109a83c3817db49487b56136f0b5fb61fe1 Mon Sep 17 00:00:00 2001 From: "debing.sun" Date: Tue, 11 Jun 2024 18:13:17 +0800 Subject: [PATCH 10/12] Send notifications when field was expired by active-expire Co-authored-by: Ozan Tezcan --- src/t_hash.c | 27 +++++++++++++++------------ tests/unit/pubsub.tcl | 14 +++++++++++--- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/t_hash.c b/src/t_hash.c index c7b2b2f93a9..f550be6556f 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -1824,27 +1824,30 @@ static ExpireAction hashTypeActiveExpire(eItem _hashObj, void *ctx) { /* Update quota left */ activeExpireCtx->fieldsToExpireQuota -= info.itemsExpired; - /* If hash has no more fields to expire, remove it from HFE DB */ - ExpireAction ret; - robj *key = createStringObject(keystr, sdslen(keystr)); - notifyKeyspaceEvent(NOTIFY_HASH,"hexpired",key,activeExpireCtx->db->id); - if (info.nextExpireTime == EB_EXPIRE_TIME_INVALID) { + /* In some cases, a field might have been deleted without updating the global DS. + * As a result, active-expire might not expire any fields, in such cases, + * we don't need to send notifications or perform other operations for this key. */ + if (info.itemsExpired) { + robj *key = createStringObject(keystr, sdslen(keystr)); + notifyKeyspaceEvent(NOTIFY_HASH,"hexpired",key,activeExpireCtx->db->id); if (hashTypeLength(hashObj, 0) == 0) { dbDelete(activeExpireCtx->db, key); notifyKeyspaceEvent(NOTIFY_GENERIC,"del",key,activeExpireCtx->db->id); } - ret = ACT_REMOVE_EXP_ITEM; + server.dirty++; + signalModifiedKey(NULL, activeExpireCtx->db, key); + decrRefCount(key); + } + + /* If hash has no more fields to expire, remove it from HFE DB */ + if (info.nextExpireTime == EB_EXPIRE_TIME_INVALID) { + return ACT_REMOVE_EXP_ITEM; } else { /* Hash has more fields to expire. Update next expiration time of the hash * and indicate to add it back to global HFE DS */ ebSetMetaExpTime(hashGetExpireMeta(hashObj), info.nextExpireTime); - ret = ACT_UPDATE_EXP_ITEM; + return ACT_UPDATE_EXP_ITEM; } - - server.dirty++; - signalModifiedKey(NULL, activeExpireCtx->db, key); - decrRefCount(key); - return ret; } /* Return the next/minimum expiry time of the hash-field. This is useful if a diff --git a/tests/unit/pubsub.tcl b/tests/unit/pubsub.tcl index b7b6565eff9..ffe22825168 100644 --- a/tests/unit/pubsub.tcl +++ b/tests/unit/pubsub.tcl @@ -360,7 +360,7 @@ start_server {tags {"pubsub network"}} { r del myhash set rd1 [redis_deferring_client] assert_equal {1} [psubscribe $rd1 *] - r hmset myhash yes 1 no 0 f1 1 f2 2 + r hmset myhash yes 1 no 0 f1 1 f2 2 f3_hdel 1 r hincrby myhash yes 10 r hexpire myhash 999999 FIELDS 1 yes r hexpireat myhash [expr {[clock seconds] + 999999}] NX FIELDS 1 no @@ -379,17 +379,25 @@ start_server {tags {"pubsub network"}} { # Test that we will get `hexpired` notification when # a hash field is removed by active expire. r hpexpire myhash 10 FIELDS 1 no - assert_equal "pmessage * __keyspace@${db}__:myhash hexpire" [$rd1 read] after 100 ;# Wait for active expire + assert_equal "pmessage * __keyspace@${db}__:myhash hexpire" [$rd1 read] assert_equal "pmessage * __keyspace@${db}__:myhash hexpired" [$rd1 read] + # Test that when a field with TTL is deleted by commands like hdel without + # updating the global DS, active expire will not send a notification. + r hpexpire myhash 100 FIELDS 1 f3_hdel + r hdel myhash f3_hdel + after 200 ;# Wait for active expire + assert_equal "pmessage * __keyspace@${db}__:myhash hexpire" [$rd1 read] + assert_equal "pmessage * __keyspace@${db}__:myhash hdel" [$rd1 read] + # Test that we will get `hexpired` notification when # a hash field is removed by lazy active. r debug set-active-expire 0 r hpexpire myhash 10 FIELDS 2 f1 f2 - assert_equal "pmessage * __keyspace@${db}__:myhash hexpire" [$rd1 read] after 20 r hmget myhash f1 f2 ;# Trigger lazy expire + assert_equal "pmessage * __keyspace@${db}__:myhash hexpire" [$rd1 read] # We should get only one `hexpired` notification even two fields was expired. assert_equal "pmessage * __keyspace@${db}__:myhash hexpired" [$rd1 read] # We should get a `del` notification after all fields were expired. From 6ff6384e7d686f13ecac84b7e2f06e2f2aa47b5c Mon Sep 17 00:00:00 2001 From: "debing.sun" Date: Tue, 11 Jun 2024 20:12:40 +0800 Subject: [PATCH 11/12] Cleanup --- src/t_hash.c | 2 +- tests/unit/pubsub.tcl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/t_hash.c b/src/t_hash.c index f550be6556f..bd157e360d9 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -2227,7 +2227,7 @@ void hmgetCommand(client *c) { deleted += (res == GETF_EXPIRED_HASH); } else { /* If hash got lazy expired since all fields are expired (o is invalid), - * then fill the rest with trivial nulls and return */ + * then fill the rest with trivial nulls and return. */ addReplyNull(c); } } diff --git a/tests/unit/pubsub.tcl b/tests/unit/pubsub.tcl index ffe22825168..e546f653424 100644 --- a/tests/unit/pubsub.tcl +++ b/tests/unit/pubsub.tcl @@ -360,7 +360,7 @@ start_server {tags {"pubsub network"}} { r del myhash set rd1 [redis_deferring_client] assert_equal {1} [psubscribe $rd1 *] - r hmset myhash yes 1 no 0 f1 1 f2 2 f3_hdel 1 + r hmset myhash yes 1 no 0 f1 1 f2 2 f3_hdel 3 r hincrby myhash yes 10 r hexpire myhash 999999 FIELDS 1 yes r hexpireat myhash [expr {[clock seconds] + 999999}] NX FIELDS 1 no From dc5719617010248b729619a9c8cbe7e0ece2a2a2 Mon Sep 17 00:00:00 2001 From: "debing.sun" Date: Tue, 11 Jun 2024 21:25:49 +0800 Subject: [PATCH 12/12] Cleanup --- src/t_hash.c | 2 +- tests/unit/pubsub.tcl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/t_hash.c b/src/t_hash.c index bd157e360d9..1c5481bdbe3 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -2212,7 +2212,7 @@ void hmgetCommand(client *c) { GetFieldRes res = GETF_OK; robj *o; int i; - int expired = 0, deleted = 0;; + int expired = 0, deleted = 0; /* Don't abort when the key cannot be found. Non-existing keys are empty * hashes, where HMGET should respond with a series of null bulks. */ diff --git a/tests/unit/pubsub.tcl b/tests/unit/pubsub.tcl index e546f653424..5ac3e8252d0 100644 --- a/tests/unit/pubsub.tcl +++ b/tests/unit/pubsub.tcl @@ -392,7 +392,7 @@ start_server {tags {"pubsub network"}} { assert_equal "pmessage * __keyspace@${db}__:myhash hdel" [$rd1 read] # Test that we will get `hexpired` notification when - # a hash field is removed by lazy active. + # a hash field is removed by lazy expire. r debug set-active-expire 0 r hpexpire myhash 10 FIELDS 2 f1 f2 after 20