store/bucket: fix data race

[GiedriusS]

Add a fix for issue #6545. Unfortunately, we don't have tests with -race at the moment but I tested this locally. Somehow the data race introduces inconsistencies in the retrieved data. I couldn't reproduce anymore after this fix.

[yeya24]

Hi @GiedriusS, can you please explain more, why this change fixed the issue? Thanks

[GiedriusS]

Hi @GiedriusS, can you please explain more, why this change fixed the issue? Thanks

That would require some digging but from my adhoc tests I cannot reproduce the original problem anymore. I guess I can try to add tests that loop infinitely and see what happens. Maybe this can be done separately? The data race needs to be fixed either way 😄

[yeya24]

@GiedriusS I am wondering if this also fixes #6495

[GiedriusS]

:O I forgot about that issue. I think this also should fix it because in my tests I was pressing execute many times and very rarely the values were completely different even though the query is the same. I'll work on tests tomorrow

[yeya24]

Found another race in chunks reader. But that's a small one and a separate issue. I just opened a separate pr #6578

[fpetkovski]

cc @rabenhorst potentially a fix for #6495.

[yeya24]

Yeah I think I can reproduce the data race using the test updated yeya24@be85c85 with multiple matchers.

Data race is at https://github.com/thanos-io/thanos/blob/main/pkg/store/bucket.go#L2208 and other place we need to read the matchers.
I think this fix is valid and tested it fixed the data race.

goos: darwin
goarch: arm64
pkg: github.com/thanos-io/thanos/pkg/store
BenchmarkBlockSeries/concurrency:_16-10         	==================
WARNING: DATA RACE
Read at 0x00c0001a6088 by goroutine 94:
  github.com/thanos-io/thanos/pkg/store.(*bucketIndexReader).ExpandedPostings.func1()
      /Users/benye/hub/gowork/thanos/pkg/store/bucket.go:2208 +0x58
  sort.insertionSort_func()
      /usr/local/go/src/sort/zsortfunc.go:12 +0xd0
  sort.pdqsort_func()
      /usr/local/go/src/sort/zsortfunc.go:73 +0x2d0
  sort.Slice()
      /usr/local/go/src/sort/slice.go:26 +0x1a4
  github.com/thanos-io/thanos/pkg/store.(*bucketIndexReader).ExpandedPostings()
      /Users/benye/hub/gowork/thanos/pkg/store/bucket.go:2207 +0xb8
  github.com/thanos-io/thanos/pkg/store.(*blockSeriesClient).ExpandPostings()
      /Users/benye/hub/gowork/thanos/pkg/store/bucket.go:969 +0xa0
  github.com/thanos-io/thanos/pkg/store.benchmarkBlockSeriesWithConcurrency.func1()
      /Users/benye/hub/gowork/thanos/pkg/store/bucket_test.go:2585 +0x28c

Previous write at 0x00c0001a6088 by goroutine 100:
  internal/reflectlite.Swapper.func3()
      /usr/local/go/src/internal/reflectlite/swapper.go:42 +0xa0
  sort.insertionSort_func()
      /usr/local/go/src/sort/zsortfunc.go:13 +0x88
  sort.pdqsort_func()
      /usr/local/go/src/sort/zsortfunc.go:73 +0x2d0
  sort.Slice()
      /usr/local/go/src/sort/slice.go:26 +0x1a4
  github.com/thanos-io/thanos/pkg/store.(*bucketIndexReader).ExpandedPostings()
      /Users/benye/hub/gowork/thanos/pkg/store/bucket.go:2207 +0xb8
  github.com/thanos-io/thanos/pkg/store.(*blockSeriesClient).ExpandPostings()
      /Users/benye/hub/gowork/thanos/pkg/store/bucket.go:969 +0xa0
  github.com/thanos-io/thanos/pkg/store.benchmarkBlockSeriesWithConcurrency.func1()
      /Users/benye/hub/gowork/thanos/pkg/store/bucket_test.go:2585 +0x28c

[GiedriusS]

Added a test for this exact race. The test fails on the main branch immediately almost:

/bin/go test -v -race -timeout 10m -run ^TestExpandedPostingsRace$ github.com/thanos-io/thanos/pkg/store

However, I deployed this prod and #6545 still occurs. I couldn't reproduce #6495 with this.

I also tried writing this test using BucketStore but with it, I wasn't able to reproduce the race :/ probably race detector slows down the code so much that the first sorting happens before all of the others.

[yeya24]

@GiedriusS I will merge this pr first to fix the data race.

[yeya24]

Thanks!