This is an automated email from the ASF dual-hosted git repository.
git-hulk 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 576f7d580 fix(timeseries): avoid deleting empty key on first write to
an empty series (#3618)
576f7d580 is described below
commit 576f7d580526b9a60a9070fc092cd6a470422ea3
Author: weimeng <[email protected]>
AuthorDate: Wed Sep 30 17:41:55 2026 +0800
fix(timeseries): avoid deleting empty key on first write to an empty series
(#3618)
On the first write to an empty time series (created via `TS.CREATE`, no
chunk
persisted yet), `upsertCommonInBatch` emits a spurious `Delete("")`,
leaving an
empty-key tombstone in the WAL.
## Root Cause
When the source series has no persisted chunk, `latest_chunk_key` stays
`""`,
but the rename guard only checked `new_key != latest_chunk_key`. Since
`new_key`
always carries the key prefix, the condition was unconditionally true
and ran
`batch->Delete("")`. The empty `latest_chunk_key` is a valid "source
empty"
state — the function already relies on it a few lines below
(`was_source_empty = latest_chunk_key.empty()`); the delete branch just
missed
the same guard.
## Fix
Add the empty-key precondition:
```cpp
if (i == 0 && !latest_chunk_key.empty() && new_key != latest_chunk_key) {
---------
Co-authored-by: weimeng <[email protected]>
Co-authored-by: RX Xiao <[email protected]>
---
src/types/redis_timeseries.cc | 4 +-
tests/cppunit/types/timeseries_test.cc | 111 +++++++++++++++++++++++++++++++++
2 files changed, 114 insertions(+), 1 deletion(-)
diff --git a/src/types/redis_timeseries.cc b/src/types/redis_timeseries.cc
index cebf453dd..7f5a744d1 100644
--- a/src/types/redis_timeseries.cc
+++ b/src/types/redis_timeseries.cc
@@ -965,7 +965,9 @@ rocksdb::Status
TimeSeries::upsertCommonInBatch(engine::Context &ctx, const Slic
auto &new_data = new_data_list[i];
auto new_chunk = CreateTSChunkFromData(new_data);
auto new_key = internalKeyFromChunkID(ns_key, metadata,
new_chunk->GetFirstTimestamp());
- if (i == 0 && new_key != latest_chunk_key) {
+ // latest_chunk_key is empty when the source time series has no persisted
chunk yet (first
+ // write); skip the delete in that case to avoid emitting a tombstone for
the empty key.
+ if (i == 0 && !latest_chunk_key.empty() && new_key != latest_chunk_key) {
s = batch->Delete(latest_chunk_key);
if (!s.ok()) return s;
}
diff --git a/tests/cppunit/types/timeseries_test.cc
b/tests/cppunit/types/timeseries_test.cc
index 5937a9288..f49680842 100644
--- a/tests/cppunit/types/timeseries_test.cc
+++ b/tests/cppunit/types/timeseries_test.cc
@@ -19,6 +19,8 @@
*/
#include <gtest/gtest.h>
+#include <rocksdb/transaction_log.h>
+#include <rocksdb/write_batch.h>
#include <memory>
#include <random>
@@ -1319,3 +1321,112 @@ TEST_F(TimeSeriesTest, DelComprehensive) {
};
check(expected_samples);
}
+
+// Dedicated fixture with a large write_buffer_size so the WAL is not flushed
(and
+// therefore not archived/deleted) while the test inspects it. The shared
TestBase
+// uses write_buffer_size = 1, which flushes on nearly every write and would
make
+// WAL inspection racy.
+class TimeSeriesWALTest : public ::testing::Test {
+ protected:
+ void SetUp() override {
+ const char *path = "test_ts_wal.conf";
+ unlink(path);
+ std::ofstream output_file(path, std::ios::out);
+ output_file << "";
+
+ auto s = config_.Load(CLIOptions(path));
+ config_.db_dir = "testdb_ts_wal";
+ config_.rocks_db.compression = rocksdb::CompressionType::kNoCompression;
+ config_.rocks_db.write_buffer_size = 64;
+ storage_ = std::make_unique<engine::Storage>(&config_);
+ s = storage_->Open();
+ ASSERT_TRUE(s.IsOK());
+ ctx_ = std::make_unique<engine::Context>(storage_.get());
+ ts_db_ = std::make_unique<redis::TimeSeries>(storage_.get(),
"ts_namespace");
+ key_ = "test_ts_wal_key";
+ }
+
+ void TearDown() override {
+ ts_db_.reset();
+ ctx_.reset();
+ storage_.reset();
+ std::error_code ec;
+ std::filesystem::remove_all(config_.db_dir, ec);
+ unlink("test_ts_wal.conf");
+ }
+
+ Config config_;
+ std::unique_ptr<engine::Storage> storage_;
+ std::unique_ptr<engine::Context> ctx_;
+ std::unique_ptr<redis::TimeSeries> ts_db_;
+ std::string key_;
+};
+
+// Regression test: the first write to an empty time series must not emit a
delete
+// for the empty key. In upsertCommonInBatch, latest_chunk_key stays empty
when the
+// source series has no persisted chunk yet; the buggy condition still ran
+// batch->Delete(latest_chunk_key) because new_key != "", leaving an empty-key
+// tombstone in the WAL.
+TEST_F(TimeSeriesWALTest, FirstAddToEmptySeriesDoesNotDeleteEmptyKey) {
+ redis::TSCreateOption option;
+ option.retention_time = 3600;
+ option.chunk_size = 1024;
+
+ // An empty time series: metadata exists but no chunk has been persisted.
+ ASSERT_TRUE(ts_db_->Create(*ctx_, key_, option).ok());
+
+ // Sequence right before the first Add, so the WAL scan below covers only
this Add.
+ auto seq_before_add = storage_->LatestSeqNumber();
+
+ TSSample sample{1000, 12.3};
+ TSChunk::AddResult result;
+ ASSERT_TRUE(ts_db_->Add(*ctx_, key_, sample, option, &result).ok());
+
+ // Scan every WAL batch produced by the Add; none of them may delete the
empty key.
+ ASSERT_TRUE(storage_->WALHasNewData(seq_before_add + 1));
+ std::unique_ptr<rocksdb::TransactionLogIterator> iter;
+ ASSERT_TRUE(storage_->GetWALIter(seq_before_add + 1, &iter).IsOK());
+
+ // WriteBatch::Iterate aborts as soon as a record hits an un-overridden
Handler
+ // method (the base class returns InvalidArgument). The first Add emits puts
(and
+ // possibly range deletes) alongside the delete we care about, so override
every
+ // mutator as a no-op and only inspect DeleteCF, where the empty-key
tombstone
+ // would appear.
+ class EmptyKeyDeleteDetector : public rocksdb::WriteBatch::Handler {
+ public:
+ rocksdb::Status PutCF([[maybe_unused]] uint32_t column_family_id,
[[maybe_unused]] const rocksdb::Slice &key,
+ [[maybe_unused]] const rocksdb::Slice &value)
override {
+ return rocksdb::Status::OK();
+ }
+ rocksdb::Status MergeCF([[maybe_unused]] uint32_t column_family_id,
[[maybe_unused]] const rocksdb::Slice &key,
+ [[maybe_unused]] const rocksdb::Slice &value)
override {
+ return rocksdb::Status::OK();
+ }
+ rocksdb::Status SingleDeleteCF([[maybe_unused]] uint32_t column_family_id,
+ [[maybe_unused]] const rocksdb::Slice &key)
override {
+ return rocksdb::Status::OK();
+ }
+ rocksdb::Status DeleteRangeCF([[maybe_unused]] uint32_t column_family_id,
+ [[maybe_unused]] const rocksdb::Slice
&begin_key,
+ [[maybe_unused]] const rocksdb::Slice
&end_key) override {
+ return rocksdb::Status::OK();
+ }
+ rocksdb::Status DeleteCF([[maybe_unused]] uint32_t column_family_id, const
rocksdb::Slice &key) override {
+ if (key.empty()) found_empty_key_delete_ = true;
+ return rocksdb::Status::OK();
+ }
+ bool FoundEmptyKeyDelete() const { return found_empty_key_delete_; }
+
+ private:
+ bool found_empty_key_delete_ = false;
+ };
+
+ EmptyKeyDeleteDetector detector;
+ while (iter->Valid()) {
+ auto batch = iter->GetBatch();
+ ASSERT_TRUE(batch.writeBatchPtr->Iterate(&detector).ok());
+ iter->Next();
+ }
+
+ EXPECT_FALSE(detector.FoundEmptyKeyDelete()) << "first Add to an empty
series should not delete the empty key";
+}
