From 732feca115e04849d771627cc726a4c593c722d5 Mon Sep 17 00:00:00 2001 From: Kamil Braun Date: Mon, 19 Jun 2023 16:34:57 +0200 Subject: [PATCH] storage_proxy: query_partition_key_range_concurrent: don't access empty range `query_partition_range_concurrent` implements an optimization when querying a token range that intersects multiple vnodes. Instead of sending a query for each vnode separately, it sometimes sends a single query to cover multiple vnodes - if the intersection of replica sets for those vnodes is large enough to satisfy the CL and good enough in terms of the heat metric. To check the latter condition, the code would take the smallest heat metric of the intersected replica set and compare them to smallest heat metrics of replica sets calculated separately for each vnode. Unfortunately, there was an edge case that the code didn't handle: the intersected replica set might be empty and the code would access an empty range. This was catched by an assertion added in 8db1d75c6c63c1db6b2ee773041370cc768c67df by the dtest `test_query_dc_with_rf_0_does_not_crash_db`. The fix is simple: check if the intersected set is empty - if so, don't calculate the heat metrics because we can decide early that the optimization doesn't apply. Also change the `assert` to `on_internal_error`. Fixes #14284 Closes #14300 --- service/storage_proxy.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/service/storage_proxy.cc b/service/storage_proxy.cc index 41c6b6349aaa..42b805e14a99 100644 --- a/service/storage_proxy.cc +++ b/service/storage_proxy.cc @@ -5586,7 +5586,9 @@ storage_proxy::query_partition_key_range_concurrent(storage_proxy::clock_type::t inet_address_vector_replica_set filtered_merged = filter_replicas_for_read(cl, *erm, merged, current_merged_preferred_replicas, pcf); // Estimate whether merging will be a win or not - if (!is_worth_merging_for_range_query(erm->get_topology(), filtered_merged, filtered_endpoints, next_filtered_endpoints)) { + if (filtered_merged.empty() + || !is_worth_merging_for_range_query( + erm->get_topology(), filtered_merged, filtered_endpoints, next_filtered_endpoints)) { break; } else if (pcf) { // check that merged set hit rate is not to low @@ -5606,7 +5608,9 @@ storage_proxy::query_partition_key_range_concurrent(storage_proxy::clock_type::t } } ep_to_hr{remote().gossiper(), pcf}; - assert (!range.empty()); + if (range.empty()) { + on_internal_error(slogger, "empty range passed to `find_min`"); + } return *boost::range::min_element(range | boost::adaptors::transformed(ep_to_hr)); }; auto merged = find_min(filtered_merged) * 1.2; // give merged set 20% boost