TSDB: Don't rely on integer overflow in head compaction check

[56quarters]

Don't rely on integer overflow for correct behavior in the Head.compactable() check.

Previously, the logic for determining if the head should be compacted relied on the default values for min and max time and integer overflow when they were checked in Head.compactable(). The check in Head.compactable() effectively did math.MinInt64 - math.MaxInt64 which overflowed and wrapped to 1. Since 1 is less than 1.5 times the chunk range, compaction did not happen. This was the correct behavior but relying on overflow wrapping is surprising.

This change adds a method for checking if the min time for the head is unset and uses it to short-circuit compaction in that case. It also replaces several explicit checks for the default value to determine if the head has not yet had any samples added.

[pstibrany]

Great find.

[machine424]

Could you adjust the PR title and the commit? They insinuates the compaction is currently triggered when the head isn't initialized, which isn't what you explain in the description.

---

How about embracing/making use of the overflow instead and just add an explicit regression test for it?

[56quarters]

Could you adjust the PR title and the commit? They insinuates the compaction is currently triggered when the head isn't initialized, which isn't what you explain in the description.

Sure.

How about embracing/making use of the overflow instead and just add an explicit regression test for it?

I'm not sure what you mean. Let the check overflow but add a test that ensures Head.compactable() always returns false with default values? I'd rather be explicit about not compacting the head when it hasn't received any samples in Head.compactable() since then looking at the code doesn't require knowing how go handles integer overflow to understand.

[machine424]

Yes, both approaches suit me.
Thanks.
Let's see what the others think.

[krajorama]

Looks sensible and makes the code more consistent. The one thing that I would like to see is a testcase in TestNoEmptyBlocks in db_test.go that fails with the old code - due to it not checking that the data is valid.

[56quarters]

The one thing that I would like to see is a testcase in TestNoEmptyBlocks in db_test.go that fails with the old code - due to it not checking that the data is valid.

I'm not actually sure I can do this. The old code worked correctly, it just relied on integer overflow which was confusing. In both the old code and the new code, Head.compactable() returns the correct result.

[krajorama]

The one thing that I would like to see is a testcase in TestNoEmptyBlocks in db_test.go that fails with the old code - due to it not checking that the data is valid.

I'm not actually sure I can do this. The old code worked correctly, it just relied on integer overflow which was confusing. In both the old code and the new code, Head.compactable() returns the correct result.

This synthetic test fails on main I think:

func TestHeadCompactibleCornerCase(t *testing.T) {
	dir := t.TempDir()
	wal, err := wlog.NewSize(nil, nil, filepath.Join(dir, "wal"), 32768, wlog.CompressionSnappy)
	require.NoError(t, err)

	opts := DefaultHeadOptions()
	opts.ChunkRange = 1  // This is the extreme bit
	opts.ChunkDirRoot = dir

	h, err := NewHead(nil, nil, wal, nil, opts, nil)
	require.NoError(t, err)
	defer func() {
		require.NoError(t, h.Close())
	}()

	require.False(t, h.compactable())
}

[56quarters]

This synthetic test fails on main I think:

Ah, I see. I'll add something like that. Thank you!

[krajorama]

LGTM

[bwplotka]

LGTM

[jesusvazquez]

LGTM