NH: Do not re-use spans between histograms (#14771)

promql, tsdb (histograms): Do not re-use spans between histograms When multiple points exist with the same native histogram schemas they share their spans. This causes a problem when a native histogram (NH) schema is modified (for example, during a Sum) then the other NH's with the same spans are also modified. As such, we should create a new Span for each NH. This will ensure NH's interfaces are safe to use without considering the effect on other histograms. At the moment this doesn't present itself as a problem because in all aggregations and functions operating on native histograms they are copied by the promql query engine first. Signed-off-by: Joshua Hesketh <josh@nitrotech.org> --------- Signed-off-by: Joshua Hesketh <josh@nitrotech.org>
2024-11-09 23:24:05 -08:00 · 2024-09-04 20:07:16 +10:00 · 2024-09-04 20:07:16 +10:00 · f2064c7987
parent 282fb1632a
commit f2064c7987
4 changed files with 189 additions and 2 deletions
--- a/tsdb/chunkenc/float_histogram.go
+++ b/tsdb/chunkenc/float_histogram.go
@ -974,6 +974,7 @@ func (it *floatHistogramIterator) Reset(b []byte) {
 	if it.atFloatHistogramCalled {
 		it.atFloatHistogramCalled = false
 		it.pBuckets, it.nBuckets = nil, nil
+		it.pSpans, it.nSpans = nil, nil
 	} else {
 		it.pBuckets, it.nBuckets = it.pBuckets[:0], it.nBuckets[:0]
 	}
@ -1069,7 +1070,7 @@ func (it *floatHistogramIterator) Next() ValueType {
 	// The case for the 2nd sample with single deltas is implicitly handled correctly with the double delta code,
 	// so we don't need a separate single delta logic for the 2nd sample.

-	// Recycle bucket slices that have not been returned yet. Otherwise, copy them.
+	// Recycle bucket and span slices that have not been returned yet. Otherwise, copy them.
 	// We can always recycle the slices for leading and trailing bits as they are
 	// never returned to the caller.
 	if it.atFloatHistogramCalled {
@ -1088,6 +1089,20 @@ func (it *floatHistogramIterator) Next() ValueType {
 		} else {
 			it.nBuckets = nil
 		}
+		if len(it.pSpans) > 0 {
+			newSpans := make([]histogram.Span, len(it.pSpans))
+			copy(newSpans, it.pSpans)
+			it.pSpans = newSpans
+		} else {
+			it.pSpans = nil
+		}
+		if len(it.nSpans) > 0 {
+			newSpans := make([]histogram.Span, len(it.nSpans))
+			copy(newSpans, it.nSpans)
+			it.nSpans = newSpans
+		} else {
+			it.nSpans = nil
+		}
 	}

 	tDod, err := readVarbitInt(&it.br)
--- a/tsdb/chunkenc/float_histogram_test.go
+++ b/tsdb/chunkenc/float_histogram_test.go
@ -1306,3 +1306,54 @@ func TestFloatHistogramAppendOnlyErrors(t *testing.T) {
 		require.EqualError(t, err, "float histogram counter reset")
 	})
 }
+
+func TestFloatHistogramUniqueSpansAfterNext(t *testing.T) {
+	// Create two histograms with the same schema and spans.
+	h1 := &histogram.FloatHistogram{
+		Schema:        1,
+		ZeroThreshold: 1e-100,
+		Count:         10,
+		ZeroCount:     2,
+		Sum:           15.0,
+		PositiveSpans: []histogram.Span{
+			{Offset: 0, Length: 2},
+			{Offset: 1, Length: 2},
+		},
+		PositiveBuckets: []float64{1, 2, 3, 4},
+		NegativeSpans: []histogram.Span{
+			{Offset: 1, Length: 1},
+		},
+		NegativeBuckets: []float64{2},
+	}
+
+	h2 := h1.Copy()
+
+	// Create a chunk and append both histograms.
+	c := NewFloatHistogramChunk()
+	app, err := c.Appender()
+	require.NoError(t, err)
+
+	_, _, _, err = app.AppendFloatHistogram(nil, 0, h1, false)
+	require.NoError(t, err)
+
+	_, _, _, err = app.AppendFloatHistogram(nil, 1, h2, false)
+	require.NoError(t, err)
+
+	// Create an iterator and advance to the first histogram.
+	it := c.Iterator(nil)
+	require.Equal(t, ValFloatHistogram, it.Next())
+	_, rh1 := it.AtFloatHistogram(nil)
+
+	// Advance to the second histogram and retrieve it.
+	require.Equal(t, ValFloatHistogram, it.Next())
+	_, rh2 := it.AtFloatHistogram(nil)
+
+	require.Equal(t, rh1.PositiveSpans, h1.PositiveSpans, "Returned positive spans are as expected")
+	require.Equal(t, rh1.NegativeSpans, h1.NegativeSpans, "Returned negative spans are as expected")
+	require.Equal(t, rh2.PositiveSpans, h1.PositiveSpans, "Returned positive spans are as expected")
+	require.Equal(t, rh2.NegativeSpans, h1.NegativeSpans, "Returned negative spans are as expected")
+
+	// Check that the spans for h1 and h2 are unique slices.
+	require.NotSame(t, &rh1.PositiveSpans[0], &rh2.PositiveSpans[0], "PositiveSpans should be unique between histograms")
+	require.NotSame(t, &rh1.NegativeSpans[0], &rh2.NegativeSpans[0], "NegativeSpans should be unique between histograms")
+}
--- a/tsdb/chunkenc/histogram.go
+++ b/tsdb/chunkenc/histogram.go
@ -1073,6 +1073,7 @@ func (it *histogramIterator) Reset(b []byte) {
 	if it.atHistogramCalled {
 		it.atHistogramCalled = false
 		it.pBuckets, it.nBuckets = nil, nil
+		it.pSpans, it.nSpans = nil, nil
 	} else {
 		it.pBuckets = it.pBuckets[:0]
 		it.nBuckets = it.nBuckets[:0]
@ -1185,8 +1186,25 @@ func (it *histogramIterator) Next() ValueType {
 	// The case for the 2nd sample with single deltas is implicitly handled correctly with the double delta code,
 	// so we don't need a separate single delta logic for the 2nd sample.

-	// Recycle bucket slices that have not been returned yet. Otherwise,
+	// Recycle bucket and span slices that have not been returned yet. Otherwise, copy them.
 	// copy them.
+	if it.atFloatHistogramCalled || it.atHistogramCalled {
+		if len(it.pSpans) > 0 {
+			newSpans := make([]histogram.Span, len(it.pSpans))
+			copy(newSpans, it.pSpans)
+			it.pSpans = newSpans
+		} else {
+			it.pSpans = nil
+		}
+		if len(it.nSpans) > 0 {
+			newSpans := make([]histogram.Span, len(it.nSpans))
+			copy(newSpans, it.nSpans)
+			it.nSpans = newSpans
+		} else {
+			it.nSpans = nil
+		}
+	}
+
 	if it.atHistogramCalled {
 		it.atHistogramCalled = false
 		if len(it.pBuckets) > 0 {
@ -1204,6 +1222,7 @@ func (it *histogramIterator) Next() ValueType {
 			it.nBuckets = nil
 		}
 	}
+
 	// FloatBuckets are set from scratch, so simply create empty ones.
 	if it.atFloatHistogramCalled {
 		it.atFloatHistogramCalled = false
--- a/tsdb/chunkenc/histogram_test.go
+++ b/tsdb/chunkenc/histogram_test.go
@ -1487,6 +1487,108 @@ func TestHistogramAppendOnlyErrors(t *testing.T) {
 	})
 }

+func TestHistogramUniqueSpansAfterNext(t *testing.T) {
+	// Create two histograms with the same schema and spans.
+	h1 := &histogram.Histogram{
+		Schema:        1,
+		ZeroThreshold: 1e-100,
+		Count:         10,
+		ZeroCount:     2,
+		Sum:           15.0,
+		PositiveSpans: []histogram.Span{
+			{Offset: 0, Length: 2},
+			{Offset: 1, Length: 2},
+		},
+		PositiveBuckets: []int64{1, 2, 3, 4},
+		NegativeSpans: []histogram.Span{
+			{Offset: 1, Length: 1},
+		},
+		NegativeBuckets: []int64{2},
+	}
+
+	h2 := h1.Copy()
+
+	// Create a chunk and append both histograms.
+	c := NewHistogramChunk()
+	app, err := c.Appender()
+	require.NoError(t, err)
+
+	_, _, _, err = app.AppendHistogram(nil, 0, h1, false)
+	require.NoError(t, err)
+
+	_, _, _, err = app.AppendHistogram(nil, 1, h2, false)
+	require.NoError(t, err)
+
+	// Create an iterator and advance to the first histogram.
+	it := c.Iterator(nil)
+	require.Equal(t, ValHistogram, it.Next())
+	_, rh1 := it.AtHistogram(nil)
+
+	// Advance to the second histogram and retrieve it.
+	require.Equal(t, ValHistogram, it.Next())
+	_, rh2 := it.AtHistogram(nil)
+
+	require.Equal(t, rh1.PositiveSpans, h1.PositiveSpans, "Returned positive spans are as expected")
+	require.Equal(t, rh1.NegativeSpans, h1.NegativeSpans, "Returned negative spans are as expected")
+	require.Equal(t, rh2.PositiveSpans, h1.PositiveSpans, "Returned positive spans are as expected")
+	require.Equal(t, rh2.NegativeSpans, h1.NegativeSpans, "Returned negative spans are as expected")
+
+	// Check that the spans for h1 and h2 are unique slices.
+	require.NotSame(t, &rh1.PositiveSpans[0], &rh2.PositiveSpans[0], "PositiveSpans should be unique between histograms")
+	require.NotSame(t, &rh1.NegativeSpans[0], &rh2.NegativeSpans[0], "NegativeSpans should be unique between histograms")
+}
+
+func TestHistogramUniqueSpansAfterNextWithAtFloatHistogram(t *testing.T) {
+	// Create two histograms with the same schema and spans.
+	h1 := &histogram.Histogram{
+		Schema:        1,
+		ZeroThreshold: 1e-100,
+		Count:         10,
+		ZeroCount:     2,
+		Sum:           15.0,
+		PositiveSpans: []histogram.Span{
+			{Offset: 0, Length: 2},
+			{Offset: 1, Length: 2},
+		},
+		PositiveBuckets: []int64{1, 2, 3, 4},
+		NegativeSpans: []histogram.Span{
+			{Offset: 1, Length: 1},
+		},
+		NegativeBuckets: []int64{2},
+	}
+
+	h2 := h1.Copy()
+
+	// Create a chunk and append both histograms.
+	c := NewHistogramChunk()
+	app, err := c.Appender()
+	require.NoError(t, err)
+
+	_, _, _, err = app.AppendHistogram(nil, 0, h1, false)
+	require.NoError(t, err)
+
+	_, _, _, err = app.AppendHistogram(nil, 1, h2, false)
+	require.NoError(t, err)
+
+	// Create an iterator and advance to the first histogram.
+	it := c.Iterator(nil)
+	require.Equal(t, ValHistogram, it.Next())
+	_, rh1 := it.AtFloatHistogram(nil)
+
+	// Advance to the second histogram and retrieve it.
+	require.Equal(t, ValHistogram, it.Next())
+	_, rh2 := it.AtFloatHistogram(nil)
+
+	require.Equal(t, rh1.PositiveSpans, h1.PositiveSpans, "Returned positive spans are as expected")
+	require.Equal(t, rh1.NegativeSpans, h1.NegativeSpans, "Returned negative spans are as expected")
+	require.Equal(t, rh2.PositiveSpans, h1.PositiveSpans, "Returned positive spans are as expected")
+	require.Equal(t, rh2.NegativeSpans, h1.NegativeSpans, "Returned negative spans are as expected")
+
+	// Check that the spans for h1 and h2 are unique slices.
+	require.NotSame(t, &rh1.PositiveSpans[0], &rh2.PositiveSpans[0], "PositiveSpans should be unique between histograms")
+	require.NotSame(t, &rh1.NegativeSpans[0], &rh2.NegativeSpans[0], "NegativeSpans should be unique between histograms")
+}
+
 func BenchmarkAppendable(b *testing.B) {
 	// Create a histogram with a bunch of spans and buckets.
 	const (