Skip to content

Commit 74e3757

Browse files
committed
Issue #755: fix the bug in maxPhysicalDebt and invariant
The prototype had a bug in the calculation of `maxPhysicalDebt` where it was not considering that incoming runs that are inputs to a merge can be overfull. Runs can be overfull because we hold back runs that are underfull. However, runs should only be overfull in the sense that they are too large for their current level. They can't be overfull with respect to the next level too. This same bug occurred in `invariant`, where a similar change was made to consider overfull runs properly. Some other minor changes to `invariant` were made to make the checked invariant a little more precise.
1 parent a63081e commit 74e3757

File tree

1 file changed

+33
-17
lines changed

1 file changed

+33
-17
lines changed

src-prototypes/ScheduledMerges.hs

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -353,7 +353,7 @@ invariant conf@LSMConfig{..} (LSMContent _ levels ul) = do
353353
-- levelling run becomes too large and is promoted, in that case
354354
-- initially there's no merge, but it is still represented as an
355355
-- 'IncomingRun', using 'Single'. Thus there are no other resident runs.
356-
MergePolicyLevelling -> assertST $ null rs
356+
MergePolicyLevelling -> assertST $ null rs && null ls
357357
-- Runs in tiering levels usually fit that size, but they can be one
358358
-- larger, if a run has been held back (creating a (T+1)-way merge).
359359
MergePolicyTiering -> assertST $ all (\r -> runToLevelNumber MergePolicyTiering conf r `elem` [ln, ln+1]) rs
@@ -383,18 +383,19 @@ invariant conf@LSMConfig{..} (LSMContent _ levels ul) = do
383383
(_, CompletedMerge r) ->
384384
assertST $ runToLevelNumber MergePolicyLevelling conf r <= ln+1
385385

386-
-- An ongoing merge for levelling should have T incoming runs of
387-
-- the right size for the level below (or slightly larger due to
388-
-- holding back underfull runs), and 1 run from this level,
389-
-- but the run from this level can be of almost any size for the
390-
-- same reasons as above. Although if this is the first merge for
391-
-- a new level, it'll have only T runs.
386+
-- An ongoing merge for levelling should have T incoming runs of the
387+
-- right size for the level below (or slightly larger due to holding
388+
-- back underfull runs), and at most 1 run from this level. The run
389+
-- from this level can be of almost any size for the same reasons as
390+
-- above. Although if this is the first merge for a new level, it'll
391+
-- have only T runs.
392392
(_, OngoingMerge _ rs _) -> do
393393
assertST $ length rs `elem` [configSizeRatio, configSizeRatio + 1]
394394
assertST $ all (\r -> runSize r > 0) rs -- don't merge empty runs
395395
let incoming = take configSizeRatio rs
396396
let resident = drop configSizeRatio rs
397397
assertST $ all (\r -> runToLevelNumber MergePolicyTiering conf r `elem` [ln-1, ln]) incoming
398+
assertST $ length resident `elem` [0, 1]
398399
assertST $ all (\r -> runToLevelNumber MergePolicyLevelling conf r <= ln+1) resident
399400

400401
MergePolicyTiering ->
@@ -421,13 +422,19 @@ invariant conf@LSMConfig{..} (LSMContent _ levels ul) = do
421422
(_, CompletedMerge r, MergeMidLevel) ->
422423
assertST $ runToLevelNumber MergePolicyTiering conf r `elem` [ln-1, ln, ln+1]
423424

424-
-- An ongoing merge for tiering should have T incoming runs of
425-
-- the right size for the level below, and at most 1 run held back
426-
-- due to being too small (which would thus also be of the size of
427-
-- the level below).
425+
-- An ongoing merge for tiering should have T incoming runs of the
426+
-- right size for the level below (or slightly larger due to holding
427+
-- back underfull runs), and at most 1 run held back due to being
428+
-- too small (which would thus also be of the size of the level
429+
-- below).
428430
(_, OngoingMerge _ rs _, _) -> do
429-
assertST $ length rs == configSizeRatio || length rs == configSizeRatio + 1
430-
assertST $ all (\r -> runToLevelNumber MergePolicyTiering conf r == ln-1) rs
431+
assertST $ length rs `elem` [configSizeRatio, configSizeRatio + 1]
432+
assertST $ all (\r -> runSize r > 0) rs -- don't merge empty runs
433+
let incoming = take configSizeRatio rs
434+
let heldBack = drop configSizeRatio rs
435+
assertST $ all (\r -> runToLevelNumber MergePolicyTiering conf r `elem` [ln-1, ln]) incoming
436+
assertST $ length heldBack `elem` [0, 1]
437+
assertST $ all (\r -> runToLevelNumber MergePolicyTiering conf r == ln-1) heldBack
431438

432439
-- We don't make many assumptions apart from what the types already enforce.
433440
-- In particular, there are no invariants on the progress of the merges,
@@ -1499,15 +1506,24 @@ newLevelMerge tr conf@LSMConfig{..} level mergePolicy mergeType rs = do
14991506
-- such that we pay off the physical and nominal debts at the same time.
15001507
--
15011508
-- We can bound the worst case physical debt: this is the maximum amount of
1502-
-- steps a merge at this level could need. Note that for levelling this is
1503-
-- includes the single run in the current level.
1509+
-- steps a merge at this level could need. See the
1510+
-- 'expectedMergingRunLengths' where-clause of the 'invariant' function for
1511+
-- the full reasoning.
15041512
maxPhysicalDebt =
15051513
case mergePolicy of
15061514
MergePolicyLevelling ->
1507-
configSizeRatio * levelNumberToMaxRunSize MergePolicyTiering conf (level-1)
1515+
-- Incoming runs, which may be slightly overfull with respect to the
1516+
-- previous level
1517+
configSizeRatio * levelNumberToMaxRunSize MergePolicyTiering conf level
1518+
-- The single run that was already on this level
15081519
+ levelNumberToMaxRunSize MergePolicyLevelling conf level
15091520
MergePolicyTiering ->
1510-
length rs * levelNumberToMaxRunSize MergePolicyTiering conf (level-1)
1521+
-- Incoming runs, which may be slightly overfull with respect to the
1522+
-- previous level
1523+
configSizeRatio * levelNumberToMaxRunSize MergePolicyTiering conf level
1524+
-- Held back run that is underfull with respect to the current
1525+
-- level
1526+
+ levelNumberToMaxRunSize MergePolicyTiering conf (level - 1)
15111527

15121528
-------------------------------------------------------------------------------
15131529
-- MergingTree abstraction

0 commit comments

Comments
 (0)