diff --git a/HISTORY.md b/HISTORY.md index 76a99ea35..29955d31a 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -6,6 +6,7 @@ * Fixed a race related to the destruction of `ColumnFamilyData` objects. The earlier logic unlocked the DB mutex before destroying the thread-local `SuperVersion` pointers, which could result in a process crash if another thread managed to get a reference to the `ColumnFamilyData` object. * Removed a call to `RenameFile()` on a non-existent info log file ("LOG") when opening a new DB. Such a call was guaranteed to fail though did not impact applications since we swallowed the error. Now we also stopped swallowing errors in renaming "LOG" file. * Fixed an issue where `OnFlushCompleted` was not called for atomic flush. +* Fixed a bug affecting the batched `MultiGet` API when used with keys spanning multiple column families and `sorted_input == false`. ### New Features * Made the EventListener extend the Customizable class. diff --git a/db/db_basic_test.cc b/db/db_basic_test.cc index f2d9f9bf7..13644025b 100644 --- a/db/db_basic_test.cc +++ b/db/db_basic_test.cc @@ -1362,6 +1362,28 @@ TEST_P(DBMultiGetTestWithParam, MultiGetMultiCFSnapshot) { } } +TEST_P(DBMultiGetTestWithParam, MultiGetMultiCFUnsorted) { + Options options = CurrentOptions(); + CreateAndReopenWithCF({"one", "two"}, options); + + ASSERT_OK(Put(1, "foo", "bar")); + ASSERT_OK(Put(2, "baz", "xyz")); + ASSERT_OK(Put(1, "abc", "def")); + + // Note: keys for the same CF do not form a consecutive range + std::vector cfs{1, 2, 1}; + std::vector keys{"foo", "baz", "abc"}; + std::vector values; + + values = + MultiGet(cfs, keys, /* snapshot */ nullptr, /* batched */ GetParam()); + + ASSERT_EQ(values.size(), 3); + ASSERT_EQ(values[0], "bar"); + ASSERT_EQ(values[1], "xyz"); + ASSERT_EQ(values[2], "def"); +} + INSTANTIATE_TEST_CASE_P(DBMultiGetTestWithParam, DBMultiGetTestWithParam, testing::Bool()); diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index a65e50e4d..e3ca3f52d 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -2290,20 +2290,18 @@ void DBImpl::MultiGet(const ReadOptions& read_options, const size_t num_keys, multiget_cf_data; size_t cf_start = 0; ColumnFamilyHandle* cf = sorted_keys[0]->column_family; + for (size_t i = 0; i < num_keys; ++i) { KeyContext* key_ctx = sorted_keys[i]; if (key_ctx->column_family != cf) { - multiget_cf_data.emplace_back( - MultiGetColumnFamilyData(cf, cf_start, i - cf_start, nullptr)); + multiget_cf_data.emplace_back(cf, cf_start, i - cf_start, nullptr); cf_start = i; cf = key_ctx->column_family; } } - { - // multiget_cf_data.emplace_back( - // MultiGetColumnFamilyData(cf, cf_start, num_keys - cf_start, nullptr)); - multiget_cf_data.emplace_back(cf, cf_start, num_keys - cf_start, nullptr); - } + + multiget_cf_data.emplace_back(cf, cf_start, num_keys - cf_start, nullptr); + std::function::iterator&)> @@ -2363,7 +2361,7 @@ struct CompareKeyContext { static_cast(lhs->column_family); uint32_t cfd_id1 = cfh->cfd()->GetID(); const Comparator* comparator = cfh->cfd()->user_comparator(); - cfh = static_cast(lhs->column_family); + cfh = static_cast(rhs->column_family); uint32_t cfd_id2 = cfh->cfd()->GetID(); if (cfd_id1 < cfd_id2) { @@ -2387,39 +2385,24 @@ struct CompareKeyContext { void DBImpl::PrepareMultiGetKeys( size_t num_keys, bool sorted_input, autovector* sorted_keys) { -#ifndef NDEBUG if (sorted_input) { - for (size_t index = 0; index < sorted_keys->size(); ++index) { - if (index > 0) { - KeyContext* lhs = (*sorted_keys)[index - 1]; - KeyContext* rhs = (*sorted_keys)[index]; - ColumnFamilyHandleImpl* cfh = - static_cast_with_check(lhs->column_family); - uint32_t cfd_id1 = cfh->cfd()->GetID(); - const Comparator* comparator = cfh->cfd()->user_comparator(); - cfh = - static_cast_with_check(lhs->column_family); - uint32_t cfd_id2 = cfh->cfd()->GetID(); - - assert(cfd_id1 <= cfd_id2); - if (cfd_id1 < cfd_id2) { - continue; - } +#ifndef NDEBUG + CompareKeyContext key_context_less; - // Both keys are from the same column family - int cmp = comparator->CompareWithoutTimestamp( - *(lhs->key), /*a_has_ts=*/false, *(rhs->key), /*b_has_ts=*/false); - assert(cmp <= 0); - } - index++; + for (size_t index = 1; index < sorted_keys->size(); ++index) { + const KeyContext* const lhs = (*sorted_keys)[index - 1]; + const KeyContext* const rhs = (*sorted_keys)[index]; + + // lhs should be <= rhs, or in other words, rhs should NOT be < lhs + assert(!key_context_less(rhs, lhs)); } - } #endif - if (!sorted_input) { - CompareKeyContext sort_comparator; - std::sort(sorted_keys->begin(), sorted_keys->begin() + num_keys, - sort_comparator); + + return; } + + std::sort(sorted_keys->begin(), sorted_keys->begin() + num_keys, + CompareKeyContext()); } void DBImpl::MultiGet(const ReadOptions& read_options,