Fixing snapshot 0 assertion

Summary:
Solution is not to change db sequence number to start from 1 because 0 value is used in multiple other places.
Fix covers only compact_iterator::findEarliestVisibleSnapshot with updated logic to support snapshot's numbering starting from 0.

Test Plan:
run:
  make all check

it should pass all tests

Reviewers: IslamAbdelRahman, sdong

Reviewed By: sdong

Subscribers: lgalanis, mgalushka, andrewkr, dhruba

Differential Revision: https://reviews.facebook.net/D56601
main
Victor Tyutyunov 9 years ago
parent 6d436a3f85
commit e5c614e1df
  1. 8
      db/compaction_iterator.cc
  2. 24
      db/db_test2.cc

@ -423,15 +423,15 @@ void CompactionIterator::PrepareOutput() {
inline SequenceNumber CompactionIterator::findEarliestVisibleSnapshot( inline SequenceNumber CompactionIterator::findEarliestVisibleSnapshot(
SequenceNumber in, SequenceNumber* prev_snapshot) { SequenceNumber in, SequenceNumber* prev_snapshot) {
assert(snapshots_->size()); assert(snapshots_->size());
SequenceNumber prev __attribute__((unused)) = 0; SequenceNumber prev __attribute__((__unused__)) = kMaxSequenceNumber;
for (const auto cur : *snapshots_) { for (const auto cur : *snapshots_) {
assert(prev <= cur); assert(prev == kMaxSequenceNumber || prev <= cur);
if (cur >= in) { if (cur >= in) {
*prev_snapshot = prev; *prev_snapshot = prev == kMaxSequenceNumber ? 0 : prev;
return cur; return cur;
} }
prev = cur; prev = cur;
assert(prev); assert(prev < kMaxSequenceNumber);
} }
*prev_snapshot = prev; *prev_snapshot = prev;
return kMaxSequenceNumber; return kMaxSequenceNumber;

@ -452,8 +452,7 @@ TEST_F(DBTest2, WalFilterTestWithChangeBatch) {
for (size_t i = 0; i < batch_keys.size(); i++) { for (size_t i = 0; i < batch_keys.size(); i++) {
for (size_t j = 0; j < batch_keys[i].size(); j++) { for (size_t j = 0; j < batch_keys[i].size(); j++) {
if (i >= change_records_from_index && j >= if (i >= change_records_from_index && j >= num_keys_to_add_in_new_batch) {
num_keys_to_add_in_new_batch) {
keys_must_not_exist.push_back(Slice(batch_keys[i][j])); keys_must_not_exist.push_back(Slice(batch_keys[i][j]));
} }
else { else {
@ -528,8 +527,7 @@ TEST_F(DBTest2, WalFilterTestWithChangeBatchExtraKeys) {
// Reopen database with option to use WAL filter // Reopen database with option to use WAL filter
options = OptionsForLogIterTest(); options = OptionsForLogIterTest();
options.wal_filter = &test_wal_filter_extra_keys; options.wal_filter = &test_wal_filter_extra_keys;
Status status = TryReopenWithColumnFamilies({ "default", "pikachu" }, Status status = TryReopenWithColumnFamilies({"default", "pikachu"}, options);
options);
ASSERT_TRUE(status.IsNotSupported()); ASSERT_TRUE(status.IsNotSupported());
// Reopen without filter, now reopen should succeed - previous // Reopen without filter, now reopen should succeed - previous
@ -612,8 +610,7 @@ TEST_F(DBTest2, WalFilterTestWithColumnFamilies) {
return "WalFilterTestWithColumnFamilies"; return "WalFilterTestWithColumnFamilies";
} }
const std::map<uint32_t, std::vector<std::string>> & const std::map<uint32_t, std::vector<std::string>>& GetColumnFamilyKeys() {
GetColumnFamilyKeys() {
return cf_wal_keys_; return cf_wal_keys_;
} }
@ -684,8 +681,7 @@ TEST_F(DBTest2, WalFilterTestWithColumnFamilies) {
// verify that handles_[0] only has post_flush keys // verify that handles_[0] only has post_flush keys
// while handles_[1] has pre and post flush keys // while handles_[1] has pre and post flush keys
auto cf_wal_keys = test_wal_filter_column_families.GetColumnFamilyKeys(); auto cf_wal_keys = test_wal_filter_column_families.GetColumnFamilyKeys();
auto name_id_map = auto name_id_map = test_wal_filter_column_families.GetColumnFamilyNameIdMap();
test_wal_filter_column_families.GetColumnFamilyNameIdMap();
size_t index = 0; size_t index = 0;
auto keys_cf = cf_wal_keys[name_id_map[kDefaultColumnFamilyName]]; auto keys_cf = cf_wal_keys[name_id_map[kDefaultColumnFamilyName]];
//default column-family, only post_flush keys are expected //default column-family, only post_flush keys are expected
@ -720,21 +716,13 @@ TEST_F(DBTest2, WalFilterTestWithColumnFamilies) {
} }
#endif // ROCKSDB_LITE #endif // ROCKSDB_LITE
TEST_F(DBTest2, DISABLED_FirstSnapshotTest) { TEST_F(DBTest2, FirstSnapshotTest) {
Options options; Options options;
options.write_buffer_size = 100000; // Small write buffer options.write_buffer_size = 100000; // Small write buffer
options = CurrentOptions(options); options = CurrentOptions(options);
CreateAndReopenWithCF({"pikachu"}, options); CreateAndReopenWithCF({"pikachu"}, options);
// This snapshot will have sequence number 0. When compaction encounters // This snapshot will have sequence number 0 what is expected behaviour.
// this snapshot, CompactionIterator::findEarliestVisibleSnapshot() will
// assert as it expects non-zero snapshots.
//
// One fix would be to simply remove this assert. However, a better fix
// might
// be to always have db sequence numbers start from 1 so that no code is
// ever
// confused by 0.
const Snapshot* s1 = db_->GetSnapshot(); const Snapshot* s1 = db_->GetSnapshot();
Put(1, "k1", std::string(100000, 'x')); // Fill memtable Put(1, "k1", std::string(100000, 'x')); // Fill memtable

Loading…
Cancel
Save