Fix resize hash tables stuck on the last non-empty slot (redis#12802)

Introduced in redis#11695 .

The tryResizeHashTables function gets stuck on the last non-empty slot
while iterating through dictionaries. It does not restart from the
beginning. The reason for this issue is a problem with the usage of
dbIteratorNextDict:

/* Returns next dictionary from the iterator, or NULL if iteration is complete. */
dict *dbIteratorNextDict(dbIterator *dbit) {
    if (dbit->next_slot == -1) return NULL;
    dbit->slot = dbit->next_slot;
    dbit->next_slot = dbGetNextNonEmptySlot(dbit->db, dbit->slot, dbit->keyType);
    return dbGetDictFromIterator(dbit);
}

When iterating to the last non-empty slot, next_slot is set to -1,
causing it to loop indefinitely on that slot. We need to modify the code
to ensure that after iterating to the last non-empty slot, it returns to
the first non-empty slot.

BTW, function tryResizeHashTables is actually iterating over slots
that have keys. However, in its implementation, it leverages the
dbIterator (which is a key iterator) to obtain slot and dictionary
information. While this approach works fine, but it is not very
intuitive. This PR also improves readability by changing the iteration
to directly iterate over slots, thereby enhancing clarity.

src/db.c

@@ -114,20 +114,7 @@ dbIterator *dbIteratorInit(redisDb *db, dbKeyType keyType) {
     return dbit;
 }
 
-/* Returns DB iterator that can be used to iterate through sub-dictionaries.
- *
- * The caller should free the resulting dbit with dbReleaseIterator. */
-dbIterator *dbIteratorInitFromSlot(redisDb *db, dbKeyType keyType, int slot) {
-    dbIterator *dbit = zmalloc(sizeof(*dbit));
-    dbit->db = db;
-    dbit->slot = slot;
-    dbit->keyType = keyType;
-    dbit->next_slot = dbGetNextNonEmptySlot(dbit->db, dbit->slot, dbit->keyType);
-    dictInitSafeIterator(&dbit->di, NULL);
-    return dbit;
-}
-
-/* Free the dbit returned by dbIteratorInit or dbIteratorInitFromSlot. */
+/* Free the dbit returned by dbIteratorInit. */
 void dbReleaseIterator(dbIterator *dbit) {
     dictIterator *iter = &dbit->di;
     dictResetIterator(iter);
@@ -687,7 +674,7 @@ long long emptyDbStructure(redisDb *dbarray, int dbnum, int async,
         dbarray[j].expires_cursor = 0;
         for (dbKeyType subdict = DB_MAIN; subdict <= DB_EXPIRES; subdict++) {
             dbarray[j].sub_dict[subdict].key_count = 0;
-            dbarray[j].sub_dict[subdict].resize_cursor = 0;
+            dbarray[j].sub_dict[subdict].resize_cursor = -1;
             if (server.cluster_enabled) {
                 if (dbarray[j].sub_dict[subdict].rehashing)
                     listEmpty(dbarray[j].sub_dict[subdict].rehashing);

src/server.c

@@ -660,20 +660,19 @@ int htNeedsResize(dict *dict) {
  * In non cluster-enabled setup, it resize main/expires dictionary based on the same condition described above. */
 void tryResizeHashTables(int dbid) {
     redisDb *db = &server.db[dbid];
-    int slot = 0;
     for (dbKeyType subdict = DB_MAIN; subdict <= DB_EXPIRES; subdict++) {
-        dbIterator *dbit = dbIteratorInitFromSlot(db, subdict, db->sub_dict[subdict].resize_cursor);
-        for (int i = 0; i < CRON_DBS_PER_CALL; i++) {
-            dict *d = dbGetDictFromIterator(dbit);
-            slot = dbIteratorGetCurrentSlot(dbit);
-            dbIteratorNextDict(dbit);
-            if (!d) break;
+        if (dbSize(db, subdict) == 0) continue;
+
+        if (db->sub_dict[subdict].resize_cursor == -1)
+            db->sub_dict[subdict].resize_cursor = findSlotByKeyIndex(db, 1, subdict);
+
+        for (int i = 0; i < CRON_DBS_PER_CALL && db->sub_dict[subdict].resize_cursor != -1; i++) {
+            int slot = db->sub_dict[subdict].resize_cursor;
+            dict *d = (subdict == DB_MAIN ? db->dict[slot] : db->expires[slot]);
             if (htNeedsResize(d))
                 dictResize(d);
+            db->sub_dict[subdict].resize_cursor = dbGetNextNonEmptySlot(db, slot, subdict);
         }
-        /* Save current iterator position in the resize_cursor. */
-        db->sub_dict[subdict].resize_cursor = slot;
-        dbReleaseIterator(dbit);
     }
 }
 
@@ -2656,7 +2655,7 @@ void initDbState(redisDb *db){
     for (dbKeyType subdict = DB_MAIN; subdict <= DB_EXPIRES; subdict++) {
         db->sub_dict[subdict].rehashing = listCreate();
         db->sub_dict[subdict].key_count = 0;
-        db->sub_dict[subdict].resize_cursor = 0;
+        db->sub_dict[subdict].resize_cursor = -1;
         db->sub_dict[subdict].slot_size_index = server.cluster_enabled ? zcalloc(sizeof(unsigned long long) * (CLUSTER_SLOTS + 1)) : NULL;
         db->sub_dict[subdict].bucket_count = 0;
     }

src/server.h

@@ -2436,7 +2436,6 @@ typedef struct dbIterator dbIterator;
 
 /* DB iterator specific functions */
 dbIterator *dbIteratorInit(redisDb *db, dbKeyType keyType);
-dbIterator *dbIteratorInitFromSlot(redisDb *db, dbKeyType keyType, int slot);
 void dbReleaseIterator(dbIterator *dbit);
 dict *dbIteratorNextDict(dbIterator *dbit);
 dict *dbGetDictFromIterator(dbIterator *dbit);

tests/unit/other.tcl

@@ -426,3 +426,49 @@ start_server {tags {"other external:skip"}} {
     }
 }
 
+start_cluster 1 0 {tags {"other external:skip cluster slow"}} {
+    test "Redis can trigger resizing" {
+        r flushall
+        # hashslot(foo) is 12182
+        for {set j 1} {$j <= 128} {incr j} {
+            r set "{foo}$j" a
+        }
+        assert_match "*table size: 128*" [r debug HTSTATS 0]
+
+        # disable resizing
+        r config set rdb-key-save-delay 10000000
+        r bgsave
+
+        # delete data to have lot's (99%) of empty buckets
+        for {set j 1} {$j <= 127} {incr j} {
+            r del "{foo}$j"
+        }
+        assert_match "*table size: 128*" [r debug HTSTATS 0]
+
+        # enable resizing
+        r config set rdb-key-save-delay 0
+        catch {exec kill -9 [get_child_pid 0]}
+        wait_for_condition 1000 10 {
+            [s rdb_bgsave_in_progress] eq 0
+        } else {
+            fail "bgsave did not stop in time."
+        }
+
+        after 200;# waiting for serverCron
+        assert_match "*table size: 4*" [r debug HTSTATS 0]
+    } {} {needs:debug}
+
+    test "Redis can rewind and trigger smaller slot resizing" {
+        # hashslot(alice) is 749, smaller than hashslot(foo),
+        # attempt to trigger a resize on it, see details in #12802.
+        for {set j 1} {$j <= 128} {incr j} {
+            r set "{alice}$j" a
+        }
+        for {set j 1} {$j <= 127} {incr j} {
+            r del "{alice}$j"
+        }
+
+        after 200;# waiting for serverCron
+        assert_match "*table size: 8*" [r debug HTSTATS 0]
+    } {} {needs:debug}
+}