Rocksdb: RepairDB crashes if a column family contains only a DeleteRange tombstone

[simpkins]

Expected behavior

Create a new empty DB. Call DeleteRange() to delete some data in a column family (even though the DB is already empty), followed by CompactRange().

Then close the DB and call RepairDB(). RepairDB() should succeed.

Actual behavior

RepairDB() crashes with an assertion in debug mode builds.
In release builds (with assertions disable) RepairDB() logs a number of errors about corruption and fails.

Steps to reproduce the behavior

Unit test that reproduces the failure in f80b4009246a6c07118dc8cdafe04b1d175490a2

[simpkins]

The problem appears to be that the version_set.cc code has num_non_empty_levels_ set to 1, and tries to write out a file thinking it is non empty. However, files[0]->smallest and files[0]->largest are both empty since the level doesn't actually contain any real entries. This triggers an assertion failure.

Backtrace from the assertion:

rocksdb/src/db/dbformat.h:222: rocksdb::Slice rocksdb::InternalKey::Encode() const: Assertion `!rep_.empty()' failed.
    @ 00007f774fdf145f rocksdb::InternalKey::Encode() const
                       ./rocksdb/src/db/dbformat.h:222
                       -> ./rocksdb/src/table/block_based_table_reader.cc
    @ 00007f7750aad7eb rocksdb::DoGenerateLevelFilesBrief(rocksdb::LevelFilesBrief*, std::vector<rocksdb::FileMetaData*, std::allocator<rocksdb::FileMetaData*> > const&, rocksdb::Arena*)
                       ./rocksdb/src/db/version_set.cc:394
    @ 00007f7750ac25a7 rocksdb::VersionStorageInfo::GenerateLevelFilesBrief()
                       ./rocksdb/src/db/version_set.cc:1331
    @ 00007f7750ac29ac rocksdb::Version::PrepareApply(rocksdb::MutableCFOptions const&, bool)
                       ./rocksdb/src/db/version_set.cc:1344
    @ 00007f7750ae3a95 rocksdb::VersionSet::ProcessManifestWrites(std::deque<rocksdb::VersionSet::ManifestWriter, std::allocator<rocksdb::VersionSet::ManifestWriter> >&, rocksdb::InstrumentedMutex*, rocksdb::Directory*, bool, rocksdb::ColumnFamilyOptions const*)
                       ./rocksdb/src/db/version_set.cc:3055
    @ 00007f7750aee8dc rocksdb::VersionSet::LogAndApply(rocksdb::autovector<rocksdb::ColumnFamilyData*, 8ul> const&, rocksdb::autovector<rocksdb::MutableCFOptions const*, 8ul> const&, rocksdb::autovector<rocksdb::autovector<rocksdb::VersionEdit*, 8ul>, 8ul> const&, rocksdb::InstrumentedMutex*, rocksdb::Directory*, bool, rocksdb::ColumnFamilyOptions const*)
                       ./rocksdb/src/db/version_set.cc:3306
    @ 00007f775001a6f1 rocksdb::VersionSet::LogAndApply(rocksdb::ColumnFamilyData*, rocksdb::MutableCFOptions const&, rocksdb::VersionEdit*, rocksdb::InstrumentedMutex*, rocksdb::Directory*, bool, rocksdb::ColumnFamilyOptions const*)
                       ./rocksdb/src/db/version_set.h:770
                       -> ./rocksdb/src/db/compaction_job.cc
    @ 00007f77508aaab8 rocksdb::(anonymous namespace)::Repairer::AddTables()
                       ./rocksdb/src/db/repair.cc:595
    @ 00007f77508a1089 rocksdb::(anonymous namespace)::Repairer::Run()
                       ./rocksdb/src/db/repair.cc:206
    @ 00007f77508a1bb8 rocksdb::RepairDB(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::DBOptions const&, std::vector<rocksdb::ColumnFamilyDescriptor, std::allocator<rocksdb::ColumnFamilyDescriptor> > const&, rocksdb::ColumnFamilyOptions const&)
                       ./rocksdb/src/db/repair.cc:668

[miasantreble]

We can either 1) keep num_non_empty_levels_ at 0 since there isn't really anything in the level, or 2) add more checks in version_set before reading smallest_key or largest_key.
@ajkr do you mind comment on which one makes more sense?

[ajkr]

I think we need to store the smallest/largest keys in SST properties meta-block. Then we can get the correct ones during Repair. We aren't able to infer the proper smallest/largest keys by looking at the range tombstone meta-block due to how range tombstones are implicitly truncated by forging smallest/largest keys in the MANIFEST, but the truncation is not reflected in any way in the SST.

Hm, this strategy is really repair-unfriendly.

[ajkr]

Also, it looks like RepairDB doesn't guarantee files are added to L0 in the right order. Without that guarantee it seems dangerous to use this feature at all because newer key versions could be covered by older key versions. I wonder why we don't handle that or if I'm missing something.