Skip to content

Commit

Permalink
storage: do not mutate ba.Requests in optimizePuts
Browse files Browse the repository at this point in the history
This fixes a source of data races in the experimental proposer-evaluated KV
PR cockroachdb#10327.

Updated `TestOptimizePuts` so that prior to the fix, it failed with

```
--- FAIL: TestOptimizePuts (0.01s)
       	replica_test.go:1522: 2: optimizePuts mutated the original request
        slice: [[0].Put.Blind: false != true [1].Put.Blind: false != true
        [2].Put.Blind: false != true [3].Put.Blind: false != true
        [4].Put.Blind: false != true [5].Put.Blind: false != true
        [6].Put.Blind: false != true [7].Put.Blind: false != true
        [8].Put.Blind: false != true [9].Put.Blind: false != true]
```
  • Loading branch information
tbg committed Nov 2, 2016
1 parent 68601cf commit d737292
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 9 deletions.
29 changes: 21 additions & 8 deletions pkg/storage/replica.go
Original file line number Diff line number Diff line change
Expand Up @@ -3341,7 +3341,13 @@ func isOnePhaseCommit(ba roachpb.BatchRequest) bool {
// range of keys being written is empty. If so, then the run can be
// set to put "blindly", meaning no iterator need be used to read
// existing values during the MVCC write.
func optimizePuts(batch engine.ReadWriter, reqs []roachpb.RequestUnion, distinctSpans bool) {
// The caller should use the returned slice (which is either equal to
// the input slice, or has been shallow-copied appropriately to avoid
// mutating the original requests).
func optimizePuts(
batch engine.ReadWriter, reqs []roachpb.RequestUnion, distinctSpans bool,
) []roachpb.RequestUnion {
origReqs := reqs
var minKey, maxKey roachpb.Key
var unique map[string]struct{}
if !distinctSpans {
Expand Down Expand Up @@ -3381,7 +3387,7 @@ func optimizePuts(batch engine.ReadWriter, reqs []roachpb.RequestUnion, distinct
}

if len(reqs) < optimizePutThreshold { // don't bother if below this threshold
return
return origReqs
}
iter := batch.NewIterator(false /* total order iterator */)
defer iter.Close()
Expand All @@ -3397,18 +3403,25 @@ func optimizePuts(batch engine.ReadWriter, reqs []roachpb.RequestUnion, distinct
}
// Set the prefix of the run which is being written to virgin
// keyspace to "blindly" put values.
for _, r := range reqs {
if iterKey == nil || bytes.Compare(iterKey, r.GetInner().Header().Key) > 0 {
switch t := r.GetInner().(type) {
newReqs := append([]roachpb.RequestUnion(nil), origReqs...)
for i := range reqs {
inner := reqs[i].GetInner()
if iterKey == nil || bytes.Compare(iterKey, inner.Header().Key) > 0 {
switch t := inner.(type) {
case *roachpb.PutRequest:
t.Blind = true
shallow := *t
shallow.Blind = true
(&newReqs[i]).MustSetInner(&shallow)
case *roachpb.ConditionalPutRequest:
t.Blind = true
shallow := *t
shallow.Blind = true
(&newReqs[i]).MustSetInner(&shallow)
default:
log.Fatalf(context.TODO(), "unexpected non-put request: %s", t)
}
}
}
return newReqs
}

func (r *Replica) executeBatch(
Expand Down Expand Up @@ -3440,7 +3453,7 @@ func (r *Replica) executeBatch(

// Optimize any contiguous sequences of put and conditional put ops.
if len(ba.Requests) >= optimizePutThreshold {
optimizePuts(batch, ba.Requests, ba.Header.DistinctSpans)
ba.Requests = optimizePuts(batch, ba.Requests, ba.Header.DistinctSpans)
}

// Update the node clock with the serviced request. This maintains a high
Expand Down
14 changes: 13 additions & 1 deletion pkg/storage/replica_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1509,7 +1509,19 @@ func TestOptimizePuts(t *testing.T) {
for _, r := range c.reqs {
batch.Add(r)
}
optimizePuts(tc.engine, batch.Requests, false)
goldenRequests := append([]roachpb.RequestUnion(nil), batch.Requests...)
for i := range goldenRequests {
clone := protoutil.Clone(goldenRequests[i].GetInner()).(roachpb.Request)
(&goldenRequests[i]).MustSetInner(clone)
}
oldRequests := batch.Requests
batch.Requests = optimizePuts(tc.engine, batch.Requests, false)
if !reflect.DeepEqual(goldenRequests, oldRequests) {
t.Fatalf("%d: optimizePuts mutated the original request slice: %s",
i, pretty.Diff(goldenRequests, oldRequests),
)
}

blind := []bool{}
for _, r := range batch.Requests {
switch t := r.GetInner().(type) {
Expand Down

0 comments on commit d737292

Please sign in to comment.