From 8ed7d45a95471b3f1ca95a3d726aa779832b90b4 Mon Sep 17 00:00:00 2001
From: Yifan Hao <haoyifan@gmail.com>
Date: Wed, 24 Nov 2021 12:10:28 -0800
Subject: [PATCH] Hash table cleanup [7/n]

1. Add missing lock / unlock in profiling code. Without
   synchronization, the profiling code could access freed memory.
2. Remove unnecessary assert.
3. Remove unncessary if check in taosHashRemoveWithData.

* Testing
sample mysql:
taos> create database db;
Query OK, 0 of 0 row(s) in database (0.002291s)

taos> use db;
Query OK, 0 of 0 row(s) in database (0.000197s)

taos> create table t (ts timestamp, a int);
Query OK, 0 of 0 row(s) in database (0.003851s)

taos> insert into t values ('2019-07-15 00:00:00', 1);
Query OK, 1 of 1 row(s) in database (0.001128s)

taos> insert into t values ('2019-07-15 01:00:00', 2);
Query OK, 1 of 1 row(s) in database (0.000249s)

taos> select * from t;
           ts            |      a      |
========================================
 2019-07-15 00:00:00.000 |           1 |
 2019-07-15 01:00:00.000 |           2 |
Query OK, 2 row(s) in set (0.000772s)

taos> drop database db;
Query OK, 0 of 0 row(s) in database (0.002308s)
---
 src/util/inc/hash.h |  2 +-
 src/util/src/hash.c | 61 +++++++++++++++++++++------------------------
 2 files changed, 30 insertions(+), 33 deletions(-)

diff --git a/src/util/inc/hash.h b/src/util/inc/hash.h
index dea868f60e..4ada5e9516 100644
--- a/src/util/inc/hash.h
+++ b/src/util/inc/hash.h
@@ -173,7 +173,7 @@ void taosHashCleanup(SHashObj *pHashObj);
  * @param pHashObj   hash table object
  * @return           maximum number of collisions
  */
-int32_t taosHashGetMaxOverflowLinkLength(const SHashObj *pHashObj);
+int32_t taosHashGetMaxOverflowLinkLength(SHashObj *pHashObj);
 
 /**
  * return the consumed memory of the hash table
diff --git a/src/util/src/hash.c b/src/util/src/hash.c
index bc202e5f8b..9aa33b0999 100644
--- a/src/util/src/hash.c
+++ b/src/util/src/hash.c
@@ -515,30 +515,27 @@ int32_t taosHashRemoveWithData(SHashObj *pHashObj, const void *key, size_t keyLe
   while (pNode) {
     if ((pNode->keyLen == keyLen) &&
         ((*(pHashObj->equalFp))(GET_HASH_NODE_KEY(pNode), key, keyLen) == 0) &&
-        pNode->removed == 0)
-      break;
+        pNode->removed == 0) {
+      code = 0;  // it is found
 
-    prevNode = pNode;
-    pNode = pNode->next;
-  }
+      atomic_sub_fetch_32(&pNode->refCount, 1);
+      pNode->removed = 1;
+      if (pNode->refCount <= 0) {
+        if (prevNode == NULL) {
+          pe->next = pNode->next;
+        } else {
+          prevNode->next = pNode->next;
+        }
 
-  if (pNode) {
-    code = 0;  // it is found
+        if (data) memcpy(data, GET_HASH_NODE_DATA(pNode), dsize);
 
-    atomic_sub_fetch_32(&pNode->refCount, 1);
-    pNode->removed = 1;
-    if (pNode->refCount <= 0) {
-      if (prevNode) {
-        prevNode->next = pNode->next;
-      } else {
-        pe->next = pNode->next;
+        pe->num--;
+        atomic_sub_fetch_64(&pHashObj->size, 1);
+        FREE_HASH_NODE(pHashObj, pNode);
       }
-
-      if (data) memcpy(data, GET_HASH_NODE_DATA(pNode), dsize);
-
-      pe->num--;
-      atomic_sub_fetch_64(&pHashObj->size, 1);
-      FREE_HASH_NODE(pHashObj, pNode);
+    } else {
+      prevNode = pNode;
+      pNode = pNode->next;
     }
   }
 
@@ -649,24 +646,28 @@ void taosHashCleanup(SHashObj *pHashObj) {
 
   taosArrayDestroy(pHashObj->pMemBlock);
 
-  memset(pHashObj, 0, sizeof(SHashObj));
   free(pHashObj);
 }
 
 // for profile only
-int32_t taosHashGetMaxOverflowLinkLength(const SHashObj *pHashObj) {
+int32_t taosHashGetMaxOverflowLinkLength(SHashObj *pHashObj) {
   if (pHashObj == NULL || taosHashTableEmpty(pHashObj)) {
     return 0;
   }
 
   int32_t num = 0;
 
+  __rd_lock(&pHashObj->lock, pHashObj->type);
   for (int32_t i = 0; i < pHashObj->size; ++i) {
     SHashEntry *pEntry = pHashObj->hashList[i];
+
+    // fine grain per entry lock is not held since this is used
+    // for profiling only and doesn't need an accurate count.
     if (num < pEntry->num) {
       num = pEntry->num;
     }
   }
+  __rd_unlock(&pHashObj->lock, pHashObj->type);
 
   return num;
 }
@@ -682,15 +683,15 @@ void taosHashTableResize(SHashObj *pHashObj) {
 
   int32_t newCapacity = (int32_t)(pHashObj->capacity << 1u);
   if (newCapacity > HASH_MAX_CAPACITY) {
-    //    uDebug("current capacity:%d, maximum capacity:%d, no resize applied due to limitation is reached",
-    //           pHashObj->capacity, HASH_MAX_CAPACITY);
+    uDebug("current capacity:%lu, maximum capacity:%d, no resize applied due to limitation is reached",
+           pHashObj->capacity, HASH_MAX_CAPACITY);
     return;
   }
 
   int64_t st = taosGetTimestampUs();
   void *pNewEntryList = realloc(pHashObj->hashList, sizeof(void *) * newCapacity);
-  if (pNewEntryList == NULL) {  // todo handle error
-    //    uDebug("cache resize failed due to out of memory, capacity remain:%d", pHashObj->capacity);
+  if (pNewEntryList == NULL) {
+    uDebug("cache resize failed due to out of memory, capacity remain:%lu", pHashObj->capacity);
     return;
   }
 
@@ -709,17 +710,13 @@ void taosHashTableResize(SHashObj *pHashObj) {
   for (int32_t i = 0; i < pHashObj->capacity; ++i) {
     SHashEntry *pe = pHashObj->hashList[i];
 
-    if (pe->num == 0) {
-      assert(pe->next == NULL);
-    } else {
-      assert(pe->next != NULL);
-    }
-
     if (pe->num == 0) {
       assert(pe->next == NULL);
       continue;
     }
 
+    assert(pe->next != NULL);
+
     while ((pNode = pe->next) != NULL) {
       int32_t j = HASH_INDEX(pNode->hashVal, pHashObj->capacity);
       if (j != i) {
-- 
GitLab