Repository: prometheus/tsdb
Pull request #560 (2019-03-19 03:24:48)

A simpler solution to #559 and fix comments in compact_test.go
fixes https://github.com/prometheus/tsdb/issues/554

alternative approach to https://github.com/prometheus/tsdb/issues/554 


krasi-georgiev: @naivewong nice! Didn't think about that approach.

Although it is more efficient I find it a bit more difficult to follow.

If you don't mind I would wait for one more opinion and will decide which one to merge 
codesome: Sorry for catching this late, both this PR and #559 are slightly off from what is desired.

Taking example of this comment again https://github.com/prometheus/tsdb/pull/550#issuecomment-473490971, this is how it should work :
Only overlapping chain of blocks should be merged, so {1,2} should be picked from this set of blocks. And later {3,4} would get picked (`compact()` runs a loop). Note that {1,2} and {3,4} don't overlap. So the result should be {1,2} and {3,4}, and not {1,2,3,4}. 

I think this can be done by just breaking the loop where we are setting `chained` to `false`.

With current fix (and even before this fix!), we can run into an issue of compacting all the blocks into a single block (If not clear how, I can give example). That's a serious bug that you found out @naivewong, thanks!
codesome: @naivewong you have got some conflicts.
bwplotka: Where are we actually fixing https://github.com/prometheus/tsdb/issues/554 ?
krasi-georgiev: > Where are we actually fixing #554 ?


with this one

```
metaRange("1", 0, 20, nil),
metaRange("2", 10, 30, nil),
metaRange("3", 40, 60, nil),
metaRange("4", 50, 70, nil),
```

codesome: @bwplotka Description of #554 needs updating, so the issue is `selectOverlappingDirs` selects wrong dirs where there are 2 disjoint sets of overlapping dirs, and does not exactly miss any dir.
krasi-georgiev: @bwplotka 	do you still any problems in this PR?

Patch:
diff --git a/.travis.yml b/.travis.yml
index c146e02..6af68c3 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -6,8 +6,6 @@ os:
   - osx
 
 go:
-  - 1.10.x
-  - 1.11.x
   - 1.12.x
 
 go_import_path: github.com/prometheus/tsdb
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 80f5023..c3c8a62 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,8 +1,12 @@
 ## master / unreleased
+ - [CHANGE] tsdb now requires golang 1.12 or higher.
  - [REMOVED] `chunks.NewReader` is removed as it wasn't used anywhere.
  - [REMOVED] `FromData` is considered unused so was removed.
  - [FEATURE] Added option WALSegmentSize -1 to disable the WAL.
+ - [BUGFIX] Bugfix in selectOverlappingDirs. Only return the first overlapping blocks.
  - [BUGFIX] Fsync the meta file to persist it on disk to avoid data loss in case of a host crash.
