From c43a37a9228cd389c215f0e39366ec2df7f2d48a Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Thu, 1 Apr 2021 05:07:19 -0700 Subject: [PATCH] Fix compression dictionary sampling with dedicated range tombstone SSTs (#8141) Summary: Return early in case there are zero data blocks when `BlockBasedTableBuilder::EnterUnbuffered()` is called. This crash can only be triggered by applying dictionary compression to SST files that contain only range tombstones. It cannot be triggered by a low buffer limit alone since we only consider entering unbuffered mode after buffering a data block causing the limit to be breached, or `Finish()`ing the file. It also cannot be triggered by a totally empty file because those go through `Abandon()` rather than `Finish()` so unbuffered mode is never entered. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8141 Test Plan: added a unit test that repro'd the "Floating point exception" Reviewed By: riversand963 Differential Revision: D27495640 Pulled By: ajkr fbshipit-source-id: a463cfba476919dc5c5c380800a75a86c31ffa23 --- HISTORY.md | 1 + db/db_range_del_test.cc | 9 +++++++++ table/block_based/block_based_table_builder.cc | 5 +++++ 3 files changed, 15 insertions(+) diff --git a/HISTORY.md b/HISTORY.md index 0365d1474..6cb04715a 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -8,6 +8,7 @@ * Use thread-safe `strerror_r()` to get error messages. * Fixed a potential hang in shutdown for a DB whose `Env` has high-pri thread pool disabled (`Env::GetBackgroundThreads(Env::Priority::HIGH) == 0`) * Made BackupEngine thread-safe and added documentation comments to clarify what is safe for multiple BackupEngine objects accessing the same backup directory. +* Fixed crash (divide by zero) when compression dictionary is applied to a file containing only range tombstones. ### Performance Improvements * On ARM platform, use `yield` instead of `wfe` to relax cpu to gain better performance. diff --git a/db/db_range_del_test.cc b/db/db_range_del_test.cc index 6154c45da..4f86c8e82 100644 --- a/db/db_range_del_test.cc +++ b/db/db_range_del_test.cc @@ -73,6 +73,15 @@ TEST_F(DBRangeDelTest, FlushOutputHasOnlyRangeTombstones) { } while (ChangeOptions(kRangeDelSkipConfigs)); } +TEST_F(DBRangeDelTest, DictionaryCompressionWithOnlyRangeTombstones) { + Options opts = CurrentOptions(); + opts.compression_opts.max_dict_bytes = 16384; + Reopen(opts); + ASSERT_OK(db_->DeleteRange(WriteOptions(), db_->DefaultColumnFamily(), "dr1", + "dr2")); + ASSERT_OK(db_->Flush(FlushOptions())); +} + TEST_F(DBRangeDelTest, CompactionOutputHasOnlyRangeTombstone) { do { Options opts = CurrentOptions(); diff --git a/table/block_based/block_based_table_builder.cc b/table/block_based/block_based_table_builder.cc index 4b9e7dec1..f94ae8a95 100644 --- a/table/block_based/block_based_table_builder.cc +++ b/table/block_based/block_based_table_builder.cc @@ -1696,6 +1696,11 @@ void BlockBasedTableBuilder::EnterUnbuffered() { ? r->compression_opts.zstd_max_train_bytes : r->compression_opts.max_dict_bytes; const size_t kNumBlocksBuffered = r->data_block_and_keys_buffers.size(); + if (kNumBlocksBuffered == 0) { + // The below code is neither safe nor necessary for handling zero data + // blocks. + return; + } // Abstract algebra teaches us that a finite cyclic group (such as the // additive group of integers modulo N) can be generated by a number that is