Skip to content

Commit

Permalink
Deprecate raw DB pointer in public APIs (#13311)
Browse files Browse the repository at this point in the history 
Summary:
Offer new DB::Open and variants that use `std::unique_ptr<DB>*` 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: #13311

Test Plan: existing tests

Reviewed By: anand1976

Differential Revision: D68340779

Pulled By: pdillinger

fbshipit-source-id: 30f4448398b479b5abecfc2406447f200a5fe073
  • Loading branch information
@pdillinger @facebook-github-bot
pdillinger authored and facebook-github-bot committed Jan 17, 2025
1 parent c86d6c5 commit 602e19f
Show file tree
Hide file tree
Showing 13 changed files with 129 additions and 50 deletions.
4 changes: 2 additions & 2 deletions db/db_impl/compacted_db_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<DB>* dbptr) {
*dbptr = nullptr;

if (options.max_open_files != -1) {
Expand All @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion db/db_impl/compacted_db_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<DB>* dbptr);

// Implementations of the DB interface
using DB::Get;
Expand Down
7 changes: 4 additions & 3 deletions db/db_impl/db_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr,
const bool seq_per_batch, const bool batch_per_txn,
const bool is_retry, bool* can_retry);
std::vector<ColumnFamilyHandle*>* handles,
std::unique_ptr<DB>* 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,
Expand Down
33 changes: 17 additions & 16 deletions db/db_impl/db_impl_open.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<DB>* dbptr) {
DBOptions db_options(options);
ColumnFamilyOptions cf_options(options);
std::vector<ColumnFamilyDescriptor> column_families;
Expand Down Expand Up @@ -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<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr) {
std::vector<ColumnFamilyHandle*>* handles,
std::unique_ptr<DB>* dbptr) {
const bool kSeqPerBatch = true;
const bool kBatchPerTxn = true;
ThreadStatusUtil::SetEnableTracking(db_options.enable_thread_tracking);
Expand All @@ -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<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr,
std::vector<ColumnFamilyHandle*>* handles, std::unique_ptr<DB>* dbptr,
std::string trim_ts) {
assert(dbptr != nullptr);
assert(handles != nullptr);
Expand Down Expand Up @@ -2231,7 +2233,7 @@ Status DB::OpenAndTrimHistory(
return s;
}

*dbptr = db;
dbptr->reset(db);
return s;
}

