Skip to content

Commit eb0a104

Browse files
craig[bot]arulajmaniwenyihu6
committed
106316: concurrency: non-locking reads of RC txns ignore exclusive locks r=nvanbenschoten a=arulajmani Non-locking reads issued by transactions that can tolerate write-skew do not block on exclusive locks after this patch. Resolves cockroachdb#94729 Release note: None 107420: allocator: remove log.VEventf for alive stores in isStoreReady… r=kvoli a=wenyihu6 When investigating an allocator simulator test, we observed a significant amount of time spent by `log.VEventf`, consuming 64% of time (according to pprof CPU analysis). This was mainly caused by `rankedCandidateListForRebalancing` repeatedly calling `IsStoreReadyForRoutineReplicaTransfer` on all stores that pass the `StoreFilterThrottled` filter. As a temporary mitigation, we deleted `log.VEventf` in `isStoreReadyForRoutineReplicaTransferInternal` for less interesting cases where stores are alive. However, our longer term strategy is to buffer such outputs in the allocator at a higher level to reduce `log.VEventf` calls. Informs: cockroachdb#107421 Release note: None ---- | Before| After | | -| - | | <img width="300" alt="Screenshot 2023-07-21 at 6 08 01 PM" src="https://github.com/cockroachdb/cockroach/assets/56973754/e544282c-9405-49b1-9469-c8e54330794b"> |<img width="300" alt="Screenshot 2023-07-22 at 11 08 33 PM" src="https://github.com/cockroachdb/cockroach/assets/56973754/02c7ee25-d3e4-4b01-9a91-131c05046895"> Co-authored-by: Arul Ajmani <[email protected]> Co-authored-by: wenyihu6 <[email protected]>
3 parents 2972f13 + f3d2a83 + 1d1a912 commit eb0a104

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

47 files changed

+1287
-1133
lines changed

