From 806459f481cdd0c43aeb9ba37f16154414cf5ca9 Mon Sep 17 00:00:00 2001
From: Moti Cohen <moticless@gmail.com>
Date: Sun, 11 Aug 2024 16:39:03 +0300
Subject: [PATCH] On HDEL last field with expiry, update global HFE DS (#13470)

Hash field expiration is optimized to avoid frequent update global HFE DS for
each field deletion. Eventually active-expiration will run and update or remove
the hash from global HFE DS gracefully. Nevertheless, statistic "subexpiry"
might reflect wrong number of hashes with HFE to the user if HDEL deletes
the last field with expiration in hash (yet there are more fields without expiration).

Following this change, if HDEL the last field with expiration in the hash then
take care to remove the hash from global HFE DS as well.
---
 src/t_hash.c                          | 33 ++++++++++++++++--
 tests/unit/type/hash-field-expire.tcl | 49 ++++++++++++++++-----------
 2 files changed, 60 insertions(+), 22 deletions(-)

diff --git a/src/t_hash.c b/src/t_hash.c
index 4752568a3..66a3083c6 100644
--- a/src/t_hash.c
+++ b/src/t_hash.c
@@ -1985,6 +1985,21 @@ uint64_t hashTypeRemoveFromExpires(ebuckets *hexpires, robj *o) {
     return expireTime;
 }
 
+int hashTypeIsFieldsWithExpire(robj *o) {
+    if (o->encoding == OBJ_ENCODING_LISTPACK) {
+        return 0;
+    } else if (o->encoding == OBJ_ENCODING_LISTPACK_EX) {
+        return EB_EXPIRE_TIME_INVALID != listpackExGetMinExpire(o);
+    } else { /* o->encoding == OBJ_ENCODING_HT */
+        dict *d = o->ptr;
+        /* If dict doesn't holds HFE metadata */
+        if (!isDictWithMetaHFE(d))
+            return 0;
+        dictExpireMetadata *meta = (dictExpireMetadata *) dictMetadata(d);
+        return ebGetTotalItems(meta->hfe, &hashFieldExpireBucketsType) != 0;
+    }
+}
+
 /* Add hash to global HFE DS and update key for notifications.
  *
  * key         - must be the same key instance that is persisted in db->dict
@@ -2315,6 +2330,14 @@ void hdelCommand(client *c) {
     if ((o = lookupKeyWriteOrReply(c,c->argv[1],shared.czero)) == NULL ||
         checkType(c,o,OBJ_HASH)) return;
 
+    /* Hash field expiration is optimized to avoid frequent update global HFE DS for
+     * each field deletion. Eventually active-expiration will run and update or remove
+     * the hash from global HFE DS gracefully. Nevertheless, statistic "subexpiry"
+     * might reflect wrong number of hashes with HFE to the user if it is the last
+     * field with expiration. The following logic checks if this is indeed the last
+     * field with expiration and removes it from global HFE DS. */
+    int isHFE = hashTypeIsFieldsWithExpire(o);
+
     for (j = 2; j < c->argc; j++) {
         if (hashTypeDelete(o,c->argv[j]->ptr,1)) {
             deleted++;
@@ -2328,9 +2351,13 @@ void hdelCommand(client *c) {
     if (deleted) {
         signalModifiedKey(c,c->db,c->argv[1]);
         notifyKeyspaceEvent(NOTIFY_HASH,"hdel",c->argv[1],c->db->id);
-        if (keyremoved)
-            notifyKeyspaceEvent(NOTIFY_GENERIC,"del",c->argv[1],
-                                c->db->id);
+        if (keyremoved) {
+            notifyKeyspaceEvent(NOTIFY_GENERIC, "del", c->argv[1], c->db->id);
+        } else {
+            if (isHFE && (hashTypeIsFieldsWithExpire(o) == 0)) /* is it last HFE */
+                ebRemove(&c->db->hexpires, &hashExpireBucketsType, o);
+        }
+
         server.dirty += deleted;
     }
     addReplyLongLong(c,deleted);
diff --git a/tests/unit/type/hash-field-expire.tcl b/tests/unit/type/hash-field-expire.tcl
index d753ca8b8..2c0ff8abd 100644
--- a/tests/unit/type/hash-field-expire.tcl
+++ b/tests/unit/type/hash-field-expire.tcl
@@ -786,6 +786,36 @@ start_server {tags {"external:skip needs:debug"}} {
         }
     }
 
+    test "Statistics - Hashes with HFEs ($type)" {
+        r config resetstat
+        r flushall
+
+        # hash1: 5 fields, 3 with TTL. subexpiry incr +1
+        r hset myhash f1 v1 f2 v2 f3 v3 f4 v4 f5 v5
+        r hpexpire myhash 150 FIELDS 3 f1 f2 f3
+        assert_match  [get_stat_subexpiry r] 1
+
+        # hash2: 5 fields, 3 with TTL. subexpiry incr +1
+        r hset myhash2 f1 v1 f2 v2 f3 v3 f4 v4 f5 v5
+        assert_match  [get_stat_subexpiry r] 1
+        r hpexpire myhash2 100 FIELDS 3 f1 f2 f3
+        assert_match  [get_stat_subexpiry r] 2
+
+        # hash3: 2 fields, 1 with TTL. HDEL field with TTL. subexpiry decr -1
+        r hset myhash3 f1 v1 f2 v2
+        r hpexpire myhash3 100 FIELDS 1 f2
+        assert_match  [get_stat_subexpiry r] 3
+        r hdel myhash3 f2
+        assert_match  [get_stat_subexpiry r] 2
+
+        # Expired fields of hash1 and hash2. subexpiry decr -2
+        wait_for_condition 50 50 {
+                [get_stat_subexpiry r] == 0
+        } else {
+                fail "Hash field expiry statistics failed"
+        }
+    }
+
     r config set hash-max-listpack-entries 512
 }
 
@@ -899,25 +929,6 @@ start_server {tags {"external:skip needs:debug"}} {
 
         r config set hash-max-listpack-value 64
     }
-
-    test "Statistics - Hashes with HFEs" {
-        r config resetstat
-        r del myhash
-        r hset myhash f1 v1 f2 v2 f3 v3 f4 v4 f5 v5
-        r hpexpire myhash 100 FIELDS 3 f1 f2 f3
-
-        assert_match  [get_stat_subexpiry r] 1
-        r hset myhash2 f1 v1 f2 v2 f3 v3 f4 v4 f5 v5
-        assert_match  [get_stat_subexpiry r] 1
-        r hpexpire myhash2 100 FIELDS 3 f1 f2 f3
-        assert_match  [get_stat_subexpiry r] 2
-
-        wait_for_condition 50 50 {
-                [get_stat_subexpiry r] == 0
-        } else {
-                fail "Hash field expiry statistics failed"
-        }
-    }
 }
 
 start_server {tags {"external:skip needs:debug"}} {