Skip to content

Commit fd89894

Browse files
damzou05020Ibrahim Jarif
authored
Compaction: Expired keys and delete markers are never purged (#1354)
The compaction process accidentally keeps one more version of expired keys and delete markers than it should, because of logic error in *levelsController.compactBuildTables(). When NumVersionsToKeep is 1, it means that expired keys and delete markers are never actually purged from the LSM tables. Co-authored-by: Julian Hegler <julian.hegler@tanium.com> Co-authored-by: Ibrahim Jarif <ibrahim@dgraph.io>
1 parent 543f353 commit fd89894

File tree

2 files changed

+212
-3
lines changed

2 files changed

+212
-3
lines changed

‎levels.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -585,13 +585,17 @@ func (s *levelsController) compactBuildTables(
585585
lastValidVersion := vs.Meta&bitDiscardEarlierVersions > 0 ||
586586
numVersions == s.kv.opt.NumVersionsToKeep
587587

588-
if isDeletedOrExpired(vs.Meta, vs.ExpiresAt) || lastValidVersion {
588+
isExpired := isDeletedOrExpired(vs.Meta, vs.ExpiresAt)
589+
590+
if isExpired || lastValidVersion {
589591
// If this version of the key is deleted or expired, skip all the rest of the
590592
// versions. Ensure that we're only removing versions below readTs.
591593
skipKey = y.SafeCopy(skipKey, it.Key())
592594

593595
switch {
594-
case lastValidVersion:
596+
// Add the key to the table only if it has not expired.
597+
// We don't want to add the deleted/expired keys.
598+
case !isExpired && lastValidVersion:
595599
// Add this key. We have set skipKey, so the following key versions
596600
// would be skipped.
597601
case hasOverlap:

‎levels_test.go

Lines changed: 206 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,11 +139,11 @@ func getAllAndCheck(t *testing.T, db *DB, expected []keyValVersion) {
139139
defer it.Close()
140140
i := 0
141141
for it.Rewind(); it.Valid(); it.Next() {
142-
require.Less(t, i, len(expected), "DB has more number of key than expected")
143142
item := it.Item()
144143
v, err := item.ValueCopy(nil)
145144
require.NoError(t, err)
146145
// fmt.Printf("k: %s v: %d val: %s\n", item.key, item.Version(), v)
146+
require.Less(t, i, len(expected), "DB has more number of key than expected")
147147
expect := expected[i]
148148
require.Equal(t, expect.key, string(item.Key()), "expected key: %s actual key: %s",
149149
expect.key, item.Key())
@@ -254,6 +254,211 @@ func TestCompaction(t *testing.T) {
254254
getAllAndCheck(t, db, []keyValVersion{{"foo", "bar", 3, 0}, {"fooz", "baz", 1, 0}})
255255
})
256256
})
257+
258+
t.Run("level 1 to level 2 with delete", func(t *testing.T) {
259+
t.Run("with overlap", func(t *testing.T) {
260+
runBadgerTest(t, &opt, func(t *testing.T, db *DB) {
261+
l1 := []keyValVersion{{"foo", "bar", 3, bitDelete}, {"fooz", "baz", 1, bitDelete}}
262+
l2 := []keyValVersion{{"foo", "bar", 2, 0}}
263+
l3 := []keyValVersion{{"foo", "bar", 1, 0}}
264+
createAndOpen(db, l1, 1)
265+
createAndOpen(db, l2, 2)
266+
createAndOpen(db, l3, 3)
267+
268+
// Set a high discard timestamp so that all the keys are below the discard timestamp.
269+
db.SetDiscardTs(10)
270+
271+
getAllAndCheck(t, db, []keyValVersion{
272+
{"foo", "bar", 3, 1},
273+
{"foo", "bar", 2, 0},
274+
{"foo", "bar", 1, 0},
275+
{"fooz", "baz", 1, 1},
276+
})
277+
cdef := compactDef{
278+
thisLevel: db.lc.levels[1],
279+
nextLevel: db.lc.levels[2],
280+
top: db.lc.levels[1].tables,
281+
bot: db.lc.levels[2].tables,
282+
}
283+
require.NoError(t, db.lc.runCompactDef(1, cdef))
284+
// foo bar version 2 should be dropped after compaction. fooz
285+
// baz version 1 will remain because overlap exists, which is
286+
// expected because `hasOverlap` is only checked once at the
287+
// beginning of `compactBuildTables` method.
288+
// everything from level 1 is now in level 2.
289+
getAllAndCheck(t, db, []keyValVersion{
290+
{"foo", "bar", 3, bitDelete},
291+
{"foo", "bar", 1, 0},
292+
{"fooz", "baz", 1, 1},
293+
})
294+
295+
cdef = compactDef{
296+
thisLevel: db.lc.levels[2],
297+
nextLevel: db.lc.levels[3],
298+
top: db.lc.levels[2].tables,
299+
bot: db.lc.levels[3].tables,
300+
}
301+
require.NoError(t, db.lc.runCompactDef(2, cdef))
302+
// everything should be removed now
303+
getAllAndCheck(t, db, []keyValVersion{})
304+
})
305+
})
306+
t.Run("without overlap", func(t *testing.T) {
307+
runBadgerTest(t, &opt, func(t *testing.T, db *DB) {
308+
l1 := []keyValVersion{{"foo", "bar", 3, bitDelete}, {"fooz", "baz", 1, bitDelete}}
309+
l2 := []keyValVersion{{"fooo", "barr", 2, 0}}
310+
createAndOpen(db, l1, 1)
311+
createAndOpen(db, l2, 2)
312+
313+
// Set a high discard timestamp so that all the keys are below the discard timestamp.
314+
db.SetDiscardTs(10)
315+
316+
getAllAndCheck(t, db, []keyValVersion{
317+
{"foo", "bar", 3, 1}, {"fooo", "barr", 2, 0}, {"fooz", "baz", 1, 1},
318+
})
319+
cdef := compactDef{
320+
thisLevel: db.lc.levels[1],
321+
nextLevel: db.lc.levels[2],
322+
top: db.lc.levels[1].tables,
323+
bot: db.lc.levels[2].tables,
324+
}
325+
require.NoError(t, db.lc.runCompactDef(1, cdef))
326+
// foo version 2 should be dropped after compaction.
327+
getAllAndCheck(t, db, []keyValVersion{{"fooo", "barr", 2, 0}})
328+
})
329+
})
330+
})
331+
}
332+
333+
func TestCompactionTwoVersions(t *testing.T) {
334+
// Disable compactions and keep two versions of each key.
335+
opt := DefaultOptions("").WithNumCompactors(0).WithNumVersionsToKeep(2)
336+
opt.managedTxns = true
337+
t.Run("with overlap", func(t *testing.T) {
338+
runBadgerTest(t, &opt, func(t *testing.T, db *DB) {
339+
l1 := []keyValVersion{{"foo", "bar", 3, 0}, {"fooz", "baz", 1, bitDelete}}
340+
l2 := []keyValVersion{{"foo", "bar", 2, 0}}
341+
l3 := []keyValVersion{{"foo", "bar", 1, 0}}
342+
createAndOpen(db, l1, 1)
343+
createAndOpen(db, l2, 2)
344+
createAndOpen(db, l3, 3)
345+
346+
// Set a high discard timestamp so that all the keys are below the discard timestamp.
347+
db.SetDiscardTs(10)
348+
349+
getAllAndCheck(t, db, []keyValVersion{
350+
{"foo", "bar", 3, 0},
351+
{"foo", "bar", 2, 0},
352+
{"foo", "bar", 1, 0},
353+
{"fooz", "baz", 1, 1},
354+
})
355+
cdef := compactDef{
356+
thisLevel: db.lc.levels[1],
357+
nextLevel: db.lc.levels[2],
358+
top: db.lc.levels[1].tables,
359+
bot: db.lc.levels[2].tables,
360+
}
361+
require.NoError(t, db.lc.runCompactDef(1, cdef))
362+
// Nothing should be dropped after compaction because number of
363+
// versions to keep is 2.
364+
getAllAndCheck(t, db, []keyValVersion{
365+
{"foo", "bar", 3, 0},
366+
{"foo", "bar", 2, 0},
367+
{"foo", "bar", 1, 0},
368+
{"fooz", "baz", 1, 1},
369+
})
370+
371+
cdef = compactDef{
372+
thisLevel: db.lc.levels[2],
373+
nextLevel: db.lc.levels[3],
374+
top: db.lc.levels[2].tables,
375+
bot: db.lc.levels[3].tables,
376+
}
377+
require.NoError(t, db.lc.runCompactDef(2, cdef))
378+
getAllAndCheck(t, db, []keyValVersion{
379+
{"foo", "bar", 3, 0},
380+
{"foo", "bar", 2, 0},
381+
})
382+
})
383+
})
384+
}
385+
386+
func TestCompactionAllVersions(t *testing.T) {
387+
// Disable compactions and keep all versions of the each key.
388+
opt := DefaultOptions("").WithNumCompactors(0).WithNumVersionsToKeep(math.MaxInt32)
389+
opt.managedTxns = true
390+
t.Run("without overlap", func(t *testing.T) {
391+
runBadgerTest(t, &opt, func(t *testing.T, db *DB) {
392+
l1 := []keyValVersion{{"foo", "bar", 3, 0}, {"fooz", "baz", 1, bitDelete}}
393+
l2 := []keyValVersion{{"foo", "bar", 2, 0}}
394+
l3 := []keyValVersion{{"foo", "bar", 1, 0}}
395+
createAndOpen(db, l1, 1)
396+
createAndOpen(db, l2, 2)
397+
createAndOpen(db, l3, 3)
398+
399+
// Set a high discard timestamp so that all the keys are below the discard timestamp.
400+
db.SetDiscardTs(10)
401+
402+
getAllAndCheck(t, db, []keyValVersion{
403+
{"foo", "bar", 3, 0},
404+
{"foo", "bar", 2, 0},
405+
{"foo", "bar", 1, 0},
406+
{"fooz", "baz", 1, 1},
407+
})
408+
cdef := compactDef{
409+
thisLevel: db.lc.levels[1],
410+
nextLevel: db.lc.levels[2],
411+
top: db.lc.levels[1].tables,
412+
bot: db.lc.levels[2].tables,
413+
}
414+
require.NoError(t, db.lc.runCompactDef(1, cdef))
415+
// Nothing should be dropped after compaction because all versions
416+
// should be kept.
417+
getAllAndCheck(t, db, []keyValVersion{
418+
{"foo", "bar", 3, 0},
419+
{"foo", "bar", 2, 0},
420+
{"foo", "bar", 1, 0},
421+
{"fooz", "baz", 1, 1},
422+
})
423+
424+
cdef = compactDef{
425+
thisLevel: db.lc.levels[2],
426+
nextLevel: db.lc.levels[3],
427+
top: db.lc.levels[2].tables,
428+
bot: db.lc.levels[3].tables,
429+
}
430+
require.NoError(t, db.lc.runCompactDef(2, cdef))
431+
getAllAndCheck(t, db, []keyValVersion{
432+
{"foo", "bar", 3, 0},
433+
{"foo", "bar", 2, 0},
434+
{"foo", "bar", 1, 0},
435+
})
436+
})
437+
})
438+
t.Run("without overlap", func(t *testing.T) {
439+
runBadgerTest(t, &opt, func(t *testing.T, db *DB) {
440+
l1 := []keyValVersion{{"foo", "bar", 3, bitDelete}, {"fooz", "baz", 1, bitDelete}}
441+
l2 := []keyValVersion{{"fooo", "barr", 2, 0}}
442+
createAndOpen(db, l1, 1)
443+
createAndOpen(db, l2, 2)
444+
445+
// Set a high discard timestamp so that all the keys are below the discard timestamp.
446+
db.SetDiscardTs(10)
447+
448+
getAllAndCheck(t, db, []keyValVersion{
449+
{"foo", "bar", 3, 1}, {"fooo", "barr", 2, 0}, {"fooz", "baz", 1, 1},
450+
})
451+
cdef := compactDef{
452+
thisLevel: db.lc.levels[1],
453+
nextLevel: db.lc.levels[2],
454+
top: db.lc.levels[1].tables,
455+
bot: db.lc.levels[2].tables,
456+
}
457+
require.NoError(t, db.lc.runCompactDef(1, cdef))
458+
// foo version 2 should be dropped after compaction.
459+
getAllAndCheck(t, db, []keyValVersion{{"fooo", "barr", 2, 0}})
460+
})
461+
})
257462
}
258463

259464
func TestHeadKeyCleanup(t *testing.T) {

0 commit comments

Comments
 (0)