From cbe303c19b4ee8d36b5f053e0cc5d86b0b8a448b Mon Sep 17 00:00:00 2001 From: duyuqi Date: Mon, 21 Mar 2022 12:04:33 -0700 Subject: [PATCH] =?UTF-8?q?fix=20a=20bug,=20c=20api,=20if=20enable=20inpla?= =?UTF-8?q?ce=5Fupdate=5Fsupport,=20and=20use=20create=20sn=E2=80=A6=20(#9?= =?UTF-8?q?471)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: c api release snapshot will core dump when enable inplace_update_support and create snapshot Pull Request resolved: https://github.com/facebook/rocksdb/pull/9471 Reviewed By: akankshamahajan15 Differential Revision: D34965103 Pulled By: riversand963 fbshipit-source-id: c3aeeb9ea7126c2eda1466102794fecf57b6ab77 --- db/c_test.c | 42 ++++++++++++++++++++++++++++++------ db/db_impl/db_impl.cc | 6 ++++++ db/db_inplace_update_test.cc | 30 ++++++++++++++++++++++++++ include/rocksdb/db.h | 2 +- 4 files changed, 72 insertions(+), 8 deletions(-) diff --git a/db/c_test.c b/db/c_test.c index 044bbfd1b..980b5dd99 100644 --- a/db/c_test.c +++ b/db/c_test.c @@ -7,12 +7,13 @@ #ifndef ROCKSDB_LITE // Lite does not support C API -#include "rocksdb/c.h" - +#include #include #include #include #include + +#include "rocksdb/c.h" #ifndef OS_WIN #include #endif @@ -89,10 +90,8 @@ static void CheckEqual(const char* expected, const char* v, size_t n) { // ok return; } else { - fprintf(stderr, "%s: expected '%s', got '%s'\n", - phase, - (expected ? expected : "(null)"), - (v ? v : "(null")); + fprintf(stderr, "%s: expected '%s', got '%s'\n", phase, + (expected ? expected : "(null)"), (v ? v : "(null)")); abort(); } } @@ -989,7 +988,36 @@ int main(int argc, char** argv) { CheckGet(db, roptions, "foo", NULL); rocksdb_release_snapshot(db, snap); } - + StartPhase("snapshot_with_memtable_inplace_update"); + { + rocksdb_close(db); + const rocksdb_snapshot_t* snap = NULL; + const char* s_key = "foo_snap"; + const char* value1 = "hello_s1"; + const char* value2 = "hello_s2"; + rocksdb_options_set_allow_concurrent_memtable_write(options, 0); + rocksdb_options_set_inplace_update_support(options, 1); + rocksdb_options_set_error_if_exists(options, 0); + db = rocksdb_open(options, dbname, &err); + CheckNoError(err); + rocksdb_put(db, woptions, s_key, 8, value1, 8, &err); + snap = rocksdb_create_snapshot(db); + assert(snap != NULL); + rocksdb_put(db, woptions, s_key, 8, value2, 8, &err); + CheckNoError(err); + rocksdb_readoptions_set_snapshot(roptions, snap); + CheckGet(db, roptions, "foo", NULL); + // snapshot syntax is invalid, because of inplace update supported is set + CheckGet(db, roptions, s_key, value2); + // restore the data and options + rocksdb_delete(db, woptions, s_key, 8, &err); + CheckGet(db, roptions, s_key, NULL); + rocksdb_release_snapshot(db, snap); + rocksdb_readoptions_set_snapshot(roptions, NULL); + rocksdb_options_set_inplace_update_support(options, 0); + rocksdb_options_set_allow_concurrent_memtable_write(options, 1); + rocksdb_options_set_error_if_exists(options, 1); + } StartPhase("repair"); { // If we do not compact here, then the lazy deletion of diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index 93d62c063..1bcdfd83a 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -3205,6 +3205,12 @@ bool CfdListContains(const CfdList& list, ColumnFamilyData* cfd) { } // namespace void DBImpl::ReleaseSnapshot(const Snapshot* s) { + if (s == nullptr) { + // DBImpl::GetSnapshot() can return nullptr when snapshot + // not supported by specifying the condition: + // inplace_update_support enabled. + return; + } const SnapshotImpl* casted_s = reinterpret_cast(s); { InstrumentedMutexLock l(&mutex_); diff --git a/db/db_inplace_update_test.cc b/db/db_inplace_update_test.cc index 450782c89..2012cabf1 100644 --- a/db/db_inplace_update_test.cc +++ b/db/db_inplace_update_test.cc @@ -169,6 +169,36 @@ TEST_F(DBTestInPlaceUpdate, InPlaceUpdateCallbackNoAction) { ASSERT_EQ(Get(1, "key"), "NOT_FOUND"); } while (ChangeCompactOptions()); } + +TEST_F(DBTestInPlaceUpdate, InPlaceUpdateAndSnapshot) { + do { + Options options = CurrentOptions(); + options.create_if_missing = true; + options.inplace_update_support = true; + options.env = env_; + options.write_buffer_size = 100000; + options.allow_concurrent_memtable_write = false; + Reopen(options); + CreateAndReopenWithCF({"pikachu"}, options); + + // Update key with values of smaller size, and + // run GetSnapshot and ReleaseSnapshot + int numValues = 2; + for (int i = numValues; i > 0; i--) { + const Snapshot* s = db_->GetSnapshot(); + ASSERT_EQ(nullptr, s); + std::string value = DummyString(i, 'a'); + ASSERT_OK(Put(1, "key", value)); + ASSERT_EQ(value, Get(1, "key")); + // release s (nullptr) + db_->ReleaseSnapshot(s); + } + + // Only 1 instance for that key. + validateNumberOfEntries(1, 1); + } while (ChangeCompactOptions()); +} + } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) { diff --git a/include/rocksdb/db.h b/include/rocksdb/db.h index 47c53d5f8..2c5217a41 100644 --- a/include/rocksdb/db.h +++ b/include/rocksdb/db.h @@ -793,7 +793,7 @@ class DB { // snapshot is no longer needed. // // nullptr will be returned if the DB fails to take a snapshot or does - // not support snapshot. + // not support snapshot (eg: inplace_update_support enabled). virtual const Snapshot* GetSnapshot() = 0; // Release a previously acquired snapshot. The caller must not