Expand Down Expand Up @@ -2301,9 +2303,10 @@ void DBImpl::TrackExistingDataFiles(

Status DBImpl::Open(const DBOptions& db_options, const std::string& dbname,
const std::vector<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr,
const bool seq_per_batch, const bool batch_per_txn,
const bool is_retry, bool* can_retry) {
std::vector<ColumnFamilyHandle*>* handles,
std::unique_ptr<DB>* 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);

Expand All @@ -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<DBImpl>(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());
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down
9 changes: 5 additions & 4 deletions db/db_impl/db_impl_readonly.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<DB>* dbptr,
bool /*error_if_wal_file_exists*/) {
Status s = OpenForReadOnlyCheckExistence(options, dbname);
if (!s.ok()) {
return s;
Expand Down Expand Up @@ -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<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr,
std::vector<ColumnFamilyHandle*>* handles, std::unique_ptr<DB>* 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);
Expand All @@ -325,7 +326,7 @@ Status DB::OpenForReadOnly(
Status DBImplReadOnly::OpenForReadOnlyWithoutCheck(
const DBOptions& db_options, const std::string& dbname,
const std::vector<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr,
std::vector<ColumnFamilyHandle*>* handles, std::unique_ptr<DB>* dbptr,
bool error_if_wal_file_exists) {
*dbptr = nullptr;
handles->clear();
Expand Down Expand Up @@ -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<ColumnFamilyHandleImpl>(h)->cfd());
Expand Down
2 changes: 1 addition & 1 deletion db/db_impl/db_impl_readonly.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ class DBImplReadOnly : public DBImpl {
static Status OpenForReadOnlyWithoutCheck(
const DBOptions& db_options, const std::string& dbname,
const std::vector<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr,
std::vector<ColumnFamilyHandle*>* handles, std::unique_ptr<DB>* dbptr,
bool error_if_wal_file_exists = false);
friend class DB;
};
Expand Down
7 changes: 4 additions & 3 deletions db/db_impl/db_impl_secondary.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<DB>* dbptr) {
*dbptr = nullptr;

DBOptions db_options(options);
Expand All @@ -757,7 +758,7 @@ Status DB::OpenAsSecondary(
const DBOptions& db_options, const std::string& dbname,
const std::string& secondary_path,
const std::vector<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr) {
std::vector<ColumnFamilyHandle*>* handles, std::unique_ptr<DB>* dbptr) {
*dbptr = nullptr;

DBOptions tmp_opts(db_options);
Expand Down Expand Up @@ -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<ColumnFamilyHandleImpl>(h)->cfd());
Expand Down
6 changes: 3 additions & 3 deletions db/write_callback_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ TEST_P(WriteCallbackPTest, WriteWithCallbackTest) {
}

ReadOptions read_options;
DB* db;
std::unique_ptr<DB> db;
DBImpl* db_impl;

ASSERT_OK(DestroyDB(dbname, options));
Expand All @@ -208,7 +208,7 @@ TEST_P(WriteCallbackPTest, WriteWithCallbackTest) {
assert(handles.size() == 1);
delete handles[0];

db_impl = dynamic_cast<DBImpl*>(db);
db_impl = dynamic_cast<DBImpl*>(db.get());
ASSERT_TRUE(db_impl);

// Writers that have called JoinBatchGroup.
Expand Down Expand Up @@ -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));
}
}
Expand Down
87 changes: 80 additions & 7 deletions include/rocksdb/db.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<DB>* dbptr);
// DEPRECATED: raw pointer variant
static Status Open(const Options& options, const std::string& name,
DB** dbptr) {
std::unique_ptr<DB> 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
Expand All @@ -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<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr);
std::vector<ColumnFamilyHandle*>* handles,
std::unique_ptr<DB>* dbptr);
// DEPRECATED: raw pointer variant
static Status Open(const DBOptions& db_options, const std::string& name,
const std::vector<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr) {
std::unique_ptr<DB> 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.
//
Expand All @@ -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<DB>* 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<DB> 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.
//
Expand All @@ -227,8 +255,20 @@ class DB {
static Status OpenForReadOnly(
const DBOptions& db_options, const std::string& name,
const std::vector<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr,
std::vector<ColumnFamilyHandle*>* handles, std::unique_ptr<DB>* 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<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr,
bool error_if_wal_file_exists = false) {
std::unique_ptr<DB> 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
Expand All @@ -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<DB>* 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<DB> 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
//
Expand Down Expand Up @@ -293,7 +342,19 @@ class DB {
const DBOptions& db_options, const std::string& name,
const std::string& secondary_path,
const std::vector<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr);
std::vector<ColumnFamilyHandle*>* handles, std::unique_ptr<DB>* dbptr);
// DEPRECATED: raw pointer variant
static Status OpenAsSecondary(
const DBOptions& db_options, const std::string& name,
const std::string& secondary_path,
const std::vector<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr) {
std::unique_ptr<DB> smart_ptr;
Status s = OpenAsSecondary(db_options, name, secondary_path,
column_families, handles, &smart_ptr);
*dbptr = smart_ptr.release();
return s;
}

// EXPERIMENTAL

Expand Down Expand Up @@ -344,8 +405,20 @@ class DB {
static Status OpenAndTrimHistory(
const DBOptions& db_options, const std::string& dbname,
const std::vector<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr,
std::vector<ColumnFamilyHandle*>* handles, std::unique_ptr<DB>* dbptr,
std::string trim_ts);
// DEPRECATED: raw pointer variant
static Status OpenAndTrimHistory(
const DBOptions& db_options, const std::string& dbname,
const std::vector<ColumnFamilyDescriptor>& column_families,
std::vector<ColumnFamilyHandle*>* handles, DB** dbptr,
std::string trim_ts) {
std::unique_ptr<DB> 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
Expand Down
Loading

0 comments on commit 602e19f

Please sign in to comment.