Fix Data race in semi-join #17417
Fix Data race in semi-join #17417
Conversation
Signed-off-by: Manan Gupta <[email protected]>
GuptaManan100
added
Type: Bug
Component: Query Serving
Backport to: release-19.0
GuptaManan100
requested review from
harshit-gangal,
systay and
frouioui
as code owners
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
|
vitess-bot
bot
added
NeedsBackportReason
GuptaManan100
removed
NeedsDescriptionUpdate
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17417 +/- ##
==========================================
+ Coverage 67.60% 67.64% +0.04%
==========================================
Files 1581 1581
Lines 253945 253777 -168
==========================================
+ Hits 171670 171674 +4
+ Misses 82275 82103 -172 ☔ View full report in Codecov by Sentry. |
| err := vcursor.StreamExecutePrimitive(ctx, jn.Left, bindVars, wantfields, func(lresult *sqltypes.Result) error { | ||
| joinVars := make(map[string]*querypb.BindVariable) | ||
| result := &sqltypes.Result{Fields: lresult.Fields} | ||
| for _, lrow := range lresult.Rows { | ||
| for k, col := range jn.Vars { | ||
| joinVars[k] = sqltypes.ValueBindVariable(lrow[col]) | ||
| } | ||
| rowAdded := false | ||
| var rowAdded atomic.Bool | ||
| err := vcursor.StreamExecutePrimitive(ctx, jn.Right, combineVars(bindVars, joinVars), false, func(rresult *sqltypes.Result) error { |
There was a problem hiding this comment.
There is another issue I mentioned: during a transaction, we might end up opening two connections, leaving one of them in limbo.
Do you have another PR that fixes it?
There was a problem hiding this comment.
I will open a separate issue for it, with separate tests and PR for it, after this one gets merged.
| } | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if rowAdded.Load() { |
There was a problem hiding this comment.
Why is this one atomic? If this needs an atomic since it otherwise races, I imagine that the append on the next line also races and needs to be guarded with a lock then?
If this doesn't race, it doesn't need to be atomic?
There was a problem hiding this comment.
Yes i know, I made the changes such that i could avoid using this atomic thing, but in the callback right above we are writing to it. We are only setting it to true but golang still complains in a -race test saying there is a concurrent write. So I had to make it atomic to avoid that. That being said, for strictly correctness purposes, we didn't need to make it atomic, but I couldn't figure out how to make the test work without making it atomic.
There was a problem hiding this comment.
both are racy as the internal StreamExecute can do parallel execute to multiple shards
There was a problem hiding this comment.
@harshit-gangal Right, so that means we need to guard the append too right? Or otherwise we lose data there potentially?
| err := jn.TryStreamExecute(context.Background(), &noopVCursor{}, map[string]*querypb.BindVariable{}, true, func(result *sqltypes.Result) error { | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
should the results be validated?
Description
As described in #17410 and #17411, there is a data race in the stream execute code of semi-join.
This PR adds a test to uncover the problem and fixes it.
Related Issue(s)
Checklist
Deployment Notes