(kvrocks) branch unstable updated: fix(storage): don't try to commit empty write batches (#3015)

[twice]

This is an automated email from the ASF dual-hosted git repository.

twice pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git


The following commit(s) were added to refs/heads/unstable by this push:
     new 398b94794 fix(storage): don't try to commit empty write batches (#3015)
398b94794 is described below

commit 398b947949d105a6d35165153e77ac9059213aef
Author: Nathan <151768548+nathanlo-...@users.noreply.github.com>
AuthorDate: Thu Jun 5 23:00:19 2025 -0400

    fix(storage): don't try to commit empty write batches (#3015)
    
    Co-authored-by: Twice <twice.m...@gmail.com>
---
 src/storage/storage.cc        |  5 +++++
 tests/cppunit/storage_test.cc | 45 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/src/storage/storage.cc b/src/storage/storage.cc
index 124fbfc78..b35c9aa03 100644
--- a/src/storage/storage.cc
+++ b/src/storage/storage.cc
@@ -700,6 +700,11 @@ rocksdb::Status Storage::Write(engine::Context &ctx, const 
rocksdb::WriteOptions
 
 rocksdb::Status Storage::writeToDB(engine::Context &ctx, const 
rocksdb::WriteOptions &options,
                                    rocksdb::WriteBatch *updates) {
+  // No point trying to commit an empty write batch: in fact this will fail on 
read-only DBs
+  // even if the write batch is empty.
+  if (updates->Count() == 0) {
+    return rocksdb::Status::OK();
+  }
   // Put replication id logdata at the end of `updates`.
   if (replid_.length() == kReplIdLength) {
     updates->PutLogData(ServerLogData(kReplIdLog, replid_).Encode());
diff --git a/tests/cppunit/storage_test.cc b/tests/cppunit/storage_test.cc
index 503d2515c..96fffd335 100644
--- a/tests/cppunit/storage_test.cc
+++ b/tests/cppunit/storage_test.cc
@@ -58,3 +58,48 @@ TEST(Storage, CreateBackup) {
   std::filesystem::remove_all(config.db_dir, ec);
   ASSERT_TRUE(!ec);
 }
+
+TEST(Storage, ReadOnlyTransactions) {
+  std::error_code ec;
+
+  Config config;
+  config.db_dir = "test_backup_dir";
+  config.slot_id_encoded = false;
+
+  std::filesystem::remove_all(config.db_dir, ec);
+  ASSERT_TRUE(!ec);
+
+  // Populate a DB with some test data so opening the read-only snapshot 
succeeds
+  {
+    auto storage = std::make_unique<engine::Storage>(&config);
+    auto s = storage->Open();
+    ASSERT_TRUE(s.IsOK());
+
+    auto ctx = engine::Context(storage.get());
+    rocksdb::WriteBatch batch;
+    batch.Put("k", "v");
+    ASSERT_TRUE(storage->Write(ctx, rocksdb::WriteOptions(), &batch).ok());
+  }
+
+  // Now load that DB in in read-only mode and try to write to it
+  {
+    auto storage = std::make_unique<engine::Storage>(&config);
+    auto s = storage->Open(DBOpenMode::kDBOpenModeForReadOnly);
+    std::cout << s.Msg() << std::endl;
+    ASSERT_TRUE(s.IsOK());
+
+    auto ctx = engine::Context(storage.get());
+
+    // An empty write batch should not cause any error, even if the storage is 
opened in
+    // read-only mode
+    rocksdb::WriteBatch readonly_batch;
+    ASSERT_TRUE(storage->Write(ctx, rocksdb::WriteOptions(), 
&readonly_batch).ok());
+
+    rocksdb::WriteBatch read_write_batch;
+    read_write_batch.Put("k", "v");
+    ASSERT_FALSE(storage->Write(ctx, rocksdb::WriteOptions(), 
&read_write_batch).ok());
+  }
+
+  std::filesystem::remove_all(config.db_dir, ec);
+  ASSERT_TRUE(!ec);
+}