Fix TestSelectScatterPartialOLAP data race#19390
Fix TestSelectScatterPartialOLAP data race#19390Devanshusharma2005 wants to merge 10 commits intovitessio:mainfrom
Conversation
Signed-off-by: Matt Lord <mattalord@gmail.com> Signed-off-by: Devanshu Sharma <devanshusharma658@gmail.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Devanshu Sharma <devanshusharma658@gmail.com>
Co-authored-by: Arthur Schreiber <schreiber.arthur@googlemail.com> Signed-off-by: Devanshu Sharma <devanshusharma658@gmail.com>
Co-authored-by: Arthur Schreiber <schreiber.arthur@googlemail.com> Signed-off-by: Devanshu Sharma <devanshusharma658@gmail.com>
Signed-off-by: Devanshu Sharma <devanshusharma658@gmail.com>
Signed-off-by: Devanshu Sharma <devanshusharma658@gmail.com>
@Devanshusharma2005 I don't think that's right. You can see how concat is implemented here: https://cs.opensource.google/go/go/+/refs/tags/go1.26.0:src/slices/slices.go;l=489-505 Essentially, it gets the full length required for the final slice, then allocates that slice and appends into it. Basically exactly what you did before. So it doesn't perform any additional allocations. |
|
@Devanshusharma2005 👋 Hey, I left a few more comments. I'm not sure the code here warrants being so performance focused, but I think it's a good learning opportunity to better understand how to make changes with a performance focus in mind. |
Hi arthur. Thank you so much for these reviews. |
Co-authored-by: Arthur Schreiber <schreiber.arthur@googlemail.com> Signed-off-by: Devanshu Sharma <devanshusharma658@gmail.com>
Signed-off-by: Devanshu Sharma <devanshusharma658@gmail.com>
Signed-off-by: Devanshu Sharma <devanshusharma658@gmail.com>
|
@arthurschreiber I’ve implemented your first |
mhamza15
left a comment
There was a problem hiding this comment.
LGTM! Just some very minor comments.
Signed-off-by: Devanshu Sharma <devanshusharma658@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19390 +/- ##
==========================================
- Coverage 69.95% 69.66% -0.29%
==========================================
Files 1610 1613 +3
Lines 216083 216465 +382
==========================================
- Hits 151159 150805 -354
- Misses 64924 65660 +736 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@arthurschreiber lets get this across the finish line ^^ |
Description
This PR fixes a potential race condition in vtgate where multiple goroutines may access or mutate
PreSessions,ShardSessions, andPostSessionsconcurrently duringRollback,Release, orReleaseAll.Previously, these flows directly iterated over session slice fields without taking a stable snapshot. If another goroutine modified those slices at the same time, it could lead to a data race.
This change introduces safe snapshot helpers on
SafeSessionand updates the relevant call sites to use them, ensuring that Rollback, Release, and ReleaseAll work on a stable copy of the session slices.The race was detected in the unit test
TestSelectScatterPartialOLAP, which would occasionally fail with-raceenabled.Related Issue(s)
Fixes #18996
BEFORE
AFTER
To Reproduce
Reproduction Steps
To reproduce the race condition in
TestSelectScatterPartialOLAP, run the test multiple times with the race detector enabled:Checklist
Deployment Notes
AI Disclosure
Used grep command to trace the function calls and map out the flow, then manually patched the logic (kept it simple since it's a small change.)