Ensure postings are always sorted #150
Conversation
| @@ -1038,8 +1038,6 @@ func (s *stripeSeries) getOrSet(hash uint64, series *memSeries) (*memSeries, boo | |||
| return prev, false | |||
| } | |||
| s.hashes[i].set(hash, series) | |||
|
|
|||
| s.hashes[i][hash] = append(s.hashes[i][hash], series) | |||
There was a problem hiding this comment.
This was a leftover and we were actually storing series twice – without any negative effects aside from space consumption.
| // be generated independently before adding them to postings. | ||
| // We repair order violations on insert. The invariant is that the first n-1 | ||
| // items in the list are already sorted. | ||
| for i := len(list) - 1; i >= 1; i-- { |
There was a problem hiding this comment.
There is still a race here, but I might be wrong. Also this case is a long shot, but hear me out :)
Let the existing list be: [1, 2, 3]
Now we insert: 4, 5, 6, 7 concurrently in this way:
[1, 2, 3, 6, 4, 5, 7].
Now, if the check for 5 runs before check for 4, which means in the end, we end up with: [1, 2, 3, 4, 6, 5, 7]
There was a problem hiding this comment.
Also, if we have [1, 2, 3] and insert 6 and then 4 and then 5.
We get at one point:
list := [1, 2, 3, 6, 4, 5]
p.m[l] = list
Even the swapping for 6, 4 that happens is not visible to the new list as it is a copy, AFAICS.
There was a problem hiding this comment.
The insertion itself is guarded by mutex, so there is no concurrency at play here.
Before each insert all elements so far are sorted. Just the last one is potentially out of order.
So the cases you describe are not possible due to this invariant.
|
👍 |
|
What's the impact if they're not sorted? It might be good to include an explanation in the commit message. |
|
Done. |
IDs for new series are handed out before the postings are locked. Thus series are not indexed in order of their IDs, which could result in only partially sorted postings list. Iterating over those silently skipped elements as the sort invariant was violated.
|
Thanks @fabxc. |
@gouthamve