Merge pull request #10834 from ClickHouse/fix-msan-failure-cache-dictionary

alexey-milovidov · web-flow · commit 6cba2dac3afd · 2020-05-12T17:58:40.000+03:00
Fix MSan failure in cache dictionary
diff --git a/src/Dictionaries/CacheDictionary.cpp b/src/Dictionaries/CacheDictionary.cpp
@@ -777,7 +777,7 @@ void CacheDictionary::updateThreadFunction()
 
         UpdateUnitPtr current_unit_ptr;
 
-        while (!update_request.empty() && update_queue.tryPop(current_unit_ptr))
+        while (update_request.size() < current_queue_size + 1 && update_queue.tryPop(current_unit_ptr))
             update_request.emplace_back(std::move(current_unit_ptr));
 
         BunchUpdateUnit bunch_update_unit(update_request);
@@ -817,7 +817,7 @@ void CacheDictionary::waitForCurrentUpdateFinish(UpdateUnitPtr & update_unit_ptr
     bool result = is_update_finished.wait_for(
             update_lock,
             std::chrono::milliseconds(timeout_for_wait),
-            [&] {return update_unit_ptr->is_done || update_unit_ptr->current_exception; });
+            [&] { return update_unit_ptr->is_done || update_unit_ptr->current_exception; });
 
     if (!result)
     {
@@ -936,7 +936,6 @@ void CacheDictionary::update(BunchUpdateUnit & bunch_update_unit) const
                     else
                         cell.setExpiresAt(std::chrono::time_point<std::chrono::system_clock>::max());
 
-
                     bunch_update_unit.informCallersAboutPresentId(id, cell_idx);
                     /// mark corresponding id as found
                     remaining_ids[id] = 1;
@@ -962,7 +961,8 @@ void CacheDictionary::update(BunchUpdateUnit & bunch_update_unit) const
         }
     }
 
-    size_t not_found_num = 0, found_num = 0;
+    size_t not_found_num = 0;
+    size_t found_num = 0;
 
     /// Check which ids have not been found and require setting null_value
     for (const auto & id_found_pair : remaining_ids)
diff --git a/src/Dictionaries/CacheDictionary.inc.h b/src/Dictionaries/CacheDictionary.inc.h
@@ -38,12 +38,16 @@ template <typename AttributeType, typename OutputType, typename DefaultGetter>
 void CacheDictionary::getItemsNumberImpl(
     Attribute & attribute, const PaddedPODArray<Key> & ids, ResultArrayType<OutputType> & out, DefaultGetter && get_default) const
 {
+    /// First fill everything with default values
+    const auto rows = ext::size(ids);
+    for (const auto row : ext::range(0, rows))
+        out[row] = get_default(row);
+
     /// Mapping: <id> -> { all indices `i` of `ids` such that `ids[i]` = <id> }
     std::unordered_map<Key, std::vector<size_t>> cache_expired_ids;
     std::unordered_map<Key, std::vector<size_t>> cache_not_found_ids;
 
     auto & attribute_array = std::get<ContainerPtrType<AttributeType>>(attribute.arrays);
-    const auto rows = ext::size(ids);
 
     size_t cache_hit = 0;
 
@@ -67,7 +71,8 @@ void CacheDictionary::getItemsNumberImpl(
             {
                 const auto & cell_idx = find_result.cell_idx;
                 const auto & cell = cells[cell_idx];
-                out[row] = cell.isDefault() ? get_default(row) : static_cast<OutputType>(attribute_array[cell_idx]);
+                if (!cell.isDefault())
+                    out[row] = static_cast<OutputType>(attribute_array[cell_idx]);
             };
 
             if (!find_result.valid)
@@ -154,14 +159,7 @@ void CacheDictionary::getItemsNumberImpl(
             out[row] = static_cast<OutputType>(attribute_value);
     };
 
-    auto on_id_not_found = [&] (const auto id, const auto)
-    {
-        for (const size_t row : cache_not_found_ids[id])
-            out[row] = get_default(row);
-
-        for (const size_t row : cache_expired_ids[id])
-            out[row] = get_default(row);
-    };
+    auto on_id_not_found = [&] (auto, auto) {};
 
     /// Request new values
     auto update_unit_ptr = std::make_shared<UpdateUnit>(required_ids, on_cell_updated, on_id_not_found);
