Unverified Commit e18a173a authored by Moti Cohen's avatar Moti Cohen Committed by GitHub
Browse files

Fix rdbLoadObject() empty hash (#13347)

As part of HFE feature, the logic of rdbLoadObject() was wrongly
modified to indicate of loaded empty hash from RDB as hash that all its
fields got expired. Rollback to `emptykey` logic. This function should
load blindly all fields, expired or not. Manually verified.

Few more minor fixes:
- remove hash double check of emptyKey
- Fix from `sds` to `hfield` in rdbLoadObject() (not really a bug. Both
are of type char*)
- Revert code rdbLoadObject() to get dbid instead of db
parent 24c85cc3
...@@ -176,7 +176,6 @@ void dumpCommand(client *c) { ...@@ -176,7 +176,6 @@ void dumpCommand(client *c) {
/* RESTORE key ttl serialized-value [REPLACE] [ABSTTL] [IDLETIME seconds] [FREQ frequency] */ /* RESTORE key ttl serialized-value [REPLACE] [ABSTTL] [IDLETIME seconds] [FREQ frequency] */
void restoreCommand(client *c) { void restoreCommand(client *c) {
uint64_t minExpiredField = EB_EXPIRE_TIME_INVALID;
long long ttl, lfu_freq = -1, lru_idle = -1, lru_clock = -1; long long ttl, lfu_freq = -1, lru_idle = -1, lru_clock = -1;
rio payload; rio payload;
int j, type, replace = 0, absttl = 0; int j, type, replace = 0, absttl = 0;
...@@ -240,7 +239,7 @@ void restoreCommand(client *c) { ...@@ -240,7 +239,7 @@ void restoreCommand(client *c) {
rioInitWithBuffer(&payload,c->argv[3]->ptr); rioInitWithBuffer(&payload,c->argv[3]->ptr);
if (((type = rdbLoadObjectType(&payload)) == -1) || if (((type = rdbLoadObjectType(&payload)) == -1) ||
((obj = rdbLoadObject(type,&payload,key->ptr,c->db,NULL, &minExpiredField)) == NULL)) ((obj = rdbLoadObject(type,&payload,key->ptr,c->db->id,NULL)) == NULL))
{ {
addReplyError(c,"Bad data format"); addReplyError(c,"Bad data format");
return; return;
...@@ -270,8 +269,11 @@ void restoreCommand(client *c) { ...@@ -270,8 +269,11 @@ void restoreCommand(client *c) {
/* If minExpiredField was set, then the object is hash with expiration /* If minExpiredField was set, then the object is hash with expiration
* on fields and need to register it in global HFE DS */ * on fields and need to register it in global HFE DS */
if (minExpiredField != EB_EXPIRE_TIME_INVALID) if (obj->type == OBJ_HASH) {
hashTypeAddToExpires(c->db, dictGetKey(de), obj, minExpiredField); uint64_t minExpiredField = hashTypeGetNextTimeToExpire(obj);
if (minExpiredField != EB_EXPIRE_TIME_INVALID)
hashTypeAddToExpires(c->db, dictGetKey(de), obj, minExpiredField);
}
if (ttl) { if (ttl) {
setExpire(c,c->db,key,ttl); setExpire(c,c->db,key,ttl);
......
...@@ -1897,10 +1897,8 @@ int lpValidateIntegrityAndDups(unsigned char *lp, size_t size, int deep, int tup ...@@ -1897,10 +1897,8 @@ int lpValidateIntegrityAndDups(unsigned char *lp, size_t size, int deep, int tup
* no fields with expiration or it is not a hash, then it will set be to * no fields with expiration or it is not a hash, then it will set be to
* EB_EXPIRE_TIME_INVALID. * EB_EXPIRE_TIME_INVALID.
*/ */
robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error)
uint64_t *minExpiredField)
{ {
uint64_t minExpField = EB_EXPIRE_TIME_INVALID;
robj *o = NULL, *ele, *dec; robj *o = NULL, *ele, *dec;
uint64_t len; uint64_t len;
unsigned int i; unsigned int i;
...@@ -2241,8 +2239,8 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, ...@@ -2241,8 +2239,8 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error,
/* All pairs should be read by now */ /* All pairs should be read by now */
serverAssert(len == 0); serverAssert(len == 0);
} else if (rdbtype == RDB_TYPE_HASH_METADATA) { } else if (rdbtype == RDB_TYPE_HASH_METADATA) {
size_t fieldLen; sds value;
sds value, field; hfield field;
uint64_t expireAt; uint64_t expireAt;
dict *dupSearchDict = NULL; dict *dupSearchDict = NULL;
...@@ -2285,9 +2283,9 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, ...@@ -2285,9 +2283,9 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error,
/* if needed create field with TTL metadata */ /* if needed create field with TTL metadata */
if (expireAt !=0) if (expireAt !=0)
field = rdbGenericLoadStringObject(rdb, RDB_LOAD_HFLD_TTL, &fieldLen); field = rdbGenericLoadStringObject(rdb, RDB_LOAD_HFLD_TTL, NULL);
else else
field = rdbGenericLoadStringObject(rdb, RDB_LOAD_HFLD, &fieldLen); field = rdbGenericLoadStringObject(rdb, RDB_LOAD_HFLD, NULL);
if (field == NULL) { if (field == NULL) {
serverLog(LL_WARNING, "failed reading hash field"); serverLog(LL_WARNING, "failed reading hash field");
...@@ -2305,9 +2303,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, ...@@ -2305,9 +2303,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error,
return NULL; return NULL;
} }
/* keep the nearest expiration to connect listpack object to db expiry */
if ((expireAt != 0) && (expireAt < minExpField)) minExpField = expireAt;
/* store the values read - either to listpack or dict */ /* store the values read - either to listpack or dict */
if (o->encoding == OBJ_ENCODING_LISTPACK_EX) { if (o->encoding == OBJ_ENCODING_LISTPACK_EX) {
/* integrity - check for key duplication (if required) */ /* integrity - check for key duplication (if required) */
...@@ -2378,11 +2373,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, ...@@ -2378,11 +2373,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error,
if (dupSearchDict != NULL) dictRelease(dupSearchDict); if (dupSearchDict != NULL) dictRelease(dupSearchDict);
/* check for empty key (if all fields were expired) */
if (hashTypeLength(o, 0) == 0) {
decrRefCount(o);
goto expiredHash;
}
} else if (rdbtype == RDB_TYPE_LIST_QUICKLIST || rdbtype == RDB_TYPE_LIST_QUICKLIST_2) { } else if (rdbtype == RDB_TYPE_LIST_QUICKLIST || rdbtype == RDB_TYPE_LIST_QUICKLIST_2) {
if ((len = rdbLoadLen(rdb,NULL)) == RDB_LENERR) return NULL; if ((len = rdbLoadLen(rdb,NULL)) == RDB_LENERR) return NULL;
if (len == 0) goto emptykey; if (len == 0) goto emptykey;
...@@ -2706,9 +2696,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, ...@@ -2706,9 +2696,6 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error,
goto emptykey; goto emptykey;
} }
/* for TTL listpack, find the minimum expiry */
minExpField = hashTypeGetNextTimeToExpire(o);
/* Convert listpack to hash table without registering in global HFE DS, /* Convert listpack to hash table without registering in global HFE DS,
* if has HFEs, since the listpack is not connected yet to the DB */ * if has HFEs, since the listpack is not connected yet to the DB */
if (hashTypeLength(o, 0) > server.hash_max_listpack_entries) if (hashTypeLength(o, 0) > server.hash_max_listpack_entries)
...@@ -3053,13 +3040,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, ...@@ -3053,13 +3040,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error,
RedisModuleIO io; RedisModuleIO io;
robj keyobj; robj keyobj;
initStaticStringObject(keyobj,key); initStaticStringObject(keyobj,key);
/* shouldn't happen since db is NULL only in RDB check mode, and moduleInitIOContext(io,mt,rdb,&keyobj,dbid);
* 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 /* Call the rdb_load method of the module providing the 10 bit
* encoding version in the lower 10 bits of the module ID. */ * encoding version in the lower 10 bits of the module ID. */
void *ptr = mt->rdb_load(&io,moduleid&1023); void *ptr = mt->rdb_load(&io,moduleid&1023);
...@@ -3099,17 +3080,12 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error, ...@@ -3099,17 +3080,12 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, redisDb* db, int *error,
return NULL; return NULL;
} }
if (minExpiredField) *minExpiredField = minExpField;
if (error) *error = 0; if (error) *error = 0;
return o; return o;
emptykey: emptykey:
if (error) *error = RDB_LOAD_ERR_EMPTY_KEY; if (error) *error = RDB_LOAD_ERR_EMPTY_KEY;
return NULL; return NULL;
expiredHash:
if (error) *error = RDB_LOAD_ERR_EXPIRED_HASH;
return NULL;
} }
/* Mark that we are loading in the global state and setup the fields /* Mark that we are loading in the global state and setup the fields
...@@ -3279,7 +3255,6 @@ int rdbLoadRio(rio *rdb, int rdbflags, rdbSaveInfo *rsi) { ...@@ -3279,7 +3255,6 @@ int rdbLoadRio(rio *rdb, int rdbflags, rdbSaveInfo *rsi) {
* currently it only allow to set db object and functionLibCtx to which the data * currently it only allow to set db object and functionLibCtx to which the data
* will be loaded (in the future it might contains more such objects). */ * will be loaded (in the future it might contains more such objects). */
int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadingCtx *rdb_loading_ctx) { int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadingCtx *rdb_loading_ctx) {
uint64_t minExpiredField = EB_EXPIRE_TIME_INVALID;
uint64_t dbid = 0; uint64_t dbid = 0;
int type, rdbver; int type, rdbver;
uint64_t db_size = 0, expires_size = 0; uint64_t db_size = 0, expires_size = 0;
...@@ -3521,7 +3496,7 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin ...@@ -3521,7 +3496,7 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin
if ((key = rdbGenericLoadStringObject(rdb,RDB_LOAD_SDS,NULL)) == NULL) if ((key = rdbGenericLoadStringObject(rdb,RDB_LOAD_SDS,NULL)) == NULL)
goto eoferr; goto eoferr;
/* Read value */ /* Read value */
val = rdbLoadObject(type,rdb,key,db,&error, &minExpiredField); val = rdbLoadObject(type,rdb,key,db->id,&error);
/* Check if the key already expired. This function is used when loading /* 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 * an RDB file from disk, either at startup, or when an RDB was
...@@ -3540,9 +3515,6 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin ...@@ -3540,9 +3515,6 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin
if(empty_keys_skipped++ < 10) if(empty_keys_skipped++ < 10)
serverLog(LL_NOTICE, "rdbLoadObject skipping empty key: %s", key); serverLog(LL_NOTICE, "rdbLoadObject skipping empty key: %s", key);
sdsfree(key); sdsfree(key);
} else if (error == RDB_LOAD_ERR_EXPIRED_HASH) {
/* Valid flow. Continue. */
sdsfree(key);
} else { } else {
sdsfree(key); sdsfree(key);
goto eoferr; goto eoferr;
...@@ -3589,8 +3561,11 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin ...@@ -3589,8 +3561,11 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin
/* If minExpiredField was set, then the object is hash with expiration /* If minExpiredField was set, then the object is hash with expiration
* on fields and need to register it in global HFE DS */ * on fields and need to register it in global HFE DS */
if (minExpiredField != EB_EXPIRE_TIME_INVALID) if (val->type == OBJ_HASH) {
hashTypeAddToExpires(db, key, val, minExpiredField); uint64_t minExpiredField = hashTypeGetNextTimeToExpire(val);
if (minExpiredField != EB_EXPIRE_TIME_INVALID)
hashTypeAddToExpires(db, key, val, minExpiredField);
}
/* Set the expire time if needed */ /* Set the expire time if needed */
if (expiretime != -1) { if (expiretime != -1) {
......
...@@ -121,8 +121,7 @@ ...@@ -121,8 +121,7 @@
/* When rdbLoadObject() returns NULL, the err flag is /* When rdbLoadObject() returns NULL, the err flag is
* set to hold the type of error that occurred */ * set to hold the type of error that occurred */
#define RDB_LOAD_ERR_EMPTY_KEY 1 /* Error of empty key */ #define RDB_LOAD_ERR_EMPTY_KEY 1 /* Error of empty key */
#define RDB_LOAD_ERR_EXPIRED_HASH 2 /* Expired hash since all its fields are expired */ #define RDB_LOAD_ERR_OTHER 2 /* Any other errors */
#define RDB_LOAD_ERR_OTHER 3 /* Any other errors */
ssize_t rdbWriteRaw(rio *rdb, void *p, size_t len); ssize_t rdbWriteRaw(rio *rdb, void *p, size_t len);
int rdbSaveType(rio *rdb, unsigned char type); int rdbSaveType(rio *rdb, unsigned char type);
...@@ -143,7 +142,7 @@ int rdbSaveToFile(const char *filename); ...@@ -143,7 +142,7 @@ int rdbSaveToFile(const char *filename);
int rdbSave(int req, char *filename, rdbSaveInfo *rsi, int rdbflags); int rdbSave(int req, char *filename, rdbSaveInfo *rsi, int rdbflags);
ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid); ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key, int dbid);
size_t rdbSavedObjectLen(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, uint64_t *minExpiredField); robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error);
void backgroundSaveDoneHandler(int exitcode, int bysignal); void backgroundSaveDoneHandler(int exitcode, int bysignal);
int rdbSaveKeyValuePair(rio *rdb, robj *key, robj *val, long long expiretime,int dbid); int rdbSaveKeyValuePair(rio *rdb, robj *key, robj *val, long long expiretime,int dbid);
ssize_t rdbSaveSingleModuleAux(rio *rdb, int when, moduleType *mt); ssize_t rdbSaveSingleModuleAux(rio *rdb, int when, moduleType *mt);
......
...@@ -175,6 +175,7 @@ void rdbCheckSetupSignals(void) { ...@@ -175,6 +175,7 @@ void rdbCheckSetupSignals(void) {
* otherwise the already open file 'fp' is checked. */ * otherwise the already open file 'fp' is checked. */
int redis_check_rdb(char *rdbfilename, FILE *fp) { int redis_check_rdb(char *rdbfilename, FILE *fp) {
uint64_t dbid; uint64_t dbid;
int selected_dbid = -1;
int type, rdbver; int type, rdbver;
char buf[1024]; char buf[1024];
long long expiretime, now = mstime(); long long expiretime, now = mstime();
...@@ -246,6 +247,7 @@ int redis_check_rdb(char *rdbfilename, FILE *fp) { ...@@ -246,6 +247,7 @@ int redis_check_rdb(char *rdbfilename, FILE *fp) {
if ((dbid = rdbLoadLen(&rdb,NULL)) == RDB_LENERR) if ((dbid = rdbLoadLen(&rdb,NULL)) == RDB_LENERR)
goto eoferr; goto eoferr;
rdbCheckInfo("Selecting DB ID %llu", (unsigned long long)dbid); rdbCheckInfo("Selecting DB ID %llu", (unsigned long long)dbid);
selected_dbid = dbid;
continue; /* Read type again. */ continue; /* Read type again. */
} else if (type == RDB_OPCODE_RESIZEDB) { } else if (type == RDB_OPCODE_RESIZEDB) {
/* RESIZEDB: Hint about the size of the keys in the currently /* RESIZEDB: Hint about the size of the keys in the currently
...@@ -331,7 +333,7 @@ int redis_check_rdb(char *rdbfilename, FILE *fp) { ...@@ -331,7 +333,7 @@ int redis_check_rdb(char *rdbfilename, FILE *fp) {
rdbstate.keys++; rdbstate.keys++;
/* Read value */ /* Read value */
rdbstate.doing = RDB_CHECK_DOING_READ_OBJECT_VALUE; rdbstate.doing = RDB_CHECK_DOING_READ_OBJECT_VALUE;
if ((val = rdbLoadObject(type,&rdb,key->ptr,NULL,NULL,NULL)) == NULL) if ((val = rdbLoadObject(type,&rdb,key->ptr,selected_dbid,NULL)) == NULL)
goto eoferr; goto eoferr;
/* Check if the key already expired. */ /* Check if the key already expired. */
if (expiretime != -1 && expiretime < now) if (expiretime != -1 && expiretime < now)
......
...@@ -1853,7 +1853,9 @@ static ExpireAction hashTypeActiveExpire(eItem _hashObj, void *ctx) { ...@@ -1853,7 +1853,9 @@ static ExpireAction hashTypeActiveExpire(eItem _hashObj, void *ctx) {
/* Return the next/minimum expiry time of the hash-field. This is useful if a /* Return the next/minimum expiry time of the hash-field. This is useful if a
* field with the minimum expiry is deleted, and you want to get the next * field with the minimum expiry is deleted, and you want to get the next
* minimum expiry. Otherwise, consider using hashTypeGetMinExpire() which will * minimum expiry. Otherwise, consider using hashTypeGetMinExpire() which will
* be faster. If there is no field with expiry, returns EB_EXPIRE_TIME_INVALID */ * be faster but less accurate.
*
* Return next min expiry. If none return EB_EXPIRE_TIME_INVALID */
uint64_t hashTypeGetNextTimeToExpire(robj *o) { uint64_t hashTypeGetNextTimeToExpire(robj *o) {
if (o->encoding == OBJ_ENCODING_LISTPACK) { if (o->encoding == OBJ_ENCODING_LISTPACK) {
return EB_EXPIRE_TIME_INVALID; return EB_EXPIRE_TIME_INVALID;
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment