From 9a89e32a957ecdc09c25dccb808d13143ac6cd93 Mon Sep 17 00:00:00 2001 From: Moti Cohen Date: Thu, 12 Sep 2024 12:40:12 +0300 Subject: [PATCH] HFE - Fix key ref by the hash on RENAME/MOVE/SWAPDB/RESTORE (#13539) If the hash previously had HFEs (hash-fields with expiration) but later no longer does, the key ref in the hash might become outdated after a MOVE, COPY, RENAME or RESTORE operation. These commands maintain the key ref only if HFEs are present. That is, we can only be sure that key ref is valid as long as the hash has HFEs. --- src/t_hash.c | 26 +++++++++++ tests/unit/type/hash-field-expire.tcl | 65 +++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/src/t_hash.c b/src/t_hash.c index f06040e33..3b6b1c290 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -1134,6 +1134,20 @@ int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cm dictEntry *de = dbFind(c->db, key->ptr); serverAssert(de != NULL); lpt->key = dictGetKey(de); + } else if (o->encoding == OBJ_ENCODING_LISTPACK_EX) { + listpackEx *lpt = o->ptr; + + /* If the hash previously had HFEs but later no longer does, the key ref + * (lpt->key) in the hash might become outdated after a MOVE/COPY/RENAME/RESOTRE + * operation. These commands maintain the key ref only if HFEs are present. + * That is, we can only be sure that key ref is valid as long as it is not + * "trash". (TODO: dbFind() can be avoided. Instead need to extend the + * lookupKey*() to return dictEntry). */ + if (lpt->meta.trash) { + dictEntry *de = dbFind(c->db, key->ptr); + serverAssert(de != NULL); + lpt->key = dictGetKey(de); + } } else if (o->encoding == OBJ_ENCODING_HT) { /* Take care dict has HFE metadata */ if (!isDictWithMetaHFE(ht)) { @@ -1151,6 +1165,18 @@ int hashTypeSetExInit(robj *key, robj *o, client *c, redisDb *db, const char *cm 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()) */ + } else { + dictExpireMetadata *m = (dictExpireMetadata *) dictMetadata(ht); + /* If the hash previously had HFEs but later no longer does, the key ref + * (m->key) in the hash might become outdated after a MOVE/COPY/RENAME/RESTORE + * operation. These commands maintain the key ref only if HFEs are present. + * That is, we can only be sure that key ref is valid as long as it is not + * "trash". */ + if (m->expireMeta.trash) { + dictEntry *de = dbFind(db, key->ptr); + serverAssert(de != NULL); + m->key = dictGetKey(de); /* reference key in keyspace */ + } } } diff --git a/tests/unit/type/hash-field-expire.tcl b/tests/unit/type/hash-field-expire.tcl index d955384a9..edbb46edb 100644 --- a/tests/unit/type/hash-field-expire.tcl +++ b/tests/unit/type/hash-field-expire.tcl @@ -599,6 +599,20 @@ start_server {tags {"external:skip needs:debug"}} { wait_for_condition 30 10 { [r exists myhash2] == 0 } else { fail "`myhash2` should be expired" } } + test "Test RENAME hash that had HFEs but not during the rename ($type)" { + r del h1 + r hset h1 f1 v1 f2 v2 + r hpexpire h1 1 FIELDS 1 f1 + after 20 + r rename h1 h1_renamed + assert_equal [r exists h1] 0 + assert_equal [r exists h1_renamed] 1 + assert_equal [r hgetall h1_renamed] {f2 v2} + r hpexpire h1_renamed 1 FIELDS 1 f2 + # Only active expire will delete the key + wait_for_condition 30 10 { [r exists h1_renamed] == 0 } else { fail "`h1_renamed` should be expired" } + } + test "MOVE to another DB hash with fields to be expired ($type)" { r select 9 r flushall @@ -642,6 +656,20 @@ start_server {tags {"external:skip needs:debug"}} { } {} {singledb:skip} + test "Test COPY hash that had HFEs but not during the copy ($type)" { + r del h1 + r hset h1 f1 v1 f2 v2 + r hpexpire h1 1 FIELDS 1 f1 + after 20 + r COPY h1 h1_copy + assert_equal [r exists h1] 1 + assert_equal [r exists h1_copy] 1 + assert_equal [r hgetall h1_copy] {f2 v2} + r hpexpire h1_copy 1 FIELDS 1 f2 + # Only active expire will delete the key + wait_for_condition 30 10 { [r exists h1_copy] == 0 } else { fail "`h1_copy` should be expired" } + } + test "Test SWAPDB hash-fields to be expired ($type)" { r select 9 r flushall @@ -663,6 +691,29 @@ start_server {tags {"external:skip needs:debug"}} { wait_for_condition 20 10 { [r exists myhash] == 0 } else { fail "'myhash' should be expired" } } {} {singledb:skip} + test "Test SWAPDB hash that had HFEs but not during the swap ($type)" { + r select 9 + r flushall + r hset myhash f1 v1 f2 v2 + r hpexpire myhash 1 NX FIELDS 1 f1 + after 10 + + r swapdb 9 10 + + # Verify the key and its field doesn't exist in the source DB + assert_equal [r exists myhash] 0 + assert_equal [r dbsize] 0 + + # Verify the key and its field exists in the target DB + r select 10 + assert_equal [r hgetall myhash] {f2 v2} + assert_equal [r dbsize] 1 + r hpexpire myhash 1 NX FIELDS 1 f2 + + # Eventually the field will be expired and the key will be deleted + wait_for_condition 20 10 { [r exists myhash] == 0 } else { fail "'myhash' should be expired" } + } {} {singledb:skip} + test "HMGET - returns empty entries if fields or hash expired ($type)" { r debug set-active-expire 0 r del h1 h2 @@ -731,6 +782,20 @@ start_server {tags {"external:skip needs:debug"}} { assert_equal [r hexpiretime myhash FIELDS 3 a b c] {2524600800 2524600801 -1} } + test {RESTORE hash that had in the past HFEs but not during the dump} { + r config set sanitize-dump-payload yes + r del myhash + r hmset myhash a 1 b 2 c 3 + r hpexpire myhash 1 fields 1 a + after 10 + set encoded [r dump myhash] + r del myhash + r restore myhash 0 $encoded + assert_equal [lsort [r hgetall myhash]] "2 3 b c" + r hpexpire myhash 1 fields 2 b c + wait_for_condition 30 10 { [r exists myhash] == 0 } else { fail "`myhash` should be expired" } + } + test {DUMP / RESTORE are able to serialize / unserialize a hash with TTL 0 for all fields} { r config set sanitize-dump-payload yes r del myhash