Skip to content

Fix underflow dirty size #401

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
liunyl merged 1 commit into from
Feb 12, 2026
Merged

Fix underflow dirty size #401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions tx_service/include/cc/object_cc_map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1970,7 +1970,7 @@ class ObjectCcMap : public TemplateCcMap<KeyT, ValueT, false, false>
cce->payload_.cur_payload_ == nullptr
? RecordStatus::Deleted
: RecordStatus::Normal;
bool was_dirty = (cce->CommitTs() > 1 && !cce->IsPersistent());
bool was_dirty = cce->IsDirty();
cce->SetCommitTsPayloadStatus(commit_ts, payload_status);
this->OnCommittedUpdate(cce, was_dirty);
if (s_obj_exist && payload_status != RecordStatus::Normal)
Expand Down Expand Up @@ -2006,7 +2006,7 @@ class ObjectCcMap : public TemplateCcMap<KeyT, ValueT, false, false>

// Emplace txn_cmd and try to commit all pending commands.
int64_t buffered_cmd_cnt_old = buffered_cmd_list.Size();
bool was_dirty = (cce->CommitTs() > 1 && !cce->IsPersistent());
bool was_dirty = cce->IsDirty();
cce->EmplaceAndCommitBufferedTxnCommand(
txn_cmd, shard_->NowInMilliseconds());
this->OnCommittedUpdate(cce, was_dirty);
Expand Down
18 changes: 11 additions & 7 deletions tx_service/include/cc/template_cc_map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10148,13 +10148,17 @@ class TemplateCcMap : public CcMap
location_infos.emplace_back(key_idx_in_page, false);
// Don't need to empalce key. But we need to update entry's
// payload
target_page->Entry(key_idx_in_page)
->UpdateCcEntry(slice_items[item_idx],
shard_->EnableMvcc(),
normal_rec_change,
target_page,
shard_,
shard_->NowInMilliseconds());
auto *target_cce = target_page->Entry(key_idx_in_page);
bool was_dirty = target_cce->IsDirty();
target_cce->UpdateCcEntry(slice_items[item_idx],
shard_->EnableMvcc(),
normal_rec_change,
target_page,
shard_,
shard_->NowInMilliseconds());
// Keep shard-level dirty-key stats in sync for overwrite path.
OnFlushed(target_cce, was_dirty);
OnCommittedUpdate(target_cce, was_dirty);
item_idx++;
key_idx_in_page++;
}
Expand Down
12 changes: 8 additions & 4 deletions tx_service/src/cc/cc_req_misc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1212,22 +1212,24 @@ bool UpdateCceCkptTsCc::Execute(CcShard &ccs)
static_cast<VersionedLruEntry<true, true> *>(ref.cce_);

assert(v_entry->CommitTs() > 1 && !v_entry->IsPersistent());
bool was_dirty = v_entry->IsDirty();
v_entry->entry_info_.SetDataStoreSize(ref.post_flush_size_);

v_entry->SetCkptTs(ref.commit_ts_);
v_entry->ClearBeingCkpt();
ccm->OnEntryFlushed(true, v_entry->IsPersistent());
ccm->OnEntryFlushed(was_dirty, v_entry->IsPersistent());
}
else
{
VersionedLruEntry<false, true> *v_entry =
static_cast<VersionedLruEntry<false, true> *>(ref.cce_);
assert(v_entry->CommitTs() > 1 && !v_entry->IsPersistent());
bool was_dirty = v_entry->IsDirty();
v_entry->entry_info_.SetDataStoreSize(ref.post_flush_size_);

v_entry->SetCkptTs(ref.commit_ts_);
v_entry->ClearBeingCkpt();
ccm->OnEntryFlushed(true, v_entry->IsPersistent());
ccm->OnEntryFlushed(was_dirty, v_entry->IsPersistent());
}
}
else
Expand All @@ -1238,19 +1240,21 @@ bool UpdateCceCkptTsCc::Execute(CcShard &ccs)
static_cast<VersionedLruEntry<true, false> *>(ref.cce_);

