From 53b703eafef16c20bad64171f1dcca8cc98eefa0 Mon Sep 17 00:00:00 2001 From: Changyu Bi Date: Mon, 19 Dec 2022 16:36:39 -0800 Subject: [PATCH] Fix an assertion failure in `CompactionOutputs::AddRangeDels()` (#11040) Summary: the [assertion](https://github.com/facebook/rocksdb/blob/c3f720c60db59c27486d8f18e094f9d1eb3c33cf/db/compaction/compaction_outputs.cc#L643) in `CompactionOutputs::AddRangeDels()` can fail after https://github.com/facebook/rocksdb/pull/10802. The assertion fails when `lower_bound_from_range_tombstone` is true during `AddRangeDels()` for a new compaction output file, while the lower bound range tombstone key has seqno 0 and op_type kTypeRangeDeletion. It can have seqno 0 when it was truncated at a point key whose seqno was zeroed out during compaction, the seqno and op_type could be set [here](https://github.com/facebook/rocksdb/blob/c3f720c60db59c27486d8f18e094f9d1eb3c33cf/db/compaction/compaction_outputs.cc#L594). This PR fixes the assertion excluding the case when `lower_bound_from_range_tombstone` is true. Pull Request resolved: https://github.com/facebook/rocksdb/pull/11040 Test Plan: CI Reviewed By: ajkr Differential Revision: D42119914 Pulled By: cbi42 fbshipit-source-id: 0897e71b5304cb02aac30f71667b590c37b72baf --- db/compaction/compaction_outputs.cc | 34 ++++++++++++++--------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/db/compaction/compaction_outputs.cc b/db/compaction/compaction_outputs.cc index bbb83961c..072221586 100644 --- a/db/compaction/compaction_outputs.cc +++ b/db/compaction/compaction_outputs.cc @@ -532,23 +532,18 @@ Status CompactionOutputs::AddRangeDels( // Pretend the smallest key has the same user key as lower_bound // (the max key in the previous table or subcompaction) in order for // files to appear key-space partitioned. - // - // When lower_bound is chosen by a subcompaction, we know that - // subcompactions over smaller keys cannot contain any keys at - // lower_bound. We also know that smaller subcompactions exist, - // because otherwise the subcompaction woud be unbounded on the left. - // As a result, we know that no other files on the output level will - // contain actual keys at lower_bound (an output file may have a - // largest key of lower_bound@kMaxSequenceNumber, but this only - // indicates a large range tombstone was truncated). Therefore, it is - // safe to use the tombstone's sequence number, to ensure that keys at - // lower_bound at lower levels are covered by truncated tombstones. - // - // If lower_bound was chosen by the smallest data key in the file, - // choose lowest seqnum so this file's smallest internal key comes - // after the previous file's largest. The fake seqnum is OK because - // the read path's file-picking code only considers user key. if (lower_bound_from_sub_compact) { + // When lower_bound is chosen by a subcompaction + // (lower_bound_from_sub_compact), we know that subcompactions over + // smaller keys cannot contain any keys at lower_bound. We also know + // that smaller subcompactions exist, because otherwise the + // subcompaction woud be unbounded on the left. As a result, we know + // that no other files on the output level will contain actual keys at + // lower_bound (an output file may have a largest key of + // lower_bound@kMaxSequenceNumber, but this only indicates a large range + // tombstone was truncated). Therefore, it is safe to use the + // tombstone's sequence number, to ensure that keys at lower_bound at + // lower levels are covered by truncated tombstones. if (ts_sz) { assert(tombstone.ts_.size() == ts_sz); smallest_candidate = InternalKey(*lower_bound, tombstone.seq_, @@ -558,6 +553,7 @@ Status CompactionOutputs::AddRangeDels( InternalKey(*lower_bound, tombstone.seq_, kTypeRangeDeletion); } } else if (lower_bound_from_range_tombstone) { + // When lower_bound is chosen from a range tombtone start key: // Range tombstone keys can be truncated at file boundaries of the files // that contain them. // @@ -591,6 +587,10 @@ Status CompactionOutputs::AddRangeDels( smallest_candidate = range_tombstone_lower_bound_; } } else { + // If lower_bound was chosen by the smallest data key in the file, + // choose lowest seqnum so this file's smallest internal key comes + // after the previous file's largest. The fake seqnum is OK because + // the read path's file-picking code only considers user key. smallest_candidate = InternalKey(*lower_bound, 0, kTypeRangeDeletion); } } @@ -640,7 +640,7 @@ Status CompactionOutputs::AddRangeDels( // it cannot have a seqnum of 0 (unless the smallest data key in a file // has a seqnum of 0). Otherwise, the truncated tombstone may expose // deleted keys at lower levels. - assert(smallest_ikey_seqnum == 0 || + assert(smallest_ikey_seqnum == 0 || lower_bound_from_range_tombstone || ExtractInternalKeyFooter(meta.smallest.Encode()) != PackSequenceAndType(0, kTypeRangeDeletion)); }