Fix `MemPostings.Add` and `MemPostings.Get` data race #15141

colega · 2024-10-10T10:38:13Z

When references are not added in order, we are sorting in place the tail of the postings list held by MemPostings, however the array backing that slice might already being used by some readers.

Corresponds to #15317.

Signed-off-by: Oleg Zaytsev <[email protected]>

We can't modify the postings list that are held in MemPostings as they might already be in use by some readers. Signed-off-by: Oleg Zaytsev <[email protected]>

I used `require.Equal` instead of `require.Len` on purpose, as when the slice is getting bigger, the error message that `require.Len` prints is somtimes too verbose. However that makes linter unhappy, linter thinks that `require.Len` is always better. Signed-off-by: Oleg Zaytsev <[email protected]>

colega · 2024-10-10T11:18:49Z

Running some benchmarks:

goos: darwin
goarch: arm64
pkg: github.com/prometheus/prometheus/tsdb
cpu: Apple M3 Pro
                                             │ HeadStripeSeriesCreate.main │     HeadStripeSeriesCreate.fix     │
                                             │           sec/op            │   sec/op     vs base               │
HeadStripeSeriesCreate-12                                      728.0n ± 8%   705.0n ± 4%  -3.16% (p=0.015 n=10)
HeadStripeSeriesCreateParallel-12                              686.8n ± 5%   678.1n ± 2%       ~ (p=0.579 n=10)
HeadStripeSeriesCreate_PreCreationFailure-12                   70.61n ± 6%   70.01n ± 0%  -0.85% (p=0.025 n=10)
geomean                                                        328.0n        322.3n       -1.76%

bboreham

Change seems fine; couple of thoughts below.

bboreham · 2024-10-10T11:33:32Z

tsdb/index/postings.go

+	old := list
+	list = make([]storage.SeriesRef, len(old), cap(old))
+	copy(list, old)


can we use slices.Clone ?

I checked, and slices.Clone doesn't create a slice of same capacity, which is what I wanted here.

bboreham · 2024-10-10T11:39:16Z

tsdb/index/postings.go

 	for i := len(list) - 1; i >= 1; i-- {
 		if list[i] >= list[i-1] {
 			break
 		}
 		list[i], list[i-1] = list[i-1], list[i]
 	}


Idea: this loop could be modified to find the insertion point, then copy the head, insert new value, copy the tail.
(and not copy on line 422).

I'm not sure that's better given that we expect this to happen few times and very few last elements would need to be swapped. Can we experiment with that in a follow up?

bboreham · 2024-10-10T11:42:15Z

tsdb/index/postings_test.go

+
+		// We don't read the value of the postings here,
+		// this is tested in TestMemPostings_Unordered_Add_Get where it's easier to achieve the determinism.
+		// This test just checks that there's no data race.


Reminded me of

prometheus/tsdb/db_test.go

Line 8644 in 53726de

copy(bb, b) // This copying of chunk bytes detects any race.

aknuds1

LGTM, should there be a changelog entry though?

bboreham · 2024-10-10T12:47:25Z

It's not typical to add changelog entries in Prometheus.
Put it in the PR title is easiest.

jesusvazquez

LGTM

aknuds1 · 2024-10-10T12:58:08Z

It's not typical to add changelog entries in Prometheus.

@bboreham so what's the convention about when to put e.g. bug fixes in the changelog? This make me uncertain about when to do so, I thought when making user facing fixes they should generally go in there.

Signed-off-by: Oleg Zaytsev <[email protected]>

colega · 2024-10-10T13:24:12Z

I pushed a commit changing the logic to avoid copying again when we just copied for appending.

If there are no common labels on the series, we don't excercise the ordering part of MemSeries, as we're just creating slices of one element for each label value. Signed-off-by: Oleg Zaytsev <[email protected]>

aknuds1 · 2024-10-11T13:09:23Z

Thanks!