assert(v_entry->CommitTs() > 1 && !v_entry->IsPersistent());
bool was_dirty = v_entry->IsDirty();
v_entry->SetCkptTs(ref.commit_ts_);
v_entry->ClearBeingCkpt();
ccm->OnEntryFlushed(true, v_entry->IsPersistent());
ccm->OnEntryFlushed(was_dirty, v_entry->IsPersistent());
}
else
{
VersionedLruEntry<false, false> *v_entry =
static_cast<VersionedLruEntry<false, false> *>(ref.cce_);

assert(v_entry->CommitTs() > 1 && !v_entry->IsPersistent());
bool was_dirty = v_entry->IsDirty();
v_entry->SetCkptTs(ref.commit_ts_);
v_entry->ClearBeingCkpt();
ccm->OnEntryFlushed(true, v_entry->IsPersistent());
ccm->OnEntryFlushed(was_dirty, v_entry->IsPersistent());
}
}
}
Expand Down
27 changes: 23 additions & 4 deletions tx_service/src/cc/cc_shard.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,16 +390,35 @@ void CcShard::AdjustDataKeyStats(const TableName &table_name,
{
assert(size_delta >= 0 ||
data_key_count_ >= static_cast<size_t>(-size_delta));
data_key_count_ = static_cast<size_t>(
static_cast<int64_t>(data_key_count_) + size_delta);
int64_t new_size = static_cast<int64_t>(data_key_count_) + size_delta;
if (new_size < 0)
{
data_key_count_ = 0;
}
else
{
data_key_count_ = static_cast<size_t>(new_size);
}
}

if (dirty_delta != 0)
{
// Sanity check in debug mode.
assert(dirty_delta >= 0 ||
dirty_data_key_count_ >= static_cast<size_t>(-dirty_delta));
dirty_data_key_count_ = static_cast<size_t>(
static_cast<int64_t>(dirty_data_key_count_) + dirty_delta);
// Clamp to avoid underflow when an entry is flushed but was never
// counted dirty (e.g. became dirty via a path that doesn't call
// OnCommittedUpdate).
int64_t new_dirty =
static_cast<int64_t>(dirty_data_key_count_) + dirty_delta;
if (new_dirty < 0)
{
dirty_data_key_count_ = 0;
}
else
{
dirty_data_key_count_ = static_cast<size_t>(new_dirty);
}
Comment on lines +406 to +421
Copy link

@coderabbitai coderabbitai bot Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Avoid debug-only aborts if underflow is now a tolerated path.

You clamp dirty_data_key_count_ to zero, but the existing assert will still fire in debug/test runs when the “flushed but never counted dirty” scenario happens. If this is now expected, consider downgrading the assert to a warning so debug builds don’t fail.

🔧 Proposed adjustment 
-        // Sanity check in debug mode.
-        assert(dirty_delta >= 0 ||
-               dirty_data_key_count_ >= static_cast<size_t>(-dirty_delta));
+        if (dirty_delta < 0 &&
+            dirty_data_key_count_ < static_cast<size_t>(-dirty_delta))
+        {
+            DLOG(WARNING) << "dirty_data_key_count_ underflow; clamping to 0";
+        }
         // Clamp to avoid underflow when an entry is flushed but was never
         // counted dirty (e.g. became dirty via a path that doesn't call
         // OnCommittedUpdate).
         int64_t new_dirty =
             static_cast<int64_t>(dirty_data_key_count_) + dirty_delta;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Sanity check in debug mode.
assert(dirty_delta >= 0 ||
dirty_data_key_count_ >= static_cast<size_t>(-dirty_delta));
dirty_data_key_count_ = static_cast<size_t>(
static_cast<int64_t>(dirty_data_key_count_) + dirty_delta);
// Clamp to avoid underflow when an entry is flushed but was never
// counted dirty (e.g. became dirty via a path that doesn't call
// OnCommittedUpdate).
int64_t new_dirty =
static_cast<int64_t>(dirty_data_key_count_) + dirty_delta;
if (new_dirty < 0)
{
dirty_data_key_count_ = 0;
}
else
{
dirty_data_key_count_ = static_cast<size_t>(new_dirty);
}
// Clamp to avoid underflow when an entry is flushed but was never
// counted dirty (e.g. became dirty via a path that doesn't call
// OnCommittedUpdate).
if (dirty_delta < 0 &&
dirty_data_key_count_ < static_cast<size_t>(-dirty_delta))
{
DLOG(WARNING) << "dirty_data_key_count_ underflow; clamping to 0";
}
int64_t new_dirty =
static_cast<int64_t>(dirty_data_key_count_) + dirty_delta;
if (new_dirty < 0)
{
dirty_data_key_count_ = 0;
}
else
{
dirty_data_key_count_ = static_cast<size_t>(new_dirty);
}
🤖 Prompt for AI Agents 
In `@tx_service/src/cc/cc_shard.cpp` around lines 406 - 421, The assert guarding
the underflow case in the dirty count path (assert(dirty_delta >= 0 ||
dirty_data_key_count_ >= static_cast<size_t>(-dirty_delta))) must be relaxed
because underflow can legitimately occur when an entry is flushed but was never
counted dirty (see OnCommittedUpdate flows); replace the assert with a runtime
check that logs a warning via the appropriate logger (mentioning
dirty_data_key_count_ and dirty_delta) instead of aborting in debug builds, then
continue with the existing clamp logic that computes new_dirty and sets
dirty_data_key_count_ accordingly.

}
}

Expand Down