From 2dd9bfe3a84f7cfe4b9e684f7dd8e8044d5f5de4 Mon Sep 17 00:00:00 2001 From: Lei Jin Date: Fri, 17 Oct 2014 21:18:36 -0700 Subject: [PATCH] Sanitize block-based table index type and check prefix_extractor Summary: Respond to issue reported https://www.facebook.com/groups/rocksdb.dev/permalink/651090261656158/ Change the Sanitize signature to take both DBOptions and CFOptions Test Plan: unit test Reviewers: sdong, yhchiang, igor Reviewed By: igor Subscribers: leveldb Differential Revision: https://reviews.facebook.net/D25041 --- db/db_impl.cc | 8 ++++---- db/db_test.cc | 11 +++++++++++ include/rocksdb/table.h | 6 ++++-- table/adaptive_table_factory.h | 5 +++-- table/block_based_table_factory.cc | 11 +++++++++++ table/block_based_table_factory.h | 5 ++--- table/cuckoo_table_factory.h | 3 ++- table/plain_table_factory.h | 5 +++-- 8 files changed, 40 insertions(+), 14 deletions(-) diff --git a/db/db_impl.cc b/db/db_impl.cc index d8df10c5a..c5bc7680a 100644 --- a/db/db_impl.cc +++ b/db/db_impl.cc @@ -282,12 +282,12 @@ DBOptions SanitizeOptions(const std::string& dbname, const DBOptions& src) { namespace { -Status SanitizeDBOptionsByCFOptions( - const DBOptions* db_opts, +Status SanitizeOptionsByTable( + const DBOptions& db_opts, const std::vector& column_families) { Status s; for (auto cf : column_families) { - s = cf.options.table_factory->SanitizeDBOptions(db_opts); + s = cf.options.table_factory->SanitizeOptions(db_opts, cf.options); if (!s.ok()) { return s; } @@ -4703,7 +4703,7 @@ Status DB::Open(const Options& options, const std::string& dbname, DB** dbptr) { Status DB::Open(const DBOptions& db_options, const std::string& dbname, const std::vector& column_families, std::vector* handles, DB** dbptr) { - Status s = SanitizeDBOptionsByCFOptions(&db_options, column_families); + Status s = SanitizeOptionsByTable(db_options, column_families); if (!s.ok()) { return s; } diff --git a/db/db_test.cc b/db/db_test.cc index 169718387..ebe946cf4 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -8354,6 +8354,17 @@ TEST(DBTest, TableOptionsSanitizeTest) { options.prefix_extractor.reset(NewNoopTransform()); Destroy(&options); ASSERT_TRUE(TryReopen(&options).IsNotSupported()); + + // Test for check of prefix_extractor when hash index is used for + // block-based table + BlockBasedTableOptions to; + to.index_type = BlockBasedTableOptions::kHashSearch; + options = Options(); + options.create_if_missing = true; + options.table_factory.reset(NewBlockBasedTableFactory(to)); + ASSERT_TRUE(TryReopen(&options).IsInvalidArgument()); + options.prefix_extractor.reset(NewFixedPrefixTransform(1)); + ASSERT_OK(TryReopen(&options)); } TEST(DBTest, DBIteratorBoundTest) { diff --git a/include/rocksdb/table.h b/include/rocksdb/table.h index 4c06c23f7..4fddab4b3 100644 --- a/include/rocksdb/table.h +++ b/include/rocksdb/table.h @@ -355,11 +355,13 @@ class TableFactory { WritableFile* file, const CompressionType compression_type, const CompressionOptions& compression_opts) const = 0; - // Sanitizes the specified DB Options. + // Sanitizes the specified DB Options and ColumnFamilyOptions. // // If the function cannot find a way to sanitize the input DB Options, // a non-ok Status will be returned. - virtual Status SanitizeDBOptions(const DBOptions* db_opts) const = 0; + virtual Status SanitizeOptions( + const DBOptions& db_opts, + const ColumnFamilyOptions& cf_opts) const = 0; // Return a string that contains printable format of table configurations. // RocksDB prints configurations at DB Open(). diff --git a/table/adaptive_table_factory.h b/table/adaptive_table_factory.h index f0920db97..3c6455f90 100644 --- a/table/adaptive_table_factory.h +++ b/table/adaptive_table_factory.h @@ -47,8 +47,9 @@ class AdaptiveTableFactory : public TableFactory { const CompressionOptions& compression_opts) const override; // Sanitizes the specified DB Options. - Status SanitizeDBOptions(const DBOptions* db_opts) const override { - if (db_opts->allow_mmap_reads == false) { + Status SanitizeOptions(const DBOptions& db_opts, + const ColumnFamilyOptions& cf_opts) const override { + if (db_opts.allow_mmap_reads == false) { return Status::NotSupported( "AdaptiveTable with allow_mmap_reads == false is not supported."); } diff --git a/table/block_based_table_factory.cc b/table/block_based_table_factory.cc index b4e2e7d1f..3155f3394 100644 --- a/table/block_based_table_factory.cc +++ b/table/block_based_table_factory.cc @@ -63,6 +63,17 @@ TableBuilder* BlockBasedTableFactory::NewTableBuilder( return table_builder; } +Status BlockBasedTableFactory::SanitizeOptions( + const DBOptions& db_opts, + const ColumnFamilyOptions& cf_opts) const { + if (table_options_.index_type == BlockBasedTableOptions::kHashSearch && + cf_opts.prefix_extractor == nullptr) { + return Status::InvalidArgument("Hash index is specified for block-based " + "table, but prefix_extractor is not given"); + } + return Status::OK(); +} + std::string BlockBasedTableFactory::GetPrintableTableOptions() const { std::string ret; ret.reserve(20000); diff --git a/table/block_based_table_factory.h b/table/block_based_table_factory.h index 2dcfda6d4..247fcd691 100644 --- a/table/block_based_table_factory.h +++ b/table/block_based_table_factory.h @@ -46,9 +46,8 @@ class BlockBasedTableFactory : public TableFactory { const CompressionOptions& compression_opts) const override; // Sanitizes the specified DB Options. - Status SanitizeDBOptions(const DBOptions* db_opts) const override { - return Status::OK(); - } + Status SanitizeOptions(const DBOptions& db_opts, + const ColumnFamilyOptions& cf_opts) const override; std::string GetPrintableTableOptions() const override; diff --git a/table/cuckoo_table_factory.h b/table/cuckoo_table_factory.h index 599908678..714fdc2a0 100644 --- a/table/cuckoo_table_factory.h +++ b/table/cuckoo_table_factory.h @@ -64,7 +64,8 @@ class CuckooTableFactory : public TableFactory { const CompressionType, const CompressionOptions&) const override; // Sanitizes the specified DB Options. - Status SanitizeDBOptions(const DBOptions* db_opts) const override { + Status SanitizeOptions(const DBOptions& db_opts, + const ColumnFamilyOptions& cf_opts) const override { return Status::OK(); } diff --git a/table/plain_table_factory.h b/table/plain_table_factory.h index e79475221..23b54f092 100644 --- a/table/plain_table_factory.h +++ b/table/plain_table_factory.h @@ -170,8 +170,9 @@ class PlainTableFactory : public TableFactory { static const char kValueTypeSeqId0 = 0xFF; // Sanitizes the specified DB Options. - Status SanitizeDBOptions(const DBOptions* db_opts) const override { - if (db_opts->allow_mmap_reads == false) { + Status SanitizeOptions(const DBOptions& db_opts, + const ColumnFamilyOptions& cf_opts) const override { + if (db_opts.allow_mmap_reads == false) { return Status::NotSupported( "PlainTable with allow_mmap_reads == false is not supported."); }