fix reader getting wrong posting offsets when querying multiple values

[yeya24]

• I added CHANGELOG entry for this change.
• Change is not relevant to the end user.

Changes

Index header reader has 2 methods to get posting offsets.
PostingsOffset receives only a single value and PostingsOffsets receives multiple values. Both methods rely on the same underlying function postingsOffset to iterate over the posting offset table and get the index range.

PostingsOffset is used in Store Gateway for query and PostingsOffsets is only used in lazy postings.

For a given slice of values, we expect that both of them return the same index ranges, using the following code.

// Batched 
rngs, err := reader.PostingsOffsets(name, values...)

// Non Batched
for _, value := range values {
  rng, err := reader.PostingsOffset(name, value)
  // Add rng to a range slice.
}

However, we observed that PostingsOffsets returned wrong index range when lazy expanded postings is enabled and querying multiple values. The bug is caused by https://github.com/thanos-io/thanos/blob/main/pkg/block/indexheader/binary_reader.go#L949 when the current index header reader is pointing to the last entry in the posting offset table but continue iterating further. This might cause invalid index size bug (reproduced in the new unit test I added) or wrong index range (end < start).

Example returned index ranges. The last one {517099476 111} is wrong. Others are either not found range {-1, -1} or normal range with start value (the first one) < end value (the second one)

[{-1 -1} {512174520 515605200} {-1 -1} {-1 -1} {515605208 517099468} {-1 -1} {-1 -1} {517099476 111}]

This will result in wrong cardinality calculation in lazy posting optimization and make the query performance worse. https://github.com/thanos-io/thanos/blob/main/pkg/store/lazy_postings.go#L64

Verification

I added a unit test to reproduce and also a test case to compare results from batched and non batched implementation.

[yeya24]

Without this fix, it will cause error invalid index size.

[alanprot]

Ok.. good catch.

I had to pull this PR and run the test to see what was going on but now I understand. :P
Good catch.. LGTM.

We could just add an extra validation on this lines as in this case the decoder buff also returned an error:

thanos/pkg/block/indexheader/binary_reader.go

Lines 947 to 949 in 17afd29

	skipNAndName(&d, &buf)
	d.UvarintBytes() // Label value.
	postingOffset := int64(d.Uvarint64())

[yeya24]

Thanks for the review. I am going to merge this pr since I believe this change is only used in lazy postings.