From 602e19fd8d8d6de81a90df81b3787fba1e059042 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Fri, 17 Jan 2025 13:33:25 -0800 Subject: [PATCH] Deprecate raw DB pointer in public APIs (#13311) Summary: Offer new DB::Open and variants that use `std::unique_ptr*` output parameters and deprecate the old versions that use `DB**` output parameters. This shouldn't have weird downstream effects because these are just static functions. (And a constructor for StackableDB) Pull Request resolved: https://github.com/facebook/rocksdb/pull/13311 Test Plan: existing tests Reviewed By: anand1976 Differential Revision: D68340779 Pulled By: pdillinger fbshipit-source-id: 30f4448398b479b5abecfc2406447f200a5fe073 --- db/db_impl/compacted_db_impl.cc | 4 +- db/db_impl/compacted_db_impl.h | 2 +- db/db_impl/db_impl.h | 7 +- db/db_impl/db_impl_open.cc | 33 +++---- db/db_impl/db_impl_readonly.cc | 9 +- db/db_impl/db_impl_readonly.h | 2 +- db/db_impl/db_impl_secondary.cc | 7 +- db/write_callback_test.cc | 6 +- include/rocksdb/db.h | 87 +++++++++++++++++-- include/rocksdb/utilities/stackable_db.h | 4 + .../public_api_changes/raw_db_ptr | 1 + .../pessimistic_transaction_db.cc | 6 +- utilities/transactions/transaction_test.h | 11 +-- 13 files changed, 129 insertions(+), 50 deletions(-) create mode 100644 unreleased_history/public_api_changes/raw_db_ptr diff --git a/db/db_impl/compacted_db_impl.cc b/db/db_impl/compacted_db_impl.cc index 0e92ffd232f..611695be66a 100644 --- a/db/db_impl/compacted_db_impl.cc +++ b/db/db_impl/compacted_db_impl.cc @@ -248,7 +248,7 @@ Status CompactedDBImpl::Init(const Options& options) { } Status CompactedDBImpl::Open(const Options& options, const std::string& dbname, - DB** dbptr) { + std::unique_ptr* dbptr) { *dbptr = nullptr; if (options.max_open_files != -1) { @@ -267,7 +267,7 @@ Status CompactedDBImpl::Open(const Options& options, const std::string& dbname, ROCKS_LOG_INFO(db->immutable_db_options_.info_log, "Opened the db as fully compacted mode"); LogFlush(db->immutable_db_options_.info_log); - *dbptr = db.release(); + *dbptr = std::move(db); } return s; } diff --git a/db/db_impl/compacted_db_impl.h b/db/db_impl/compacted_db_impl.h index 03853a5dda1..098016f5aac 100644 --- a/db/db_impl/compacted_db_impl.h +++ b/db/db_impl/compacted_db_impl.h @@ -22,7 +22,7 @@ class CompactedDBImpl : public DBImpl { ~CompactedDBImpl() override; static Status Open(const Options& options, const std::string& dbname, - DB** dbptr); + std::unique_ptr* dbptr); // Implementations of the DB interface using DB::Get; diff --git a/db/db_impl/db_impl.h b/db/db_impl/db_impl.h index 9a3f666064f..1e1af5eb780 100644 --- a/db/db_impl/db_impl.h +++ b/db/db_impl/db_impl.h @@ -1092,9 +1092,10 @@ class DBImpl : public DB { // This is to be used only by internal rocksdb classes. static Status Open(const DBOptions& db_options, const std::string& name, const std::vector& column_families, - std::vector* handles, DB** dbptr, - const bool seq_per_batch, const bool batch_per_txn, - const bool is_retry, bool* can_retry); + std::vector* handles, + std::unique_ptr* dbptr, const bool seq_per_batch, + const bool batch_per_txn, const bool is_retry, + bool* can_retry); static IOStatus CreateAndNewDirectory( FileSystem* fs, const std::string& dirname, diff --git a/db/db_impl/db_impl_open.cc b/db/db_impl/db_impl_open.cc index c47ea9da57d..d09b0a6eb77 100644 --- a/db/db_impl/db_impl_open.cc +++ b/db/db_impl/db_impl_open.cc @@ -2126,7 +2126,8 @@ Status DBImpl::WriteLevel0TableForRecovery(int job_id, ColumnFamilyData* cfd, return s; } -Status DB::Open(const Options& options, const std::string& dbname, DB** dbptr) { +Status DB::Open(const Options& options, const std::string& dbname, + std::unique_ptr* dbptr) { DBOptions db_options(options); ColumnFamilyOptions cf_options(options); std::vector column_families; @@ -2154,7 +2155,8 @@ 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) { + std::vector* handles, + std::unique_ptr* dbptr) { const bool kSeqPerBatch = true; const bool kBatchPerTxn = true; ThreadStatusUtil::SetEnableTracking(db_options.enable_thread_tracking); @@ -2176,7 +2178,7 @@ Status DB::Open(const DBOptions& db_options, const std::string& dbname, Status DB::OpenAndTrimHistory( const DBOptions& db_options, const std::string& dbname, const std::vector& column_families, - std::vector* handles, DB** dbptr, + std::vector* handles, std::unique_ptr* dbptr, std::string trim_ts) { assert(dbptr != nullptr); assert(handles != nullptr); @@ -2231,7 +2233,7 @@ Status DB::OpenAndTrimHistory( return s; } - *dbptr = db; + dbptr->reset(db); return s; } @@ -2301,9 +2303,10 @@ void DBImpl::TrackExistingDataFiles( Status DBImpl::Open(const DBOptions& db_options, const std::string& dbname, const std::vector& column_families, - std::vector* handles, DB** dbptr, - const bool seq_per_batch, const bool batch_per_txn, - const bool is_retry, bool* can_retry) { + std::vector* handles, + std::unique_ptr* dbptr, const bool seq_per_batch, + const bool batch_per_txn, const bool is_retry, + bool* can_retry) { const WriteOptions write_options(Env::IOActivity::kDBOpen); const ReadOptions read_options(Env::IOActivity::kDBOpen); @@ -2327,10 +2330,10 @@ Status DBImpl::Open(const DBOptions& db_options, const std::string& dbname, std::max(max_write_buffer_size, cf.options.write_buffer_size); } - DBImpl* impl = new DBImpl(db_options, dbname, seq_per_batch, batch_per_txn); + auto impl = std::make_unique(db_options, dbname, seq_per_batch, + batch_per_txn); if (!impl->immutable_db_options_.info_log) { s = impl->init_logger_creation_s_; - delete impl; return s; } else { assert(impl->init_logger_creation_s_.ok()); @@ -2363,7 +2366,6 @@ Status DBImpl::Open(const DBOptions& db_options, const std::string& dbname, s = impl->CreateArchivalDirectory(); } if (!s.ok()) { - delete impl; return s; } @@ -2465,7 +2467,7 @@ Status DBImpl::Open(const DBOptions& db_options, const std::string& dbname, impl->versions_->GetColumnFamilySet()->GetColumnFamily(cf.name); if (cfd != nullptr) { handles->push_back( - new ColumnFamilyHandleImpl(cfd, impl, &impl->mutex_)); + new ColumnFamilyHandleImpl(cfd, impl.get(), &impl->mutex_)); impl->NewThreadStatusCfInfo(cfd); } else { if (db_options.create_missing_column_families) { @@ -2527,7 +2529,6 @@ Status DBImpl::Open(const DBOptions& db_options, const std::string& dbname, // The WriteOptionsFile() will release and lock the mutex internally. persist_options_status = impl->WriteOptionsFile(write_options, true /*db_mutex_already_held*/); - *dbptr = impl; impl->opened_successfully_ = true; } else { persist_options_status.PermitUncheckedError(); @@ -2578,7 +2579,7 @@ Status DBImpl::Open(const DBOptions& db_options, const std::string& dbname, if (s.ok()) { ROCKS_LOG_HEADER(impl->immutable_db_options_.info_log, "DB pointer %p", - impl); + impl.get()); LogFlush(impl->immutable_db_options_.info_log); if (!impl->WALBufferIsEmpty()) { s = impl->FlushWAL(write_options, false); @@ -2612,13 +2613,13 @@ Status DBImpl::Open(const DBOptions& db_options, const std::string& dbname, recovery_ctx.is_new_db_); } impl->options_mutex_.Unlock(); - if (!s.ok()) { + if (s.ok()) { + *dbptr = std::move(impl); + } else { for (auto* h : *handles) { delete h; } handles->clear(); - delete impl; - *dbptr = nullptr; } return s; } diff --git a/db/db_impl/db_impl_readonly.cc b/db/db_impl/db_impl_readonly.cc index 4a49870f6b3..b33c09aace0 100644 --- a/db/db_impl/db_impl_readonly.cc +++ b/db/db_impl/db_impl_readonly.cc @@ -275,7 +275,8 @@ Status OpenForReadOnlyCheckExistence(const DBOptions& db_options, } // namespace Status DB::OpenForReadOnly(const Options& options, const std::string& dbname, - DB** dbptr, bool /*error_if_wal_file_exists*/) { + std::unique_ptr* dbptr, + bool /*error_if_wal_file_exists*/) { Status s = OpenForReadOnlyCheckExistence(options, dbname); if (!s.ok()) { return s; @@ -309,7 +310,7 @@ Status DB::OpenForReadOnly(const Options& options, const std::string& dbname, Status DB::OpenForReadOnly( const DBOptions& db_options, const std::string& dbname, const std::vector& column_families, - std::vector* handles, DB** dbptr, + std::vector* handles, std::unique_ptr* dbptr, bool error_if_wal_file_exists) { // If dbname does not exist in the file system, should not do anything Status s = OpenForReadOnlyCheckExistence(db_options, dbname); @@ -325,7 +326,7 @@ Status DB::OpenForReadOnly( Status DBImplReadOnly::OpenForReadOnlyWithoutCheck( const DBOptions& db_options, const std::string& dbname, const std::vector& column_families, - std::vector* handles, DB** dbptr, + std::vector* handles, std::unique_ptr* dbptr, bool error_if_wal_file_exists) { *dbptr = nullptr; handles->clear(); @@ -356,7 +357,7 @@ Status DBImplReadOnly::OpenForReadOnlyWithoutCheck( impl->mutex_.Unlock(); sv_context.Clean(); if (s.ok()) { - *dbptr = impl; + dbptr->reset(impl); for (auto* h : *handles) { impl->NewThreadStatusCfInfo( static_cast_with_check(h)->cfd()); diff --git a/db/db_impl/db_impl_readonly.h b/db/db_impl/db_impl_readonly.h index c0e5cd6a377..9566f547bfe 100644 --- a/db/db_impl/db_impl_readonly.h +++ b/db/db_impl/db_impl_readonly.h @@ -171,7 +171,7 @@ class DBImplReadOnly : public DBImpl { static Status OpenForReadOnlyWithoutCheck( const DBOptions& db_options, const std::string& dbname, const std::vector& column_families, - std::vector* handles, DB** dbptr, + std::vector* handles, std::unique_ptr* dbptr, bool error_if_wal_file_exists = false); friend class DB; }; diff --git a/db/db_impl/db_impl_secondary.cc b/db/db_impl/db_impl_secondary.cc index 7586bc3d128..06dcc1a7a90 100644 --- a/db/db_impl/db_impl_secondary.cc +++ b/db/db_impl/db_impl_secondary.cc @@ -735,7 +735,8 @@ Status DBImplSecondary::TryCatchUpWithPrimary() { } Status DB::OpenAsSecondary(const Options& options, const std::string& dbname, - const std::string& secondary_path, DB** dbptr) { + const std::string& secondary_path, + std::unique_ptr* dbptr) { *dbptr = nullptr; DBOptions db_options(options); @@ -757,7 +758,7 @@ Status DB::OpenAsSecondary( const DBOptions& db_options, const std::string& dbname, const std::string& secondary_path, const std::vector& column_families, - std::vector* handles, DB** dbptr) { + std::vector* handles, std::unique_ptr* dbptr) { *dbptr = nullptr; DBOptions tmp_opts(db_options); @@ -824,7 +825,7 @@ Status DB::OpenAsSecondary( impl->mutex_.Unlock(); sv_context.Clean(); if (s.ok()) { - *dbptr = impl; + dbptr->reset(impl); for (auto h : *handles) { impl->NewThreadStatusCfInfo( static_cast_with_check(h)->cfd()); diff --git a/db/write_callback_test.cc b/db/write_callback_test.cc index 54c5ed8635d..53094eca4b9 100644 --- a/db/write_callback_test.cc +++ b/db/write_callback_test.cc @@ -191,7 +191,7 @@ TEST_P(WriteCallbackPTest, WriteWithCallbackTest) { } ReadOptions read_options; - DB* db; + std::unique_ptr db; DBImpl* db_impl; ASSERT_OK(DestroyDB(dbname, options)); @@ -208,7 +208,7 @@ TEST_P(WriteCallbackPTest, WriteWithCallbackTest) { assert(handles.size() == 1); delete handles[0]; - db_impl = dynamic_cast(db); + db_impl = dynamic_cast(db.get()); ASSERT_TRUE(db_impl); // Writers that have called JoinBatchGroup. @@ -402,7 +402,7 @@ TEST_P(WriteCallbackPTest, WriteWithCallbackTest) { ASSERT_EQ(seq.load(), db_impl->TEST_GetLastVisibleSequence()); - delete db; + db.reset(); ASSERT_OK(DestroyDB(dbname, options)); } } diff --git a/include/rocksdb/db.h b/include/rocksdb/db.h index d2273f45432..a647e3045a4 100644 --- a/include/rocksdb/db.h +++ b/include/rocksdb/db.h @@ -179,7 +179,15 @@ class DB { // // Caller must delete *dbptr when it is no longer needed. static Status Open(const Options& options, const std::string& name, - DB** dbptr); + std::unique_ptr* dbptr); + // DEPRECATED: raw pointer variant + static Status Open(const Options& options, const std::string& name, + DB** dbptr) { + std::unique_ptr smart_ptr; + Status s = Open(options, name, &smart_ptr); + *dbptr = smart_ptr.release(); + return s; + } // Open DB with column families. // db_options specify database specific options @@ -197,7 +205,17 @@ class DB { // DestroyColumnFamilyHandle() with all the handles. static Status Open(const DBOptions& db_options, const std::string& name, const std::vector& column_families, - std::vector* handles, DB** dbptr); + std::vector* handles, + std::unique_ptr* dbptr); + // DEPRECATED: raw pointer variant + static Status Open(const DBOptions& db_options, const std::string& name, + const std::vector& column_families, + std::vector* handles, DB** dbptr) { + std::unique_ptr smart_ptr; + Status s = Open(db_options, name, column_families, handles, &smart_ptr); + *dbptr = smart_ptr.release(); + return s; + } // OpenForReadOnly() creates a Read-only instance that supports reads alone. // @@ -214,8 +232,18 @@ class DB { // Open the database for read only. // static Status OpenForReadOnly(const Options& options, const std::string& name, - DB** dbptr, + std::unique_ptr* dbptr, bool error_if_wal_file_exists = false); + // DEPRECATED: raw pointer variant + static Status OpenForReadOnly(const Options& options, const std::string& name, + DB** dbptr, + bool error_if_wal_file_exists = false) { + std::unique_ptr smart_ptr; + Status s = + OpenForReadOnly(options, name, &smart_ptr, error_if_wal_file_exists); + *dbptr = smart_ptr.release(); + return s; + } // Open the database for read only with column families. // @@ -227,8 +255,20 @@ class DB { static Status OpenForReadOnly( const DBOptions& db_options, const std::string& name, const std::vector& column_families, - std::vector* handles, DB** dbptr, + std::vector* handles, std::unique_ptr* dbptr, bool error_if_wal_file_exists = false); + // DEPRECATED: raw pointer variant + static Status OpenForReadOnly( + const DBOptions& db_options, const std::string& name, + const std::vector& column_families, + std::vector* handles, DB** dbptr, + bool error_if_wal_file_exists = false) { + std::unique_ptr smart_ptr; + Status s = OpenForReadOnly(db_options, name, column_families, handles, + &smart_ptr, error_if_wal_file_exists); + *dbptr = smart_ptr.release(); + return s; + } // OpenAsSecondary() creates a secondary instance that supports read-only // operations and supports dynamic catch up with the primary (through a @@ -255,7 +295,16 @@ class DB { // // Return OK on success, non-OK on failures. static Status OpenAsSecondary(const Options& options, const std::string& name, - const std::string& secondary_path, DB** dbptr); + const std::string& secondary_path, + std::unique_ptr* dbptr); + // DEPRECATED: raw pointer variant + static Status OpenAsSecondary(const Options& options, const std::string& name, + const std::string& secondary_path, DB** dbptr) { + std::unique_ptr smart_ptr; + Status s = OpenAsSecondary(options, name, secondary_path, &smart_ptr); + *dbptr = smart_ptr.release(); + return s; + } // Open DB as secondary instance with specified column families // @@ -293,7 +342,19 @@ class DB { const DBOptions& db_options, const std::string& name, const std::string& secondary_path, const std::vector& column_families, - std::vector* handles, DB** dbptr); + std::vector* handles, std::unique_ptr* dbptr); + // DEPRECATED: raw pointer variant + static Status OpenAsSecondary( + const DBOptions& db_options, const std::string& name, + const std::string& secondary_path, + const std::vector& column_families, + std::vector* handles, DB** dbptr) { + std::unique_ptr smart_ptr; + Status s = OpenAsSecondary(db_options, name, secondary_path, + column_families, handles, &smart_ptr); + *dbptr = smart_ptr.release(); + return s; + } // EXPERIMENTAL @@ -344,8 +405,20 @@ class DB { static Status OpenAndTrimHistory( const DBOptions& db_options, const std::string& dbname, const std::vector& column_families, - std::vector* handles, DB** dbptr, + std::vector* handles, std::unique_ptr* dbptr, std::string trim_ts); + // DEPRECATED: raw pointer variant + static Status OpenAndTrimHistory( + const DBOptions& db_options, const std::string& dbname, + const std::vector& column_families, + std::vector* handles, DB** dbptr, + std::string trim_ts) { + std::unique_ptr smart_ptr; + Status s = OpenAndTrimHistory(db_options, dbname, column_families, handles, + &smart_ptr, trim_ts); + *dbptr = smart_ptr.release(); + return s; + } // Manually, synchronously attempt to resume DB writes after a write failure // to the underlying filesystem. See diff --git a/include/rocksdb/utilities/stackable_db.h b/include/rocksdb/utilities/stackable_db.h index 376df668d76..0e604dea538 100644 --- a/include/rocksdb/utilities/stackable_db.h +++ b/include/rocksdb/utilities/stackable_db.h @@ -27,6 +27,10 @@ class StackableDB : public DB { explicit StackableDB(std::shared_ptr db) : db_(db.get()), shared_db_ptr_(db) {} + // StackableDB take sole ownership of the underlying db. + explicit StackableDB(std::unique_ptr&& db) + : db_(db.get()), shared_db_ptr_(std::move(db)) {} + ~StackableDB() override { if (shared_db_ptr_ == nullptr) { delete db_; diff --git a/unreleased_history/public_api_changes/raw_db_ptr b/unreleased_history/public_api_changes/raw_db_ptr new file mode 100644 index 00000000000..8a0c0efb555 --- /dev/null +++ b/unreleased_history/public_api_changes/raw_db_ptr @@ -0,0 +1 @@ +* Offer new DB::Open and variants that use `std::unique_ptr*` output parameters and deprecate the old versions that use `DB**` output parameters. diff --git a/utilities/transactions/pessimistic_transaction_db.cc b/utilities/transactions/pessimistic_transaction_db.cc index 429d1994ccc..37fd80e8625 100644 --- a/utilities/transactions/pessimistic_transaction_db.cc +++ b/utilities/transactions/pessimistic_transaction_db.cc @@ -230,7 +230,7 @@ Status TransactionDB::Open( const std::vector& column_families, std::vector* handles, TransactionDB** dbptr) { Status s; - DB* db = nullptr; + std::unique_ptr db; if (txn_db_options.write_policy == WRITE_COMMITTED && db_options.unordered_write) { return Status::NotSupported( @@ -269,8 +269,8 @@ Status TransactionDB::Open( static_cast(txn_db_options.write_policy)); // if WrapDB return non-ok, db will be deleted in WrapDB() via // ~StackableDB(). - s = WrapDB(db, txn_db_options, compaction_enabled_cf_indices, *handles, - dbptr); + s = WrapDB(db.release(), txn_db_options, compaction_enabled_cf_indices, + *handles, dbptr); } return s; } diff --git a/utilities/transactions/transaction_test.h b/utilities/transactions/transaction_test.h index a779bf8767c..72f7e7036bf 100644 --- a/utilities/transactions/transaction_test.h +++ b/utilities/transactions/transaction_test.h @@ -163,7 +163,7 @@ class TransactionTestBase : public ::testing::Test { std::vector* handles) { std::vector compaction_enabled_cf_indices; TransactionDB::PrepareWrap(&options, &cfs, &compaction_enabled_cf_indices); - DB* root_db = nullptr; + std::unique_ptr root_db; Options options_copy(options); const bool use_seq_per_batch = txn_db_options.write_policy == WRITE_PREPARED || @@ -174,9 +174,8 @@ class TransactionTestBase : public ::testing::Test { Status s = DBImpl::Open(options_copy, dbname, cfs, handles, &root_db, use_seq_per_batch, use_batch_per_txn, /*is_retry=*/false, /*can_retry=*/nullptr); - auto stackable_db = std::make_unique(root_db); + auto stackable_db = std::make_unique(std::move(root_db)); if (s.ok()) { - assert(root_db != nullptr); // If WrapStackableDB() returns non-ok, then stackable_db is already // deleted within WrapStackableDB(). s = TransactionDB::WrapStackableDB(stackable_db.release(), txn_db_options, @@ -194,7 +193,7 @@ class TransactionTestBase : public ::testing::Test { TransactionDB::PrepareWrap(&options, &column_families, &compaction_enabled_cf_indices); std::vector handles; - DB* root_db = nullptr; + std::unique_ptr root_db; Options options_copy(options); const bool use_seq_per_batch = txn_db_options.write_policy == WRITE_PREPARED || @@ -206,11 +205,9 @@ class TransactionTestBase : public ::testing::Test { &root_db, use_seq_per_batch, use_batch_per_txn, /*is_retry=*/false, /*can_retry=*/nullptr); if (!s.ok()) { - delete root_db; return s; } - StackableDB* stackable_db = new StackableDB(root_db); - assert(root_db != nullptr); + StackableDB* stackable_db = new StackableDB(std::move(root_db)); assert(handles.size() == 1); s = TransactionDB::WrapStackableDB(stackable_db, txn_db_options, compaction_enabled_cf_indices, handles,