Bound the noncurrent and orphan-delete-marker listing cursors - #2696
Draft
delthas wants to merge 1 commit into
Draft
Bound the noncurrent and orphan-delete-marker listing cursors#2696delthas wants to merge 1 commit into
delthas wants to merge 1 commit into
Conversation
DelimiterNonCurrent and DelimiterOrphanDeleteMarker stop themselves once filter() has seen maxScannedLifecycleListingEntries entries, but set no limit on the backend cursor. Everything the cursor returns past that point is read and thrown away. On a bucket whose pages return no results the budget is spent in full on every page, and behind mongos the shard cursor is opened with no batchSize and fills a 16MB batch each time: ~13,300 documents read per page against ~10,000 consumed. Bound the cursor at maxScannedLifecycleListingEntries + 1, in DelimiterVersions so both listings inherit it. In v1 the bound goes on BOTH param sets: the two ranges are served by separate cursors and either one can supply every entry the listing consumes, so each carries the full bound rather than a share of it. The bound is +1 and carries no v0 doubling. filter() counts every entry the cursor delivers, masters and versions alike, in both formats, and returns FILTER_END on the entry after the budget is spent. DelimiterCurrent multiplies by 2 on v0 only because its counter lives in addContents(), which v0 version keys never reach; copying that factor here would double the bound for nothing. Its comment is corrected to say so, since it currently claims the opposite. Getting the bound wrong is not a perf regression. A cursor that ends early is indistinguishable from the end of the keyspace, and DelimiterOrphanDeleteMarker.result() flushes its held candidate whenever the scan budget was not reached -- so an under-sized bound reports a delete marker as orphaned while unseen versions sit behind the cursor, and lifecycle then deletes a marker that resurrects an old version. Tests: unit coverage for the params on v0 and v1 and for the invariant the bound rests on (the listing reaches FILTER_END within `limit` entries), plus a functional spec paging real MongoDB listings to exhaustion across a sweep of scan budgets. That directory had no maxScannedLifecycleListingEntries coverage at all. Expected effect is ~25% fewer documents read per page (13,342 -> 10,001 at the default budget), not the 13x the ticket first claimed: the only safe bound is the scan budget, not MaxKeys. Issue: ARSN-629
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## bugfix/ARSN-620/lc-phd #2696 +/- ##
=======================================================
Coverage 74.75% 74.76%
=======================================================
Files 229 229
Lines 18715 18726 +11
Branches 3898 3871 -27
=======================================================
+ Hits 13991 14001 +10
- Misses 4719 4720 +1
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DelimiterNonCurrentandDelimiterOrphanDeleteMarkerstop themselves oncefilter()has seenmaxScannedLifecycleListingEntriesentries, but set no limit on the backend cursor. Everything the cursor returns past that point is read and thrown away. On a bucket whose pages return no results the budget is spent in full on every page, and behind mongos the shard cursor is opened with nobatchSizeand fills a 16MB batch each time: ~13,300 documents read per page against ~10,000 consumed.Stacked on #2685 (ARSN-620), which must merge first. Based on
bugfix/ARSN-620/lc-phdso the diff shows only this commit. A bound makes pages truncate on essentially every page of an orphan sweep, and truncation is only safe once the resume marker reliably advances.The change
_cursorLimit()inDelimiterVersions, returningmaxScannedLifecycleListingEntries + 1, applied ingenMDParamsV0()and to both param sets ingenMDParamsV1()— the two ranges are served by separate cursors and either one can supply every entry the listing consumes, so each carries the full bound rather than a share of it. Placed in the base class so both listings inherit it and the reasoning lives in one place.No v0 doubling, and this matters.
filter()counts every entry the cursor delivers, masters and versions alike, in both formats.DelimiterCurrentmultiplies by 2 on v0 only because its counter lives inaddContents(), which v0 version keys never reach — copying that factor here would double the bound for nothing. Its comment currently claims the counter "maps directly to cursor documents regardless of bucket format", which the ×2 block below it contradicts; corrected as a drive-by so the next person doesn't copy it.Getting the bound wrong is not a perf regression. A cursor that ends early is indistinguishable from the end of the keyspace, and
DelimiterOrphanDeleteMarker.result()flushes its held candidate whenever the scan budget was not reached — so an under-sized bound reports a delete marker as orphaned while unseen versions sit behind the cursor, and lifecycle then deletes a marker that resurrects an old version.On the expected benefit
~25% fewer documents read per page (13,342 → 10,001 at the default budget), not the 13x ARSN-629 originally claimed. The only safe bound is the scan budget, not
MaxKeys: nothing ties raw entries consumed toMaxKeysfor these two listings — 10,000 single-version keys return zero results while burning the whole budget. The ticket's 13x was computed againstMaxKeys + 1 = 1,001, which is not a bound these listings can honour.batchSizeis the complementary lever and buys the same ~25% on this workload, since the dominant orphan-sweep pages consume the full budget; noted for a separate ticket.Pre-existing bugs surfaced, not fixed here
Confirmed by re-running the full budget matrix with
_cursorLimit()stashed — identical results, so neither is caused by this change. Both are documented in the spec where the sweep skips them:DelimiterNonCurrentcannot advance at very small budgets: resuming re-reads the marker version to recover the stale date, so the budget is consumed by skipped entries plus that re-read andNextVersionIdMarkernever moves. The listing pages forever.DelimiterOrphanDeleteMarkersilently loses the last orphan when the keyspace ends on the exact entry the budget ends.filter()never sees the entry that would raiseFILTER_END, soIsTruncatedstaysfalsewhile_isMaxScannedEntriesReached()is alreadytrue—result()then suppresses the held candidate and reports the listing complete. The orphan is dropped permanently, with no truncation to recover it. Same failure shape as the hazard this PR guards against, reached through the scan budget instead of the cursor. Worth its own ticket.Notes for review
development/8.5is further along, and ARSN-620 Fix lifecycle listings stuck on PHD master keys #2685 carries no bump either. Whoever rebases the stack sets the version.tests/unitFAIL list on this branch (GapCache, MergeStream, server, MongoClientInterface, putObject) is byte-identical to the clean ARSN-620 tree.Issue: ARSN-629