…heus#15141)" This reverts commit 50ef0dc. Memory allocation goes so high in Prombench that the system is unusable.

…heus#15141)" This reverts commit 50ef0dc. Memory allocation goes so high in Prombench that the system is unusable. Signed-off-by: Bryan Boreham <[email protected]>

bboreham · 2024-11-03T12:08:44Z

Apparently the case where we copy is quite common when running Prombench, and therefore I think this will impact real users.

I have proposed to revert at #15316.

bboreham · 2024-11-03T12:18:57Z

Maybe, instead of adding all the burden to MemPostings.Add(), we could make ListPostings deal with partially-sorted lists?

Is there an issue detailing how this problem was found?

aknuds1 · 2024-11-03T12:25:22Z

Is there an issue detailing how this problem was found?

@bboreham A public facing issue wasn't added, I can make one.

…heus#15141)" This reverts commit 50ef0dc. Memory allocation goes so high in Prombench that the system is unusable. Signed-off-by: Bryan Boreham <[email protected]>

aknuds1 · 2024-11-03T13:13:21Z

@bboreham I made an issue.

Revert "Fix `MemPostings.Add` and `MemPostings.Get` data race (#15141)"

* Tests for Mempostings.{Add,Get} data race * Fix MemPostings.{Add,Get} data race We can't modify the postings list that are held in MemPostings as they might already be in use by some readers. * Modify BenchmarkHeadStripeSeriesCreate to have common labels If there are no common labels on the series, we don't excercise the ordering part of MemSeries, as we're just creating slices of one element for each label value. --------- Signed-off-by: Oleg Zaytsev <[email protected]>

…heus#15141)" This reverts commit b6285be. Memory allocation went through the roof.

Revert "Fix `MemPostings.Add` and `MemPostings.Get` data race (prometheus#15141)"

kushalShukla-web · 2025-06-28T13:11:48Z

Maybe, instead of adding all the burden to MemPostings.Add(), we could make ListPostings deal with partially-sorted lists?

Hi @bboreham, if we remove the sorting part from addFor, it means we’ll need to sort during retrieval, which I believe could increase the time complexity. Wouldn’t that make Get() slower?

colega · 2025-07-14T08:38:00Z

@kushalShukla-web I was thinking that we could involve isolation here, and committing to the sorted slice once all related transactions are complete, while keeping an unsorted tail for the series created in the ongoing transactions.

This would require having isolation enabled everywhere though.

kushalShukla-web · 2025-10-15T11:18:46Z

@colega In that case, we need to change m map[string]map[string][]storage.SeriesRef into two maps: committed map[string]map[string][]storage.SeriesRef and tails map[string]map[string][]storage.SeriesRef. we can avoid the copying part if we do in this way.

colega added 2 commits October 10, 2024 12:26

Tests for Mempostings.{Add,Get} data race

bfd7acc

Signed-off-by: Oleg Zaytsev <[email protected]>

Fix MemPostings.{Add,Get} data race

402a0e8

We can't modify the postings list that are held in MemPostings as they might already be in use by some readers. Signed-off-by: Oleg Zaytsev <[email protected]>

colega requested a review from jesusvazquez as a code owner October 10, 2024 10:38

bboreham previously approved these changes Oct 10, 2024

View reviewed changes

aknuds1 previously approved these changes Oct 10, 2024

View reviewed changes

jesusvazquez previously approved these changes Oct 10, 2024

View reviewed changes

Only copy again slice if it wasn't copied already

512cb16

Signed-off-by: Oleg Zaytsev <[email protected]>

colega dismissed stale reviews from jesusvazquez, aknuds1, and bboreham via 512cb16 October 10, 2024 13:21

Modify BenchmarkHeadStripeSeriesCreate to have common labels

f898284

If there are no common labels on the series, we don't excercise the ordering part of MemSeries, as we're just creating slices of one element for each label value. Signed-off-by: Oleg Zaytsev <[email protected]>

