Compare commits

...
Author SHA1 Message Date
Xingbo Wang 131359330f Wait for in-flight secondary read-view reclamation
Summary:
Why the change is required:

Secondary catch-up must release reader-free retired read views before scanning
for obsolete files. T290043099 and duplicate T290043100 exposed a race where
that ordering was lost, leaving two obsolete table files open and failing
DBSecondaryTest.SecondaryCloseFiles. The assertion failure also exposed unsafe
test cleanup because the secondary DB could outlive the test-local Env.

Previous behavior:

Retiring a read view always woke the background reclaimer. The worker could
remove a reader-free view from the retirement queue and pause before destroying
its SuperVersions. Synchronous cleanup then saw no queued work, ignored the
in-flight reclamation, and continued to obsolete-file discovery while the old
files were still referenced. The test also used a static lambda that captured
the first fixture across repeated runs and closed the secondary only on the
success path.

New behavior:

Catch-up synchronously reclaims reader-free views, or waits when a competing
background worker has already claimed them. Views still held by active readers
remain non-blocking and are reclaimed asynchronously after the last reader
releases them. The test now closes the secondary before its local Env on every
exit path and creates its verification lambda per test invocation.

Implementation:

Publishing a replacement view no longer wakes the background reclaimer; the
immediately following synchronous cleanup owns that work. Reader release still
wakes the worker. Cleanup waits on the existing in-flight counter when a worker
won an earlier race. The test uses a Defer guard for lifetime ordering and
removes the repeat-unsafe static lambda.

Safety, compatibility, and limitations:

This changes no public API, ABI, file format, recovery behavior, or read
consistency. Cleanup does not wait for views retained by active readers, so the
non-blocking multi-column-family iterator behavior is preserved. It waits only
for reclamation already in progress. Validation covered the internal Linux
ASAN/UBSAN target; the external GitHub Actions matrix was not run. Repository
make-based format/source checks require a Git worktree and were unavailable in
this Sapling checkout.

Reviewed By: jaykorean

Differential Revision: D121816425
2026-09-29 15:31:36 -07:00
2 changed files with 9 additions and 2 deletions

No files matched your search

+8 -1
View File
@@ -100,7 +100,11 @@ void DBImplSecondary::RetireSecondaryReadView(
std::lock_guard<std::mutex> lock(retired_secondary_read_views_mutex_);
retired_secondary_read_views_.emplace_back(std::move(read_view));
}
retired_secondary_read_views_cv_.notify_all();
// Publish and close synchronously reclaim after retiring a view. Do not wake
// the background reclaimer here: it could take a reader-free view first and
// let catch-up scan for obsolete files before releasing the view's
// SuperVersions. A reader that makes a retained view reclaimable later wakes
// the reclaimer in ReleaseSecondaryReadView().
}
bool DBImplSecondary::HasReclaimableSecondaryReadView() const {
@@ -180,6 +184,9 @@ void DBImplSecondary::ReclaimRetiredSecondaryReadViewsLoop() {
void DBImplSecondary::CleanupRetiredSecondaryReadViews() {
ReclaimRetiredSecondaryReadViews();
std::unique_lock<std::mutex> lock(retired_secondary_read_views_mutex_);
retired_secondary_read_views_cv_.wait(
lock, [this]() { return secondary_read_view_reclaims_in_flight_ == 0; });
}
void DBImplSecondary::DrainRetiredSecondaryReadViews() {
+1 -1
View File
@@ -1021,7 +1021,7 @@ TEST_F(DBSecondaryTest, SecondaryCloseFiles) {
// Close the secondary now, before traced_env is destroyed, on every path.
Defer close_secondary([this]() { CloseSecondary(); });
static const auto verify_db = [&]() {
const auto verify_db = [&]() {
std::unique_ptr<Iterator> iter1(dbfull()->NewIterator(ReadOptions()));
std::unique_ptr<Iterator> iter2(db_secondary_->NewIterator(ReadOptions()));
for (iter1->SeekToFirst(), iter2->SeekToFirst();