‎pkg/kv/kvserver/allocator/storepool/store_pool.go‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1312,8 +1312,6 @@ func (sp *StorePool) isStoreReadyForRoutineReplicaTransferInternal(
13121312
}
13131313
switch status {
13141314
case storeStatusThrottled, storeStatusAvailable:
1315-
log.VEventf(ctx, 3,
1316-
"s%d is a live target, candidate for rebalancing", targetStoreID)
13171315
return true
13181316
case storeStatusDead, storeStatusUnknown, storeStatusDecommissioning, storeStatusSuspect, storeStatusDraining:
13191317
log.VEventf(ctx, 3,

‎pkg/kv/kvserver/concurrency/concurrency_manager_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ func TestConcurrencyManagerBasic(t *testing.T) {
116116
d.ScanArgs(t, "epoch", &epoch)
117117
}
118118

119-
iso := scanIsoLevel(t, d)
119+
iso := concurrency.ScanIsoLevel(t, d)
120120
priority := scanTxnPriority(t, d)
121121

122122
uncertaintyLimit := ts

‎pkg/kv/kvserver/concurrency/datadriven_util_test.go‎

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import (
1515
"testing"
1616

1717
"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
18-
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation"
1918
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock"
2019
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/poison"
2120
"github.com/cockroachdb/cockroach/pkg/roachpb"
@@ -47,26 +46,6 @@ func scanTimestampWithName(t *testing.T, d *datadriven.TestData, name string) hl
4746
return ts
4847
}
4948

50-
func scanIsoLevel(t *testing.T, d *datadriven.TestData) isolation.Level {
51-
const key = "iso"
52-
if !d.HasArg(key) {
53-
return isolation.Serializable
54-
}
55-
var isoS string
56-
d.ScanArgs(t, key, &isoS)
57-
switch isoS {
58-
case "serializable":
59-
return isolation.Serializable
60-
case "snapshot":
61-
return isolation.Snapshot
62-
case "read-committed":
63-
return isolation.ReadCommitted
64-
default:
65-
d.Fatalf(t, "unknown isolation level: %s", isoS)
66-
return 0
67-
}
68-
}
69-
7049
func scanTxnPriority(t *testing.T, d *datadriven.TestData) enginepb.TxnPriority {
7150
priority := scanUserPriority(t, d)
7251
// NB: don't use roachpb.MakePriority to avoid randomness.

‎pkg/kv/kvserver/concurrency/lock_table.go‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -767,9 +767,14 @@ func (g *lockTableGuardImpl) curLockMode() lock.Mode {
767767
var reqMode lock.Mode
768768
switch g.curStrength() {
769769
case lock.None:
770-
reqMode = lock.MakeModeNone(g.ts, isolation.Serializable)
770+
iso := isolation.Serializable
771+
if g.txn != nil {
772+
iso = g.txn.IsoLevel
773+
}
774+
reqMode = lock.MakeModeNone(g.ts, iso)
771775
case lock.Exclusive:
772-
reqMode = lock.MakeModeExclusive(g.ts, isolation.Serializable)
776+
assert(g.txn != nil, "only transactional requests can acquire exclusive locks")
777+
reqMode = lock.MakeModeExclusive(g.ts, g.txn.IsoLevel)
773778
case lock.Intent:
774779
reqMode = lock.MakeModeIntent(g.ts)
775780
default:
@@ -1209,7 +1214,7 @@ func (l *lockState) safeFormat(sb *redact.StringBuilder, txnStatusCache *txnStat
12091214
}
12101215
txn, ts := l.getLockHolder()
12111216
if txn != nil { // lock is held
1212-
sb.Printf(" holder: txn: %v epoch: %d, ts: %v, info: ", redact.Safe(txn.ID), redact.Safe(txn.Epoch), redact.Safe(ts))
1217+
sb.Printf(" holder: txn: %v epoch: %d, iso: %s, ts: %v, info: ", redact.Safe(txn.ID), redact.Safe(txn.Epoch), redact.Safe(txn.IsoLevel), redact.Safe(ts))
12131218
if !l.holder.replicatedInfo.isEmpty() {
12141219
l.holder.replicatedInfo.safeFormat(sb)
12151220
if !l.holder.unreplicatedInfo.isEmpty() {
@@ -1717,8 +1722,7 @@ func (l *lockState) getLockMode() lock.Mode {
17171722
if l.isHeldReplicated() {
17181723
return lock.MakeModeIntent(lockHolderTS)
17191724
}
1720-
// TODO(arul): Thread in the correct isolation level here.
1721-
return lock.MakeModeExclusive(lockHolderTS, isolation.Serializable)
1725+
return lock.MakeModeExclusive(lockHolderTS, lockHolderTxn.IsoLevel)
17221726
}
17231727

17241728
// Removes the current lock holder from the lock.

‎pkg/kv/kvserver/concurrency/lock_table_test.go‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"time"
2323

2424
"github.com/cockroachdb/cockroach/pkg/keys"
25+
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation"
2526
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock"
2627
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/poison"
2728
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/lockspanset"
@@ -61,7 +62,7 @@ time-tick [m=<int>] [s=<int>] [ms=<int>] [ns=<int>]
6162
6263
Forces the manual clock to tick forward m minutes, s seconds, ms milliseconds, and ns nanoseconds.
6364
64-
new-txn txn=<name> ts=<int>[,<int>] epoch=<int> [seq=<int>]
65+
new-txn txn=<name> ts=<int>[,<int>] epoch=<int> [seq=<int>] [iso=<level>]
6566
----
6667
6768
Creates a TxnMeta.
@@ -247,6 +248,7 @@ func TestLockTableBasic(t *testing.T) {
247248
if d.HasArg("seq") {
248249
d.ScanArgs(t, "seq", &seq)
249250
}
251+
iso := ScanIsoLevel(t, d)
250252
txnMeta, ok := txnsByName[txnName]
251253
var id uuid.UUID
252254
if ok {
@@ -259,6 +261,7 @@ func TestLockTableBasic(t *testing.T) {
259261
Epoch: enginepb.TxnEpoch(epoch),
260262
Sequence: enginepb.TxnSeq(seq),
261263
WriteTimestamp: ts,
264+
IsoLevel: iso,
262265
}
263266
return ""
264267

@@ -719,6 +722,26 @@ func scanSpans(
719722
return latchSpans, lockSpans
720723
}
721724

725+
func ScanIsoLevel(t *testing.T, d *datadriven.TestData) isolation.Level {
726+
const key = "iso"
727+
if !d.HasArg(key) {
728+
return isolation.Serializable
729+
}
730+
var isoS string
731+
d.ScanArgs(t, key, &isoS)
732+
switch isoS {
733+
case "serializable":
734+
return isolation.Serializable
735+
case "snapshot":
736+
return isolation.Snapshot
737+
case "read-committed":
738+
return isolation.ReadCommitted
739+
default:
740+
d.Fatalf(t, "unknown isolation level: %s", isoS)
741+
return 0
742+
}
743+
}
744+
722745
func ScanLockStrength(t *testing.T, d *datadriven.TestData) lock.Strength {
723746
var strS string
724747
d.ScanArgs(t, "strength", &strS)
@@ -1868,10 +1891,10 @@ func TestLockStateSafeFormat(t *testing.T) {
18681891
seqs: []enginepb.TxnSeq{1},
18691892
}
18701893
require.EqualValues(t,
1871-
" lock: ‹\"KEY\"›\n holder: txn: 6ba7b810-9dad-11d1-80b4-00c04fd430c8 epoch: 0, ts: 0.000000123,7, info: unrepl seqs: [1]\n",
1894+
" lock: ‹\"KEY\"›\n holder: txn: 6ba7b810-9dad-11d1-80b4-00c04fd430c8 epoch: 0, iso: Serializable, ts: 0.000000123,7, info: unrepl seqs: [1]\n",
18721895
redact.Sprint(l))
18731896
require.EqualValues(t,
1874-
" lock: ‹×›\n holder: txn: 6ba7b810-9dad-11d1-80b4-00c04fd430c8 epoch: 0, ts: 0.000000123,7, info: unrepl seqs: [1]\n",
1897+
" lock: ‹×›\n holder: txn: 6ba7b810-9dad-11d1-80b4-00c04fd430c8 epoch: 0, iso: Serializable, ts: 0.000000123,7, info: unrepl seqs: [1]\n",
18751898
redact.Sprint(l).Redact())
18761899
}
18771900

‎pkg/kv/kvserver/concurrency/testdata/concurrency_manager/basic‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ debug-lock-table
5858
----
5959
num=1
6060
lock: "k"
61-
holder: txn: 00000002-0000-0000-0000-000000000000 epoch: 0, ts: 12.000000000,1, info: unrepl seqs: [0]
61+
holder: txn: 00000002-0000-0000-0000-000000000000 epoch: 0, iso: Serializable, ts: 12.000000000,1, info: unrepl seqs: [0]
6262

6363
finish req=req2
6464
----
@@ -68,7 +68,7 @@ debug-lock-table
6868
----
6969
num=1
7070
lock: "k"
71-
holder: txn: 00000002-0000-0000-0000-000000000000 epoch: 0, ts: 12.000000000,1, info: unrepl seqs: [0]
71+
holder: txn: 00000002-0000-0000-0000-000000000000 epoch: 0, iso: Serializable, ts: 12.000000000,1, info: unrepl seqs: [0]
7272

7373
reset
7474
----

0 commit comments

Comments
 (0)