aknuds1 approved these changes Oct 11, 2024

View reviewed changes

aknuds1 merged commit 50ef0dc into prometheus:main Oct 11, 2024

aknuds1 mentioned this pull request Oct 29, 2024

Fix MemPostings.Add and MemPostings.Get data race grafana/mimir-prometheus#730

Merged

bboreham added a commit to bboreham/prometheus that referenced this pull request Nov 3, 2024

Revert "Fix MemPostings.Add and MemPostings.Get data race (promet…

d87ec32

…heus#15141)" This reverts commit 50ef0dc. Memory allocation goes so high in Prombench that the system is unusable.

bboreham mentioned this pull request Nov 3, 2024

Revert "Fix MemPostings.Add and MemPostings.Get data race (#15141)" #15316

Merged

bboreham added a commit to bboreham/prometheus that referenced this pull request Nov 3, 2024

Revert "Fix MemPostings.Add and MemPostings.Get data race (promet…

9d8268e

…heus#15141)" This reverts commit 50ef0dc. Memory allocation goes so high in Prombench that the system is unusable.

bboreham mentioned this pull request Nov 3, 2024

Bisect series error #15315

Closed

aknuds1 mentioned this pull request Nov 3, 2024

Data race detected in tsdb/index.MemPostings.addFor. #15317

Open

bboreham added a commit that referenced this pull request Nov 3, 2024

Merge pull request #15316 from bboreham/revert-15141

a1a78a1

Revert "Fix `MemPostings.Add` and `MemPostings.Get` data race (#15141)"

alanprot mentioned this pull request Dec 11, 2024

Expanded Postings Cache can cache results without the nearly created series under high load. cortexproject/cortex#6417

Merged

3 tasks

julienduchesne pushed a commit to julienduchesne/prometheus that referenced this pull request Dec 13, 2024

Revert "Fix MemPostings.Add and MemPostings.Get data race (promet…

b5cb88f

…heus#15141)" This reverts commit b6285be. Memory allocation went through the roof.

julienduchesne pushed a commit to julienduchesne/prometheus that referenced this pull request Dec 13, 2024

Merge pull request prometheus#740 from grafana/revert-730

dea6247

Revert "Fix `MemPostings.Add` and `MemPostings.Get` data race (prometheus#15141)"

julienduchesne pushed a commit to julienduchesne/prometheus that referenced this pull request Dec 13, 2024

Fix MemPostings.Add and MemPostings.Get data race (prometheus#15141)

54fc60d

Fix MemPostings.Add and MemPostings.Get data race #15141

Fix MemPostings.Add and MemPostings.Get data race #15141

Uh oh!

Conversation

colega commented Oct 10, 2024 • edited by aknuds1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colega commented Oct 10, 2024

Uh oh!

bboreham left a comment

Choose a reason for hiding this comment

Uh oh!

bboreham Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

colega Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

bboreham Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

colega Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

bboreham Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

aknuds1 left a comment

Choose a reason for hiding this comment

Uh oh!

bboreham commented Oct 10, 2024

Uh oh!

jesusvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

aknuds1 commented Oct 10, 2024

Uh oh!

colega commented Oct 10, 2024

Uh oh!

aknuds1 commented Oct 11, 2024

Uh oh!

bboreham commented Nov 3, 2024

Uh oh!

bboreham commented Nov 3, 2024

Uh oh!

aknuds1 commented Nov 3, 2024

Uh oh!

aknuds1 commented Nov 3, 2024

Uh oh!

kushalShukla-web commented Jun 28, 2025

Uh oh!

colega commented Jul 14, 2025

Uh oh!

kushalShukla-web commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fix `MemPostings.Add` and `MemPostings.Get` data race #15141

Fix `MemPostings.Add` and `MemPostings.Get` data race #15141

colega commented Oct 10, 2024 •

edited by aknuds1

Loading

kushalShukla-web commented Oct 15, 2025 •

edited

Loading