Hi @beorn7 @fpetkovski,
Since you both last worked on this part of the code, I wanted to check if you could kindly take a look at this PR. I’d appreciate any feedback when you get a chance. Thanks!

Thanks for catching this. We suspected there are more cases where we don't copy enough, see #16581.

But it needs some investigation if this PR is the right way to solve this particular instance.

@krajorama might have state on this (actually better than myself). I'll preliminarily assign this to him.

I'm out sick and will take some time to look at this myself.

I've literally made the same patch more than a year ago https://github.com/prometheus/prometheus/pull/13659/files for a different reason 😞 .

I don't remember the exact reason we abandoned that PR, but I'm sure we thought we can sort out the copy/not to copy problem later, as described in the spec and #16581.

To me copying is always safe, and the code is the same as mine, so I'm just reviewing the test part basically.

I've used the debugger to step through subquery eval and the histograms returned from the storageSeriesIterator are indeed going directly into the pool used by matrixIterSlice , which means the pool is poisoned.

We could fix the issue in matrixIterSlice as well by checking if what the iterator returned is the same or different pointer. But that's not the current pattern and I'd rather be consistent to not complicate the code further. Using the pool means this fix won't add too much overhead either.

I've spent half a day trying to figure out a simpler test to trigger the error. But the error isn't as simple as setting the same pointer twice in the matrixSelectorHPool pool in matrixIterSlice.

As far as I understand the problem really starts in MemoizedSeriesIterator which is used in evaluating the subquery. This iterator calls into chunkenc.FloatHistogram.AtFloatHistogram which does a shallow copy of the histogram. Which means that the result of the subquery will have histograms that share the same physical spans, buckets. Thus setting any of the spans/buckets will result in setting all.

I've tested an alternative fix where I patch the MemoizedSeriesIterator:

diff --git a/storage/memoized_iterator.go b/storage/memoized_iterator.go
index 273b3caa1..11b8be7da 100644
--- a/storage/memoized_iterator.go
+++ b/storage/memoized_iterator.go
@@ -133,7 +133,8 @@ func (b *MemoizedSeriesIterator) At() (int64, float64) {
 
 // AtFloatHistogram returns the current float-histogram element of the iterator.
 func (b *MemoizedSeriesIterator) AtFloatHistogram() (int64, *histogram.FloatHistogram) {
-       return b.it.AtFloatHistogram(nil)
+       fh := &histogram.FloatHistogram{}
+       return b.it.AtFloatHistogram(fh)
 }
 
 // AtT returns the timestamp of the current element of the iterator.

Anyway, this is not as efficient as your fix, so I've added it here for demonstration only.

What really threw me was that the error isn't deterministic, even though the code looks deterministic. The only thing running in parallel is the GC, but I thought that we didn't really use that due to the pooling. But setting GOGC=off makes the problem much worse, one iteration of the subquery is enough to trigger the problem. The bug comes out non-deterministic even then , due to sync.Pool still doing it's own garbage collection. Only if I replace sync.Pool with a dumb slice based implementation, do I get deterministic failure right away.

And the failure is that the pool contains histograms with shared spans/buckets, so when we do the rate , the normal copy takes place from the TSDB and data is copied multiple times into the same memory. The last one wins.

On the other hand if sync.Pool is faster at letting go of the unused pool item, then we get to rate with clean pool and the problem doesn't come up.

I don't really see how we could make the test deterministic, unless we make the use of the pool optional. Which could make actual sense in some scenarios, when we do not have to make a copy after all.

@bboreham suggested to check if https://pkg.go.dev/testing/synctest has some way of getting rid of the non determinism in sync.Pool for the test. If not , could be worth an issue on Go.

I've put this on my backlog, but @harry671003 if you got some time, feel free to check yourself please. NOTE: Prometheus needs to update the GO version first as this is a new feature since 1.24.

Thanks for the reviews and merge! Appreciate all the insights - learned a lot through this.

Regarding the synctest suggestion, I’ll keep this in mind and once Prometheus upgrades to Go 1.24, I’ll explore whether synctest can help stabilize the test.