+ - [BUGFIX] Fix fd and vm_area leak on error path in chunks.NewDirReader.
+ - [BUGFIX] Fix fd and vm_area leak on error path in index.NewFileReader.
 
 ## 0.6.1
   - [BUGFIX] Update `last` after appending a non-overlapping chunk in `chunks.MergeOverlappingChunks`. [#539](https://github.com/prometheus/tsdb/pull/539)
diff --git a/chunks/chunks.go b/chunks/chunks.go
index 9e75407..70cb119 100644
--- a/chunks/chunks.go
+++ b/chunks/chunks.go
@@ -139,7 +139,7 @@ func (w *Writer) finalizeTail() error {
 	if err := w.wbuf.Flush(); err != nil {
 		return err
 	}
-	if err := fileutil.Fsync(tf); err != nil {
+	if err := tf.Sync(); err != nil {
 		return err
 	}
 	// As the file was pre-allocated, we truncate any superfluous zero bytes.
@@ -408,13 +408,16 @@ func NewDirReader(dir string, pool chunkenc.Pool) (*Reader, error) {
 	}
 
 	var (
-		bs []ByteSlice
-		cs []io.Closer
+		bs   []ByteSlice
+		cs   []io.Closer
+		merr tsdb_errors.MultiError
 	)
 	for _, fn := range files {
 		f, err := fileutil.OpenMmapFile(fn)
 		if err != nil {
-			return nil, errors.Wrapf(err, "mmap files")
+			merr.Add(errors.Wrap(err, "mmap files"))
+			merr.Add(closeAll(cs))
+			return nil, merr
 		}
 		cs = append(cs, f)
 		bs = append(bs, realByteSlice(f.Bytes()))
@@ -422,7 +425,6 @@ func NewDirReader(dir string, pool chunkenc.Pool) (*Reader, error) {
 
 	reader, err := newReader(bs, cs, pool)
 	if err != nil {
-		var merr tsdb_errors.MultiError
 		merr.Add(err)
 		merr.Add(closeAll(cs))
 		return nil, merr
diff --git a/compact.go b/compact.go
index c8d293f..4a360d0 100644
--- a/compact.go
+++ b/compact.go
@@ -267,8 +267,8 @@ func (c *LeveledCompactor) selectDirs(ds []dirMeta) []dirMeta {
 	return nil
 }
 
-// selectOverlappingDirs returns all dirs with overlaping time ranges.
-// It expects sorted input by mint.
+// selectOverlappingDirs returns all dirs with overlapping time ranges.
+// It expects sorted input by mint and returns the overlapping dirs in the same order as received.
 func (c *LeveledCompactor) selectOverlappingDirs(ds []dirMeta) []string {
 	if len(ds) < 2 {
 		return nil
@@ -281,6 +281,8 @@ func (c *LeveledCompactor) selectOverlappingDirs(ds []dirMeta) []string {
 				overlappingDirs = append(overlappingDirs, ds[i].dir)
 			}
 			overlappingDirs = append(overlappingDirs, d.dir)
+		} else if len(overlappingDirs) > 0 {
+			break
 		}
 		if d.meta.MaxTime > globalMaxt {
 			globalMaxt = d.meta.MaxTime
@@ -626,7 +628,7 @@ func (c *LeveledCompactor) write(dest string, meta *BlockMeta, blocks ...BlockRe
 		}
 	}()
 
-	if err := fileutil.Fsync(df); err != nil {
+	if err := df.Sync(); err != nil {
 		return errors.Wrap(err, "sync temporary dir file")
 	}
 
@@ -1026,7 +1028,7 @@ func renameFile(from, to string) error {
 		return err
 	}
 
-	if err = fileutil.Fsync(pdir); err != nil {
+	if err = pdir.Sync(); err != nil {
 		pdir.Close()
 		return err
 	}
diff --git a/db.go b/db.go
index 2ed31ea..52b21c2 100644
--- a/db.go
+++ b/db.go
@@ -37,6 +37,7 @@ import (
 	"github.com/prometheus/tsdb/chunkenc"
 	tsdb_errors "github.com/prometheus/tsdb/errors"
 	"github.com/prometheus/tsdb/fileutil"
+	_ "github.com/prometheus/tsdb/goversion"
 	"github.com/prometheus/tsdb/labels"
 	"github.com/prometheus/tsdb/wal"
 	"golang.org/x/sync/errgroup"
diff --git a/fileutil/fileutil.go b/fileutil/fileutil.go
index 154fa18..c55a2b8 100644
--- a/fileutil/fileutil.go
+++ b/fileutil/fileutil.go
@@ -118,7 +118,7 @@ func Rename(from, to string) error {
 		return err
 	}
 
-	if err = Fsync(pdir); err != nil {
+	if err = pdir.Sync(); err != nil {
 		pdir.Close()
 		return err
 	}
@@ -141,7 +141,7 @@ func Replace(from, to string) error {
 		return err
 	}
 
-	if err = Fsync(pdir); err != nil {
+	if err = pdir.Sync(); err != nil {
 		pdir.Close()
 		return err
 	}
diff --git a/fileutil/sync.go b/fileutil/sync.go
index 54dd41f..2e64a40 100644
--- a/fileutil/sync.go
+++ b/fileutil/sync.go
@@ -18,11 +18,6 @@ package fileutil
 
 import "os"
 
-// Fsync is a wrapper around file.Sync(). Special handling is needed on darwin platform.
-func Fsync(f *os.File) error {
-	return f.Sync()
-}
-
 // Fdatasync is a wrapper around file.Sync(). Special handling is needed on linux platform.
 func Fdatasync(f *os.File) error {
 	return f.Sync()
diff --git a/fileutil/sync_darwin.go b/fileutil/sync_darwin.go
index c2f39bf..2af1b0f 100644
--- a/fileutil/sync_darwin.go
+++ b/fileutil/sync_darwin.go
@@ -18,23 +18,10 @@ package fileutil
 
 import (
 	"os"
-	"syscall"
 )
 
-// Fsync on HFS/OSX flushes the data on to the physical drive but the drive
-// may not write it to the persistent media for quite sometime and it may be
-// written in out-of-order sequence. Using F_FULLFSYNC ensures that the
-// physical drive's buffer will also get flushed to the media.
-func Fsync(f *os.File) error {
-	_, _, errno := syscall.Syscall(syscall.SYS_FCNTL, f.Fd(), uintptr(syscall.F_FULLFSYNC), uintptr(0))
-	if errno == 0 {
-		return nil
-	}
-	return errno
-}
-
 // Fdatasync on darwin platform invokes fcntl(F_FULLFSYNC) for actual persistence
 // on physical drive media.
 func Fdatasync(f *os.File) error {
-	return Fsync(f)
+	return f.Sync()
 }
diff --git a/fileutil/sync_linux.go b/fileutil/sync_linux.go
index 1bbced9..8b4fc82 100644
--- a/fileutil/sync_linux.go
+++ b/fileutil/sync_linux.go
@@ -21,11 +21,6 @@ import (
 	"syscall"
 )
 
-// Fsync is a wrapper around file.Sync(). Special handling is needed on darwin platform.
-func Fsync(f *os.File) error {
-	return f.Sync()
-}
-
 // Fdatasync is similar to fsync(), but does not flush modified metadata
 // unless that metadata is needed in order to allow a subsequent data retrieval
 // to be correctly handled.
diff --git a/goversion/goversion.go b/goversion/goversion.go
new file mode 100644
index 0000000..8b194d4
--- /dev/null
+++ b/goversion/goversion.go
@@ -0,0 +1,19 @@
+// Copyright 2017 The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// +build go1.12
+
+// Package goversion enforces the go version suported by the tsdb module.
+package goversion
+
+const _SoftwareRequiresGOVERSION1_12 = uint8(0)
diff --git a/goversion/init.go b/goversion/init.go
new file mode 100644
index 0000000..dd15e1f
--- /dev/null
+++ b/goversion/init.go
@@ -0,0 +1,17 @@
+// Copyright 2017 The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package goversion
+
+// This will fail to compile if the Go runtime version isn't >= 1.12.
+var _ = _SoftwareRequiresGOVERSION1_12
diff --git a/index/index.go b/index/index.go
index 580291a..9e5cecc 100644
--- a/index/index.go
+++ b/index/index.go
@@ -29,6 +29,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/prometheus/tsdb/chunks"
 	"github.com/prometheus/tsdb/encoding"
+	tsdb_errors "github.com/prometheus/tsdb/errors"
 	"github.com/prometheus/tsdb/fileutil"
 	"github.com/prometheus/tsdb/labels"
 )
@@ -192,7 +193,7 @@ func NewWriter(fn string) (*Writer, error) {
 	if err != nil {
 		return nil, err
 	}
-	if err := fileutil.Fsync(df); err != nil {
+	if err := df.Sync(); err != nil {
 		return nil, errors.Wrap(err, "sync dir")
 	}
 
@@ -554,7 +555,7 @@ func (w *Writer) Close() error {
 	if err := w.fbuf.Flush(); err != nil {
 		return err
 	}
-	if err := fileutil.Fsync(w.f); err != nil {
+	if err := w.f.Sync(); err != nil {
 		return err
 	}
 	return w.f.Close()
@@ -625,7 +626,15 @@ func NewFileReader(path string) (*Reader, error) {
 	if err != nil {
 		return nil, err
 	}
-	return newReader(realByteSlice(f.Bytes()), f)
+	r, err := newReader(realByteSlice(f.Bytes()), f)
+	if err != nil {
+		var merr tsdb_errors.MultiError
+		merr.Add(err)
+		merr.Add(f.Close())
+		return nil, merr
+	}
+
+	return r, nil
 }
 
 func newReader(b ByteSlice, c io.Closer) (*Reader, error) {
diff --git a/repair.go b/repair.go
index 4aeffb5..a7fa084 100644
--- a/repair.go
+++ b/repair.go
@@ -23,7 +23,6 @@ import (
 	"github.com/go-kit/kit/log"
 	"github.com/go-kit/kit/log/level"
 	"github.com/pkg/errors"
-	"github.com/prometheus/tsdb/fileutil"
 )
 
 // repairBadIndexVersion repairs an issue in index and meta.json persistence introduced in
@@ -75,7 +74,7 @@ func repairBadIndexVersion(logger log.Logger, dir string) error {
 		if _, err := repl.WriteAt([]byte{2}, 4); err != nil {
 			return wrapErr(err, d)
 		}
-		if err := fileutil.Fsync(repl); err != nil {
+		if err := repl.Sync(); err != nil {
 			return wrapErr(err, d)
 		}
 		if err := repl.Close(); err != nil {
diff --git a/tombstones.go b/tombstones.go
index 4c36c7b..ec02554 100644
--- a/tombstones.go
+++ b/tombstones.go
@@ -24,6 +24,7 @@ import (
 
 	"github.com/pkg/errors"
 	"github.com/prometheus/tsdb/encoding"
+	tsdb_errors "github.com/prometheus/tsdb/errors"
 )
 
 const tombstoneFilename = "tombstones"
@@ -100,6 +101,12 @@ func writeTombstoneFile(dir string, tr TombstoneReader) error {
 		return err
 	}
 
+	var merr tsdb_errors.MultiError
+	if merr.Add(f.Sync()); merr.Err() != nil {
+		merr.Add(f.Close())
+		return merr.Err()
+	}
+
 	if err = f.Close(); err != nil {
 		return err
 	}
diff --git a/wal/wal.go b/wal/wal.go
index b370326..0001147 100644
--- a/wal/wal.go
+++ b/wal/wal.go
@@ -595,7 +595,7 @@ func (w *WAL) Truncate(i int) (err error) {
 
 func (w *WAL) fsync(f *Segment) error {
 	start := time.Now()
-	err := fileutil.Fsync(f.File)
+	err := f.File.Sync()
 	w.fsyncDuration.Observe(time.Since(start).Seconds())
 	return err
 }


Test patch:
diff --git a/compact_test.go b/compact_test.go
index 1e6a962..90819bc 100644
--- a/compact_test.go
+++ b/compact_test.go
@@ -173,27 +173,25 @@ func TestLeveledCompactor_plan(t *testing.T) {
 	}, nil)
 	testutil.Ok(t, err)
 
-	cases := []struct {
+	cases := map[string]struct {
 		metas    []dirMeta
 		expected []string
 	}{
-		{
+		"Outside Range": {
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
 			},
 			expected: nil,
 		},
-		// We should wait for four blocks of size 20 to appear before compacting.
-		{
+		"We should wait for four blocks of size 20 to appear before compacting.": {
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 20, 40, nil),
 			},
 			expected: nil,
 		},
-		// We should wait for a next block of size 20 to appear before compacting
-		// the existing ones. We have three, but we ignore the fresh one from WAl.
-		{
+		`We should wait for a next block of size 20 to appear before compacting
+		the existing ones. We have three, but we ignore the fresh one from WAl`: {
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 20, 40, nil),
@@ -201,8 +199,7 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: nil,
 		},
-		// Block to fill the entire parent range appeared – should be compacted.
-		{
+		"Block to fill the entire parent range appeared – should be compacted": {
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 20, 40, nil),
@@ -211,9 +208,8 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: []string{"1", "2", "3"},
 		},
-		// Block for the next parent range appeared with gap with size 20. Nothing will happen in the first one
-		// anymore but we ignore fresh one still, so no compaction.
-		{
+		`Block for the next parent range appeared with gap with size 20. Nothing will happen in the first one
+		anymore but we ignore fresh one still, so no compaction`: {
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 20, 40, nil),
@@ -221,9 +217,8 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: nil,
 		},
-		// Block for the next parent range appeared, and we have a gap with size 20 between second and third block.
-		// We will not get this missed gap anymore and we should compact just these two.
-		{
+		`Block for the next parent range appeared, and we have a gap with size 20 between second and third block.
+		We will not get this missed gap anymore and we should compact just these two.`: {
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 20, 40, nil),
@@ -232,8 +227,7 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: []string{"1", "2"},
 		},
-		{
-			// We have 20, 20, 20, 60, 60 range blocks. "5" is marked as fresh one.
+		"We have 20, 20, 20, 60, 60 range blocks. '5' is marked as fresh one": {
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 20, 40, nil),
@@ -243,8 +237,7 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: []string{"1", "2", "3"},
 		},
-		{
-			// We have 20, 60, 20, 60, 240 range blocks. We can compact 20 + 60 + 60.
+		"We have 20, 60, 20, 60, 240 range blocks. We can compact 20 + 60 + 60": {
 			metas: []dirMeta{
 				metaRange("2", 20, 40, nil),
 				metaRange("4", 60, 120, nil),
@@ -254,8 +247,7 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: []string{"2", "4", "6"},
 		},
-		// Do not select large blocks that have many tombstones when there is no fresh block.
-		{
+		"Do not select large blocks that have many tombstones when there is no fresh block": {
 			metas: []dirMeta{
 				metaRange("1", 0, 540, &BlockStats{
 					NumSeries:     10,
@@ -264,8 +256,7 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: nil,
 		},
-		// Select large blocks that have many tombstones when fresh appears.
-		{
+		"Select large blocks that have many tombstones when fresh appears": {
 			metas: []dirMeta{
 				metaRange("1", 0, 540, &BlockStats{
 					NumSeries:     10,
@@ -275,8 +266,7 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: []string{"1"},
 		},
-		// For small blocks, do not compact tombstones, even when fresh appears.
-		{
+		"For small blocks, do not compact tombstones, even when fresh appears.": {
 			metas: []dirMeta{
 				metaRange("1", 0, 60, &BlockStats{
 					NumSeries:     10,
@@ -286,9 +276,8 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: nil,
 		},
-		// Regression test: we were stuck in a compact loop where we always recompacted
-		// the same block when tombstones and series counts were zero.
-		{
+		`Regression test: we were stuck in a compact loop where we always recompacted
+		the same block when tombstones and series counts were zero`: {
 			metas: []dirMeta{
 				metaRange("1", 0, 540, &BlockStats{
 					NumSeries:     0,
@@ -298,12 +287,11 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: nil,
 		},
-		// Regression test: we were wrongly assuming that new block is fresh from WAL when its ULID is newest.
-		// We need to actually look on max time instead.
-		//
-		// With previous, wrong approach "8" block was ignored, so we were wrongly compacting 5 and 7 and introducing
-		// block overlaps.
-		{
+		`Regression test: we were wrongly assuming that new block is fresh from WAL when its ULID is newest.
+		We need to actually look on max time instead.
+		
+		With previous, wrong approach "8" block was ignored, so we were wrongly compacting 5 and 7 and introducing
+		block overlaps`: {
 			metas: []dirMeta{
 				metaRange("5", 0, 360, nil),
 				metaRange("6", 540, 560, nil), // Fresh one.
@@ -312,8 +300,10 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: []string{"7", "8"},
 		},
-		// For overlapping blocks.
-		{
+		// |--------------|
+		//               |----------------|
+		//                                |--------------|
+		"Overlapping blocks 1": {
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 19, 40, nil),
@@ -321,7 +311,10 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: []string{"1", "2"},
 		},
-		{
+		// |--------------|
+		//                |--------------|
+		//                        |--------------|
+		"Overlapping blocks 2": {
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 20, 40, nil),
@@ -329,7 +322,10 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: []string{"2", "3"},
 		},
-		{
+		// |--------------|
+		//         |---------------------|
+		//                       |--------------|
+		"Overlapping blocks 3": {
 			metas: []dirMeta{
 				metaRange("1", 0, 20, nil),
 				metaRange("2", 10, 40, nil),
@@ -337,7 +333,11 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: []string{"1", "2", "3"},
 		},
-		{
+		// |--------------|
+		//               |--------------------------------|
+		//                |--------------|
+		//                               |--------------|
+		"Overlapping blocks 4": {
 			metas: []dirMeta{
 				metaRange("5", 0, 360, nil),
 				metaRange("6", 340, 560, nil),
@@ -346,10 +346,23 @@ func TestLeveledCompactor_plan(t *testing.T) {
 			},
 			expected: []string{"5", "6", "7", "8"},
 		},
+		// |--------------|
+		//               |--------------|
+		//                                            |--------------|
+		//                                                          |--------------|
+		"Overlapping blocks 5": {
+			metas: []dirMeta{
+				metaRange("1", 0, 10, nil),
+				metaRange("2", 9, 20, nil),
+				metaRange("3", 30, 40, nil),
+				metaRange("4", 39, 50, nil),
+			},
+			expected: []string{"1", "2"},
+		},
 	}
 
-	for _, c := range cases {
-		if !t.Run("", func(t *testing.T) {
+	for title, c := range cases {
+		if !t.Run(title, func(t *testing.T) {
 			res, err := compactor.plan(c.metas)
 			testutil.Ok(t, err)
 			testutil.Equals(t, c.expected, res)
diff --git a/db_test.go b/db_test.go
index 3d6aa6a..54197df 100644
--- a/db_test.go
+++ b/db_test.go
@@ -1706,8 +1706,10 @@ func TestCorrectNumTombstones(t *testing.T) {
 
 func TestVerticalCompaction(t *testing.T) {
 	cases := []struct {
-		blockSeries [][]Series
-		expSeries   map[string][]tsdbutil.Sample
+		blockSeries          [][]Series
+		expSeries            map[string][]tsdbutil.Sample
+		expBlockNum          int
+		expOverlappingBlocks int
 	}{
 		// Case 0
 		// |--------------|
@@ -1734,6 +1736,8 @@ func TestVerticalCompaction(t *testing.T) {
 				sample{8, 99}, sample{9, 99}, sample{10, 99}, sample{11, 99},
 				sample{12, 99}, sample{13, 99}, sample{14, 99},
 			}},
+			expBlockNum:          1,
+			expOverlappingBlocks: 1,
 		},
 		// Case 1
 		// |-------------------------------|
@@ -1760,6 +1764,8 @@ func TestVerticalCompaction(t *testing.T) {
 				sample{8, 99}, sample{9, 99}, sample{10, 99}, sample{11, 0},
 				sample{13, 0}, sample{17, 0},
 			}},
+			expBlockNum:          1,
+			expOverlappingBlocks: 1,
 		},
 		// Case 2
 		// |-------------------------------|
@@ -1794,6 +1800,8 @@ func TestVerticalCompaction(t *testing.T) {
 				sample{14, 59}, sample{15, 59}, sample{17, 59}, sample{20, 59},
 				sample{21, 59}, sample{22, 59},
 			}},
+			expBlockNum:          1,
+			expOverlappingBlocks: 1,
 		},
 		// Case 3
 		// |-------------------|
@@ -1828,6 +1836,8 @@ func TestVerticalCompaction(t *testing.T) {
 				sample{15, 59}, sample{16, 99}, sample{17, 59}, sample{20, 59},
 				sample{21, 59}, sample{22, 59},
 			}},
+			expBlockNum:          1,
+			expOverlappingBlocks: 1,
 		},
 		// Case 4
 		// |-------------------------------------|
@@ -1864,6 +1874,8 @@ func TestVerticalCompaction(t *testing.T) {
 				sample{13, 99}, sample{15, 99}, sample{16, 99}, sample{17, 99},
 				sample{20, 0}, sample{22, 0},
 			}},
+			expBlockNum:          1,
+			expOverlappingBlocks: 1,
 		},
 		// Case 5: series are merged properly when there are multiple series.
 		// |-------------------------------------|
@@ -1958,6 +1970,53 @@ func TestVerticalCompaction(t *testing.T) {
 					sample{20, 0}, sample{22, 0},
 				},
 			},
+			expBlockNum:          1,
+			expOverlappingBlocks: 1,
+		},
+		// Case 6
+		// |--------------|
+		//        |----------------|
+		//                                         |--------------|
+		//                                                  |----------------|
+		{
+			blockSeries: [][]Series{
+				[]Series{
+					newSeries(map[string]string{"a": "b"}, []tsdbutil.Sample{
+						sample{0, 0}, sample{1, 0}, sample{2, 0}, sample{4, 0},
+						sample{5, 0}, sample{7, 0}, sample{8, 0}, sample{9, 0},
+					}),
+				},
+				[]Series{
+					newSeries(map[string]string{"a": "b"}, []tsdbutil.Sample{
+						sample{3, 99}, sample{5, 99}, sample{6, 99}, sample{7, 99},
+						sample{8, 99}, sample{9, 99}, sample{10, 99}, sample{11, 99},
+						sample{12, 99}, sample{13, 99}, sample{14, 99},
+					}),
+				},
+				[]Series{
+					newSeries(map[string]string{"a": "b"}, []tsdbutil.Sample{
+						sample{20, 0}, sample{21, 0}, sample{22, 0}, sample{24, 0},
+						sample{25, 0}, sample{27, 0}, sample{28, 0}, sample{29, 0},
+					}),
+				},
+				[]Series{
+					newSeries(map[string]string{"a": "b"}, []tsdbutil.Sample{
+						sample{23, 99}, sample{25, 99}, sample{26, 99}, sample{27, 99},
+						sample{28, 99}, sample{29, 99}, sample{30, 99}, sample{31, 99},
+					}),
+				},
+			},
+			expSeries: map[string][]tsdbutil.Sample{`{a="b"}`: {
+				sample{0, 0}, sample{1, 0}, sample{2, 0}, sample{3, 99},
+				sample{4, 0}, sample{5, 99}, sample{6, 99}, sample{7, 99},
+				sample{8, 99}, sample{9, 99}, sample{10, 99}, sample{11, 99},
+				sample{12, 99}, sample{13, 99}, sample{14, 99},
+				sample{20, 0}, sample{21, 0}, sample{22, 0}, sample{23, 99},
+				sample{24, 0}, sample{25, 99}, sample{26, 99}, sample{27, 99},
+				sample{28, 99}, sample{29, 99}, sample{30, 99}, sample{31, 99},
+			}},
+			expBlockNum:          2,
+			expOverlappingBlocks: 2,
 		},
 	}
 
@@ -1995,9 +2054,9 @@ func TestVerticalCompaction(t *testing.T) {
 			testutil.Equals(t, 0, int(prom_testutil.ToFloat64(lc.metrics.overlappingBlocks)), "overlapping blocks count should be still 0 here")
 			err = db.compact()
 			testutil.Ok(t, err)
-			testutil.Equals(t, 1, len(db.Blocks()), "Wrong number of blocks [after compact]")
+			testutil.Equals(t, c.expBlockNum, len(db.Blocks()), "Wrong number of blocks [after compact]")
 
-			testutil.Equals(t, 1, int(prom_testutil.ToFloat64(lc.metrics.overlappingBlocks)), "overlapping blocks count mismatch")
+			testutil.Equals(t, c.expOverlappingBlocks, int(prom_testutil.ToFloat64(lc.metrics.overlappingBlocks)), "overlapping blocks count mismatch")
 
 			// Query test after merging the overlapping blocks.
 			querier, err = db.Querier(0, 100)
diff --git a/goversion/goversio_test.go b/goversion/goversio_test.go
new file mode 100644
index 0000000..9ed3c11
--- /dev/null
+++ b/goversion/goversio_test.go
@@ -0,0 +1,27 @@
+// Copyright 2017 The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package goversion_test
+
+import (
+	"testing"
+
+	_ "github.com/prometheus/tsdb/goversion"
+)
+
+// This test is is intentionally blank and exists only so `go test` believes
+// there is something to test.
+//
+// The blank import above is actually what invokes the test of this package. If
+// the import succeeds (the code compiles), the test passed.
+func Test(t *testing.T) {}
diff --git a/index/index_test.go b/index/index_test.go
index 70aa097..b82d07d 100644
--- a/index/index_test.go
+++ b/index/index_test.go
@@ -412,3 +412,18 @@ func TestReaderWithInvalidBuffer(t *testing.T) {
 	_, err := NewReader(b)
 	testutil.NotOk(t, err)
 }
+
+// TestNewFileReaderErrorNoOpenFiles ensures that in case of an error no file remains open.
+func TestNewFileReaderErrorNoOpenFiles(t *testing.T) {
+	dir := testutil.NewTemporaryDirectory("block", t)
+
+	idxName := filepath.Join(dir.Path(), "index")
+	err := ioutil.WriteFile(idxName, []byte("corrupted contents"), 0644)
+	testutil.Ok(t, err)
+
+	_, err = NewFileReader(idxName)
+	testutil.NotOk(t, err)
+
+	// dir.Close will fail on Win if idxName fd is not closed on error path.
+	dir.Close()
+}