From 770beaa7711004e102c2fd30901fa19105aaad47 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Tue, 16 Apr 2024 17:55:56 +0300 Subject: [PATCH 01/29] Add RDB de/serialization to metadata hashes (WIP) serialize metadata hashes to RDB using a new type, saving per each field the key, one byte indicating what else is saved and than (optionally) the value and the TTL (if available). --- src/rdb.c | 202 +++++++++++++++++++++++++++++++++++--- src/rdb.h | 7 +- src/server.c | 2 + src/server.h | 2 + src/t_hash.c | 2 + tests/integration/rdb.tcl | 61 ++++++++++++ tests/support/util.tcl | 10 +- 7 files changed, 272 insertions(+), 14 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index a6bf7197f291..3b8d1ef615a3 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -504,6 +504,7 @@ ssize_t rdbSaveStringObject(rio *rdb, robj *obj) { * RDB_LOAD_PLAIN: Return a plain string allocated with zmalloc() * instead of a Redis object with an sds in it. * RDB_LOAD_SDS: Return an SDS string instead of a Redis object. + * RDB_LOAD_HFLD: Return a hash field object (mstr) * * On I/O error NULL is returned. */ @@ -694,11 +695,12 @@ int rdbSaveObjectType(rio *rdb, robj *o) { case OBJ_HASH: if (o->encoding == OBJ_ENCODING_LISTPACK) return rdbSaveType(rdb,RDB_TYPE_HASH_LISTPACK); - else if (o->encoding == OBJ_ENCODING_LISTPACK_EX) - return -1; - else if (o->encoding == OBJ_ENCODING_HT) - return rdbSaveType(rdb,RDB_TYPE_HASH); - else + else if (o->encoding == OBJ_ENCODING_HT) { + if (hashTypeGetMinExpire(o) == EB_EXPIRE_TIME_INVALID) + return rdbSaveType(rdb,RDB_TYPE_HASH); + else + return rdbSaveType(rdb,RDB_TYPE_HASH_METADATA); + } else serverPanic("Unknown hash encoding"); case OBJ_STREAM: return rdbSaveType(rdb,RDB_TYPE_STREAM_LISTPACKS_3); @@ -947,17 +949,29 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { } else if (o->encoding == OBJ_ENCODING_HT) { dictIterator *di = dictGetIterator(o->ptr); dictEntry *de; + int withTtl = 0; /* whether there is at least one field with a valid TTL */ + unsigned char hashEntryType; + uint64_t ttl = 0; + /* save number of fileds in hash */ if ((n = rdbSaveLen(rdb,dictSize((dict*)o->ptr))) == -1) { dictReleaseIterator(di); return -1; } nwritten += n; + /* use standard hash format if no TTL was set for any field */ + if (hashTypeGetMinExpire(o) != EB_EXPIRE_TIME_INVALID) + withTtl = 1; + + redisDebug("saving hash table as dict, withTtl %d", withTtl); + + /* save all hash fields */ while((de = dictNext(di)) != NULL) { hfield field = dictGetKey(de); sds value = dictGetVal(de); + /* save the key */ if ((n = rdbSaveRawString(rdb,(unsigned char*)field, hfieldlen(field))) == -1) { @@ -965,13 +979,54 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { return -1; } nwritten += n; - if ((n = rdbSaveRawString(rdb,(unsigned char*)value, - sdslen(value))) == -1) - { - dictReleaseIterator(di); - return -1; + + redisDebug("saved hash key %s", (char *)field); + + /* if this is a hash table with a TTL for at least one field, + * save it in the "with metadata" format - one byte for included + * metadata types, and than the types themselves (note that the + * order is fixed, i.e. future extensions may only add new + * metadata types at the end) */ + if (withTtl) { + hashEntryType = 0; + if (value != NULL) hashEntryType |= RDB_HASH_ENTRY_VALUE; + if (hfieldIsExpireAttached(field)) { + ttl = hfieldGetExpireTime(field); + hashEntryType |= RDB_HASH_ENTRY_TTL; + } else + ttl = 0; + if ((n = rdbSaveType(rdb, hashEntryType)) == -1) { + dictReleaseIterator(di); + return -1; + } + nwritten += n; + + redisDebug("saved hash type 0x%x", hashEntryType); + } + + /* save the value */ + if (value != NULL) { + if ((n = rdbSaveRawString(rdb,(unsigned char*)value, + sdslen(value))) == -1) + { + dictReleaseIterator(di); + return -1; + } + nwritten += n; + + redisDebug("saved hash value %s", (char *)value); + } + + /* save the TTL */ + if (withTtl && (ttl != 0)) { + if ((n = rdbSaveLen(rdb, ttl)) == -1) { + dictReleaseIterator(di); + return -1; + } + nwritten += n; + + redisDebug("saved hash ttl %lu", ttl); } - nwritten += n; } dictReleaseIterator(di); } else { @@ -2185,6 +2240,131 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { /* All pairs should be read by now */ serverAssert(len == 0); + } else if (rdbtype == RDB_TYPE_HASH_METADATA) { + + hfield field; + char *hkey; + size_t keyLen; + int hashEntryType; + sds value; + uint64_t ttl; + int ret; + mstime_t now = mstime(); + + len = rdbLoadLen(rdb, NULL); + if (len == RDB_LENERR) return NULL; + if (len == 0) goto emptykey; + + redisDebug("loading metada-enriched hash with %lu keys", len); + + o = createHashObject(); + /* TODO: need to add listpack-first,-than-convert-to-hash logic like + for simple (noe metadata) hash when listpack encoding eith TTL will + be ready */ + hashTypeConvert(NULL, o, OBJ_ENCODING_HT); + dict *d = o->ptr; + + while (len > 0) { + len--; + + /* read the key */ + if ((hkey = rdbGenericLoadStringObject(rdb, RDB_LOAD_PLAIN, &keyLen)) == NULL) { + decrRefCount(o); + return NULL; + } + + redisDebug("read key %s", hkey); + + /* read the type */ + if ((hashEntryType = rdbLoadType(rdb)) == -1) { + decrRefCount(o); + zfree(hkey); + return NULL; + } + + redisDebug("read hash type 0x%x", hashEntryType); + + /* if the type indicated value exists, read it */ + if ((unsigned char)hashEntryType & RDB_HASH_ENTRY_VALUE) { + if ((value = rdbGenericLoadStringObject(rdb,RDB_LOAD_SDS,NULL)) == NULL) { + decrRefCount(o); + zfree(hkey); + return NULL; + } + + redisDebug("read hash value %s", (char *)value); + } + + /* if the type indicated TTL exists, read it */ + if ((unsigned char)hashEntryType & RDB_HASH_ENTRY_TTL) { + if (rdbLoadLenByRef(rdb, NULL, &ttl) == -1) { + decrRefCount(o); + sds_free(value); + zfree(hkey); + return NULL; + } + + redisDebug("read hash TTL %lu", ttl); + + } else + ttl = 0; + + /* Check if the hash field already expired. This function is used when + * loading an RDB file from disk, either at startup, or when an RDB was + * received from the master. In the latter case, the master is + * responsible for hash field expiry. If we would expire hash fiedls here, + * the snapshot taken by the master may not be reflected on the slave. + * Similarly, if the base AOF is RDB format, we want to load all + * the hash fields there are, since the log of operations in the incr AOF + * is assumed to work in the exact keyspace state. + * Expired hash fields on the master are silenlty discarded. + * Note that if all fileds in a hash has expired, the hash would not be + * created in memory (because it is created on the first valid field), and + * thus the key would be discarded as an "empty key" */ + if (ttl != 0 && iAmMaster() && ((mstime_t)ttl < now)) { /* note: TTL was saved to RDB as unix-time in milliseconds */ + /* TODO: consider replication (like in rdbLoadAddKeyToDb) */ + server.rdb_last_load_hash_fields_expired++; + zfree(hkey); + sdsfree(value); + redisDebug("key is expired, discarding %d", 1); + continue; + } + + /* compose the key and TTL (if exists) to compose a hash field */ + if ((field = hfieldNew(hkey, keyLen, ttl != 0)) == NULL) { + decrRefCount(o); + sds_free(value); + zfree(hkey); + return NULL; + } else + zfree(hkey); /* was copied into the new mstr hash field */ + + /* TODO: add the TTL to hfield, possibly after inserting the hfiled to the table*/ + + /* Add pair to hash table */ + dictUseStoredKeyApi(d, 1); + ret = dictAdd(d, field, value); + dictUseStoredKeyApi(d, 0); + if (ret == DICT_ERR) { + rdbReportCorruptRDB("Duplicate hash fields detected"); + decrRefCount(o); + sdsfree(value); + hfieldFree(field); + return NULL; + } + } + + /* All pairs should be read by now */ + serverAssert(len == 0); + + /* check for empty key (if all keys were expired) */ + if (dictIsEmpty(d)) { + decrRefCount(o); + + redisDebug("empty hash, returning empty key (all keys excpired?) %d", 1); + goto emptykey; + } + } else if (rdbtype == RDB_TYPE_LIST_QUICKLIST || rdbtype == RDB_TYPE_LIST_QUICKLIST_2) { if ((len = rdbLoadLen(rdb,NULL)) == RDB_LENERR) return NULL; if (len == 0) goto emptykey; diff --git a/src/rdb.h b/src/rdb.h index 02bb5e3478c2..2c4fb2a00d9d 100644 --- a/src/rdb.h +++ b/src/rdb.h @@ -73,10 +73,11 @@ #define RDB_TYPE_STREAM_LISTPACKS_2 19 #define RDB_TYPE_SET_LISTPACK 20 #define RDB_TYPE_STREAM_LISTPACKS_3 21 +#define RDB_TYPE_HASH_METADATA 22 /* NOTE: WHEN ADDING NEW RDB TYPE, UPDATE rdbIsObjectType(), and rdb_type_string[] */ /* Test if a type is an object type. */ -#define rdbIsObjectType(t) (((t) >= 0 && (t) <= 7) || ((t) >= 9 && (t) <= 21)) +#define rdbIsObjectType(t) (((t) >= 0 && (t) <= 7) || ((t) >= 9 && (t) <= 22)) /* Special RDB opcodes (saved/loaded with rdbSaveType/rdbLoadType). */ #define RDB_OPCODE_SLOT_INFO 244 /* Individual slot info, such as slot id and size (cluster mode only). */ @@ -120,6 +121,10 @@ #define RDB_LOAD_ERR_EMPTY_KEY 1 /* Error of empty key */ #define RDB_LOAD_ERR_OTHER 2 /* Any other errors */ +/* Flags for hash entry content when reading/writing a RDB_TYPE_HASH_METADATA */ +#define RDB_HASH_ENTRY_VALUE (1<<0) /* this hash entry contains a value */ +#define RDB_HASH_ENTRY_TTL (1<<1) /* this hash entry contains a TTL */ + ssize_t rdbWriteRaw(rio *rdb, void *p, size_t len); int rdbSaveType(rio *rdb, unsigned char type); int rdbLoadType(rio *rdb); diff --git a/src/server.c b/src/server.c index e0366fcea73f..382beddedfc5 100644 --- a/src/server.c +++ b/src/server.c @@ -2707,6 +2707,7 @@ void initServer(void) { server.rdb_save_time_start = -1; server.rdb_last_load_keys_expired = 0; server.rdb_last_load_keys_loaded = 0; + server.rdb_last_load_hash_fields_expired = 0; server.dirty = 0; resetServerStats(); /* A few stats we don't want to reset: server startup time, and peak mem. */ @@ -5770,6 +5771,7 @@ sds genRedisInfoString(dict *section_dict, int all_sections, int everything) { "rdb_last_cow_size:%zu\r\n", server.stat_rdb_cow_bytes, "rdb_last_load_keys_expired:%lld\r\n", server.rdb_last_load_keys_expired, "rdb_last_load_keys_loaded:%lld\r\n", server.rdb_last_load_keys_loaded, + "rdb_last_load_hash_fields_expired:%lld\r\n", server.rdb_last_load_hash_fields_expired, "aof_enabled:%d\r\n", server.aof_state != AOF_OFF, "aof_rewrite_in_progress:%d\r\n", server.child_type == CHILD_TYPE_AOF, "aof_rewrite_scheduled:%d\r\n", server.aof_rewrite_scheduled, diff --git a/src/server.h b/src/server.h index d21345f47fa7..482b7cab1c36 100644 --- a/src/server.h +++ b/src/server.h @@ -1802,6 +1802,7 @@ struct redisServer { long long dirty_before_bgsave; /* Used to restore dirty on failed BGSAVE */ long long rdb_last_load_keys_expired; /* number of expired keys when loading RDB */ long long rdb_last_load_keys_loaded; /* number of loaded keys when loading RDB */ + long long rdb_last_load_hash_fields_expired; /* number of expired hash fields when loading RDB */ struct saveparam *saveparams; /* Save points array for RDB */ int saveparamslen; /* Number of saving points */ char *rdb_filename; /* Name of RDB file */ @@ -3210,6 +3211,7 @@ uint64_t hfieldGetExpireTime(hfield field); static inline void hfieldFree(hfield field) { mstrFree(&mstrFieldKind, field); } static inline void *hfieldGetAllocPtr(hfield field) { return mstrGetAllocPtr(&mstrFieldKind, field); } static inline size_t hfieldlen(hfield field) { return mstrlen(field);} +uint64_t hfieldGetExpireTime(hfield field); /* Pub / Sub */ int pubsubUnsubscribeAllChannels(client *c, int notify); diff --git a/src/t_hash.c b/src/t_hash.c index 36240a646917..dda20c4e3e8d 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -23,6 +23,7 @@ static ExpireAction hashTypeActiveExpire(eItem hashObj, void *ctx); static void hfieldPersist(robj *hashObj, hfield field); static void updateGlobalHfeDs(redisDb *db, robj *o, uint64_t minExpire, uint64_t minExpireFields); static uint64_t hashTypeGetNextTimeToExpire(robj *o); +static uint64_t hashTypeGetMinExpire(robj *keyObj); /* hash dictType funcs */ static int dictHfieldKeyCompare(dict *d, const void *key1, const void *key2); @@ -2650,6 +2651,7 @@ static ExpireMeta* hfieldGetExpireMeta(const eItem field) { return mstrMetaRef(field, &mstrFieldKind, (int) HFIELD_META_EXPIRE); } +/* returned value is unix time in milliseconds */ uint64_t hfieldGetExpireTime(hfield field) { if (!hfieldIsExpireAttached(field)) return EB_EXPIRE_TIME_INVALID; diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index cce21671f89c..523795f92a1d 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -416,4 +416,65 @@ start_server {} { } {OK} } +# verifies writing and reading hash key with expiring and persistent fields +test {hash field expiration save and load rdb} { + start_server{overrides {hash-max-listpack-entries 0}} { + r FLUSHALL + + r HMSET key a 1 b 2 c 3 d 4 + r HEXPIREAT key 2524600800 2 a b + r HPEXPIRE key 100 d + + # sleep 100 ms to make sure d will expire after restart + after 100 + + r save + restart_server 0 true false + wait_done_loading r + + assert_equal [r hget key a] 1 + assert_equal [r hget key b] 2 + assert_equal [r hget key c] 3 + assert_equal [r hget key d] nil + assert_equal [r hexpiretime key 3 a b c] {2524600800, 2524600800, -1} + } +} + +# verifies writing hash with several expired keys, and active-expiring it on load +test {hash field expiration save and load rdb} { + start_server{overrides {hash-max-listpack-entries 0}} { + r FLUSHALL + + r HMSET key a 1 b 2 c 3 d 4 + r HPEXPIRE key 100 4 a b c d + + # sleep 100 ms to make sure all fields will expire after restart + after 100 + + r save + restart_server 0 true false + wait_done_loading r + + assert_equal [r hgetall key] {} + } +} + +# verifies a long TTL value (6 bytes) is saved and loaded correctly +test {hash field expiration save and load rdb} { + start_server{overrides {hash-max-listpack-entries 0}} { + r FLUSHALL + + r HSET key a 1 + # set expiry to 0xabcdef987654 (6 bytes) + r HPEXPIREAT key 188900976391764 1 a + + r save + restart_server 0 true false + wait_done_loading r + + assert_equal [r hget key a ] 1 + assert_equal [r hexpiretime key 1 a] {188900976391764} + } +} + } ;# tags diff --git a/tests/support/util.tcl b/tests/support/util.tcl index 0270e92222af..0598bdc24ffb 100644 --- a/tests/support/util.tcl +++ b/tests/support/util.tcl @@ -438,8 +438,14 @@ proc csvdump r { hash { set fields [{*}$r hgetall $k] set newfields {} - foreach {k v} $fields { - lappend newfields [list $k $v] + foreach {f v} $fields { + set expirylist [{*}$r hexpiretime $k 1 $f] + if {$expirylist eq (-1)} { + lappend newfields [list $f $v] + } else { + set e [lindex $expirylist 0] + lappend newfields [list $f $e $v] # TODO: extract the actual ttl value from the list in $e + } } set fields [lsort -index 0 $newfields] foreach kv $fields { From 3cc8f9d0ccaa4e8c9ccc2b08d8e891675eb33c54 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Tue, 30 Apr 2024 14:45:07 +0300 Subject: [PATCH 02/29] Add RDB de/serialization to metadata hashes in listpack encoding (WIP) --- src/cluster.c | 2 +- src/ebuckets.c | 2 +- src/listpack.c | 84 ++++++----- src/listpack.h | 1 + src/rdb.c | 283 +++++++++++++++++++++++++++++--------- src/rdb.h | 5 +- src/redis-check-rdb.c | 2 +- src/server.h | 5 + src/t_hash.c | 26 +++- tests/integration/rdb.tcl | 152 +++++++++++++++----- 10 files changed, 420 insertions(+), 142 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index d8c05d383fa2..6015a99d0930 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -237,7 +237,7 @@ void restoreCommand(client *c) { rioInitWithBuffer(&payload,c->argv[3]->ptr); if (((type = rdbLoadObjectType(&payload)) == -1) || - ((obj = rdbLoadObject(type,&payload,key->ptr,c->db->id,NULL)) == NULL)) + ((obj = rdbLoadObject(type,&payload,key->ptr,c->db,c->db->id,NULL)) == NULL)) { addReplyError(c,"Bad data format"); return; diff --git a/src/ebuckets.c b/src/ebuckets.c index b5dacb3dd916..f9fb1d67b45a 100644 --- a/src/ebuckets.c +++ b/src/ebuckets.c @@ -1403,7 +1403,7 @@ int ebRemove(ebuckets *eb, EbucketsType *type, eItem item) { * @param item - The eItem to be added to the ebucket. * @param expireTime - The expiration time of the item. * - * @return 1 if the item was successfully added; Otherwise, return 0 on failure. + * @return 0 if the item was successfully added; Otherwise, return 1 on failure. */ int ebAdd(ebuckets *eb, EbucketsType *type, eItem item, uint64_t expireTime) { int res; diff --git a/src/listpack.c b/src/listpack.c index c147b1699525..5acc16a326d8 100644 --- a/src/listpack.c +++ b/src/listpack.c @@ -219,7 +219,6 @@ int lpStringToInt64(const char *s, unsigned long slen, int64_t *value) { * */ unsigned char *lpNew(size_t capacity) { unsigned char *lp = lp_malloc(capacity > LP_HDR_SIZE+1 ? capacity : LP_HDR_SIZE+1); - if (lp == NULL) return NULL; lpSetTotalBytes(lp,LP_HDR_SIZE+1); lpSetNumElements(lp,0); lp[LP_HDR_SIZE] = LP_EOF; @@ -245,51 +244,61 @@ unsigned char* lpShrinkToFit(unsigned char *lp) { static inline void lpEncodeIntegerGetType(int64_t v, unsigned char *intenc, uint64_t *enclen) { if (v >= 0 && v <= 127) { /* Single byte 0-127 integer. */ - intenc[0] = v; - *enclen = 1; + if (intenc != NULL) intenc[0] = v; + if (enclen != NULL) *enclen = 1; } else if (v >= -4096 && v <= 4095) { /* 13 bit integer. */ if (v < 0) v = ((int64_t)1<<13)+v; - intenc[0] = (v>>8)|LP_ENCODING_13BIT_INT; - intenc[1] = v&0xff; - *enclen = 2; + if (intenc != NULL) { + intenc[0] = (v>>8)|LP_ENCODING_13BIT_INT; + intenc[1] = v&0xff; + } + if (enclen != NULL) *enclen = 2; } else if (v >= -32768 && v <= 32767) { /* 16 bit integer. */ if (v < 0) v = ((int64_t)1<<16)+v; - intenc[0] = LP_ENCODING_16BIT_INT; - intenc[1] = v&0xff; - intenc[2] = v>>8; - *enclen = 3; + if (intenc != NULL) { + intenc[0] = LP_ENCODING_16BIT_INT; + intenc[1] = v&0xff; + intenc[2] = v>>8; + } + if (enclen != NULL) *enclen = 3; } else if (v >= -8388608 && v <= 8388607) { /* 24 bit integer. */ if (v < 0) v = ((int64_t)1<<24)+v; - intenc[0] = LP_ENCODING_24BIT_INT; - intenc[1] = v&0xff; - intenc[2] = (v>>8)&0xff; - intenc[3] = v>>16; - *enclen = 4; + if (intenc != NULL) { + intenc[0] = LP_ENCODING_24BIT_INT; + intenc[1] = v&0xff; + intenc[2] = (v>>8)&0xff; + intenc[3] = v>>16; + } + if (enclen != NULL) *enclen = 4; } else if (v >= -2147483648 && v <= 2147483647) { /* 32 bit integer. */ if (v < 0) v = ((int64_t)1<<32)+v; - intenc[0] = LP_ENCODING_32BIT_INT; - intenc[1] = v&0xff; - intenc[2] = (v>>8)&0xff; - intenc[3] = (v>>16)&0xff; - intenc[4] = v>>24; - *enclen = 5; + if (intenc != NULL) { + intenc[0] = LP_ENCODING_32BIT_INT; + intenc[1] = v&0xff; + intenc[2] = (v>>8)&0xff; + intenc[3] = (v>>16)&0xff; + intenc[4] = v>>24; + } + if (enclen != NULL) *enclen = 5; } else { /* 64 bit integer. */ uint64_t uv = v; - intenc[0] = LP_ENCODING_64BIT_INT; - intenc[1] = uv&0xff; - intenc[2] = (uv>>8)&0xff; - intenc[3] = (uv>>16)&0xff; - intenc[4] = (uv>>24)&0xff; - intenc[5] = (uv>>32)&0xff; - intenc[6] = (uv>>40)&0xff; - intenc[7] = (uv>>48)&0xff; - intenc[8] = uv>>56; - *enclen = 9; + if (intenc != NULL) { + intenc[0] = LP_ENCODING_64BIT_INT; + intenc[1] = uv&0xff; + intenc[2] = (uv>>8)&0xff; + intenc[3] = (uv>>16)&0xff; + intenc[4] = (uv>>24)&0xff; + intenc[5] = (uv>>32)&0xff; + intenc[6] = (uv>>40)&0xff; + intenc[7] = (uv>>48)&0xff; + intenc[8] = uv>>56; + } + if (enclen != NULL) *enclen = 9; } } @@ -1199,13 +1208,16 @@ size_t lpBytes(unsigned char *lp) { return lpGetTotalBytes(lp); } -/* Returns the size of a listpack consisting of an integer repeated 'rep' times. */ -size_t lpEstimateBytesRepeatedInteger(long long lval, unsigned long rep) { +size_t lpEstimateBytesInteger(long long lval) { uint64_t enclen; - unsigned char intenc[LP_MAX_INT_ENCODING_LEN]; - lpEncodeIntegerGetType(lval, intenc, &enclen); + lpEncodeIntegerGetType(lval, NULL, &enclen); unsigned long backlen = lpEncodeBacklen(NULL, enclen); - return LP_HDR_SIZE + (enclen + backlen) * rep + 1; + return enclen + backlen; +} + +/* Returns the size of a listpack consisting of an integer repeated 'rep' times. */ +size_t lpEstimateBytesRepeatedInteger(long long lval, unsigned long rep) { + return LP_HDR_SIZE + lpEstimateBytesInteger(lval) * rep + 1; } /* Seek the specified element and returns the pointer to the seeked element. diff --git a/src/listpack.h b/src/listpack.h index b46939c0e62c..954929047218 100644 --- a/src/listpack.h +++ b/src/listpack.h @@ -61,6 +61,7 @@ unsigned char *lpLast(unsigned char *lp); unsigned char *lpNext(unsigned char *lp, unsigned char *p); unsigned char *lpPrev(unsigned char *lp, unsigned char *p); size_t lpBytes(unsigned char *lp); +size_t lpEstimateBytesInteger(long long lval); size_t lpEstimateBytesRepeatedInteger(long long lval, unsigned long rep); unsigned char *lpSeek(unsigned char *lp, long index); typedef int (*listpackValidateEntryCB)(unsigned char *p, unsigned int head_count, void *userdata); diff --git a/src/rdb.c b/src/rdb.c index 3b8d1ef615a3..deefc660b76a 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -695,6 +695,8 @@ int rdbSaveObjectType(rio *rdb, robj *o) { case OBJ_HASH: if (o->encoding == OBJ_ENCODING_LISTPACK) return rdbSaveType(rdb,RDB_TYPE_HASH_LISTPACK); + else if (o->encoding == OBJ_ENCODING_LISTPACK_EX) + return rdbSaveType(rdb,RDB_TYPE_HASH_LISTPACK_TTL); else if (o->encoding == OBJ_ENCODING_HT) { if (hashTypeGetMinExpire(o) == EB_EXPIRE_TIME_INVALID) return rdbSaveType(rdb,RDB_TYPE_HASH); @@ -941,10 +943,12 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { } } else if (o->type == OBJ_HASH) { /* Save a hash value */ - if (o->encoding == OBJ_ENCODING_LISTPACK) { - size_t l = lpBytes((unsigned char*)o->ptr); + if ((o->encoding == OBJ_ENCODING_LISTPACK) || + (o->encoding == OBJ_ENCODING_LISTPACK_EX)) { + unsigned char *lp_ptr = hashTypeListpackGetLp(o); + size_t l = lpBytes(lp_ptr); - if ((n = rdbSaveRawString(rdb,o->ptr,l)) == -1) return -1; + if ((n = rdbSaveRawString(rdb,lp_ptr,l)) == -1) return -1; nwritten += n; } else if (o->encoding == OBJ_ENCODING_HT) { dictIterator *di = dictGetIterator(o->ptr); @@ -1839,19 +1843,19 @@ static int _listZiplistEntryConvertAndValidate(unsigned char *p, unsigned int he /* callback for to check the listpack doesn't have duplicate records */ static int _lpEntryValidation(unsigned char *p, unsigned int head_count, void *userdata) { struct { - int pairs; + int uniqeness_factor; long count; dict *fields; } *data = userdata; if (data->fields == NULL) { data->fields = dictCreate(&hashDictType); - dictExpand(data->fields, data->pairs ? head_count/2 : head_count); + dictExpand(data->fields, head_count/data->uniqeness_factor); } /* If we're checking pairs, then even records are field names. Otherwise * we're checking all elements. Add to dict and check that's not a dup */ - if (!data->pairs || ((data->count) & 1) == 0) { + if (data->count % data->uniqeness_factor == 0) { unsigned char *str; int64_t slen; unsigned char buf[LP_INTBUF_SIZE]; @@ -1872,23 +1876,25 @@ static int _lpEntryValidation(unsigned char *p, unsigned int head_count, void *u /* Validate the integrity of the listpack structure. * when `deep` is 0, only the integrity of the header is validated. * when `deep` is 1, we scan all the entries one by one. - * when `pairs` is 0, all elements need to be unique (it's a set) - * when `pairs` is 1, odd elements need to be unique (it's a key-value map) */ -int lpValidateIntegrityAndDups(unsigned char *lp, size_t size, int deep, int pairs) { + * uniqness_factor indicates which elemnts should be verified for uniquness. + * when it's 1, each elemnt should be unique (set) + * when it's 2, each second element should be unique (key-value map) + * when it's 3, each third element should be unique (key-value-ttl map) */ +int lpValidateIntegrityAndDups(unsigned char *lp, size_t size, int deep, int uniqeness_factor) { if (!deep) return lpValidateIntegrity(lp, size, 0, NULL, NULL); /* Keep track of the field names to locate duplicate ones */ struct { - int pairs; + int uniqeness_factor; long count; dict *fields; /* Initialisation at the first callback. */ - } data = {pairs, 0, NULL}; + } data = {uniqeness_factor, 0, NULL}; int ret = lpValidateIntegrity(lp, size, 1, _lpEntryValidation, &data); - /* make sure we have an even number of records. */ - if (pairs && data.count & 1) + /* the number of redorcds should be a multiple of the uniqueness factor */ + if (data.count % uniqeness_factor != 0) ret = 0; if (data.fields) dictRelease(data.fields); @@ -1899,7 +1905,7 @@ int lpValidateIntegrityAndDups(unsigned char *lp, size_t size, int deep, int pai * On success a newly allocated object is returned, otherwise NULL. * When the function returns NULL and if 'error' is not NULL, the * integer pointed by 'error' is set to the type of error that occurred */ -robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { +robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int *error) { robj *o = NULL, *ele, *dec; uint64_t len; unsigned int i; @@ -2127,7 +2133,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { /* Too many entries? Use a hash table right from the start. */ if (len > server.hash_max_listpack_entries) - hashTypeConvert(o, OBJ_ENCODING_HT, NULL); + hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); else if (deep_integrity_validation) { /* In this mode, we need to guarantee that the server won't crash * later when the ziplist is converted to a dict. @@ -2136,8 +2142,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { dupSearchDict = dictCreate(&hashDictType); } - - /* Load every field and value into the ziplist */ + /* Load every field and value into the listpack */ while (o->encoding == OBJ_ENCODING_LISTPACK && len > 0) { len--; /* Load raw strings */ @@ -2172,7 +2177,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { sdslen(value) > server.hash_max_listpack_value || !lpSafeToAdd(o->ptr, hfieldlen(field) + sdslen(value))) { - hashTypeConvert(o, OBJ_ENCODING_HT, NULL); + hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); dictUseStoredKeyApi((dict *)o->ptr, 1); ret = dictAdd((dict*)o->ptr, field, value); dictUseStoredKeyApi((dict *)o->ptr, 0); @@ -2247,9 +2252,10 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { size_t keyLen; int hashEntryType; sds value; - uint64_t ttl; + uint64_t ttl, minExpire = UINT64_MAX; int ret; mstime_t now = mstime(); + dict *dupSearchDict = NULL; len = rdbLoadLen(rdb, NULL); if (len == RDB_LENERR) return NULL; @@ -2258,11 +2264,24 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { redisDebug("loading metada-enriched hash with %lu keys", len); o = createHashObject(); - /* TODO: need to add listpack-first,-than-convert-to-hash logic like - for simple (noe metadata) hash when listpack encoding eith TTL will - be ready */ - hashTypeConvert(NULL, o, OBJ_ENCODING_HT); - dict *d = o->ptr; + /* Too many entries? Use a hash table right from the start. */ + if (len > server.hash_max_listpack_entries) { + hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); + + redisDebug("using dict encoding, hash keys %lu, max listpack keys %lu", len, server.hash_max_listpack_entries); + + } else { + hashTypeConvert(o, OBJ_ENCODING_LISTPACK_EX, &db->hexpires); + if (deep_integrity_validation) { + /* In this mode, we need to guarantee that the server won't crash + * later when the listpack is converted to a dict. + * Create a set (dict with no values) for dup search. + * We can dismiss it as soon as we convert the listpack to a hash. */ + dupSearchDict = dictCreate(&hashDictType); + } + + redisDebug("using listpck TTL encoding, hash keys %lu, max listpack keys %lu", len, server.hash_max_listpack_entries); + } while (len > 0) { len--; @@ -2270,6 +2289,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { /* read the key */ if ((hkey = rdbGenericLoadStringObject(rdb, RDB_LOAD_PLAIN, &keyLen)) == NULL) { decrRefCount(o); + if (dupSearchDict != NULL) dictRelease(dupSearchDict); return NULL; } @@ -2278,6 +2298,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { /* read the type */ if ((hashEntryType = rdbLoadType(rdb)) == -1) { decrRefCount(o); + if (dupSearchDict != NULL) dictRelease(dupSearchDict); zfree(hkey); return NULL; } @@ -2288,6 +2309,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { if ((unsigned char)hashEntryType & RDB_HASH_ENTRY_VALUE) { if ((value = rdbGenericLoadStringObject(rdb,RDB_LOAD_SDS,NULL)) == NULL) { decrRefCount(o); + if (dupSearchDict != NULL) dictRelease(dupSearchDict); zfree(hkey); return NULL; } @@ -2299,6 +2321,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { if ((unsigned char)hashEntryType & RDB_HASH_ENTRY_TTL) { if (rdbLoadLenByRef(rdb, NULL, &ttl) == -1) { decrRefCount(o); + if (dupSearchDict != NULL) dictRelease(dupSearchDict); sds_free(value); zfree(hkey); return NULL; @@ -2306,9 +2329,13 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { redisDebug("read hash TTL %lu", ttl); - } else + } else { ttl = 0; + redisDebug("no ttl value, using default %d", 0); + + } + /* Check if the hash field already expired. This function is used when * loading an RDB file from disk, either at startup, or when an RDB was * received from the master. In the latter case, the master is @@ -2326,45 +2353,117 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { server.rdb_last_load_hash_fields_expired++; zfree(hkey); sdsfree(value); - redisDebug("key is expired, discarding %d", 1); + + redisDebug("key %s is expired (ttl %lu < now %lld), discarding", hkey, ttl, now); + continue; } - /* compose the key and TTL (if exists) to compose a hash field */ - if ((field = hfieldNew(hkey, keyLen, ttl != 0)) == NULL) { - decrRefCount(o); - sds_free(value); + /* keep the nearest expiration to connect listpack object to db expiry */ + if (ttl < minExpire) minExpire = ttl; + + /* store the values read - either to listpack or dict */ + if (o->encoding == OBJ_ENCODING_LISTPACK_EX) { + /* integrity - check for key duplication (if required) */ + if (dupSearchDict) { + sds field_dup = sdsnewlen(field, hfieldlen(field)); + + if (dictAdd(dupSearchDict, field_dup, NULL) != DICT_OK) { + rdbReportCorruptRDB("Hash with dup elements"); + dictRelease(dupSearchDict); + decrRefCount(o); + sdsfree(field_dup); + sdsfree(value); + zfree(hkey); + return NULL; + } + } + + /* check if the values can be saved to listpack (or should convert to dict encoding) */ + if (keyLen > server.hash_max_listpack_value || + sdslen(value) > server.hash_max_listpack_value || + lpEstimateBytesInteger(ttl) > server.hash_max_listpack_value || + !lpSafeToAdd(listpackExGetListpack(o), keyLen + sdslen(value) + lpEstimateBytesInteger(ttl))) + { + /* convert to hash */ + hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); + + redisDebug("converting to dict encoding %d", 0); + + if (len > DICT_HT_INITIAL_SIZE) { /* TODO: this is NOT the originla len, but this is also the case for simple hash, is this a bug? */ + if (dictTryExpand(o->ptr, len) != DICT_OK) { + rdbReportCorruptRDB("OOM in dictTryExpand %llu", (unsigned long long)len); + decrRefCount(o); + if (dupSearchDict != NULL) dictRelease(dupSearchDict); + sdsfree(value); + zfree(hkey); + return NULL; + } + } + + /* don't add the values to the new hash: the next if will catch and the values will be added there */ + } else { + void *lp = listpackExGetListpack(o); + + redisDebug("savin key %s, value %s, TTL %lu to listpack at %p", hkey, value, ttl, lp); + + lp = lpAppend(lp, (unsigned char *)hkey, keyLen); + lp = lpAppend(lp, (unsigned char*)value, sdslen(value)); /* TODO: what if there is no value? */ + lp = lpAppendInteger(lp, ttl); + listpackExUpdateListpack(o, lp); zfree(hkey); - return NULL; - } else - zfree(hkey); /* was copied into the new mstr hash field */ + sdsfree(value); + } + } - /* TODO: add the TTL to hfield, possibly after inserting the hfiled to the table*/ + if (o->encoding == OBJ_ENCODING_HT) { + /* compose the key and TTL (if exists) to a hash field */ + if ((field = hfieldNew(hkey, keyLen, ttl != 0)) == NULL) { + decrRefCount(o); + if (dupSearchDict != NULL) dictRelease(dupSearchDict); + sds_free(value); + zfree(hkey); + return NULL; + } else + zfree(hkey); /* was copied into the new mstr hash field */ - /* Add pair to hash table */ - dictUseStoredKeyApi(d, 1); - ret = dictAdd(d, field, value); - dictUseStoredKeyApi(d, 0); - if (ret == DICT_ERR) { - rdbReportCorruptRDB("Duplicate hash fields detected"); - decrRefCount(o); - sdsfree(value); - hfieldFree(field); - return NULL; + /* TODO: add the TTL to hfield, possibly after inserting the hfiled to the table */ + + /* Add pair to dict */ + dictUseStoredKeyApi((dict *)o->ptr, 1); + ret = dictAdd((dict *)o->ptr, field, value); + dictUseStoredKeyApi((dict *)o->ptr, 0); + if (ret == DICT_ERR) { + rdbReportCorruptRDB("Duplicate hash fields detected"); + decrRefCount(o); + if (dupSearchDict != NULL) dictRelease(dupSearchDict); + sdsfree(value); + hfieldFree(field); + return NULL; + } } } /* All pairs should be read by now */ serverAssert(len == 0); - /* check for empty key (if all keys were expired) */ - if (dictIsEmpty(d)) { - decrRefCount(o); + if (dupSearchDict != NULL) dictRelease(dupSearchDict); - redisDebug("empty hash, returning empty key (all keys excpired?) %d", 1); + /* check for empty key (if all fields were expired) */ + if (hashTypeLength(o, 0) == 0) { + decrRefCount(o); + redisDebug("empty hash, returning empty key (all fields expired?) %d", 1); goto emptykey; } + /* if the resulting object is a listpack, need to add the minimum TTL to the DB ebuckets */ + if ((o->encoding == OBJ_ENCODING_LISTPACK_EX) && (minExpire != 0) && (db != NULL)) { /* DB can be NULL when checking rdb */ + if (ebAdd(&db->hexpires, &hashExpireBucketsType, o, minExpire) != 0) { + rdbReportError(0, __LINE__, "failed linking listpack to db expiration"); + decrRefCount(o); + return NULL; + } + } } else if (rdbtype == RDB_TYPE_LIST_QUICKLIST || rdbtype == RDB_TYPE_LIST_QUICKLIST_2) { if ((len = rdbLoadLen(rdb,NULL)) == RDB_LENERR) return NULL; if (len == 0) goto emptykey; @@ -2447,7 +2546,8 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { rdbtype == RDB_TYPE_ZSET_ZIPLIST || rdbtype == RDB_TYPE_ZSET_LISTPACK || rdbtype == RDB_TYPE_HASH_ZIPLIST || - rdbtype == RDB_TYPE_HASH_LISTPACK) + rdbtype == RDB_TYPE_HASH_LISTPACK || + rdbtype == RDB_TYPE_HASH_LISTPACK_TTL) { size_t encoded_len; unsigned char *encoded = @@ -2563,7 +2663,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { break; case RDB_TYPE_SET_LISTPACK: if (deep_integrity_validation) server.stat_dump_payload_sanitizations++; - if (!lpValidateIntegrityAndDups(encoded, encoded_len, deep_integrity_validation, 0)) { + if (!lpValidateIntegrityAndDups(encoded, encoded_len, deep_integrity_validation, 1)) { rdbReportCorruptRDB("Set listpack integrity check failed."); zfree(encoded); o->ptr = NULL; @@ -2611,7 +2711,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { } case RDB_TYPE_ZSET_LISTPACK: if (deep_integrity_validation) server.stat_dump_payload_sanitizations++; - if (!lpValidateIntegrityAndDups(encoded, encoded_len, deep_integrity_validation, 1)) { + if (!lpValidateIntegrityAndDups(encoded, encoded_len, deep_integrity_validation, 2)) { rdbReportCorruptRDB("Zset listpack integrity check failed."); zfree(encoded); o->ptr = NULL; @@ -2656,23 +2756,80 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { break; } case RDB_TYPE_HASH_LISTPACK: + case RDB_TYPE_HASH_LISTPACK_TTL: + + + /* listpack-encoded hash with TTL requires its own struct + * pointed to by o->ptr */ + if (rdbtype == RDB_TYPE_HASH_LISTPACK_TTL) { + o->ptr = listpackExCreateFromListpack(encoded); + redisDebug("reading TTL listpack %d", 0); + } else + redisDebug("reading non-TTL listpack %d", 0); + + /* set type and encoding (required for correct free function to be called on error) */ + o->type = OBJ_HASH; + o->encoding = + (rdbtype == RDB_TYPE_HASH_LISTPACK ? OBJ_ENCODING_LISTPACK : OBJ_ENCODING_LISTPACK_EX); + + /* uniqueness_factor is the number of elements for each key: + * key + value for simple hash, key + value + tll for hash with TTL*/ + int uniquness_factor = (rdbtype == RDB_TYPE_HASH_LISTPACK ? 2 : 3); + /* validate read data */ if (deep_integrity_validation) server.stat_dump_payload_sanitizations++; - if (!lpValidateIntegrityAndDups(encoded, encoded_len, deep_integrity_validation, 1)) { + if (!lpValidateIntegrityAndDups(encoded, encoded_len, + deep_integrity_validation, uniquness_factor)) { rdbReportCorruptRDB("Hash listpack integrity check failed."); - zfree(encoded); - o->ptr = NULL; decrRefCount(o); return NULL; } - o->type = OBJ_HASH; - o->encoding = OBJ_ENCODING_LISTPACK; + + /* for TTL listpack, look for and delete expired fields*/ + uint64_t minExpire = 0; + if (rdbtype == RDB_TYPE_HASH_LISTPACK_TTL) { + ExpireInfo info = { + .onExpireItem = NULL, + .ctx = NULL, + .maxToExpire = UINT64_MAX, + .now = mstime() + }; + listpackExExpire(o, &info, &server.rdb_last_load_hash_fields_expired); + minExpire = info.nextExpireTime; + + redisDebug("after checking all entries for expiration, next expiry: %lu", minExpire); + } + + /* if all keys expired and the listpack is empty, delete it */ if (hashTypeLength(o, 0) == 0) { + + redisDebug("all fields expired, returning empty key, %d", 0); + decrRefCount(o); goto emptykey; } - if (hashTypeLength(o, 0) > server.hash_max_listpack_entries) - hashTypeConvert(o, OBJ_ENCODING_HT, NULL); + /* check if need to convert to dict encoding */ + if ((hashTypeLength(o, 0) > server.hash_max_listpack_entries)) { /* TODO: each field length is not verified against server.hash_max_listpack_value */ + + redisDebug("converting listpack with %lu entries to dict, hash_max_listpack_entries %zu", hashTypeLength(o, 0), server.hash_max_listpack_entries); + + hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); + + /* TODO: should dict try expand be done here as well? */ + + } else if (rdbtype == RDB_TYPE_HASH_LISTPACK_TTL) { + /* connect the listpack to the DB-global expiry data structure */ + if ((minExpire != 0) && (db != NULL)) { /* DB can be NULL when checking rdb */ + + redisDebug("calling ebAdd, db->hexpires %p, o %p, minExpire %lu", db->hexpires, (void*)o, minExpire); + + if (ebAdd(&db->hexpires, &hashExpireBucketsType, o, minExpire) != 0) { + rdbReportError(0, __LINE__, "failed linking listpack to db expiration"); + decrRefCount(o); + return NULL; + } + } + } break; default: /* totally unreachable */ @@ -3467,7 +3624,7 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin if ((key = rdbGenericLoadStringObject(rdb,RDB_LOAD_SDS,NULL)) == NULL) goto eoferr; /* Read value */ - val = rdbLoadObject(type,rdb,key,db->id,&error); + val = rdbLoadObject(type,rdb,key,db,db->id,&error); /* Check if the key already expired. This function is used when loading * an RDB file from disk, either at startup, or when an RDB was @@ -3575,12 +3732,12 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin if (empty_keys_skipped) { serverLog(LL_NOTICE, - "Done loading RDB, keys loaded: %lld, keys expired: %lld, empty keys skipped: %lld.", - server.rdb_last_load_keys_loaded, server.rdb_last_load_keys_expired, empty_keys_skipped); + "Done loading RDB, keys loaded: %lld, keys expired: %lld, hash fields expired: %lld, empty keys skipped: %lld.", + server.rdb_last_load_keys_loaded, server.rdb_last_load_keys_expired, server.rdb_last_load_hash_fields_expired, empty_keys_skipped); } else { serverLog(LL_NOTICE, - "Done loading RDB, keys loaded: %lld, keys expired: %lld.", - server.rdb_last_load_keys_loaded, server.rdb_last_load_keys_expired); + "Done loading RDB, keys loaded: %lld, keys expired: %lld, hash fields expired: %lld.", + server.rdb_last_load_keys_loaded, server.rdb_last_load_keys_expired, server.rdb_last_load_hash_fields_expired); } return C_OK; diff --git a/src/rdb.h b/src/rdb.h index 2c4fb2a00d9d..b0d453fd9263 100644 --- a/src/rdb.h +++ b/src/rdb.h @@ -74,10 +74,11 @@ #define RDB_TYPE_SET_LISTPACK 20 #define RDB_TYPE_STREAM_LISTPACKS_3 21 #define RDB_TYPE_HASH_METADATA 22 +#define RDB_TYPE_HASH_LISTPACK_TTL 23 /* NOTE: WHEN ADDING NEW RDB TYPE, UPDATE rdbIsObjectType(), and rdb_type_string[] */ /* Test if a type is an object type. */ -#define rdbIsObjectType(t) (((t) >= 0 && (t) <= 7) || ((t) >= 9 && (t) <= 22)) +#define rdbIsObjectType(t) (((t) >= 0 && (t) <= 7) || ((t) >= 9 && (t) <= 23)) /* Special RDB opcodes (saved/loaded with rdbSaveType/rdbLoadType). */ #define RDB_OPCODE_SLOT_INFO 244 /* Individual slot info, such as slot id and size (cluster mode only). */ @@ -144,7 +145,7 @@ int rdbSaveToFile(const char *filename); int rdbSave(int req, char *filename, rdbSaveInfo *rsi, int rdbflags); ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid); size_t rdbSavedObjectLen(robj *o, robj *key, int dbid); -robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error); +robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int *error); void backgroundSaveDoneHandler(int exitcode, int bysignal); int rdbSaveKeyValuePair(rio *rdb, robj *key, robj *val, long long expiretime,int dbid); ssize_t rdbSaveSingleModuleAux(rio *rdb, int when, moduleType *mt); diff --git a/src/redis-check-rdb.c b/src/redis-check-rdb.c index b77bad32d8b1..960be8c7b48f 100644 --- a/src/redis-check-rdb.c +++ b/src/redis-check-rdb.c @@ -331,7 +331,7 @@ int redis_check_rdb(char *rdbfilename, FILE *fp) { rdbstate.keys++; /* Read value */ rdbstate.doing = RDB_CHECK_DOING_READ_OBJECT_VALUE; - if ((val = rdbLoadObject(type,&rdb,key->ptr,selected_dbid,NULL)) == NULL) goto eoferr; + if ((val = rdbLoadObject(type,&rdb,key->ptr,NULL,selected_dbid,NULL)) == NULL) goto eoferr; /* Check if the key already expired. */ if (expiretime != -1 && expiretime < now) rdbstate.already_expired++; diff --git a/src/server.h b/src/server.h index 482b7cab1c36..3e7893bed2fc 100644 --- a/src/server.h +++ b/src/server.h @@ -3198,9 +3198,14 @@ void hashTypeAddToExpires(redisDb *db, sds key, robj *hashObj, uint64_t expireTi void hashTypeFree(robj *o); int hashTypeIsExpired(const robj *o, uint64_t expireAt); unsigned char *hashTypeListpackGetLp(robj *o); + uint64_t hashTypeGetMinExpire(robj *o); void hashTypeUpdateKeyRef(robj *o, sds newkey); ebuckets *hashTypeGetDictMetaHFE(dict *d); +void *listpackExCreateFromListpack(void *lp); +void *listpackExGetListpack(const robj *o); +void listpackExUpdateListpack(const robj *o, void *lp); +void listpackExExpire(robj *o, ExpireInfo *info, long long *expire_stat); /* Hash-Field data type (of t_hash.c) */ hfield hfieldNew(const void *field, size_t fieldlen, int withExpireMeta); diff --git a/src/t_hash.c b/src/t_hash.c index dda20c4e3e8d..9ed72df74df2 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -23,7 +23,6 @@ static ExpireAction hashTypeActiveExpire(eItem hashObj, void *ctx); static void hfieldPersist(robj *hashObj, hfield field); static void updateGlobalHfeDs(redisDb *db, robj *o, uint64_t minExpire, uint64_t minExpireFields); static uint64_t hashTypeGetNextTimeToExpire(robj *o); -static uint64_t hashTypeGetMinExpire(robj *keyObj); /* hash dictType funcs */ static int dictHfieldKeyCompare(dict *d, const void *key1, const void *key2); @@ -326,6 +325,24 @@ static struct listpackEx *listpackExCreate(void) { return lpt; } +void *listpackExCreateFromListpack(void *lp) { + listpackEx *lpt = zcalloc(sizeof(*lpt)); + lpt->lp = lp; + lpt->meta.trash = 1; + lpt->key = NULL; + return lpt; +} + +void *listpackExGetListpack(const robj *o) { + listpackEx *lpt = o->ptr; + return lpt->lp; +} + +void listpackExUpdateListpack(const robj *o, void *lp) { + listpackEx *lpt = o->ptr; + lpt->lp = lp; +} + static void listpackExFree(listpackEx *lpt) { lpFree(lpt->lp); zfree(lpt); @@ -385,7 +402,7 @@ static uint64_t listpackExGetMinExpire(robj *o) { } /* Walk over fields and delete the expired ones. */ -static void listpackExExpire(robj *o, ExpireInfo *info) { +void listpackExExpire(robj *o, ExpireInfo *info, long long *expire_stat) { serverAssert(o->encoding == OBJ_ENCODING_LISTPACK_EX); uint64_t min = EB_EXPIRE_TIME_INVALID; unsigned char *ptr, *field, *s; @@ -409,7 +426,7 @@ static void listpackExExpire(robj *o, ExpireInfo *info) { if (val == HASH_LP_NO_TTL || (uint64_t) val > info->now) break; - server.stat_expired_hash_fields++; + (*expire_stat)++; lpt->lp = lpDeleteRangeWithEntry(lpt->lp, &field, 3); ptr = field; info->itemsExpired++; @@ -1816,8 +1833,7 @@ static ExpireAction hashTypeActiveExpire(eItem _hashObj, void *ctx) { .now = commandTimeSnapshot(), .itemsExpired = 0}; - listpackExExpire(hashObj, &info); - keystr = ((listpackEx*)hashObj->ptr)->key; + listpackExExpire(hashObj, &info, &server.stat_expired_hash_fields); } else { serverAssert(hashObj->encoding == OBJ_ENCODING_HT); diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index 523795f92a1d..567480a0a03f 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -416,65 +416,151 @@ start_server {} { } {OK} } +set server_path [tmpdir "server.partial-hfield-exp-test"] + # verifies writing and reading hash key with expiring and persistent fields -test {hash field expiration save and load rdb} { - start_server{overrides {hash-max-listpack-entries 0}} { - r FLUSHALL +start_server [list overrides [list "dir" $server_path]] { + test {hash field expiration save and load rdb one expired field} { + foreach type {listpack ht} { + if {$type eq "ht"} { + r config set hash-max-listpack-entries 0 + } else { + r config set hash-max-listpack-entries 512 + } - r HMSET key a 1 b 2 c 3 d 4 - r HEXPIREAT key 2524600800 2 a b - r HPEXPIRE key 100 d + puts "before saving key for type $type" + gets stdin - # sleep 100 ms to make sure d will expire after restart - after 100 + r FLUSHALL - r save - restart_server 0 true false - wait_done_loading r + r HMSET key a 1 b 2 c 3 d 4 + r HEXPIREAT key 2524600800 2 a b + r HPEXPIRE key 100 1 d - assert_equal [r hget key a] 1 - assert_equal [r hget key b] 2 - assert_equal [r hget key c] 3 - assert_equal [r hget key d] nil - assert_equal [r hexpiretime key 3 a b c] {2524600800, 2524600800, -1} + r save + # sleep 100 ms to make sure d will expire after restart + after 100 + restart_server 0 true false + wait_done_loading r + + puts "before verifying key for type $type" + gets stdin + + assert_equal [r hget key a] 1 + assert_equal [r hget key b] 2 + assert_equal [r hget key c] 3 + assert_equal [r hget key d] {} + assert_equal [r hexpiretime key 3 a b c] {2524600800 2524600800 -1} + } } } +set server_path [tmpdir "server.all-hfield-exp-test"] + # verifies writing hash with several expired keys, and active-expiring it on load -test {hash field expiration save and load rdb} { - start_server{overrides {hash-max-listpack-entries 0}} { +test {hash field expiration save and load rdb all fields expired} { + start_server {} { + foreach type {listpack ht} { + if {$type eq "ht"} { + r config set hash-max-listpack-entries 0 + } else { + r config set hash-max-listpack-entries 512 + } + + r FLUSHALL + + r HMSET key a 1 b 2 c 3 d 4 + r HPEXPIRE key 100 4 a b c d + + r save + # sleep 100 ms to make sure all fields will expire after restart + after 100 + + restart_server 0 true false + wait_done_loading r + + assert_equal [r hgetall key] {} + } + } +} + +# verifies a long TTL value (6 bytes) is saved and loaded correctly +test {hash field expiration save and load rdb long TTL} { + start_server {} { + foreach type {listpack ht} { + if {$type eq "ht"} { + r config set hash-max-listpack-entries 0 + } else { + r config set hash-max-listpack-entries 512 + } + + r FLUSHALL + + r HSET key a 1 + # set expiry to 0xabcdef987654 (6 bytes) + r HPEXPIREAT key 188900976391764 1 a + + r save + restart_server 0 true false + wait_done_loading r + + assert_equal [r hget key a ] 1 + assert_equal [r hpexpiretime key 1 a] {188900976391764} + } + } +} + +set server_path [tmpdir "server.listpack-to-dict-test"] + +test {save listpack, load dict} { + start_server {overrides {hash-max-listpack-entries 512}} { + r FLUSHALL r HMSET key a 1 b 2 c 3 d 4 - r HPEXPIRE key 100 4 a b c d + r HPEXPIRE key 100 1 d + r save # sleep 100 ms to make sure all fields will expire after restart after 100 + } - r save - restart_server 0 true false - wait_done_loading r + start_server {overrides {hash-max-listpack-entries 0}} { - assert_equal [r hgetall key] {} + assert_equal [r hget key a] 1 + assert_equal [r hget key b] 2 + assert_equal [r hget key c] 3 + assert_equal [r hget key d] {} } } -# verifies a long TTL value (6 bytes) is saved and loaded correctly -test {hash field expiration save and load rdb} { - start_server{overrides {hash-max-listpack-entries 0}} { +set server_path [tmpdir "server.dict-to-listpack-test"] + +test {save dict, load listpack} { + start_server {overrides {hash-max-listpack-entries 0}} { r FLUSHALL - r HSET key a 1 - # set expiry to 0xabcdef987654 (6 bytes) - r HPEXPIREAT key 188900976391764 1 a + r HMSET key a 1 b 2 c 3 d 4 + r HPEXPIRE key 100 1 d r save - restart_server 0 true false - wait_done_loading r + # sleep 100 ms to make sure all fields will expire after restart + after 100 + } - assert_equal [r hget key a ] 1 - assert_equal [r hexpiretime key 1 a] {188900976391764} + start_server {overrides {hash-max-listpack-entries 512}} { + + assert_equal [r hget key a] 1 + assert_equal [r hget key b] 2 + assert_equal [r hget key c] 3 + assert_equal [r hget key d] {} } } + +#Also: +#2. try listpack verification during load +#3. think about listpack with duplicated fields when one of them is already expired: decision: Need to fail! verify + + } ;# tags From 3c00e304ae01a925ad82b34f38eef79e2a860ebd Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Thu, 2 May 2024 15:12:06 +0300 Subject: [PATCH 03/29] Fix RDB reading of hash metadata to set expiration in addition to value --- src/rdb.c | 88 +++++++++++++++++++++++++++------------------------- src/server.h | 3 ++ src/t_hash.c | 68 +++++++++++++++++++++++++++++++++++----- 3 files changed, 109 insertions(+), 50 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index deefc660b76a..a24fd8a91cc6 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -2247,15 +2247,13 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * serverAssert(len == 0); } else if (rdbtype == RDB_TYPE_HASH_METADATA) { - hfield field; - char *hkey; - size_t keyLen; + size_t fieldLen; int hashEntryType; - sds value; + sds value, field; uint64_t ttl, minExpire = UINT64_MAX; - int ret; mstime_t now = mstime(); dict *dupSearchDict = NULL; + void *hashAddCtx = NULL; len = rdbLoadLen(rdb, NULL); if (len == RDB_LENERR) return NULL; @@ -2267,6 +2265,11 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * /* Too many entries? Use a hash table right from the start. */ if (len > server.hash_max_listpack_entries) { hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); + hashAddCtx = HashTypeGroupSetInit(key, o, db); + if (hashAddCtx == NULL) { + decrRefCount(o); + return NULL; + } redisDebug("using dict encoding, hash keys %lu, max listpack keys %lu", len, server.hash_max_listpack_entries); @@ -2287,19 +2290,21 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * len--; /* read the key */ - if ((hkey = rdbGenericLoadStringObject(rdb, RDB_LOAD_PLAIN, &keyLen)) == NULL) { + if ((field = rdbGenericLoadStringObject(rdb, RDB_LOAD_SDS, &fieldLen)) == NULL) { decrRefCount(o); + if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); if (dupSearchDict != NULL) dictRelease(dupSearchDict); return NULL; } - redisDebug("read key %s", hkey); + redisDebug("read key %s", field); /* read the type */ if ((hashEntryType = rdbLoadType(rdb)) == -1) { decrRefCount(o); + if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); if (dupSearchDict != NULL) dictRelease(dupSearchDict); - zfree(hkey); + sdsfree(field); return NULL; } @@ -2309,8 +2314,9 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * if ((unsigned char)hashEntryType & RDB_HASH_ENTRY_VALUE) { if ((value = rdbGenericLoadStringObject(rdb,RDB_LOAD_SDS,NULL)) == NULL) { decrRefCount(o); + if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); if (dupSearchDict != NULL) dictRelease(dupSearchDict); - zfree(hkey); + sdsfree(field); return NULL; } @@ -2321,9 +2327,10 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * if ((unsigned char)hashEntryType & RDB_HASH_ENTRY_TTL) { if (rdbLoadLenByRef(rdb, NULL, &ttl) == -1) { decrRefCount(o); + if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); if (dupSearchDict != NULL) dictRelease(dupSearchDict); - sds_free(value); - zfree(hkey); + sdsfree(value); + sdsfree(field); return NULL; } @@ -2351,10 +2358,10 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * if (ttl != 0 && iAmMaster() && ((mstime_t)ttl < now)) { /* note: TTL was saved to RDB as unix-time in milliseconds */ /* TODO: consider replication (like in rdbLoadAddKeyToDb) */ server.rdb_last_load_hash_fields_expired++; - zfree(hkey); + sdsfree(field); sdsfree(value); - redisDebug("key %s is expired (ttl %lu < now %lld), discarding", hkey, ttl, now); + redisDebug("key %s is expired (ttl %lu < now %lld), discarding", field, ttl, now); continue; } @@ -2366,37 +2373,47 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * if (o->encoding == OBJ_ENCODING_LISTPACK_EX) { /* integrity - check for key duplication (if required) */ if (dupSearchDict) { - sds field_dup = sdsnewlen(field, hfieldlen(field)); + sds field_dup = sdsnewlen(field, sdslen(field)); if (dictAdd(dupSearchDict, field_dup, NULL) != DICT_OK) { rdbReportCorruptRDB("Hash with dup elements"); dictRelease(dupSearchDict); + if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); decrRefCount(o); sdsfree(field_dup); sdsfree(value); - zfree(hkey); + sdsfree(field); return NULL; } } /* check if the values can be saved to listpack (or should convert to dict encoding) */ - if (keyLen > server.hash_max_listpack_value || + if (sdslen(field) > server.hash_max_listpack_value || sdslen(value) > server.hash_max_listpack_value || lpEstimateBytesInteger(ttl) > server.hash_max_listpack_value || - !lpSafeToAdd(listpackExGetListpack(o), keyLen + sdslen(value) + lpEstimateBytesInteger(ttl))) + !lpSafeToAdd(listpackExGetListpack(o), sdslen(field) + sdslen(value) + lpEstimateBytesInteger(ttl))) { /* convert to hash */ hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); + hashAddCtx = HashTypeGroupSetInit(key, o, db); + if (hashAddCtx == NULL) { + decrRefCount(o); + if (dupSearchDict != NULL) dictRelease(dupSearchDict); + sdsfree(value); + sdsfree(field); + return NULL; + } redisDebug("converting to dict encoding %d", 0); - if (len > DICT_HT_INITIAL_SIZE) { /* TODO: this is NOT the originla len, but this is also the case for simple hash, is this a bug? */ + if (len > DICT_HT_INITIAL_SIZE) { /* TODO: this is NOT the original len, but this is also the case for simple hash, is this a bug? */ if (dictTryExpand(o->ptr, len) != DICT_OK) { rdbReportCorruptRDB("OOM in dictTryExpand %llu", (unsigned long long)len); decrRefCount(o); + hashTypeGroupSetDone(hashAddCtx); if (dupSearchDict != NULL) dictRelease(dupSearchDict); sdsfree(value); - zfree(hkey); + sdsfree(field); return NULL; } } @@ -2405,41 +2422,25 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * } else { void *lp = listpackExGetListpack(o); - redisDebug("savin key %s, value %s, TTL %lu to listpack at %p", hkey, value, ttl, lp); + redisDebug("savin key %s, value %s, TTL %lu to listpack at %p", field, value, ttl, lp); - lp = lpAppend(lp, (unsigned char *)hkey, keyLen); - lp = lpAppend(lp, (unsigned char*)value, sdslen(value)); /* TODO: what if there is no value? */ + lp = lpAppend(lp, (unsigned char *)field, sdslen(field)); + lp = lpAppend(lp, (unsigned char *)value, sdslen(value)); /* TODO: what if there is no value? */ lp = lpAppendInteger(lp, ttl); listpackExUpdateListpack(o, lp); - zfree(hkey); + sdsfree(field); sdsfree(value); } } if (o->encoding == OBJ_ENCODING_HT) { - /* compose the key and TTL (if exists) to a hash field */ - if ((field = hfieldNew(hkey, keyLen, ttl != 0)) == NULL) { + if (hashTypeGroupSet(hashAddCtx, db, o, field, value, ttl) != C_OK) { decrRefCount(o); if (dupSearchDict != NULL) dictRelease(dupSearchDict); - sds_free(value); - zfree(hkey); + hashTypeGroupSetDone(hashAddCtx); + sdsfree(value); + sdsfree(field); return NULL; - } else - zfree(hkey); /* was copied into the new mstr hash field */ - - /* TODO: add the TTL to hfield, possibly after inserting the hfiled to the table */ - - /* Add pair to dict */ - dictUseStoredKeyApi((dict *)o->ptr, 1); - ret = dictAdd((dict *)o->ptr, field, value); - dictUseStoredKeyApi((dict *)o->ptr, 0); - if (ret == DICT_ERR) { - rdbReportCorruptRDB("Duplicate hash fields detected"); - decrRefCount(o); - if (dupSearchDict != NULL) dictRelease(dupSearchDict); - sdsfree(value); - hfieldFree(field); - return NULL; } } } @@ -2448,6 +2449,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * serverAssert(len == 0); if (dupSearchDict != NULL) dictRelease(dupSearchDict); + if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); /* check for empty key (if all fields were expired) */ if (hashTypeLength(o, 0) == 0) { diff --git a/src/server.h b/src/server.h index 3e7893bed2fc..010b9cb47f2f 100644 --- a/src/server.h +++ b/src/server.h @@ -3206,6 +3206,9 @@ void *listpackExCreateFromListpack(void *lp); void *listpackExGetListpack(const robj *o); void listpackExUpdateListpack(const robj *o, void *lp); void listpackExExpire(robj *o, ExpireInfo *info, long long *expire_stat); +void *HashTypeGroupSetInit(sds key, robj *o, redisDb *db); +int hashTypeGroupSet(void* ctx, redisDb *db, robj *o, sds field, sds value, uint64_t expire_at); +void hashTypeGroupSetDone(void *ctx); /* Hash-Field data type (of t_hash.c) */ hfield hfieldNew(const void *field, size_t fieldlen, int withExpireMeta); diff --git a/src/t_hash.c b/src/t_hash.c index 9ed72df74df2..135a18d9b53d 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -242,7 +242,7 @@ static SetExRes hashTypeSetExListpack(redisDb *db, robj *o, sds field, HashTypeS int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cmd, FieldSetCond fieldSetCond, FieldGet fieldGet, - ExpireSetCond expireSetCond, HashTypeSetEx *ex); + ExpireSetCond expireSetCond, HashTypeSetEx *ex, int fromRDB); SetExRes hashTypeSetEx(redisDb *db, robj *o, sds field, HashTypeSet *setKeyVal, uint64_t expireAt, HashTypeSetEx *exInfo); @@ -1066,15 +1066,65 @@ SetExRes hashTypeSetEx(redisDb *db, robj *o, sds field, HashTypeSet *setKeyVal, return res; } +/* + * These 3 functions are a simplification of hashTypeSetExInit, + * hashTypeSetEx and hashTypeSetExDone. They provide a simpler API + * for adding new entries, w/o exposing all the types required for the + * original flavor. + * They are currently used for RDB reading + */ +void *HashTypeGroupSetInit(sds key, robj *o, redisDb *db) { + HashTypeSetEx *set = zmalloc(sizeof(HashTypeSetEx)); + int res; + robj keyobj; + initStaticStringObject(keyobj, key); + res = hashTypeSetExInit(&keyobj, o, NULL, db, NULL, FIELD_DONT_OVRWRT, + FIELD_GET_NONE, HFE_NX, set, 1); + if (res != C_OK) { + zfree(set); + return NULL; + } + + return set; +} + +int hashTypeGroupSet(void* ctx, redisDb *db, robj *o, sds field, sds value, uint64_t expire_at) { + HashTypeSet setKeyVal = {.value = value, .flags = 0}; + SetExRes res; + if (expire_at == 0) + res = hashTypeSetEx(db, o, field, &setKeyVal, expire_at, NULL); + else + res = hashTypeSetEx(db, o, field, &setKeyVal, expire_at, ctx); + if (res != HSETEX_OK) { + return C_ERR; + } + return C_OK; +} + +void hashTypeGroupSetDone(void *ctx) { + hashTypeSetExDone(ctx); + zfree(ctx); +} + /* * Init HashTypeSetEx struct before calling hashTypeSetEx() * * Don't have to provide client and "cmd". If provided, then notification once * done by function hashTypeSetExDone(). + * + * NOTE: when calling from RDB reading process, the key is not yet available + * in the DB. Thereofre, it is not possible to retrieve a pointer to the key to + * be used in the expire meta struct. + * The expireMeta struct needs to hold a pointer to the key string that is + * persistent for the key lifetime. When reading RDB, the key was laready read + * and the string it was read into will be later used in the DB. However, + * if calling this function from any place else and using this flag, need to + * provide a key string in the key robj that will remain persistent for as long + * as the key itself is. */ int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cmd, FieldSetCond fieldSetCond, FieldGet fieldGet, ExpireSetCond expireSetCond, - HashTypeSetEx *ex) + HashTypeSetEx *ex, int fromRDB) { dict *ht = o->ptr; @@ -1108,11 +1158,15 @@ int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cm /* Find the key in the keyspace. Need to keep reference to the key for * notifications or even removal of the hash */ - dictEntry *de = dbFind(db, key->ptr); - serverAssert(de != NULL); + if (!fromRDB) { + dictEntry *de = dbFind(db, key->ptr); + serverAssert(de != NULL); - /* Fillup dict HFE metadata */ - m->key = dictGetKey(de); /* reference key in keyspace */ + /* Fillup dict HFE metadata */ + m->key = dictGetKey(de); /* reference key in keyspace */ + } else { + m->key = key->ptr; + } m->hfe = ebCreate(); /* Allocate HFE DS */ m->expireMeta.trash = 1; /* mark as trash (as long it wasn't ebAdd()) */ } @@ -2924,7 +2978,7 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime FIELD_DONT_CREATE2, FIELD_GET_NONE, expireSetCond, - &exCtx); + &exCtx, 0); addReplyArrayLen(c, numFields); for (int i = 0 ; i < numFields ; i++) { From dfb45ce17e35efb777cc734ce0fa0b152ac43116 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Thu, 2 May 2024 16:51:56 +0300 Subject: [PATCH 04/29] Modify hash-md RDB format; remove type and use key-value-ttl tuple for all keys --- src/rdb.c | 90 +++++++++++++------------------------------ src/rdb.h | 4 -- src/redis-check-rdb.c | 2 + 3 files changed, 29 insertions(+), 67 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index a24fd8a91cc6..456a8955a5fe 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -954,7 +954,6 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { dictIterator *di = dictGetIterator(o->ptr); dictEntry *de; int withTtl = 0; /* whether there is at least one field with a valid TTL */ - unsigned char hashEntryType; uint64_t ttl = 0; /* save number of fileds in hash */ @@ -991,38 +990,25 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { * metadata types, and than the types themselves (note that the * order is fixed, i.e. future extensions may only add new * metadata types at the end) */ - if (withTtl) { - hashEntryType = 0; - if (value != NULL) hashEntryType |= RDB_HASH_ENTRY_VALUE; - if (hfieldIsExpireAttached(field)) { - ttl = hfieldGetExpireTime(field); - hashEntryType |= RDB_HASH_ENTRY_TTL; - } else - ttl = 0; - if ((n = rdbSaveType(rdb, hashEntryType)) == -1) { - dictReleaseIterator(di); - return -1; - } - nwritten += n; - - redisDebug("saved hash type 0x%x", hashEntryType); + if (withTtl && hfieldIsExpireAttached(field)) { + ttl = hfieldGetExpireTime(field); + } else { + ttl = 0; } /* save the value */ - if (value != NULL) { - if ((n = rdbSaveRawString(rdb,(unsigned char*)value, - sdslen(value))) == -1) - { - dictReleaseIterator(di); - return -1; - } - nwritten += n; - - redisDebug("saved hash value %s", (char *)value); + if ((n = rdbSaveRawString(rdb,(unsigned char*)value, + sdslen(value))) == -1) + { + dictReleaseIterator(di); + return -1; } + nwritten += n; + + redisDebug("saved hash value %s", (char *)value); /* save the TTL */ - if (withTtl && (ttl != 0)) { + if (withTtl) { if ((n = rdbSaveLen(rdb, ttl)) == -1) { dictReleaseIterator(di); return -1; @@ -2248,7 +2234,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * } else if (rdbtype == RDB_TYPE_HASH_METADATA) { size_t fieldLen; - int hashEntryType; sds value, field; uint64_t ttl, minExpire = UINT64_MAX; mstime_t now = mstime(); @@ -2289,7 +2274,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * while (len > 0) { len--; - /* read the key */ + /* read the field name */ if ((field = rdbGenericLoadStringObject(rdb, RDB_LOAD_SDS, &fieldLen)) == NULL) { decrRefCount(o); if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); @@ -2297,10 +2282,10 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * return NULL; } - redisDebug("read key %s", field); + redisDebug("read hash field name %s", field); - /* read the type */ - if ((hashEntryType = rdbLoadType(rdb)) == -1) { + /* read the value */ + if ((value = rdbGenericLoadStringObject(rdb,RDB_LOAD_SDS,NULL)) == NULL) { decrRefCount(o); if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); if (dupSearchDict != NULL) dictRelease(dupSearchDict); @@ -2308,40 +2293,19 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * return NULL; } - redisDebug("read hash type 0x%x", hashEntryType); - - /* if the type indicated value exists, read it */ - if ((unsigned char)hashEntryType & RDB_HASH_ENTRY_VALUE) { - if ((value = rdbGenericLoadStringObject(rdb,RDB_LOAD_SDS,NULL)) == NULL) { - decrRefCount(o); - if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); - if (dupSearchDict != NULL) dictRelease(dupSearchDict); - sdsfree(field); - return NULL; - } + redisDebug("read hash value %s", (char *)value); - redisDebug("read hash value %s", (char *)value); + /* read the TTL */ + if (rdbLoadLenByRef(rdb, NULL, &ttl) == -1) { + decrRefCount(o); + if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); + if (dupSearchDict != NULL) dictRelease(dupSearchDict); + sdsfree(value); + sdsfree(field); + return NULL; } - /* if the type indicated TTL exists, read it */ - if ((unsigned char)hashEntryType & RDB_HASH_ENTRY_TTL) { - if (rdbLoadLenByRef(rdb, NULL, &ttl) == -1) { - decrRefCount(o); - if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); - if (dupSearchDict != NULL) dictRelease(dupSearchDict); - sdsfree(value); - sdsfree(field); - return NULL; - } - - redisDebug("read hash TTL %lu", ttl); - - } else { - ttl = 0; - - redisDebug("no ttl value, using default %d", 0); - - } + redisDebug("read hash TTL %lu", ttl); /* Check if the hash field already expired. This function is used when * loading an RDB file from disk, either at startup, or when an RDB was diff --git a/src/rdb.h b/src/rdb.h index b0d453fd9263..7f0674ca1a65 100644 --- a/src/rdb.h +++ b/src/rdb.h @@ -122,10 +122,6 @@ #define RDB_LOAD_ERR_EMPTY_KEY 1 /* Error of empty key */ #define RDB_LOAD_ERR_OTHER 2 /* Any other errors */ -/* Flags for hash entry content when reading/writing a RDB_TYPE_HASH_METADATA */ -#define RDB_HASH_ENTRY_VALUE (1<<0) /* this hash entry contains a value */ -#define RDB_HASH_ENTRY_TTL (1<<1) /* this hash entry contains a TTL */ - ssize_t rdbWriteRaw(rio *rdb, void *p, size_t len); int rdbSaveType(rio *rdb, unsigned char type); int rdbLoadType(rio *rdb); diff --git a/src/redis-check-rdb.c b/src/redis-check-rdb.c index 960be8c7b48f..53a6868fbd08 100644 --- a/src/redis-check-rdb.c +++ b/src/redis-check-rdb.c @@ -80,6 +80,8 @@ char *rdb_type_string[] = { "stream-v2", "set-listpack", "stream-v3", + "hash-md-hashtable", + "hash-md-listpack", }; /* Show a few stats collected into 'rdbstate' */ From d6c3b246c96cbb7f112f889da9be0d1e41dee907 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Thu, 2 May 2024 18:10:54 +0300 Subject: [PATCH 05/29] rename --- src/t_hash.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/t_hash.c b/src/t_hash.c index 135a18d9b53d..df55d4572f84 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -242,7 +242,7 @@ static SetExRes hashTypeSetExListpack(redisDb *db, robj *o, sds field, HashTypeS int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cmd, FieldSetCond fieldSetCond, FieldGet fieldGet, - ExpireSetCond expireSetCond, HashTypeSetEx *ex, int fromRDB); + ExpireSetCond expireSetCond, HashTypeSetEx *ex, int keyInDB); SetExRes hashTypeSetEx(redisDb *db, robj *o, sds field, HashTypeSet *setKeyVal, uint64_t expireAt, HashTypeSetEx *exInfo); @@ -1079,7 +1079,7 @@ void *HashTypeGroupSetInit(sds key, robj *o, redisDb *db) { robj keyobj; initStaticStringObject(keyobj, key); res = hashTypeSetExInit(&keyobj, o, NULL, db, NULL, FIELD_DONT_OVRWRT, - FIELD_GET_NONE, HFE_NX, set, 1); + FIELD_GET_NONE, HFE_NX, set, 0); if (res != C_OK) { zfree(set); return NULL; @@ -1116,15 +1116,15 @@ void hashTypeGroupSetDone(void *ctx) { * in the DB. Thereofre, it is not possible to retrieve a pointer to the key to * be used in the expire meta struct. * The expireMeta struct needs to hold a pointer to the key string that is - * persistent for the key lifetime. When reading RDB, the key was laready read + * persistent for the key lifetime. When reading RDB, the key was already read * and the string it was read into will be later used in the DB. However, - * if calling this function from any place else and using this flag, need to - * provide a key string in the key robj that will remain persistent for as long - * as the key itself is. + * if calling this function from any place else and using this flag, you'll need + * to provide a key string in the key robj that will remain persistent for as + * long as the key itself is. */ int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cmd, FieldSetCond fieldSetCond, FieldGet fieldGet, ExpireSetCond expireSetCond, - HashTypeSetEx *ex, int fromRDB) + HashTypeSetEx *ex, int keyInDB) { dict *ht = o->ptr; @@ -1158,7 +1158,7 @@ int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cm /* Find the key in the keyspace. Need to keep reference to the key for * notifications or even removal of the hash */ - if (!fromRDB) { + if (keyInDB) { dictEntry *de = dbFind(db, key->ptr); serverAssert(de != NULL); @@ -2978,7 +2978,7 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime FIELD_DONT_CREATE2, FIELD_GET_NONE, expireSetCond, - &exCtx, 0); + &exCtx, 1); addReplyArrayLen(c, numFields); for (int i = 0 ; i < numFields ; i++) { From 60287f78a04738f362739fab3e01f00e407e51fa Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Sun, 5 May 2024 11:21:43 +0300 Subject: [PATCH 06/29] typos --- src/rdb.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index 456a8955a5fe..a0ce5d064f07 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -956,7 +956,7 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { int withTtl = 0; /* whether there is at least one field with a valid TTL */ uint64_t ttl = 0; - /* save number of fileds in hash */ + /* save number of fields in hash */ if ((n = rdbSaveLen(rdb,dictSize((dict*)o->ptr))) == -1) { dictReleaseIterator(di); return -1; @@ -1862,11 +1862,11 @@ static int _lpEntryValidation(unsigned char *p, unsigned int head_count, void *u /* Validate the integrity of the listpack structure. * when `deep` is 0, only the integrity of the header is validated. * when `deep` is 1, we scan all the entries one by one. - * uniqness_factor indicates which elemnts should be verified for uniquness. - * when it's 1, each elemnt should be unique (set) + * uniqueness_factor indicates which elements should be verified for uniqueness. + * when it's 1, each element should be unique (set) * when it's 2, each second element should be unique (key-value map) * when it's 3, each third element should be unique (key-value-ttl map) */ -int lpValidateIntegrityAndDups(unsigned char *lp, size_t size, int deep, int uniqeness_factor) { +int lpValidateIntegrityAndDups(unsigned char *lp, size_t size, int deep, int uniqueness_factor) { if (!deep) return lpValidateIntegrity(lp, size, 0, NULL, NULL); @@ -1875,12 +1875,12 @@ int lpValidateIntegrityAndDups(unsigned char *lp, size_t size, int deep, int uni int uniqeness_factor; long count; dict *fields; /* Initialisation at the first callback. */ - } data = {uniqeness_factor, 0, NULL}; + } data = {uniqueness_factor, 0, NULL}; int ret = lpValidateIntegrity(lp, size, 1, _lpEntryValidation, &data); - /* the number of redorcds should be a multiple of the uniqueness factor */ - if (data.count % uniqeness_factor != 0) + /* the number of records should be a multiple of the uniqueness factor */ + if (data.count % uniqueness_factor != 0) ret = 0; if (data.fields) dictRelease(data.fields); @@ -2310,13 +2310,13 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * /* Check if the hash field already expired. This function is used when * loading an RDB file from disk, either at startup, or when an RDB was * received from the master. In the latter case, the master is - * responsible for hash field expiry. If we would expire hash fiedls here, + * responsible for hash field expiry. If we would expire hash fields here, * the snapshot taken by the master may not be reflected on the slave. * Similarly, if the base AOF is RDB format, we want to load all * the hash fields there are, since the log of operations in the incr AOF * is assumed to work in the exact keyspace state. - * Expired hash fields on the master are silenlty discarded. - * Note that if all fileds in a hash has expired, the hash would not be + * Expired hash fields on the master are silently discarded. + * Note that if all fields in a hash has expired, the hash would not be * created in memory (because it is created on the first valid field), and * thus the key would be discarded as an "empty key" */ if (ttl != 0 && iAmMaster() && ((mstime_t)ttl < now)) { /* note: TTL was saved to RDB as unix-time in milliseconds */ From d3b4b46dc7249a37dfff9fdfa3083504d5b6bbbd Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Sun, 5 May 2024 13:00:45 +0300 Subject: [PATCH 07/29] return line accidentally removed during rebase --- src/t_hash.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/t_hash.c b/src/t_hash.c index df55d4572f84..2e31796ce933 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -1888,6 +1888,7 @@ static ExpireAction hashTypeActiveExpire(eItem _hashObj, void *ctx) { .itemsExpired = 0}; listpackExExpire(hashObj, &info, &server.stat_expired_hash_fields); + keystr = ((listpackEx*)hashObj->ptr)->key; } else { serverAssert(hashObj->encoding == OBJ_ENCODING_HT); From 52198ee7c7e6ddb7a5542aac967fbd65b8396305 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Sun, 5 May 2024 18:13:27 +0300 Subject: [PATCH 08/29] fix UTs for HFE RDB de/serialization --- tests/integration/rdb.tcl | 62 +++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 28 deletions(-) diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index 567480a0a03f..289ac89ef3cd 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -418,19 +418,17 @@ start_server {} { set server_path [tmpdir "server.partial-hfield-exp-test"] -# verifies writing and reading hash key with expiring and persistent fields +# verifies writing and reading hash key with expiring and persistent fields +# TODO: re-visit when removing active expiration on load for listpack start_server [list overrides [list "dir" $server_path]] { - test {hash field expiration save and load rdb one expired field} { - foreach type {listpack ht} { - if {$type eq "ht"} { + foreach type {listpack dict} { + test "hash field expiration save and load rdb one expired field, ($type)" { + if {$type eq "dict"} { r config set hash-max-listpack-entries 0 } else { r config set hash-max-listpack-entries 512 } - puts "before saving key for type $type" - gets stdin - r FLUSHALL r HMSET key a 1 b 2 c 3 d 4 @@ -443,9 +441,6 @@ start_server [list overrides [list "dir" $server_path]] { restart_server 0 true false wait_done_loading r - puts "before verifying key for type $type" - gets stdin - assert_equal [r hget key a] 1 assert_equal [r hget key b] 2 assert_equal [r hget key c] 3 @@ -458,10 +453,11 @@ start_server [list overrides [list "dir" $server_path]] { set server_path [tmpdir "server.all-hfield-exp-test"] # verifies writing hash with several expired keys, and active-expiring it on load -test {hash field expiration save and load rdb all fields expired} { - start_server {} { - foreach type {listpack ht} { - if {$type eq "ht"} { +# TODO: re-visit when removing active expiration on load for listpack +start_server [list overrides [list "dir" $server_path]] { + foreach type {listpack dict} { + test "hash field expiration save and load rdb all fields expired, ($type)" { + if {$type eq "dict"} { r config set hash-max-listpack-entries 0 } else { r config set hash-max-listpack-entries 512 @@ -484,11 +480,13 @@ test {hash field expiration save and load rdb all fields expired} { } } +set server_path [tmpdir "server.all-hfield-exp-test"] + # verifies a long TTL value (6 bytes) is saved and loaded correctly -test {hash field expiration save and load rdb long TTL} { - start_server {} { - foreach type {listpack ht} { - if {$type eq "ht"} { +start_server [list overrides [list "dir" $server_path]] { + foreach type {listpack dict} { + test "hash field expiration save and load rdb long TTL, ($type)" { + if {$type eq "dict"} { r config set hash-max-listpack-entries 0 } else { r config set hash-max-listpack-entries 512 @@ -512,48 +510,56 @@ test {hash field expiration save and load rdb long TTL} { set server_path [tmpdir "server.listpack-to-dict-test"] -test {save listpack, load dict} { - start_server {overrides {hash-max-listpack-entries 512}} { +test "save listpack, load dict" { + start_server [list overrides [list "dir" $server_path enable-debug-command yes]] { + r config set hash-max-listpack-entries 512 r FLUSHALL r HMSET key a 1 b 2 c 3 d 4 + assert_match "*encoding:listpack*" [r debug object key] r HPEXPIRE key 100 1 d - r save # sleep 100 ms to make sure all fields will expire after restart after 100 - } - start_server {overrides {hash-max-listpack-entries 0}} { + # change configuration and reload - result should be dict-encoded key + r config set hash-max-listpack-entries 0 + r debug reload assert_equal [r hget key a] 1 assert_equal [r hget key b] 2 assert_equal [r hget key c] 3 assert_equal [r hget key d] {} + assert_match "*encoding:hashtable*" [r debug object key] } } set server_path [tmpdir "server.dict-to-listpack-test"] -test {save dict, load listpack} { - start_server {overrides {hash-max-listpack-entries 0}} { +# TODO: re-visit when removing active expiration on load for listpack +test "save dict, load listpack" { + start_server [list overrides [list "dir" $server_path enable-debug-command yes]] { + r config set hash-max-listpack-entries 0 + r FLUSHALL r HMSET key a 1 b 2 c 3 d 4 + assert_match "*encoding:hashtable*" [r debug object key] r HPEXPIRE key 100 1 d - r save # sleep 100 ms to make sure all fields will expire after restart after 100 - } - start_server {overrides {hash-max-listpack-entries 512}} { + # change configuration and reload - result should be LP-encoded key + r config set hash-max-listpack-entries 512 + r debug reload assert_equal [r hget key a] 1 assert_equal [r hget key b] 2 assert_equal [r hget key c] 3 assert_equal [r hget key d] {} + assert_match "*encoding:listpack*" [r debug object key] } } From 4866704da4220fecd4ced5be7dd9736e18278e80 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Sun, 5 May 2024 18:30:15 +0300 Subject: [PATCH 09/29] remove redisDebug logs --- src/rdb.c | 48 ++---------------------------------------------- 1 file changed, 2 insertions(+), 46 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index a0ce5d064f07..a8f59a7d1bd2 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -967,8 +967,6 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { if (hashTypeGetMinExpire(o) != EB_EXPIRE_TIME_INVALID) withTtl = 1; - redisDebug("saving hash table as dict, withTtl %d", withTtl); - /* save all hash fields */ while((de = dictNext(di)) != NULL) { hfield field = dictGetKey(de); @@ -983,8 +981,6 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { } nwritten += n; - redisDebug("saved hash key %s", (char *)field); - /* if this is a hash table with a TTL for at least one field, * save it in the "with metadata" format - one byte for included * metadata types, and than the types themselves (note that the @@ -1005,8 +1001,6 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { } nwritten += n; - redisDebug("saved hash value %s", (char *)value); - /* save the TTL */ if (withTtl) { if ((n = rdbSaveLen(rdb, ttl)) == -1) { @@ -1014,8 +1008,6 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { return -1; } nwritten += n; - - redisDebug("saved hash ttl %lu", ttl); } } dictReleaseIterator(di); @@ -2243,9 +2235,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * len = rdbLoadLen(rdb, NULL); if (len == RDB_LENERR) return NULL; if (len == 0) goto emptykey; - - redisDebug("loading metada-enriched hash with %lu keys", len); - o = createHashObject(); /* Too many entries? Use a hash table right from the start. */ if (len > server.hash_max_listpack_entries) { @@ -2255,9 +2244,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * decrRefCount(o); return NULL; } - - redisDebug("using dict encoding, hash keys %lu, max listpack keys %lu", len, server.hash_max_listpack_entries); - } else { hashTypeConvert(o, OBJ_ENCODING_LISTPACK_EX, &db->hexpires); if (deep_integrity_validation) { @@ -2267,8 +2253,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * * We can dismiss it as soon as we convert the listpack to a hash. */ dupSearchDict = dictCreate(&hashDictType); } - - redisDebug("using listpck TTL encoding, hash keys %lu, max listpack keys %lu", len, server.hash_max_listpack_entries); } while (len > 0) { @@ -2282,8 +2266,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * return NULL; } - redisDebug("read hash field name %s", field); - /* read the value */ if ((value = rdbGenericLoadStringObject(rdb,RDB_LOAD_SDS,NULL)) == NULL) { decrRefCount(o); @@ -2293,8 +2275,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * return NULL; } - redisDebug("read hash value %s", (char *)value); - /* read the TTL */ if (rdbLoadLenByRef(rdb, NULL, &ttl) == -1) { decrRefCount(o); @@ -2305,8 +2285,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * return NULL; } - redisDebug("read hash TTL %lu", ttl); - /* Check if the hash field already expired. This function is used when * loading an RDB file from disk, either at startup, or when an RDB was * received from the master. In the latter case, the master is @@ -2324,9 +2302,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * server.rdb_last_load_hash_fields_expired++; sdsfree(field); sdsfree(value); - - redisDebug("key %s is expired (ttl %lu < now %lld), discarding", field, ttl, now); - continue; } @@ -2368,8 +2343,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * return NULL; } - redisDebug("converting to dict encoding %d", 0); - if (len > DICT_HT_INITIAL_SIZE) { /* TODO: this is NOT the original len, but this is also the case for simple hash, is this a bug? */ if (dictTryExpand(o->ptr, len) != DICT_OK) { rdbReportCorruptRDB("OOM in dictTryExpand %llu", (unsigned long long)len); @@ -2385,11 +2358,8 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * /* don't add the values to the new hash: the next if will catch and the values will be added there */ } else { void *lp = listpackExGetListpack(o); - - redisDebug("savin key %s, value %s, TTL %lu to listpack at %p", field, value, ttl, lp); - lp = lpAppend(lp, (unsigned char *)field, sdslen(field)); - lp = lpAppend(lp, (unsigned char *)value, sdslen(value)); /* TODO: what if there is no value? */ + lp = lpAppend(lp, (unsigned char *)value, sdslen(value)); lp = lpAppendInteger(lp, ttl); listpackExUpdateListpack(o, lp); sdsfree(field); @@ -2418,7 +2388,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * /* check for empty key (if all fields were expired) */ if (hashTypeLength(o, 0) == 0) { decrRefCount(o); - redisDebug("empty hash, returning empty key (all fields expired?) %d", 1); goto emptykey; } @@ -2729,9 +2698,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * * pointed to by o->ptr */ if (rdbtype == RDB_TYPE_HASH_LISTPACK_TTL) { o->ptr = listpackExCreateFromListpack(encoded); - redisDebug("reading TTL listpack %d", 0); - } else - redisDebug("reading non-TTL listpack %d", 0); + } /* set type and encoding (required for correct free function to be called on error) */ o->type = OBJ_HASH; @@ -2761,24 +2728,16 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * }; listpackExExpire(o, &info, &server.rdb_last_load_hash_fields_expired); minExpire = info.nextExpireTime; - - redisDebug("after checking all entries for expiration, next expiry: %lu", minExpire); } /* if all keys expired and the listpack is empty, delete it */ if (hashTypeLength(o, 0) == 0) { - - redisDebug("all fields expired, returning empty key, %d", 0); - decrRefCount(o); goto emptykey; } /* check if need to convert to dict encoding */ if ((hashTypeLength(o, 0) > server.hash_max_listpack_entries)) { /* TODO: each field length is not verified against server.hash_max_listpack_value */ - - redisDebug("converting listpack with %lu entries to dict, hash_max_listpack_entries %zu", hashTypeLength(o, 0), server.hash_max_listpack_entries); - hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); /* TODO: should dict try expand be done here as well? */ @@ -2786,9 +2745,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * } else if (rdbtype == RDB_TYPE_HASH_LISTPACK_TTL) { /* connect the listpack to the DB-global expiry data structure */ if ((minExpire != 0) && (db != NULL)) { /* DB can be NULL when checking rdb */ - - redisDebug("calling ebAdd, db->hexpires %p, o %p, minExpire %lu", db->hexpires, (void*)o, minExpire); - if (ebAdd(&db->hexpires, &hashExpireBucketsType, o, minExpire) != 0) { rdbReportError(0, __LINE__, "failed linking listpack to db expiration"); decrRefCount(o); From 213f24fa82901be41a797008dc0984ecd0c25684 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Tue, 7 May 2024 11:07:35 +0300 Subject: [PATCH 10/29] HFE RDB de/serialization - PR comments --- src/ebuckets.c | 3 ++- src/listpack.c | 5 +++-- src/listpack.h | 2 +- src/rdb.c | 54 +++++++++++++++++++++++++++----------------------- src/rdb.h | 2 +- src/server.h | 6 +----- src/t_hash.c | 24 +++------------------- 7 files changed, 40 insertions(+), 56 deletions(-) diff --git a/src/ebuckets.c b/src/ebuckets.c index f9fb1d67b45a..fd1a557f57ea 100644 --- a/src/ebuckets.c +++ b/src/ebuckets.c @@ -1403,7 +1403,8 @@ int ebRemove(ebuckets *eb, EbucketsType *type, eItem item) { * @param item - The eItem to be added to the ebucket. * @param expireTime - The expiration time of the item. * - * @return 0 if the item was successfully added; Otherwise, return 1 on failure. + * @return 0 (C_OK) if the item was successfully added; + * Otherwise, return -1 (C_ERR) on failure. */ int ebAdd(ebuckets *eb, EbucketsType *type, eItem item, uint64_t expireTime) { int res; diff --git a/src/listpack.c b/src/listpack.c index 5acc16a326d8..0346100dee01 100644 --- a/src/listpack.c +++ b/src/listpack.c @@ -219,6 +219,7 @@ int lpStringToInt64(const char *s, unsigned long slen, int64_t *value) { * */ unsigned char *lpNew(size_t capacity) { unsigned char *lp = lp_malloc(capacity > LP_HDR_SIZE+1 ? capacity : LP_HDR_SIZE+1); + if (lp == NULL) return NULL; lpSetTotalBytes(lp,LP_HDR_SIZE+1); lpSetNumElements(lp,0); lp[LP_HDR_SIZE] = LP_EOF; @@ -1208,7 +1209,7 @@ size_t lpBytes(unsigned char *lp) { return lpGetTotalBytes(lp); } -size_t lpEstimateBytesInteger(long long lval) { +size_t lpEntrySizeInteger(long long lval) { uint64_t enclen; lpEncodeIntegerGetType(lval, NULL, &enclen); unsigned long backlen = lpEncodeBacklen(NULL, enclen); @@ -1217,7 +1218,7 @@ size_t lpEstimateBytesInteger(long long lval) { /* Returns the size of a listpack consisting of an integer repeated 'rep' times. */ size_t lpEstimateBytesRepeatedInteger(long long lval, unsigned long rep) { - return LP_HDR_SIZE + lpEstimateBytesInteger(lval) * rep + 1; + return LP_HDR_SIZE + lpEntrySizeInteger(lval) * rep + 1; } /* Seek the specified element and returns the pointer to the seeked element. diff --git a/src/listpack.h b/src/listpack.h index 954929047218..bd16fcf239c6 100644 --- a/src/listpack.h +++ b/src/listpack.h @@ -61,7 +61,7 @@ unsigned char *lpLast(unsigned char *lp); unsigned char *lpNext(unsigned char *lp, unsigned char *p); unsigned char *lpPrev(unsigned char *lp, unsigned char *p); size_t lpBytes(unsigned char *lp); -size_t lpEstimateBytesInteger(long long lval); +size_t lpEntrySizeInteger(long long lval); size_t lpEstimateBytesRepeatedInteger(long long lval, unsigned long rep); unsigned char *lpSeek(unsigned char *lp, long index); typedef int (*listpackValidateEntryCB)(unsigned char *p, unsigned int head_count, void *userdata); diff --git a/src/rdb.c b/src/rdb.c index a8f59a7d1bd2..e7ec6aef3b7e 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -944,7 +944,8 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { } else if (o->type == OBJ_HASH) { /* Save a hash value */ if ((o->encoding == OBJ_ENCODING_LISTPACK) || - (o->encoding == OBJ_ENCODING_LISTPACK_EX)) { + (o->encoding == OBJ_ENCODING_LISTPACK_EX)) + { unsigned char *lp_ptr = hashTypeListpackGetLp(o); size_t l = lpBytes(lp_ptr); @@ -953,7 +954,7 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { } else if (o->encoding == OBJ_ENCODING_HT) { dictIterator *di = dictGetIterator(o->ptr); dictEntry *de; - int withTtl = 0; /* whether there is at least one field with a valid TTL */ + int with_ttl = 0; /* whether there is at least one field with a valid TTL */ uint64_t ttl = 0; /* save number of fields in hash */ @@ -965,7 +966,7 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { /* use standard hash format if no TTL was set for any field */ if (hashTypeGetMinExpire(o) != EB_EXPIRE_TIME_INVALID) - withTtl = 1; + with_ttl = 1; /* save all hash fields */ while((de = dictNext(di)) != NULL) { @@ -986,7 +987,7 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { * metadata types, and than the types themselves (note that the * order is fixed, i.e. future extensions may only add new * metadata types at the end) */ - if (withTtl && hfieldIsExpireAttached(field)) { + if (with_ttl && hfieldIsExpireAttached(field)) { ttl = hfieldGetExpireTime(field); } else { ttl = 0; @@ -1002,7 +1003,7 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { nwritten += n; /* save the TTL */ - if (withTtl) { + if (with_ttl) { if ((n = rdbSaveLen(rdb, ttl)) == -1) { dictReleaseIterator(di); return -1; @@ -1821,19 +1822,19 @@ static int _listZiplistEntryConvertAndValidate(unsigned char *p, unsigned int he /* callback for to check the listpack doesn't have duplicate records */ static int _lpEntryValidation(unsigned char *p, unsigned int head_count, void *userdata) { struct { - int uniqeness_factor; + int tuple_len; long count; dict *fields; } *data = userdata; if (data->fields == NULL) { data->fields = dictCreate(&hashDictType); - dictExpand(data->fields, head_count/data->uniqeness_factor); + dictExpand(data->fields, head_count/data->tuple_len); } /* If we're checking pairs, then even records are field names. Otherwise * we're checking all elements. Add to dict and check that's not a dup */ - if (data->count % data->uniqeness_factor == 0) { + if (data->count % data->tuple_len == 0) { unsigned char *str; int64_t slen; unsigned char buf[LP_INTBUF_SIZE]; @@ -1854,25 +1855,25 @@ static int _lpEntryValidation(unsigned char *p, unsigned int head_count, void *u /* Validate the integrity of the listpack structure. * when `deep` is 0, only the integrity of the header is validated. * when `deep` is 1, we scan all the entries one by one. - * uniqueness_factor indicates which elements should be verified for uniqueness. + * tuple_len indicates what is a logical entry tuple size. * when it's 1, each element should be unique (set) * when it's 2, each second element should be unique (key-value map) * when it's 3, each third element should be unique (key-value-ttl map) */ -int lpValidateIntegrityAndDups(unsigned char *lp, size_t size, int deep, int uniqueness_factor) { +int lpValidateIntegrityAndDups(unsigned char *lp, size_t size, int deep, int tuple_len) { if (!deep) return lpValidateIntegrity(lp, size, 0, NULL, NULL); /* Keep track of the field names to locate duplicate ones */ struct { - int uniqeness_factor; + int tuple_len; long count; dict *fields; /* Initialisation at the first callback. */ - } data = {uniqueness_factor, 0, NULL}; + } data = {tuple_len, 0, NULL}; int ret = lpValidateIntegrity(lp, size, 1, _lpEntryValidation, &data); - /* the number of records should be a multiple of the uniqueness factor */ - if (data.count % uniqueness_factor != 0) + /* the number of records should be a multiple of the tuple length */ + if (data.count % tuple_len != 0) ret = 0; if (data.fields) dictRelease(data.fields); @@ -2329,8 +2330,8 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * /* check if the values can be saved to listpack (or should convert to dict encoding) */ if (sdslen(field) > server.hash_max_listpack_value || sdslen(value) > server.hash_max_listpack_value || - lpEstimateBytesInteger(ttl) > server.hash_max_listpack_value || - !lpSafeToAdd(listpackExGetListpack(o), sdslen(field) + sdslen(value) + lpEstimateBytesInteger(ttl))) + lpEntrySizeInteger(ttl) > server.hash_max_listpack_value || + !lpSafeToAdd(((listpackEx*)o->ptr)->lp, sdslen(field) + sdslen(value) + lpEntrySizeInteger(ttl))) { /* convert to hash */ hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); @@ -2357,11 +2358,11 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * /* don't add the values to the new hash: the next if will catch and the values will be added there */ } else { - void *lp = listpackExGetListpack(o); + void *lp = ((listpackEx*)o->ptr)->lp; lp = lpAppend(lp, (unsigned char *)field, sdslen(field)); lp = lpAppend(lp, (unsigned char *)value, sdslen(value)); lp = lpAppendInteger(lp, ttl); - listpackExUpdateListpack(o, lp); + ((listpackEx*)o->ptr)->lp = lp; sdsfree(field); sdsfree(value); } @@ -2692,12 +2693,14 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * } case RDB_TYPE_HASH_LISTPACK: case RDB_TYPE_HASH_LISTPACK_TTL: - - /* listpack-encoded hash with TTL requires its own struct * pointed to by o->ptr */ if (rdbtype == RDB_TYPE_HASH_LISTPACK_TTL) { - o->ptr = listpackExCreateFromListpack(encoded); + listpackEx *lpt = zcalloc(sizeof(*lpt)); + lpt->lp = encoded; + lpt->meta.trash = 1; + lpt->key = NULL; + o->ptr = lpt; } /* set type and encoding (required for correct free function to be called on error) */ @@ -2705,13 +2708,13 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * o->encoding = (rdbtype == RDB_TYPE_HASH_LISTPACK ? OBJ_ENCODING_LISTPACK : OBJ_ENCODING_LISTPACK_EX); - /* uniqueness_factor is the number of elements for each key: + /* tuple_len is the number of elements for each key: * key + value for simple hash, key + value + tll for hash with TTL*/ - int uniquness_factor = (rdbtype == RDB_TYPE_HASH_LISTPACK ? 2 : 3); + int tuple_len = (rdbtype == RDB_TYPE_HASH_LISTPACK ? 2 : 3); /* validate read data */ if (deep_integrity_validation) server.stat_dump_payload_sanitizations++; if (!lpValidateIntegrityAndDups(encoded, encoded_len, - deep_integrity_validation, uniquness_factor)) { + deep_integrity_validation, tuple_len)) { rdbReportCorruptRDB("Hash listpack integrity check failed."); decrRefCount(o); return NULL; @@ -2726,7 +2729,8 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * .maxToExpire = UINT64_MAX, .now = mstime() }; - listpackExExpire(o, &info, &server.rdb_last_load_hash_fields_expired); + listpackExExpire(o, &info); + server.rdb_last_load_hash_fields_expired += info.itemsExpired; minExpire = info.nextExpireTime; } diff --git a/src/rdb.h b/src/rdb.h index 7f0674ca1a65..db9c0ef0b83d 100644 --- a/src/rdb.h +++ b/src/rdb.h @@ -141,7 +141,7 @@ int rdbSaveToFile(const char *filename); int rdbSave(int req, char *filename, rdbSaveInfo *rsi, int rdbflags); ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid); size_t rdbSavedObjectLen(robj *o, robj *key, int dbid); -robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int *error); +robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb *db, int dbid, int *error); void backgroundSaveDoneHandler(int exitcode, int bysignal); int rdbSaveKeyValuePair(rio *rdb, robj *key, robj *val, long long expiretime,int dbid); ssize_t rdbSaveSingleModuleAux(rio *rdb, int when, moduleType *mt); diff --git a/src/server.h b/src/server.h index 010b9cb47f2f..5c29991330e9 100644 --- a/src/server.h +++ b/src/server.h @@ -3198,14 +3198,10 @@ void hashTypeAddToExpires(redisDb *db, sds key, robj *hashObj, uint64_t expireTi void hashTypeFree(robj *o); int hashTypeIsExpired(const robj *o, uint64_t expireAt); unsigned char *hashTypeListpackGetLp(robj *o); - uint64_t hashTypeGetMinExpire(robj *o); void hashTypeUpdateKeyRef(robj *o, sds newkey); ebuckets *hashTypeGetDictMetaHFE(dict *d); -void *listpackExCreateFromListpack(void *lp); -void *listpackExGetListpack(const robj *o); -void listpackExUpdateListpack(const robj *o, void *lp); -void listpackExExpire(robj *o, ExpireInfo *info, long long *expire_stat); +void listpackExExpire(robj *o, ExpireInfo *info); void *HashTypeGroupSetInit(sds key, robj *o, redisDb *db); int hashTypeGroupSet(void* ctx, redisDb *db, robj *o, sds field, sds value, uint64_t expire_at); void hashTypeGroupSetDone(void *ctx); diff --git a/src/t_hash.c b/src/t_hash.c index 2e31796ce933..d3ab6a9351d2 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -325,24 +325,6 @@ static struct listpackEx *listpackExCreate(void) { return lpt; } -void *listpackExCreateFromListpack(void *lp) { - listpackEx *lpt = zcalloc(sizeof(*lpt)); - lpt->lp = lp; - lpt->meta.trash = 1; - lpt->key = NULL; - return lpt; -} - -void *listpackExGetListpack(const robj *o) { - listpackEx *lpt = o->ptr; - return lpt->lp; -} - -void listpackExUpdateListpack(const robj *o, void *lp) { - listpackEx *lpt = o->ptr; - lpt->lp = lp; -} - static void listpackExFree(listpackEx *lpt) { lpFree(lpt->lp); zfree(lpt); @@ -402,7 +384,7 @@ static uint64_t listpackExGetMinExpire(robj *o) { } /* Walk over fields and delete the expired ones. */ -void listpackExExpire(robj *o, ExpireInfo *info, long long *expire_stat) { +void listpackExExpire(robj *o, ExpireInfo *info) { serverAssert(o->encoding == OBJ_ENCODING_LISTPACK_EX); uint64_t min = EB_EXPIRE_TIME_INVALID; unsigned char *ptr, *field, *s; @@ -426,7 +408,6 @@ void listpackExExpire(robj *o, ExpireInfo *info, long long *expire_stat) { if (val == HASH_LP_NO_TTL || (uint64_t) val > info->now) break; - (*expire_stat)++; lpt->lp = lpDeleteRangeWithEntry(lpt->lp, &field, 3); ptr = field; info->itemsExpired++; @@ -1887,7 +1868,8 @@ static ExpireAction hashTypeActiveExpire(eItem _hashObj, void *ctx) { .now = commandTimeSnapshot(), .itemsExpired = 0}; - listpackExExpire(hashObj, &info, &server.stat_expired_hash_fields); + listpackExExpire(hashObj, &info); + server.stat_expired_hash_fields += info.itemsExpired; keystr = ((listpackEx*)hashObj->ptr)->key; } else { serverAssert(hashObj->encoding == OBJ_ENCODING_HT); From 4109d96f059dae0f4893530a38243c8e2b294268 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Tue, 7 May 2024 11:46:46 +0300 Subject: [PATCH 11/29] HFE RDB de/serialization - PR comments #2 --- src/t_hash.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/t_hash.c b/src/t_hash.c index d3ab6a9351d2..9fcc8d62146f 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -967,6 +967,9 @@ SetExRes hashTypeSetExpiry(HashTypeSetEx *ex, sds field, uint64_t expireAt, dict * * 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) From 36506812908174d45170dacbd36c0ba3615ea58f Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Thu, 9 May 2024 15:16:18 +0300 Subject: [PATCH 12/29] fix rebase glitch --- src/server.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/server.h b/src/server.h index 5c29991330e9..0c4d9f209e63 100644 --- a/src/server.h +++ b/src/server.h @@ -3205,6 +3205,7 @@ void listpackExExpire(robj *o, ExpireInfo *info); void *HashTypeGroupSetInit(sds key, robj *o, redisDb *db); int hashTypeGroupSet(void* ctx, redisDb *db, robj *o, sds field, sds value, uint64_t expire_at); void hashTypeGroupSetDone(void *ctx); +uint64_t hashTypeGetMinExpire(robj *keyObj); /* Hash-Field data type (of t_hash.c) */ hfield hfieldNew(const void *field, size_t fieldlen, int withExpireMeta); From 0bc645eab832be58a3c033dfb209f15f1e4d1b8a Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Thu, 9 May 2024 15:36:25 +0300 Subject: [PATCH 13/29] HFE RDB de/serialization - PR comments #3 --- tests/integration/rdb.tcl | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index 289ac89ef3cd..a0428f284857 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -441,10 +441,7 @@ start_server [list overrides [list "dir" $server_path]] { restart_server 0 true false wait_done_loading r - assert_equal [r hget key a] 1 - assert_equal [r hget key b] 2 - assert_equal [r hget key c] 3 - assert_equal [r hget key d] {} + assert_equal [lsort [r hgetall key]] "1 2 3 a b c" assert_equal [r hexpiretime key 3 a b c] {2524600800 2524600800 -1} } } @@ -527,10 +524,7 @@ test "save listpack, load dict" { r config set hash-max-listpack-entries 0 r debug reload - assert_equal [r hget key a] 1 - assert_equal [r hget key b] 2 - assert_equal [r hget key c] 3 - assert_equal [r hget key d] {} + assert_equal [lsort [r hgetall key]] "1 2 3 a b c" assert_match "*encoding:hashtable*" [r debug object key] } } @@ -555,10 +549,7 @@ test "save dict, load listpack" { r config set hash-max-listpack-entries 512 r debug reload - assert_equal [r hget key a] 1 - assert_equal [r hget key b] 2 - assert_equal [r hget key c] 3 - assert_equal [r hget key d] {} + assert_equal [lsort [r hgetall key]] "1 2 3 a b c" assert_match "*encoding:listpack*" [r debug object key] } } From e600e9b31dd08a2e93b0ab7c75abe4b6e49e3a84 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Thu, 9 May 2024 16:22:00 +0300 Subject: [PATCH 14/29] set hash objects and expiry directly --- src/rdb.c | 34 +++------------------------------- src/server.h | 4 +--- src/t_hash.c | 52 +++++++++++++++++++--------------------------------- 3 files changed, 23 insertions(+), 67 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index e7ec6aef3b7e..e4df20191cfb 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -2231,7 +2231,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * uint64_t ttl, minExpire = UINT64_MAX; mstime_t now = mstime(); dict *dupSearchDict = NULL; - void *hashAddCtx = NULL; len = rdbLoadLen(rdb, NULL); if (len == RDB_LENERR) return NULL; @@ -2240,11 +2239,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * /* Too many entries? Use a hash table right from the start. */ if (len > server.hash_max_listpack_entries) { hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); - hashAddCtx = HashTypeGroupSetInit(key, o, db); - if (hashAddCtx == NULL) { - decrRefCount(o); - return NULL; - } } else { hashTypeConvert(o, OBJ_ENCODING_LISTPACK_EX, &db->hexpires); if (deep_integrity_validation) { @@ -2262,7 +2256,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * /* read the field name */ if ((field = rdbGenericLoadStringObject(rdb, RDB_LOAD_SDS, &fieldLen)) == NULL) { decrRefCount(o); - if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); if (dupSearchDict != NULL) dictRelease(dupSearchDict); return NULL; } @@ -2270,7 +2263,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * /* read the value */ if ((value = rdbGenericLoadStringObject(rdb,RDB_LOAD_SDS,NULL)) == NULL) { decrRefCount(o); - if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); if (dupSearchDict != NULL) dictRelease(dupSearchDict); sdsfree(field); return NULL; @@ -2279,7 +2271,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * /* read the TTL */ if (rdbLoadLenByRef(rdb, NULL, &ttl) == -1) { decrRefCount(o); - if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); if (dupSearchDict != NULL) dictRelease(dupSearchDict); sdsfree(value); sdsfree(field); @@ -2318,7 +2309,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * if (dictAdd(dupSearchDict, field_dup, NULL) != DICT_OK) { rdbReportCorruptRDB("Hash with dup elements"); dictRelease(dupSearchDict); - if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); decrRefCount(o); sdsfree(field_dup); sdsfree(value); @@ -2335,20 +2325,11 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * { /* convert to hash */ hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); - hashAddCtx = HashTypeGroupSetInit(key, o, db); - if (hashAddCtx == NULL) { - decrRefCount(o); - if (dupSearchDict != NULL) dictRelease(dupSearchDict); - sdsfree(value); - sdsfree(field); - return NULL; - } if (len > DICT_HT_INITIAL_SIZE) { /* TODO: this is NOT the original len, but this is also the case for simple hash, is this a bug? */ if (dictTryExpand(o->ptr, len) != DICT_OK) { rdbReportCorruptRDB("OOM in dictTryExpand %llu", (unsigned long long)len); decrRefCount(o); - hashTypeGroupSetDone(hashAddCtx); if (dupSearchDict != NULL) dictRelease(dupSearchDict); sdsfree(value); sdsfree(field); @@ -2369,10 +2350,9 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * } if (o->encoding == OBJ_ENCODING_HT) { - if (hashTypeGroupSet(hashAddCtx, db, o, field, value, ttl) != C_OK) { + if (hashTypeSetExRdb(db, o, field, value, ttl) != C_OK) { decrRefCount(o); if (dupSearchDict != NULL) dictRelease(dupSearchDict); - hashTypeGroupSetDone(hashAddCtx); sdsfree(value); sdsfree(field); return NULL; @@ -2384,22 +2364,14 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * serverAssert(len == 0); if (dupSearchDict != NULL) dictRelease(dupSearchDict); - if (hashAddCtx != NULL) hashTypeGroupSetDone(hashAddCtx); /* check for empty key (if all fields were expired) */ if (hashTypeLength(o, 0) == 0) { decrRefCount(o); goto emptykey; } - - /* if the resulting object is a listpack, need to add the minimum TTL to the DB ebuckets */ - if ((o->encoding == OBJ_ENCODING_LISTPACK_EX) && (minExpire != 0) && (db != NULL)) { /* DB can be NULL when checking rdb */ - if (ebAdd(&db->hexpires, &hashExpireBucketsType, o, minExpire) != 0) { - rdbReportError(0, __LINE__, "failed linking listpack to db expiration"); - decrRefCount(o); - return NULL; - } - } + if (db != NULL) + hashTypeAddToExpires(db, key, o, minExpire); } else if (rdbtype == RDB_TYPE_LIST_QUICKLIST || rdbtype == RDB_TYPE_LIST_QUICKLIST_2) { if ((len = rdbLoadLen(rdb,NULL)) == RDB_LENERR) return NULL; if (len == 0) goto emptykey; diff --git a/src/server.h b/src/server.h index 0c4d9f209e63..9a2fd9495420 100644 --- a/src/server.h +++ b/src/server.h @@ -3202,9 +3202,7 @@ uint64_t hashTypeGetMinExpire(robj *o); void hashTypeUpdateKeyRef(robj *o, sds newkey); ebuckets *hashTypeGetDictMetaHFE(dict *d); void listpackExExpire(robj *o, ExpireInfo *info); -void *HashTypeGroupSetInit(sds key, robj *o, redisDb *db); -int hashTypeGroupSet(void* ctx, redisDb *db, robj *o, sds field, sds value, uint64_t expire_at); -void hashTypeGroupSetDone(void *ctx); +int hashTypeSetExRdb(redisDb *db, robj *o, sds field, sds value, uint64_t expire_at); uint64_t hashTypeGetMinExpire(robj *keyObj); /* Hash-Field data type (of t_hash.c) */ diff --git a/src/t_hash.c b/src/t_hash.c index 9fcc8d62146f..c8b1902de03d 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -1051,44 +1051,30 @@ SetExRes hashTypeSetEx(redisDb *db, robj *o, sds field, HashTypeSet *setKeyVal, } /* - * These 3 functions are a simplification of hashTypeSetExInit, - * hashTypeSetEx and hashTypeSetExDone. They provide a simpler API - * for adding new entries, w/o exposing all the types required for the - * original flavor. - * They are currently used for RDB reading + * hashTypeSetExRdb provide a simplified API for setting fields & expiry by RDB load + * + * It is the duty of RDB reading process to track minimal expiration time of the + * fields and eventually call hashTypeAddToExpires() to update global HFE DS with + * next expiration time. */ -void *HashTypeGroupSetInit(sds key, robj *o, redisDb *db) { - HashTypeSetEx *set = zmalloc(sizeof(HashTypeSetEx)); - int res; - robj keyobj; - initStaticStringObject(keyobj, key); - res = hashTypeSetExInit(&keyobj, o, NULL, db, NULL, FIELD_DONT_OVRWRT, - FIELD_GET_NONE, HFE_NX, set, 0); - if (res != C_OK) { - zfree(set); - return NULL; - } +int hashTypeSetExRdb(redisDb *db, robj *o, sds field, sds value, uint64_t expire_at) { + /* Dummy struct to be used in hashTypeSetEx() */ + HashTypeSetEx setEx = { + .fieldSetCond = FIELD_DONT_OVRWRT, /* Shouldn't be any duplication */ + .expireSetCond = HFE_NX, /* Should set expiry once each field */ + .minExpire = EB_EXPIRE_TIME_INVALID, /* Won't be used. Accounting made by RDB already */ + .key = NULL, /* Not going to call hashTypeSetExDone() */ + .hashObj = o, + .minExpireFields = EB_EXPIRE_TIME_INVALID, /* Not needed by RDB */ + .c = NULL, /* No notification required */ + .cmd = NULL, /* No notification required */ + }; - return set; -} - -int hashTypeGroupSet(void* ctx, redisDb *db, robj *o, sds field, sds value, uint64_t expire_at) { HashTypeSet setKeyVal = {.value = value, .flags = 0}; - SetExRes res; - if (expire_at == 0) - res = hashTypeSetEx(db, o, field, &setKeyVal, expire_at, NULL); - else - res = hashTypeSetEx(db, o, field, &setKeyVal, expire_at, ctx); - if (res != HSETEX_OK) { - return C_ERR; - } - return C_OK; + SetExRes res = hashTypeSetEx(db, o, field, &setKeyVal, expire_at, (expire_at) ? &setEx : NULL); + return (res == HSETEX_OK || res == HSET_UPDATE) ? C_OK : C_ERR; } -void hashTypeGroupSetDone(void *ctx) { - hashTypeSetExDone(ctx); - zfree(ctx); -} /* * Init HashTypeSetEx struct before calling hashTypeSetEx() From 0152265dd03fd19618aacf5017d6583ccf42d726 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Thu, 9 May 2024 18:26:01 +0300 Subject: [PATCH 15/29] remove listpack-encoded hash active expiry on load --- src/rdb.c | 27 ++++++--------------------- src/server.h | 1 + src/t_hash.c | 1 - tests/integration/rdb.tcl | 7 ++----- 4 files changed, 9 insertions(+), 27 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index e4df20191cfb..7a54edbbf5c4 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -2692,26 +2692,15 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * return NULL; } - /* for TTL listpack, look for and delete expired fields*/ - uint64_t minExpire = 0; - if (rdbtype == RDB_TYPE_HASH_LISTPACK_TTL) { - ExpireInfo info = { - .onExpireItem = NULL, - .ctx = NULL, - .maxToExpire = UINT64_MAX, - .now = mstime() - }; - listpackExExpire(o, &info); - server.rdb_last_load_hash_fields_expired += info.itemsExpired; - minExpire = info.nextExpireTime; - } - - /* if all keys expired and the listpack is empty, delete it */ + /* if listpack is empty, delete it */ if (hashTypeLength(o, 0) == 0) { decrRefCount(o); goto emptykey; } + /* for TTL listpack, find the minimum expiry */ + uint64_t minExpire = hashTypeGetNextTimeToExpire(o); + /* check if need to convert to dict encoding */ if ((hashTypeLength(o, 0) > server.hash_max_listpack_entries)) { /* TODO: each field length is not verified against server.hash_max_listpack_value */ hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); @@ -2720,12 +2709,8 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * } else if (rdbtype == RDB_TYPE_HASH_LISTPACK_TTL) { /* connect the listpack to the DB-global expiry data structure */ - if ((minExpire != 0) && (db != NULL)) { /* DB can be NULL when checking rdb */ - if (ebAdd(&db->hexpires, &hashExpireBucketsType, o, minExpire) != 0) { - rdbReportError(0, __LINE__, "failed linking listpack to db expiration"); - decrRefCount(o); - return NULL; - } + if ((minExpire != EB_EXPIRE_TIME_INVALID) && (db != NULL)) { /* DB can be NULL when checking rdb */ + hashTypeAddToExpires(db, key, o, minExpire); } } break; diff --git a/src/server.h b/src/server.h index 9a2fd9495420..7d6fc4587980 100644 --- a/src/server.h +++ b/src/server.h @@ -3204,6 +3204,7 @@ ebuckets *hashTypeGetDictMetaHFE(dict *d); void listpackExExpire(robj *o, ExpireInfo *info); int hashTypeSetExRdb(redisDb *db, robj *o, sds field, sds value, uint64_t expire_at); uint64_t hashTypeGetMinExpire(robj *keyObj); +uint64_t hashTypeGetNextTimeToExpire(robj *o); /* Hash-Field data type (of t_hash.c) */ hfield hfieldNew(const void *field, size_t fieldlen, int withExpireMeta); diff --git a/src/t_hash.c b/src/t_hash.c index c8b1902de03d..7b87c737fe23 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -22,7 +22,6 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime static ExpireAction hashTypeActiveExpire(eItem hashObj, void *ctx); static void hfieldPersist(robj *hashObj, hfield field); static void updateGlobalHfeDs(redisDb *db, robj *o, uint64_t minExpire, uint64_t minExpireFields); -static uint64_t hashTypeGetNextTimeToExpire(robj *o); /* hash dictType funcs */ static int dictHfieldKeyCompare(dict *d, const void *key1, const void *key2); diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index a0428f284857..ace4d0640e56 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -419,7 +419,6 @@ start_server {} { set server_path [tmpdir "server.partial-hfield-exp-test"] # verifies writing and reading hash key with expiring and persistent fields -# TODO: re-visit when removing active expiration on load for listpack start_server [list overrides [list "dir" $server_path]] { foreach type {listpack dict} { test "hash field expiration save and load rdb one expired field, ($type)" { @@ -450,7 +449,6 @@ start_server [list overrides [list "dir" $server_path]] { set server_path [tmpdir "server.all-hfield-exp-test"] # verifies writing hash with several expired keys, and active-expiring it on load -# TODO: re-visit when removing active expiration on load for listpack start_server [list overrides [list "dir" $server_path]] { foreach type {listpack dict} { test "hash field expiration save and load rdb all fields expired, ($type)" { @@ -477,7 +475,7 @@ start_server [list overrides [list "dir" $server_path]] { } } -set server_path [tmpdir "server.all-hfield-exp-test"] +set server_path [tmpdir "server.long-ttl-test"] # verifies a long TTL value (6 bytes) is saved and loaded correctly start_server [list overrides [list "dir" $server_path]] { @@ -524,14 +522,13 @@ test "save listpack, load dict" { r config set hash-max-listpack-entries 0 r debug reload - assert_equal [lsort [r hgetall key]] "1 2 3 a b c" + assert_equal [lsort [r hgetall key]] "1 2 3 4 a b c d" assert_match "*encoding:hashtable*" [r debug object key] } } set server_path [tmpdir "server.dict-to-listpack-test"] -# TODO: re-visit when removing active expiration on load for listpack test "save dict, load listpack" { start_server [list overrides [list "dir" $server_path enable-debug-command yes]] { r config set hash-max-listpack-entries 0 From 679b7f185d2ceeaa10e17d4e1b41681344220105 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Sun, 12 May 2024 17:22:50 +0300 Subject: [PATCH 16/29] HFE RDB de/serialization - PR comments #4 --- src/cluster.c | 2 +- src/rdb.c | 108 +++++++++++++++++++------------------- src/rdb.h | 4 +- src/redis-check-rdb.c | 4 +- src/t_hash.c | 1 + tests/integration/rdb.tcl | 52 ++++++++++++++++-- 6 files changed, 109 insertions(+), 62 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index 6015a99d0930..6a786a2c731f 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -237,7 +237,7 @@ void restoreCommand(client *c) { rioInitWithBuffer(&payload,c->argv[3]->ptr); if (((type = rdbLoadObjectType(&payload)) == -1) || - ((obj = rdbLoadObject(type,&payload,key->ptr,c->db,c->db->id,NULL)) == NULL)) + ((obj = rdbLoadObject(type,&payload,key->ptr,c->db,NULL, 0)) == NULL)) { addReplyError(c,"Bad data format"); return; diff --git a/src/rdb.c b/src/rdb.c index 7a54edbbf5c4..50fb4ef2e35f 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -696,7 +696,7 @@ int rdbSaveObjectType(rio *rdb, robj *o) { if (o->encoding == OBJ_ENCODING_LISTPACK) return rdbSaveType(rdb,RDB_TYPE_HASH_LISTPACK); else if (o->encoding == OBJ_ENCODING_LISTPACK_EX) - return rdbSaveType(rdb,RDB_TYPE_HASH_LISTPACK_TTL); + return rdbSaveType(rdb,RDB_TYPE_HASH_LISTPACK_EX); else if (o->encoding == OBJ_ENCODING_HT) { if (hashTypeGetMinExpire(o) == EB_EXPIRE_TIME_INVALID) return rdbSaveType(rdb,RDB_TYPE_HASH); @@ -954,8 +954,12 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { } else if (o->encoding == OBJ_ENCODING_HT) { dictIterator *di = dictGetIterator(o->ptr); dictEntry *de; - int with_ttl = 0; /* whether there is at least one field with a valid TTL */ - uint64_t ttl = 0; + /* Determine the hash layout to use based on the presence of at least + * one field with a valid TTL. If such a field exists, employ the + * RDB_TYPE_HASH_METADATA layout, including tuples of [field][value][ttl]. + * Otherwise, use the standard RDB_TYPE_HASH layout containing only + * the tuples [field][value]. */ + int with_ttl = (hashTypeGetMinExpire(o) != EB_EXPIRE_TIME_INVALID); /* save number of fields in hash */ if ((n = rdbSaveLen(rdb,dictSize((dict*)o->ptr))) == -1) { @@ -964,10 +968,6 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { } nwritten += n; - /* use standard hash format if no TTL was set for any field */ - if (hashTypeGetMinExpire(o) != EB_EXPIRE_TIME_INVALID) - with_ttl = 1; - /* save all hash fields */ while((de = dictNext(di)) != NULL) { hfield field = dictGetKey(de); @@ -982,17 +982,6 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { } nwritten += n; - /* if this is a hash table with a TTL for at least one field, - * save it in the "with metadata" format - one byte for included - * metadata types, and than the types themselves (note that the - * order is fixed, i.e. future extensions may only add new - * metadata types at the end) */ - if (with_ttl && hfieldIsExpireAttached(field)) { - ttl = hfieldGetExpireTime(field); - } else { - ttl = 0; - } - /* save the value */ if ((n = rdbSaveRawString(rdb,(unsigned char*)value, sdslen(value))) == -1) @@ -1004,6 +993,9 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { /* save the TTL */ if (with_ttl) { + uint64_t ttl = hfieldGetExpireTime(field); + /* 0 is used to indicate no TTL is set for this field */ + if (ttl == EB_EXPIRE_TIME_INVALID) ttl = 0; if ((n = rdbSaveLen(rdb, ttl)) == -1) { dictReleaseIterator(di); return -1; @@ -1856,9 +1848,8 @@ static int _lpEntryValidation(unsigned char *p, unsigned int head_count, void *u * when `deep` is 0, only the integrity of the header is validated. * when `deep` is 1, we scan all the entries one by one. * tuple_len indicates what is a logical entry tuple size. - * when it's 1, each element should be unique (set) - * when it's 2, each second element should be unique (key-value map) - * when it's 3, each third element should be unique (key-value-ttl map) */ + * Whether tuple is of size 1 (set), 2 (feild-value) or 3 (field-value[-ttl]), + * first element in the tuple must be unique */ int lpValidateIntegrityAndDups(unsigned char *lp, size_t size, int deep, int tuple_len) { if (!deep) return lpValidateIntegrity(lp, size, 0, NULL, NULL); @@ -1884,7 +1875,8 @@ int lpValidateIntegrityAndDups(unsigned char *lp, size_t size, int deep, int tup * On success a newly allocated object is returned, otherwise NULL. * When the function returns NULL and if 'error' is not NULL, the * integer pointed by 'error' is set to the type of error that occurred */ -robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int *error) { +robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, + int rdbflags) { robj *o = NULL, *ele, *dec; uint64_t len; unsigned int i; @@ -2103,6 +2095,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * sds value; hfield field; dict *dupSearchDict = NULL; + ebuckets *hexpires = (db != NULL ? &db->hexpires : NULL); len = rdbLoadLen(rdb, NULL); if (len == RDB_LENERR) return NULL; @@ -2112,7 +2105,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * /* Too many entries? Use a hash table right from the start. */ if (len > server.hash_max_listpack_entries) - hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); + hashTypeConvert(o, OBJ_ENCODING_HT, hexpires); else if (deep_integrity_validation) { /* In this mode, we need to guarantee that the server won't crash * later when the ziplist is converted to a dict. @@ -2156,7 +2149,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * sdslen(value) > server.hash_max_listpack_value || !lpSafeToAdd(o->ptr, hfieldlen(field) + sdslen(value))) { - hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); + hashTypeConvert(o, OBJ_ENCODING_HT, hexpires); dictUseStoredKeyApi((dict *)o->ptr, 1); ret = dictAdd((dict*)o->ptr, field, value); dictUseStoredKeyApi((dict *)o->ptr, 0); @@ -2228,9 +2221,10 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * size_t fieldLen; sds value, field; - uint64_t ttl, minExpire = UINT64_MAX; + uint64_t expire, minExpire = EB_EXPIRE_TIME_INVALID; mstime_t now = mstime(); dict *dupSearchDict = NULL; + ebuckets *hexpires = (db != NULL ? &db->hexpires : NULL); len = rdbLoadLen(rdb, NULL); if (len == RDB_LENERR) return NULL; @@ -2238,9 +2232,9 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * o = createHashObject(); /* Too many entries? Use a hash table right from the start. */ if (len > server.hash_max_listpack_entries) { - hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); + hashTypeConvert(o, OBJ_ENCODING_HT, hexpires); } else { - hashTypeConvert(o, OBJ_ENCODING_LISTPACK_EX, &db->hexpires); + hashTypeConvert(o, OBJ_ENCODING_LISTPACK_EX, hexpires); if (deep_integrity_validation) { /* In this mode, we need to guarantee that the server won't crash * later when the listpack is converted to a dict. @@ -2269,7 +2263,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * } /* read the TTL */ - if (rdbLoadLenByRef(rdb, NULL, &ttl) == -1) { + if (rdbLoadLenByRef(rdb, NULL, &expire) == -1) { decrRefCount(o); if (dupSearchDict != NULL) dictRelease(dupSearchDict); sdsfree(value); @@ -2289,7 +2283,8 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * * Note that if all fields in a hash has expired, the hash would not be * created in memory (because it is created on the first valid field), and * thus the key would be discarded as an "empty key" */ - if (ttl != 0 && iAmMaster() && ((mstime_t)ttl < now)) { /* note: TTL was saved to RDB as unix-time in milliseconds */ + if (expire != 0 && iAmMaster() && ((mstime_t)expire < now) && /* note: expire was saved to RDB as unix-time in milliseconds */ + !(rdbflags & RDBFLAGS_AOF_PREAMBLE)) { /* TODO: consider replication (like in rdbLoadAddKeyToDb) */ server.rdb_last_load_hash_fields_expired++; sdsfree(field); @@ -2298,7 +2293,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * } /* keep the nearest expiration to connect listpack object to db expiry */ - if (ttl < minExpire) minExpire = ttl; + if (expire < minExpire) minExpire = expire; /* store the values read - either to listpack or dict */ if (o->encoding == OBJ_ENCODING_LISTPACK_EX) { @@ -2320,11 +2315,10 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * /* check if the values can be saved to listpack (or should convert to dict encoding) */ if (sdslen(field) > server.hash_max_listpack_value || sdslen(value) > server.hash_max_listpack_value || - lpEntrySizeInteger(ttl) > server.hash_max_listpack_value || - !lpSafeToAdd(((listpackEx*)o->ptr)->lp, sdslen(field) + sdslen(value) + lpEntrySizeInteger(ttl))) + !lpSafeToAdd(((listpackEx*)o->ptr)->lp, sdslen(field) + sdslen(value) + lpEntrySizeInteger(expire))) { /* convert to hash */ - hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); + hashTypeConvert(o, OBJ_ENCODING_HT, hexpires); if (len > DICT_HT_INITIAL_SIZE) { /* TODO: this is NOT the original len, but this is also the case for simple hash, is this a bug? */ if (dictTryExpand(o->ptr, len) != DICT_OK) { @@ -2342,7 +2336,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * void *lp = ((listpackEx*)o->ptr)->lp; lp = lpAppend(lp, (unsigned char *)field, sdslen(field)); lp = lpAppend(lp, (unsigned char *)value, sdslen(value)); - lp = lpAppendInteger(lp, ttl); + lp = lpAppendInteger(lp, expire); ((listpackEx*)o->ptr)->lp = lp; sdsfree(field); sdsfree(value); @@ -2350,19 +2344,21 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * } if (o->encoding == OBJ_ENCODING_HT) { - if (hashTypeSetExRdb(db, o, field, value, ttl) != C_OK) { + /* WA for check-rdb mode, when there's no DB so can't attach expired items to ebuckets */ + if (db == NULL) { + hashTypeSet(db, o, field, value, 0); + } else { + if (hashTypeSetExRdb(db, o, field, value, expire) != C_OK) { decrRefCount(o); if (dupSearchDict != NULL) dictRelease(dupSearchDict); sdsfree(value); sdsfree(field); return NULL; + } } } } - /* All pairs should be read by now */ - serverAssert(len == 0); - if (dupSearchDict != NULL) dictRelease(dupSearchDict); /* check for empty key (if all fields were expired) */ @@ -2370,7 +2366,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * decrRefCount(o); goto emptykey; } - if (db != NULL) + if ((db != NULL) && (o->encoding == OBJ_ENCODING_LISTPACK_EX)) hashTypeAddToExpires(db, key, o, minExpire); } else if (rdbtype == RDB_TYPE_LIST_QUICKLIST || rdbtype == RDB_TYPE_LIST_QUICKLIST_2) { if ((len = rdbLoadLen(rdb,NULL)) == RDB_LENERR) return NULL; @@ -2455,7 +2451,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * rdbtype == RDB_TYPE_ZSET_LISTPACK || rdbtype == RDB_TYPE_HASH_ZIPLIST || rdbtype == RDB_TYPE_HASH_LISTPACK || - rdbtype == RDB_TYPE_HASH_LISTPACK_TTL) + rdbtype == RDB_TYPE_HASH_LISTPACK_EX) { size_t encoded_len; unsigned char *encoded = @@ -2664,21 +2660,19 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * break; } case RDB_TYPE_HASH_LISTPACK: - case RDB_TYPE_HASH_LISTPACK_TTL: + case RDB_TYPE_HASH_LISTPACK_EX: /* listpack-encoded hash with TTL requires its own struct * pointed to by o->ptr */ - if (rdbtype == RDB_TYPE_HASH_LISTPACK_TTL) { + o->type = OBJ_HASH; + if (rdbtype == RDB_TYPE_HASH_LISTPACK_EX) { listpackEx *lpt = zcalloc(sizeof(*lpt)); lpt->lp = encoded; lpt->meta.trash = 1; - lpt->key = NULL; + lpt->key = key; o->ptr = lpt; - } - - /* set type and encoding (required for correct free function to be called on error) */ - o->type = OBJ_HASH; - o->encoding = - (rdbtype == RDB_TYPE_HASH_LISTPACK ? OBJ_ENCODING_LISTPACK : OBJ_ENCODING_LISTPACK_EX); + o->encoding = OBJ_ENCODING_LISTPACK_EX; + } else + o->encoding = OBJ_ENCODING_LISTPACK; /* tuple_len is the number of elements for each key: * key + value for simple hash, key + value + tll for hash with TTL*/ @@ -2702,12 +2696,14 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * uint64_t minExpire = hashTypeGetNextTimeToExpire(o); /* check if need to convert to dict encoding */ - if ((hashTypeLength(o, 0) > server.hash_max_listpack_entries)) { /* TODO: each field length is not verified against server.hash_max_listpack_value */ + if ((db != NULL) && + (hashTypeLength(o, 0) > server.hash_max_listpack_entries)) /* TODO: each field length is not verified against server.hash_max_listpack_value */ + { hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); /* TODO: should dict try expand be done here as well? */ - } else if (rdbtype == RDB_TYPE_HASH_LISTPACK_TTL) { + } else if (rdbtype == RDB_TYPE_HASH_LISTPACK_EX) { /* connect the listpack to the DB-global expiry data structure */ if ((minExpire != EB_EXPIRE_TIME_INVALID) && (db != NULL)) { /* DB can be NULL when checking rdb */ hashTypeAddToExpires(db, key, o, minExpire); @@ -3052,7 +3048,13 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int dbid, int * RedisModuleIO io; robj keyobj; initStaticStringObject(keyobj,key); - moduleInitIOContext(io,mt,rdb,&keyobj,dbid); + /* shouldn't happen since db is NULL only in RDB check mode, and + * in this mode the module load code returns few lines above after + * checking module name, few lines above. So this check is only + * for safety. + */ + if (db == NULL) return NULL; + moduleInitIOContext(io,mt,rdb,&keyobj,db->id); /* Call the rdb_load method of the module providing the 10 bit * encoding version in the lower 10 bits of the module ID. */ void *ptr = mt->rdb_load(&io,moduleid&1023); @@ -3507,7 +3509,7 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin if ((key = rdbGenericLoadStringObject(rdb,RDB_LOAD_SDS,NULL)) == NULL) goto eoferr; /* Read value */ - val = rdbLoadObject(type,rdb,key,db,db->id,&error); + val = rdbLoadObject(type,rdb,key,db,&error,rdbflags); /* Check if the key already expired. This function is used when loading * an RDB file from disk, either at startup, or when an RDB was diff --git a/src/rdb.h b/src/rdb.h index db9c0ef0b83d..9ec7c348f7e5 100644 --- a/src/rdb.h +++ b/src/rdb.h @@ -74,7 +74,7 @@ #define RDB_TYPE_SET_LISTPACK 20 #define RDB_TYPE_STREAM_LISTPACKS_3 21 #define RDB_TYPE_HASH_METADATA 22 -#define RDB_TYPE_HASH_LISTPACK_TTL 23 +#define RDB_TYPE_HASH_LISTPACK_EX 23 /* NOTE: WHEN ADDING NEW RDB TYPE, UPDATE rdbIsObjectType(), and rdb_type_string[] */ /* Test if a type is an object type. */ @@ -141,7 +141,7 @@ int rdbSaveToFile(const char *filename); int rdbSave(int req, char *filename, rdbSaveInfo *rsi, int rdbflags); ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid); size_t rdbSavedObjectLen(robj *o, robj *key, int dbid); -robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb *db, int dbid, int *error); +robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb *db, int *error, int rdbflags); void backgroundSaveDoneHandler(int exitcode, int bysignal); int rdbSaveKeyValuePair(rio *rdb, robj *key, robj *val, long long expiretime,int dbid); ssize_t rdbSaveSingleModuleAux(rio *rdb, int when, moduleType *mt); diff --git a/src/redis-check-rdb.c b/src/redis-check-rdb.c index 53a6868fbd08..a6d336e558d2 100644 --- a/src/redis-check-rdb.c +++ b/src/redis-check-rdb.c @@ -175,7 +175,6 @@ void rdbCheckSetupSignals(void) { * otherwise the already open file 'fp' is checked. */ int redis_check_rdb(char *rdbfilename, FILE *fp) { uint64_t dbid; - int selected_dbid = -1; int type, rdbver; char buf[1024]; long long expiretime, now = mstime(); @@ -247,7 +246,6 @@ int redis_check_rdb(char *rdbfilename, FILE *fp) { if ((dbid = rdbLoadLen(&rdb,NULL)) == RDB_LENERR) goto eoferr; rdbCheckInfo("Selecting DB ID %llu", (unsigned long long)dbid); - selected_dbid = dbid; continue; /* Read type again. */ } else if (type == RDB_OPCODE_RESIZEDB) { /* RESIZEDB: Hint about the size of the keys in the currently @@ -333,7 +331,7 @@ int redis_check_rdb(char *rdbfilename, FILE *fp) { rdbstate.keys++; /* Read value */ rdbstate.doing = RDB_CHECK_DOING_READ_OBJECT_VALUE; - if ((val = rdbLoadObject(type,&rdb,key->ptr,NULL,selected_dbid,NULL)) == NULL) goto eoferr; + if ((val = rdbLoadObject(type,&rdb,key->ptr,NULL,NULL,0)) == NULL) goto eoferr; /* Check if the key already expired. */ if (expiretime != -1 && expiretime < now) rdbstate.already_expired++; diff --git a/src/t_hash.c b/src/t_hash.c index 7b87c737fe23..65a66570c56f 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -1686,6 +1686,7 @@ void hashTypeConvertListpackEx(robj *o, int enc, ebuckets *hexpires) { } } +/* NOTE: hexpires might be NULL */ void hashTypeConvert(robj *o, int enc, ebuckets *hexpires) { if (o->encoding == OBJ_ENCODING_LISTPACK) { hashTypeConvertListpack(o, enc); diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index ace4d0640e56..0932c75bfc0e 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -442,6 +442,12 @@ start_server [list overrides [list "dir" $server_path]] { assert_equal [lsort [r hgetall key]] "1 2 3 a b c" assert_equal [r hexpiretime key 3 a b c] {2524600800 2524600800 -1} + assert_equal [s rdb_last_load_keys_loaded] 1 + if {$type eq "dict"} { + assert_equal [s rdb_last_load_hash_fields_expired] 1 + } else { + assert_equal [s rdb_last_load_hash_fields_expired] 0 + } } } } @@ -470,6 +476,16 @@ start_server [list overrides [list "dir" $server_path]] { restart_server 0 true false wait_done_loading r + if {$type eq "dict"} { + assert_equal [s rdb_last_load_keys_loaded] 0 + assert_equal [s rdb_last_load_hash_fields_expired] 4 + } else { + assert_equal [s rdb_last_load_keys_loaded] 1 + assert_equal [s rdb_last_load_hash_fields_expired] 0 + } + + # in listpack encoding, the fileds (and key) will be expired by + # lazy expiry assert_equal [r hgetall key] {} } } @@ -551,10 +567,40 @@ test "save dict, load listpack" { } } +set server_path [tmpdir "server.active-expiry-after-load"] -#Also: -#2. try listpack verification during load -#3. think about listpack with duplicated fields when one of them is already expired: decision: Need to fail! verify +# verifies a field is correctly expired by active expiry AFTER loading from RDB +start_server [list overrides [list "dir" $server_path]] { + foreach type {listapck dict} { + test "active field expiry after load, ($type)" { + if {$type eq "dict"} { + r config set hash-max-listpack-entries 0 + } else { + r config set hash-max-listpack-entries 512 + } + r FLUSHALL + + r HMSET key a 1 b 2 c 3 d 4 + r HEXPIREAT key 2524600800 2 a b + r HPEXPIRE key 200 2 c d + + r save + restart_server 0 true false + wait_done_loading r + + # sleep 1 sec to make sure 'c' and 'd' will active-expire + after 1000 + + assert_equal [s rdb_last_load_keys_loaded] 1 + assert_equal [s rdb_last_load_hash_fields_expired] 0 + assert_equal [s expired_hash_fields] 2 + + # hgetall might lazy expire fields, so it's only called after the stat asserts + assert_equal [lsort [r hgetall key]] "1 2 a b" + assert_equal [r hexpiretime key 4 a b c d] {2524600800 2524600800 -2 -2} + } + } +} } ;# tags From 039907494ee0a4773d2f7673889b67cb00adcb2a Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Sun, 12 May 2024 17:26:50 +0300 Subject: [PATCH 17/29] typos --- tests/integration/rdb.tcl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index 0932c75bfc0e..fca3c0e11f70 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -484,7 +484,7 @@ start_server [list overrides [list "dir" $server_path]] { assert_equal [s rdb_last_load_hash_fields_expired] 0 } - # in listpack encoding, the fileds (and key) will be expired by + # in listpack encoding, the fields (and key) will be expired by # lazy expiry assert_equal [r hgetall key] {} } @@ -571,7 +571,7 @@ set server_path [tmpdir "server.active-expiry-after-load"] # verifies a field is correctly expired by active expiry AFTER loading from RDB start_server [list overrides [list "dir" $server_path]] { - foreach type {listapck dict} { + foreach type {listpack dict} { test "active field expiry after load, ($type)" { if {$type eq "dict"} { r config set hash-max-listpack-entries 0 From a06570454664215a5e7ebe5d1ebc8139a48c5a92 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Wed, 15 May 2024 12:02:30 +0300 Subject: [PATCH 18/29] fix dict encoding initialization on RDB reading --- src/rdb.c | 14 +++++++------- src/server.h | 1 + src/t_hash.c | 14 +++++++++++++- tests/integration/rdb.tcl | 7 +++---- 4 files changed, 24 insertions(+), 12 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index 50fb4ef2e35f..386bdb7fa58e 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -2233,6 +2233,8 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, /* Too many entries? Use a hash table right from the start. */ if (len > server.hash_max_listpack_entries) { hashTypeConvert(o, OBJ_ENCODING_HT, hexpires); + dictTypeAddMeta((dict**)&o->ptr, &mstrHashDictTypeWithHFE); + initDictExpireMetadata(key, o); } else { hashTypeConvert(o, OBJ_ENCODING_LISTPACK_EX, hexpires); if (deep_integrity_validation) { @@ -2293,7 +2295,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, } /* keep the nearest expiration to connect listpack object to db expiry */ - if (expire < minExpire) minExpire = expire; + if ((expire != 0) && (expire < minExpire)) minExpire = expire; /* store the values read - either to listpack or dict */ if (o->encoding == OBJ_ENCODING_LISTPACK_EX) { @@ -2344,8 +2346,9 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, } if (o->encoding == OBJ_ENCODING_HT) { - /* WA for check-rdb mode, when there's no DB so can't attach expired items to ebuckets */ - if (db == NULL) { + /* WA for check-rdb mode, when there's no DB so can't attach expired items to ebuckets, + * or when no expiry was not set for this field */ + if ((db == NULL) || (expire == 0)) { hashTypeSet(db, o, field, value, 0); } else { if (hashTypeSetExRdb(db, o, field, value, expire) != C_OK) { @@ -2366,7 +2369,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, decrRefCount(o); goto emptykey; } - if ((db != NULL) && (o->encoding == OBJ_ENCODING_LISTPACK_EX)) + if ((db != NULL) && (minExpire != EB_EXPIRE_TIME_INVALID)) hashTypeAddToExpires(db, key, o, minExpire); } else if (rdbtype == RDB_TYPE_LIST_QUICKLIST || rdbtype == RDB_TYPE_LIST_QUICKLIST_2) { if ((len = rdbLoadLen(rdb,NULL)) == RDB_LENERR) return NULL; @@ -2700,9 +2703,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, (hashTypeLength(o, 0) > server.hash_max_listpack_entries)) /* TODO: each field length is not verified against server.hash_max_listpack_value */ { hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); - - /* TODO: should dict try expand be done here as well? */ - } else if (rdbtype == RDB_TYPE_HASH_LISTPACK_EX) { /* connect the listpack to the DB-global expiry data structure */ if ((minExpire != EB_EXPIRE_TIME_INVALID) && (db != NULL)) { /* DB can be NULL when checking rdb */ diff --git a/src/server.h b/src/server.h index 7d6fc4587980..1e3247a5f64e 100644 --- a/src/server.h +++ b/src/server.h @@ -3205,6 +3205,7 @@ void listpackExExpire(robj *o, ExpireInfo *info); int hashTypeSetExRdb(redisDb *db, robj *o, sds field, sds value, uint64_t expire_at); uint64_t hashTypeGetMinExpire(robj *keyObj); uint64_t hashTypeGetNextTimeToExpire(robj *o); +void initDictExpireMetadata(sds key, robj *o); /* Hash-Field data type (of t_hash.c) */ hfield hfieldNew(const void *field, size_t fieldlen, int withExpireMeta); diff --git a/src/t_hash.c b/src/t_hash.c index 65a66570c56f..1d40a2654565 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -1033,6 +1033,7 @@ SetExRes hashTypeSetEx(redisDb *db, robj *o, sds field, HashTypeSet *setKeyVal, /* If need to set value */ if (isSetKeyValue) { + redisDebug("going to set value %s for field %s", setKeyVal->value, field); if (flags & HASH_SET_TAKE_VALUE) { dictSetVal(ht, de, setKeyVal->value); flags &= ~HASH_SET_TAKE_VALUE; @@ -1055,6 +1056,8 @@ SetExRes hashTypeSetEx(redisDb *db, robj *o, sds field, HashTypeSet *setKeyVal, * It is the duty of RDB reading process to track minimal expiration time of the * fields and eventually call hashTypeAddToExpires() to update global HFE DS with * next expiration time. + * + * To just add a field with no expiry, use hashTypeSet instead. */ int hashTypeSetExRdb(redisDb *db, robj *o, sds field, sds value, uint64_t expire_at) { /* Dummy struct to be used in hashTypeSetEx() */ @@ -1074,6 +1077,15 @@ int hashTypeSetExRdb(redisDb *db, robj *o, sds field, sds value, uint64_t expire return (res == HSETEX_OK || res == HSET_UPDATE) ? C_OK : C_ERR; } +void initDictExpireMetadata(sds key, robj *o) +{ + dict *ht = o->ptr; + + dictExpireMetadata *m = (dictExpireMetadata *) dictMetadata(ht); + m->key = key; + m->hfe = ebCreate(); /* Allocate HFE DS */ + m->expireMeta.trash = 1; /* mark as trash (as long it wasn't ebAdd()) */ +} /* * Init HashTypeSetEx struct before calling hashTypeSetEx() @@ -1082,7 +1094,7 @@ int hashTypeSetExRdb(redisDb *db, robj *o, sds field, sds value, uint64_t expire * done by function hashTypeSetExDone(). * * NOTE: when calling from RDB reading process, the key is not yet available - * in the DB. Thereofre, it is not possible to retrieve a pointer to the key to + * in the DB. Therefore, it is not possible to retrieve a pointer to the key to * be used in the expire meta struct. * The expireMeta struct needs to hold a pointer to the key string that is * persistent for the key lifetime. When reading RDB, the key was already read diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index fca3c0e11f70..ea5876b169e3 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -570,8 +570,8 @@ test "save dict, load listpack" { set server_path [tmpdir "server.active-expiry-after-load"] # verifies a field is correctly expired by active expiry AFTER loading from RDB -start_server [list overrides [list "dir" $server_path]] { - foreach type {listpack dict} { +foreach type {listpack dict} { + start_server [list overrides [list "dir" $server_path enable-debug-command yes]] { test "active field expiry after load, ($type)" { if {$type eq "dict"} { r config set hash-max-listpack-entries 0 @@ -586,8 +586,7 @@ start_server [list overrides [list "dir" $server_path]] { r HPEXPIRE key 200 2 c d r save - restart_server 0 true false - wait_done_loading r + r debug reload # sleep 1 sec to make sure 'c' and 'd' will active-expire after 1000 From d1eb0c7278f78827b48ed7b158597892eb627d60 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Wed, 15 May 2024 12:35:04 +0300 Subject: [PATCH 19/29] added fields w/o expiry to active-expiry test --- tests/integration/rdb.tcl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index ea5876b169e3..ceec2a3b2098 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -581,7 +581,7 @@ foreach type {listpack dict} { r FLUSHALL - r HMSET key a 1 b 2 c 3 d 4 + r HMSET key a 1 b 2 c 3 d 4 e 5 f 6 r HEXPIREAT key 2524600800 2 a b r HPEXPIRE key 200 2 c d @@ -596,8 +596,8 @@ foreach type {listpack dict} { assert_equal [s expired_hash_fields] 2 # hgetall might lazy expire fields, so it's only called after the stat asserts - assert_equal [lsort [r hgetall key]] "1 2 a b" - assert_equal [r hexpiretime key 4 a b c d] {2524600800 2524600800 -2 -2} + assert_equal [lsort [r hgetall key]] "1 2 5 6 a b e f" + assert_equal [r hexpiretime key 6 a b c d e f] {2524600800 2524600800 -2 -2 -1 -1} } } } From ae2c2879a31287bf995af744295162a28e2f6b8e Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Wed, 15 May 2024 14:15:22 +0300 Subject: [PATCH 20/29] fix mem leak --- src/rdb.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/rdb.c b/src/rdb.c index 386bdb7fa58e..69e905a1a422 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -2359,6 +2359,8 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, return NULL; } } + sdsfree(field); + sdsfree(value); } } From daaad1d57d52e1769f67a64b52fbefd046e6d8c7 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Wed, 15 May 2024 15:01:38 +0300 Subject: [PATCH 21/29] more tests --- tests/integration/rdb.tcl | 68 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index ceec2a3b2098..16140af84814 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -602,4 +602,72 @@ foreach type {listpack dict} { } } +set server_path [tmpdir "server.lazy-expiry-after-load"] + +foreach type {listpack dict} { + start_server [list overrides [list "dir" $server_path enable-debug-command yes]] { + test "lazy field expiry after load, ($type)" { + if {$type eq "dict"} { + r config set hash-max-listpack-entries 0 + } else { + r config set hash-max-listpack-entries 512 + } + r debug set-active-expire 0 + + r FLUSHALL + + r HMSET key a 1 b 2 c 3 d 4 e 5 f 6 + r HEXPIREAT key 2524600800 2 a b + r HPEXPIRE key 200 2 c d + + r save + r debug reload + + # sleep 1 sec to make sure 'c' and 'd' will lazy-expire when calling hgetall + after 1000 + + assert_equal [s rdb_last_load_keys_loaded] 1 + assert_equal [s rdb_last_load_hash_fields_expired] 0 + assert_equal [s expired_hash_fields] 0 + + # hgetall will lazy expire fields, so it's only called after the stat asserts + assert_equal [lsort [r hgetall key]] "1 2 5 6 a b e f" + assert_equal [r hexpiretime key 6 a b c d e f] {2524600800 2524600800 -2 -2 -1 -1} + } + } +} + +set server_path [tmpdir "server.unexpired-items-rax-list-boundary"] + +foreach type {listpack dict} { + start_server [list overrides [list "dir" $server_path enable-debug-command yes]] { + test "load un-expired items below and above rax-list boundary, ($type)" { + + r flushall + + set hash_sizes {15 16 17 31 32 33} + foreach h $hash_sizes { + for {set i 1} {$i <= $h} {incr i} { + r hset key$h f$i v$i + r hexpireat key$h 2524600800 1 f$i + } + } + + r save + + restart_server 0 true false + wait_done_loading r + + set hash_sizes {15 16 17 31 32 33} + foreach h $hash_sizes { + for {set i 1} {$i <= $h} {incr i} { + # random expiration time + assert_equal [r hget key$h f$i] v$i + assert_equal [r hexpiretime key$h 1 f$i] 2524600800 + } + } + } + } +} + } ;# tags From 06a182dad48df72388d0afffa82003ecdccc9ce5 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Wed, 15 May 2024 16:30:28 +0300 Subject: [PATCH 22/29] HFE RDB de/serialization - PR comments #5 --- src/cluster.c | 2 +- src/rdb.c | 19 ++++++++--------- src/rdb.h | 2 +- src/redis-check-rdb.c | 6 +++--- src/server.h | 2 ++ src/t_hash.c | 25 +++++++++-------------- tests/integration/rdb.tcl | 43 +++++++++++---------------------------- 7 files changed, 38 insertions(+), 61 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index 6a786a2c731f..f0d25ea8f24c 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -237,7 +237,7 @@ void restoreCommand(client *c) { rioInitWithBuffer(&payload,c->argv[3]->ptr); if (((type = rdbLoadObjectType(&payload)) == -1) || - ((obj = rdbLoadObject(type,&payload,key->ptr,c->db,NULL, 0)) == NULL)) + ((obj = rdbLoadObject(type,&payload,key->ptr,c->db,0,NULL)) == NULL)) { addReplyError(c,"Bad data format"); return; diff --git a/src/rdb.c b/src/rdb.c index 69e905a1a422..817bae817a1f 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -1875,8 +1875,8 @@ int lpValidateIntegrityAndDups(unsigned char *lp, size_t size, int deep, int tup * On success a newly allocated object is returned, otherwise NULL. * When the function returns NULL and if 'error' is not NULL, the * integer pointed by 'error' is set to the type of error that occurred */ -robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, - int rdbflags) { +robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int rdbflags, + int *error) { robj *o = NULL, *ele, *dec; uint64_t len; unsigned int i; @@ -2251,6 +2251,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, /* read the field name */ if ((field = rdbGenericLoadStringObject(rdb, RDB_LOAD_SDS, &fieldLen)) == NULL) { + serverLog(LL_WARNING, "failed reading hash field"); decrRefCount(o); if (dupSearchDict != NULL) dictRelease(dupSearchDict); return NULL; @@ -2258,6 +2259,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, /* read the value */ if ((value = rdbGenericLoadStringObject(rdb,RDB_LOAD_SDS,NULL)) == NULL) { + serverLog(LL_WARNING, "failed reading hash value"); decrRefCount(o); if (dupSearchDict != NULL) dictRelease(dupSearchDict); sdsfree(field); @@ -2266,6 +2268,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, /* read the TTL */ if (rdbLoadLenByRef(rdb, NULL, &expire) == -1) { + serverLog(LL_WARNING, "failed reading hash TTL"); decrRefCount(o); if (dupSearchDict != NULL) dictRelease(dupSearchDict); sdsfree(value); @@ -2335,11 +2338,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, /* don't add the values to the new hash: the next if will catch and the values will be added there */ } else { - void *lp = ((listpackEx*)o->ptr)->lp; - lp = lpAppend(lp, (unsigned char *)field, sdslen(field)); - lp = lpAppend(lp, (unsigned char *)value, sdslen(value)); - lp = lpAppendInteger(lp, expire); - ((listpackEx*)o->ptr)->lp = lp; + listpackExAddNew(o, field, value, expire); sdsfree(field); sdsfree(value); } @@ -2352,6 +2351,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, hashTypeSet(db, o, field, value, 0); } else { if (hashTypeSetExRdb(db, o, field, value, expire) != C_OK) { + serverLog(LL_WARNING, "failed adding hash field %s to key %s", field, key); decrRefCount(o); if (dupSearchDict != NULL) dictRelease(dupSearchDict); sdsfree(value); @@ -2670,9 +2670,8 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, * pointed to by o->ptr */ o->type = OBJ_HASH; if (rdbtype == RDB_TYPE_HASH_LISTPACK_EX) { - listpackEx *lpt = zcalloc(sizeof(*lpt)); + listpackEx *lpt = listpackExCreate(); lpt->lp = encoded; - lpt->meta.trash = 1; lpt->key = key; o->ptr = lpt; o->encoding = OBJ_ENCODING_LISTPACK_EX; @@ -3511,7 +3510,7 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin if ((key = rdbGenericLoadStringObject(rdb,RDB_LOAD_SDS,NULL)) == NULL) goto eoferr; /* Read value */ - val = rdbLoadObject(type,rdb,key,db,&error,rdbflags); + val = rdbLoadObject(type,rdb,key,db,rdbflags,&error); /* Check if the key already expired. This function is used when loading * an RDB file from disk, either at startup, or when an RDB was diff --git a/src/rdb.h b/src/rdb.h index 9ec7c348f7e5..2f4c49954aa6 100644 --- a/src/rdb.h +++ b/src/rdb.h @@ -141,7 +141,7 @@ int rdbSaveToFile(const char *filename); int rdbSave(int req, char *filename, rdbSaveInfo *rsi, int rdbflags); ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid); size_t rdbSavedObjectLen(robj *o, robj *key, int dbid); -robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb *db, int *error, int rdbflags); +robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb *db, int rdbflags, int *error); void backgroundSaveDoneHandler(int exitcode, int bysignal); int rdbSaveKeyValuePair(rio *rdb, robj *key, robj *val, long long expiretime,int dbid); ssize_t rdbSaveSingleModuleAux(rio *rdb, int when, moduleType *mt); diff --git a/src/redis-check-rdb.c b/src/redis-check-rdb.c index a6d336e558d2..edadb7dd7c05 100644 --- a/src/redis-check-rdb.c +++ b/src/redis-check-rdb.c @@ -80,8 +80,8 @@ char *rdb_type_string[] = { "stream-v2", "set-listpack", "stream-v3", - "hash-md-hashtable", - "hash-md-listpack", + "hash-hashtable-md", + "hash-listpack-md", }; /* Show a few stats collected into 'rdbstate' */ @@ -331,7 +331,7 @@ int redis_check_rdb(char *rdbfilename, FILE *fp) { rdbstate.keys++; /* Read value */ rdbstate.doing = RDB_CHECK_DOING_READ_OBJECT_VALUE; - if ((val = rdbLoadObject(type,&rdb,key->ptr,NULL,NULL,0)) == NULL) goto eoferr; + if ((val = rdbLoadObject(type,&rdb,key->ptr,NULL,0,NULL)) == NULL) goto eoferr; /* Check if the key already expired. */ if (expiretime != -1 && expiretime < now) rdbstate.already_expired++; diff --git a/src/server.h b/src/server.h index 1e3247a5f64e..70a778c25326 100644 --- a/src/server.h +++ b/src/server.h @@ -3206,6 +3206,8 @@ int hashTypeSetExRdb(redisDb *db, robj *o, sds field, sds value, uint64_t expire uint64_t hashTypeGetMinExpire(robj *keyObj); uint64_t hashTypeGetNextTimeToExpire(robj *o); void initDictExpireMetadata(sds key, robj *o); +struct listpackEx *listpackExCreate(void); +void listpackExAddNew(robj *o, sds field, sds value, uint64_t expireAt); /* Hash-Field data type (of t_hash.c) */ hfield hfieldNew(const void *field, size_t fieldlen, int withExpireMeta); diff --git a/src/t_hash.c b/src/t_hash.c index 1d40a2654565..a5bbb0cff865 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -241,7 +241,7 @@ static SetExRes hashTypeSetExListpack(redisDb *db, robj *o, sds field, HashTypeS int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cmd, FieldSetCond fieldSetCond, FieldGet fieldGet, - ExpireSetCond expireSetCond, HashTypeSetEx *ex, int keyInDB); + ExpireSetCond expireSetCond, HashTypeSetEx *ex); SetExRes hashTypeSetEx(redisDb *db, robj *o, sds field, HashTypeSet *setKeyVal, uint64_t expireAt, HashTypeSetEx *exInfo); @@ -316,7 +316,7 @@ static void hashDictWithExpireOnRelease(dict *d) { #define HASH_LP_NO_TTL 0 -static struct listpackEx *listpackExCreate(void) { +struct listpackEx *listpackExCreate(void) { listpackEx *lpt = zcalloc(sizeof(*lpt)); lpt->meta.trash = 1; lpt->lp = NULL; @@ -540,7 +540,7 @@ static void listpackExUpdateExpiry(robj *o, sds field, } /* Add new field ordered by expire time. */ -static void listpackExAddNew(robj *o, sds field, sds value, uint64_t expireAt) { +void listpackExAddNew(robj *o, sds field, sds value, uint64_t expireAt) { unsigned char *fptr, *s, *elem; listpackEx *lpt = o->ptr; @@ -1077,8 +1077,7 @@ int hashTypeSetExRdb(redisDb *db, robj *o, sds field, sds value, uint64_t expire return (res == HSETEX_OK || res == HSET_UPDATE) ? C_OK : C_ERR; } -void initDictExpireMetadata(sds key, robj *o) -{ +void initDictExpireMetadata(sds key, robj *o) { dict *ht = o->ptr; dictExpireMetadata *m = (dictExpireMetadata *) dictMetadata(ht); @@ -1105,7 +1104,7 @@ void initDictExpireMetadata(sds key, robj *o) */ int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cmd, FieldSetCond fieldSetCond, FieldGet fieldGet, ExpireSetCond expireSetCond, - HashTypeSetEx *ex, int keyInDB) + HashTypeSetEx *ex) { dict *ht = o->ptr; @@ -1139,15 +1138,11 @@ int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cm /* Find the key in the keyspace. Need to keep reference to the key for * notifications or even removal of the hash */ - if (keyInDB) { - dictEntry *de = dbFind(db, key->ptr); - serverAssert(de != NULL); + dictEntry *de = dbFind(db, key->ptr); + serverAssert(de != NULL); - /* Fillup dict HFE metadata */ - m->key = dictGetKey(de); /* reference key in keyspace */ - } else { - m->key = key->ptr; - } + /* Fillup dict HFE metadata */ + m->key = dictGetKey(de); /* reference key in keyspace */ m->hfe = ebCreate(); /* Allocate HFE DS */ m->expireMeta.trash = 1; /* mark as trash (as long it wasn't ebAdd()) */ } @@ -2962,7 +2957,7 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime FIELD_DONT_CREATE2, FIELD_GET_NONE, expireSetCond, - &exCtx, 1); + &exCtx); addReplyArrayLen(c, numFields); for (int i = 0 ; i < numFields ; i++) { diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index 16140af84814..1efa25cdf80a 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -420,13 +420,9 @@ set server_path [tmpdir "server.partial-hfield-exp-test"] # verifies writing and reading hash key with expiring and persistent fields start_server [list overrides [list "dir" $server_path]] { - foreach type {listpack dict} { + foreach {type lp_entries} {listpack 512 dict 0} { test "hash field expiration save and load rdb one expired field, ($type)" { - if {$type eq "dict"} { - r config set hash-max-listpack-entries 0 - } else { - r config set hash-max-listpack-entries 512 - } + r config set hash-max-listpack-entries $lp_entries r FLUSHALL @@ -456,13 +452,9 @@ set server_path [tmpdir "server.all-hfield-exp-test"] # verifies writing hash with several expired keys, and active-expiring it on load start_server [list overrides [list "dir" $server_path]] { - foreach type {listpack dict} { + foreach {type lp_entries} {listpack 512 dict 0} { test "hash field expiration save and load rdb all fields expired, ($type)" { - if {$type eq "dict"} { - r config set hash-max-listpack-entries 0 - } else { - r config set hash-max-listpack-entries 512 - } + r config set hash-max-listpack-entries $lp_entries r FLUSHALL @@ -495,13 +487,9 @@ set server_path [tmpdir "server.long-ttl-test"] # verifies a long TTL value (6 bytes) is saved and loaded correctly start_server [list overrides [list "dir" $server_path]] { - foreach type {listpack dict} { + foreach {type lp_entries} {listpack 512 dict 0} { test "hash field expiration save and load rdb long TTL, ($type)" { - if {$type eq "dict"} { - r config set hash-max-listpack-entries 0 - } else { - r config set hash-max-listpack-entries 512 - } + r config set hash-max-listpack-entries $lp_entries r FLUSHALL @@ -570,14 +558,10 @@ test "save dict, load listpack" { set server_path [tmpdir "server.active-expiry-after-load"] # verifies a field is correctly expired by active expiry AFTER loading from RDB -foreach type {listpack dict} { +foreach {type lp_entries} {listpack 512 dict 0} { start_server [list overrides [list "dir" $server_path enable-debug-command yes]] { test "active field expiry after load, ($type)" { - if {$type eq "dict"} { - r config set hash-max-listpack-entries 0 - } else { - r config set hash-max-listpack-entries 512 - } + r config set hash-max-listpack-entries $lp_entries r FLUSHALL @@ -604,14 +588,10 @@ foreach type {listpack dict} { set server_path [tmpdir "server.lazy-expiry-after-load"] -foreach type {listpack dict} { +foreach {type lp_entries} {listpack 512 dict 0} { start_server [list overrides [list "dir" $server_path enable-debug-command yes]] { test "lazy field expiry after load, ($type)" { - if {$type eq "dict"} { - r config set hash-max-listpack-entries 0 - } else { - r config set hash-max-listpack-entries 512 - } + r config set hash-max-listpack-entries $lp_entries r debug set-active-expire 0 r FLUSHALL @@ -639,9 +619,10 @@ foreach type {listpack dict} { set server_path [tmpdir "server.unexpired-items-rax-list-boundary"] -foreach type {listpack dict} { +foreach {type lp_entries} {listpack 512 dict 0} { start_server [list overrides [list "dir" $server_path enable-debug-command yes]] { test "load un-expired items below and above rax-list boundary, ($type)" { + r config set hash-max-listpack-entries $lp_entries r flushall From 41015856f921d7fd20963896e9126a2d3146ab3a Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Wed, 15 May 2024 16:58:18 +0300 Subject: [PATCH 23/29] save dict-encoded hash with expiry in ttl-field-value order --- src/rdb.c | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index 817bae817a1f..7d9d1618a360 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -973,6 +973,18 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { hfield field = dictGetKey(de); sds value = dictGetVal(de); + /* save the TTL */ + if (with_ttl) { + uint64_t ttl = hfieldGetExpireTime(field); + /* 0 is used to indicate no TTL is set for this field */ + if (ttl == EB_EXPIRE_TIME_INVALID) ttl = 0; + if ((n = rdbSaveLen(rdb, ttl)) == -1) { + dictReleaseIterator(di); + return -1; + } + nwritten += n; + } + /* save the key */ if ((n = rdbSaveRawString(rdb,(unsigned char*)field, hfieldlen(field))) == -1) @@ -990,18 +1002,6 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { return -1; } nwritten += n; - - /* save the TTL */ - if (with_ttl) { - uint64_t ttl = hfieldGetExpireTime(field); - /* 0 is used to indicate no TTL is set for this field */ - if (ttl == EB_EXPIRE_TIME_INVALID) ttl = 0; - if ((n = rdbSaveLen(rdb, ttl)) == -1) { - dictReleaseIterator(di); - return -1; - } - nwritten += n; - } } dictReleaseIterator(di); } else { @@ -2249,6 +2249,16 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int rdbflags, while (len > 0) { len--; + /* read the TTL */ + if (rdbLoadLenByRef(rdb, NULL, &expire) == -1) { + serverLog(LL_WARNING, "failed reading hash TTL"); + decrRefCount(o); + if (dupSearchDict != NULL) dictRelease(dupSearchDict); + sdsfree(value); + sdsfree(field); + return NULL; + } + /* read the field name */ if ((field = rdbGenericLoadStringObject(rdb, RDB_LOAD_SDS, &fieldLen)) == NULL) { serverLog(LL_WARNING, "failed reading hash field"); @@ -2266,16 +2276,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int rdbflags, return NULL; } - /* read the TTL */ - if (rdbLoadLenByRef(rdb, NULL, &expire) == -1) { - serverLog(LL_WARNING, "failed reading hash TTL"); - decrRefCount(o); - if (dupSearchDict != NULL) dictRelease(dupSearchDict); - sdsfree(value); - sdsfree(field); - return NULL; - } - /* Check if the hash field already expired. This function is used when * loading an RDB file from disk, either at startup, or when an RDB was * received from the master. In the latter case, the master is From 2fd411247baa3ab29c4f320cbe5eed6c7ed60cfb Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Wed, 15 May 2024 17:24:55 +0300 Subject: [PATCH 24/29] fix freeing uninitialized values --- src/rdb.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index 7d9d1618a360..d4779f926f1e 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -2254,8 +2254,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int rdbflags, serverLog(LL_WARNING, "failed reading hash TTL"); decrRefCount(o); if (dupSearchDict != NULL) dictRelease(dupSearchDict); - sdsfree(value); - sdsfree(field); return NULL; } From 293565861abe780086ccd374a4c1fec67e2d6fe0 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Wed, 15 May 2024 18:52:11 +0300 Subject: [PATCH 25/29] HFE RDB de/serialization - PR comments #6 --- src/t_hash.c | 13 +------------ tests/integration/rdb.tcl | 39 ++++++++++++++++++++++++++------------- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/t_hash.c b/src/t_hash.c index a5bbb0cff865..cf4dce1bff9e 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -1033,7 +1033,6 @@ SetExRes hashTypeSetEx(redisDb *db, robj *o, sds field, HashTypeSet *setKeyVal, /* If need to set value */ if (isSetKeyValue) { - redisDebug("going to set value %s for field %s", setKeyVal->value, field); if (flags & HASH_SET_TAKE_VALUE) { dictSetVal(ht, de, setKeyVal->value); flags &= ~HASH_SET_TAKE_VALUE; @@ -1091,16 +1090,6 @@ void initDictExpireMetadata(sds key, robj *o) { * * Don't have to provide client and "cmd". If provided, then notification once * done by function hashTypeSetExDone(). - * - * NOTE: when calling from RDB reading process, the key is not yet available - * in the DB. Therefore, it is not possible to retrieve a pointer to the key to - * be used in the expire meta struct. - * The expireMeta struct needs to hold a pointer to the key string that is - * persistent for the key lifetime. When reading RDB, the key was already read - * and the string it was read into will be later used in the DB. However, - * if calling this function from any place else and using this flag, you'll need - * to provide a key string in the key robj that will remain persistent for as - * long as the key itself is. */ int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cmd, FieldSetCond fieldSetCond, FieldGet fieldGet, ExpireSetCond expireSetCond, @@ -1693,7 +1682,7 @@ void hashTypeConvertListpackEx(robj *o, int enc, ebuckets *hexpires) { } } -/* NOTE: hexpires might be NULL */ +/* NOTE: hexpires can be NULL (Won't attept to register in global HFE DS) */ void hashTypeConvert(robj *o, int enc, ebuckets *hexpires) { if (o->encoding == OBJ_ENCODING_LISTPACK) { hashTypeConvertListpack(o, enc); diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index 1efa25cdf80a..5758cea23f8a 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -431,14 +431,16 @@ start_server [list overrides [list "dir" $server_path]] { r HPEXPIRE key 100 1 d r save - # sleep 100 ms to make sure d will expire after restart - after 100 + # sleep 101 ms to make sure d will expire after restart + after 101 restart_server 0 true false wait_done_loading r assert_equal [lsort [r hgetall key]] "1 2 3 a b c" assert_equal [r hexpiretime key 3 a b c] {2524600800 2524600800 -1} assert_equal [s rdb_last_load_keys_loaded] 1 + # hash keys saved in listpack encoding are loaded as a blob, + # so individual field expiry is not verified on load if {$type eq "dict"} { assert_equal [s rdb_last_load_hash_fields_expired] 1 } else { @@ -462,12 +464,20 @@ start_server [list overrides [list "dir" $server_path]] { r HPEXPIRE key 100 4 a b c d r save - # sleep 100 ms to make sure all fields will expire after restart - after 100 + # sleep 101 ms to make sure all fields will expire after restart + after 101 restart_server 0 true false wait_done_loading r + # hash keys saved as listpack-encoded are saved and loaded as a blob + # so individual field validation is not checked during load. + # Therefore, if the key was saved as dict it is expected that + # all 4 fields were expired during load, and thus the key was + # "declared" an empty key. + # On the other hand, if the key was saved as listpack, it is + # expected that no field was expired on load and the key was loaded, + # even though all its fields are actually expired. if {$type eq "dict"} { assert_equal [s rdb_last_load_keys_loaded] 0 assert_equal [s rdb_last_load_hash_fields_expired] 4 @@ -519,8 +529,8 @@ test "save listpack, load dict" { assert_match "*encoding:listpack*" [r debug object key] r HPEXPIRE key 100 1 d - # sleep 100 ms to make sure all fields will expire after restart - after 100 + # sleep 101 ms to make sure all fields will expire after restart + after 101 # change configuration and reload - result should be dict-encoded key r config set hash-max-listpack-entries 0 @@ -543,8 +553,8 @@ test "save dict, load listpack" { assert_match "*encoding:hashtable*" [r debug object key] r HPEXPIRE key 100 1 d - # sleep 100 ms to make sure all fields will expire after restart - after 100 + # sleep 101 ms to make sure all fields will expire after restart + after 101 # change configuration and reload - result should be LP-encoded key r config set hash-max-listpack-entries 512 @@ -572,12 +582,15 @@ foreach {type lp_entries} {listpack 512 dict 0} { r save r debug reload - # sleep 1 sec to make sure 'c' and 'd' will active-expire - after 1000 + # wait at most 2 secs to make sure 'c' and 'd' will active-expire + wait_for_condition 20 100 { + [s expired_hash_fields] == 2 + } else { + fail "expired hash fields is [s expired_hash_fields] != 2" + } assert_equal [s rdb_last_load_keys_loaded] 1 assert_equal [s rdb_last_load_hash_fields_expired] 0 - assert_equal [s expired_hash_fields] 2 # hgetall might lazy expire fields, so it's only called after the stat asserts assert_equal [lsort [r hgetall key]] "1 2 5 6 a b e f" @@ -603,8 +616,8 @@ foreach {type lp_entries} {listpack 512 dict 0} { r save r debug reload - # sleep 1 sec to make sure 'c' and 'd' will lazy-expire when calling hgetall - after 1000 + # sleep 500 msec to make sure 'c' and 'd' will lazy-expire when calling hgetall + after 500 assert_equal [s rdb_last_load_keys_loaded] 1 assert_equal [s rdb_last_load_hash_fields_expired] 0 From 237d19c785b91fa045def01a605bacd52f963fc3 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Wed, 15 May 2024 18:54:56 +0300 Subject: [PATCH 26/29] typo --- src/t_hash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/t_hash.c b/src/t_hash.c index cf4dce1bff9e..e64d64c53534 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -1682,7 +1682,7 @@ void hashTypeConvertListpackEx(robj *o, int enc, ebuckets *hexpires) { } } -/* NOTE: hexpires can be NULL (Won't attept to register in global HFE DS) */ +/* NOTE: hexpires can be NULL (Won't attempt to register in global HFE DS) */ void hashTypeConvert(robj *o, int enc, ebuckets *hexpires) { if (o->encoding == OBJ_ENCODING_LISTPACK) { hashTypeConvertListpack(o, enc); From 53f24dff7cca454e008e6b05222e0b96568212a0 Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Thu, 16 May 2024 13:11:31 +0300 Subject: [PATCH 27/29] fix dict->listpack, listpack->dict tests --- src/rdb.c | 3 ++- tests/integration/rdb.tcl | 23 +++++++++++++++++------ 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index d4779f926f1e..ea581626fa51 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -2287,7 +2287,8 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int rdbflags, * created in memory (because it is created on the first valid field), and * thus the key would be discarded as an "empty key" */ if (expire != 0 && iAmMaster() && ((mstime_t)expire < now) && /* note: expire was saved to RDB as unix-time in milliseconds */ - !(rdbflags & RDBFLAGS_AOF_PREAMBLE)) { + !(rdbflags & RDBFLAGS_AOF_PREAMBLE)) + { /* TODO: consider replication (like in rdbLoadAddKeyToDb) */ server.rdb_last_load_hash_fields_expired++; sdsfree(field); diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index 5758cea23f8a..d2ce7165b0b9 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -528,15 +528,21 @@ test "save listpack, load dict" { r HMSET key a 1 b 2 c 3 d 4 assert_match "*encoding:listpack*" [r debug object key] r HPEXPIRE key 100 1 d + r save - # sleep 101 ms to make sure all fields will expire after restart - after 101 + # sleep 200 ms to make sure 'd' will expire after when reloading + after 200 # change configuration and reload - result should be dict-encoded key r config set hash-max-listpack-entries 0 r debug reload - assert_equal [lsort [r hgetall key]] "1 2 3 4 a b c d" + # first verify d was not expired during load (no expiry when loading + # a hash that was saved listpack-encoded) + assert_equal [s rdb_last_load_keys_loaded] 1 + assert_equal [s rdb_last_load_hash_fields_expired] 0 + + assert_equal [lsort [r hgetall key]] "1 2 3 a b c" assert_match "*encoding:hashtable*" [r debug object key] } } @@ -551,15 +557,20 @@ test "save dict, load listpack" { r HMSET key a 1 b 2 c 3 d 4 assert_match "*encoding:hashtable*" [r debug object key] - r HPEXPIRE key 100 1 d + r HPEXPIRE key 2000 1 d + r save - # sleep 101 ms to make sure all fields will expire after restart - after 101 + # sleep 2001 ms to make sure 'd' will expire during reload + after 2001 # change configuration and reload - result should be LP-encoded key r config set hash-max-listpack-entries 512 r debug reload + # verify d was expired during load + assert_equal [s rdb_last_load_keys_loaded] 1 + assert_equal [s rdb_last_load_hash_fields_expired] 1 + assert_equal [lsort [r hgetall key]] "1 2 3 a b c" assert_match "*encoding:listpack*" [r debug object key] } From 5eb92e05c7575ee09c82223711b78403b4e28d7e Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Thu, 16 May 2024 15:49:12 +0300 Subject: [PATCH 28/29] more test fixes and fixing listpack to dict conversion not added to global HFE DS --- src/rdb.c | 14 ++++++++++++++ tests/integration/rdb.tcl | 15 ++++++++------- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index ea581626fa51..10da6bef5a5f 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -2703,6 +2703,20 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int rdbflags, (hashTypeLength(o, 0) > server.hash_max_listpack_entries)) /* TODO: each field length is not verified against server.hash_max_listpack_value */ { hashTypeConvert(o, OBJ_ENCODING_HT, &db->hexpires); + /* + * hashTypeAddToExpires is presumably called from within + * the convert function (from listpackEx to dict), BUT, + * this call depends on the lpt->meta field to be updated, + * which is not the case here as hashTypeAddToExpires was + * not yet called for the listpack (which is what updating + * its meta). + * Instead, this "manual" call is added here. + * Another approach would be to have the conversion function + * find the minExpire by itself when iterating on the listpack + * instead of relying on the meta and use this value for the + * final ebAdd call. + */ + hashTypeAddToExpires(db, key, o, minExpire); } else if (rdbtype == RDB_TYPE_HASH_LISTPACK_EX) { /* connect the listpack to the DB-global expiry data structure */ if ((minExpire != EB_EXPIRE_TIME_INVALID) && (db != NULL)) { /* DB can be NULL when checking rdb */ diff --git a/tests/integration/rdb.tcl b/tests/integration/rdb.tcl index d2ce7165b0b9..b1bf3d2c5fa4 100644 --- a/tests/integration/rdb.tcl +++ b/tests/integration/rdb.tcl @@ -535,13 +535,14 @@ test "save listpack, load dict" { # change configuration and reload - result should be dict-encoded key r config set hash-max-listpack-entries 0 - r debug reload + r debug reload nosave # first verify d was not expired during load (no expiry when loading # a hash that was saved listpack-encoded) assert_equal [s rdb_last_load_keys_loaded] 1 assert_equal [s rdb_last_load_hash_fields_expired] 0 + # d should be lazy expired in hgetall assert_equal [lsort [r hgetall key]] "1 2 3 a b c" assert_match "*encoding:hashtable*" [r debug object key] } @@ -557,15 +558,15 @@ test "save dict, load listpack" { r HMSET key a 1 b 2 c 3 d 4 assert_match "*encoding:hashtable*" [r debug object key] - r HPEXPIRE key 2000 1 d + r HPEXPIRE key 200 1 d r save - # sleep 2001 ms to make sure 'd' will expire during reload - after 2001 + # sleep 201 ms to make sure 'd' will expire during reload + after 201 # change configuration and reload - result should be LP-encoded key r config set hash-max-listpack-entries 512 - r debug reload + r debug reload nosave # verify d was expired during load assert_equal [s rdb_last_load_keys_loaded] 1 @@ -591,7 +592,7 @@ foreach {type lp_entries} {listpack 512 dict 0} { r HPEXPIRE key 200 2 c d r save - r debug reload + r debug reload nosave # wait at most 2 secs to make sure 'c' and 'd' will active-expire wait_for_condition 20 100 { @@ -625,7 +626,7 @@ foreach {type lp_entries} {listpack 512 dict 0} { r HPEXPIRE key 200 2 c d r save - r debug reload + r debug reload nosave # sleep 500 msec to make sure 'c' and 'd' will lazy-expire when calling hgetall after 500 From a5280ca33c6faba3bd9f1c75d4f135cbe51e008f Mon Sep 17 00:00:00 2001 From: Ronen Kalish Date: Thu, 16 May 2024 17:54:37 +0300 Subject: [PATCH 29/29] HFE RDB de/serialization - PR comments #7 --- src/listpack.c | 1 + src/rdb.c | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/listpack.c b/src/listpack.c index 0346100dee01..390d8f320b7f 100644 --- a/src/listpack.c +++ b/src/listpack.c @@ -1209,6 +1209,7 @@ size_t lpBytes(unsigned char *lp) { return lpGetTotalBytes(lp); } +/* Returns the size 'lval' will require when encoded, in bytes */ size_t lpEntrySizeInteger(long long lval) { uint64_t enclen; lpEncodeIntegerGetType(lval, NULL, &enclen); diff --git a/src/rdb.c b/src/rdb.c index 10da6bef5a5f..b6b1190ce399 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -956,7 +956,7 @@ ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid) { dictEntry *de; /* Determine the hash layout to use based on the presence of at least * one field with a valid TTL. If such a field exists, employ the - * RDB_TYPE_HASH_METADATA layout, including tuples of [field][value][ttl]. + * RDB_TYPE_HASH_METADATA layout, including tuples of [ttl][field][value]. * Otherwise, use the standard RDB_TYPE_HASH layout containing only * the tuples [field][value]. */ int with_ttl = (hashTypeGetMinExpire(o) != EB_EXPIRE_TIME_INVALID); @@ -2229,6 +2229,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int rdbflags, len = rdbLoadLen(rdb, NULL); if (len == RDB_LENERR) return NULL; if (len == 0) goto emptykey; + /* TODO: create listpackEx or HT directly*/ o = createHashObject(); /* Too many entries? Use a hash table right from the start. */ if (len > server.hash_max_listpack_entries) {