Skip to content

Commit

Permalink
compaction: fix handling of empty user key range deletes
Browse files Browse the repository at this point in the history
Previously, a range deletion beginning with an empty user key could
cause an infinite loop during a flush or compaction. If a compaction
output was empty and the rangedel fragmenter contained the tombstone
beginning with an empty user key, the compaction logic could confuse an
absent limit with the rangedel fragmenter's start key and skip writing
an output table.

Fix cockroachdb#1022.
  • Loading branch information
jbowens committed Dec 15, 2020
1 parent 100e1a4 commit 745f6c8
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 3 deletions.
8 changes: 5 additions & 3 deletions compaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -2047,9 +2047,11 @@ func (d *DB) runCompaction(
// then the sstable will only contain range tombstones. The smallest
// key in the sstable will be the start key of the first range
// tombstone added. We need to ensure that this start key is distinct
// from the limit (key) passed to finishOutput, otherwise we would
// generate an sstable where the largest key is smaller than the
// from the limit (key) passed to finishOutput (if set), otherwise we
// would generate an sstable where the largest key is smaller than the
// smallest key due to how the largest key boundary is set below.
// NB: It is permissible for the range tombstone start key to be the
// empty string.
// TODO: It is unfortunate that we have to do this check here rather
// than when we decide to finish the sstable in the runCompaction
// loop. A better structure currently eludes us.
Expand All @@ -2058,7 +2060,7 @@ func (d *DB) runCompaction(
if len(iter.tombstones) > 0 {
startKey = iter.tombstones[0].Start.UserKey
}
if d.cmp(startKey, key) == 0 {
if key != nil && d.cmp(startKey, key) == 0 {
return nil
}
}
Expand Down
10 changes: 10 additions & 0 deletions flush_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,3 +92,13 @@ func TestManualFlush(t *testing.T) {
}
})
}

// TestFlushDelRangeEmptyKey tests flushing a range tombstone that begins with
// an empty key. The empty key is a valid key but can be confused with nil.
func TestFlushDelRangeEmptyKey(t *testing.T) {
d, err := Open("", &Options{FS: vfs.NewMem()})
require.NoError(t, err)
require.NoError(t, d.DeleteRange([]byte{}, []byte("z"), nil))
require.NoError(t, d.Flush())
require.NoError(t, d.Close())
}

0 comments on commit 745f6c8

Please sign in to comment.