storage/concurrency/lock: define lock Strength and Durability modes #44054
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @sumeerbhola)
pkg/storage/concurrency/lock/locking.go, line 24 at r1 (raw file):
// to access the same keys. // // Compatability Matrix
Compatibility
pkg/storage/concurrency/lock/locking.go, line 30 at r1 (raw file):
// each other and that they can not both be held on a given key by different // transactions, concurrently. A cell without an X means that the two strengths // are compatable with each other and that they can be held on a given key by
ditto
pkg/storage/concurrency/lock/locking.go, line 50 at r1 (raw file):
// // [†] reads under optimistic concurrency control in CockroachDB only conflict // with Exclusive locks if the read's tiemstamp is equal to or greater than the
timestamp
pkg/storage/concurrency/lock/locking.go, line 51 at r1 (raw file):
// [†] reads under optimistic concurrency control in CockroachDB only conflict // with Exclusive locks if the read's tiemstamp is equal to or greater than the // locks timestamp. If the read's timestamp is below the Exclusive locks
lock's (twice)
pkg/storage/concurrency/lock/locking.go, line 52 at r1 (raw file):
// with Exclusive locks if the read's tiemstamp is equal to or greater than the // locks timestamp. If the read's timestamp is below the Exclusive locks // timestamp then the two are compatable.
tible
pkg/storage/concurrency/lock/locking.go, line 61 at r1 (raw file):
// Shared (S) locks are used by read-only operations and allow concurrent // transactions to read under pessimistic concurrency control. Shared locks // are compatable with each other but are not compatable with Upgrade or
compatible
this happens a lot, so probably just search for "patab" across all files
pkg/storage/concurrency/lock/locking.go, line 107 at r1 (raw file):
// performed the optimistic read may be unable to perform a successful read // refresh if it attempts to refresh to a timestamp at or past the timestamp // of the lock conversion.
The optimistic read may also prevent the writing transaction from committing at the timestamp at which it had hoped it would commit. I don't know if this is worth mentioning, but I think we are hoping clients can use targeted SFU to avoid needing to restart, in which case you do want to avoid having your writes pushed because once your timestamp is pushed, you need to re-verify the optimistic reads. Or maybe we're assuming that all unprotected reads will refresh just fine? That's reasonable too. Anyway, it's likely that we want to comment on this all a bit.
pkg/storage/concurrency/lock/locking.go, line 113 at r1 (raw file):
// provide a transaction with exclusive access to a key. When an Exclusive // lock is held by a transaction on a given key, no other transaction can // read or write to that key. The lock holder is free to read and write to
read from or write to
pkg/storage/concurrency/lock/locking.go, line 124 at r1 (raw file):
const ( _ Durability = 1 << iota
This isn't a bitfield, so use = iota here.
pkg/storage/concurrency/lock/locking.go, line 136 at r1 (raw file):
// Replicated locks are held on at least a quorum of Replicas in a Range. // They are slower to acquire and replease than Unreplicated locks because
s/please/lease/
There was a problem hiding this comment.
Reviewable status:
pkg/storage/concurrency/lock/locking.go, line 1 at r1 (raw file):
// Copyright 2019 The Cockroach Authors.
nit: 2020
pkg/storage/concurrency/lock/locking.go, line 64 at r1 (raw file):
// Exclusive locks. This means that multiple transactions can hold a Shared // lock on the same key at the same time, but no other transaction can // modify the key at the same time. A holder of a Shared lock on a key is
I am assuming this is using the term "key" and not "row" because this is down in the kv layer, yes?
It may be worth clarifying up top that a table row can correspond to many key-value pairs because of indices and it is up to the higher layer to decide which key-value pairs a lock is acquired for. And if a row corresponds to k1-v1 and k2-v2 and the higher layer acquires conflicting locks on k1, k2 respectively for two different operations, they will proceed concurrently.
pkg/storage/concurrency/lock/locking.go, line 65 at r1 (raw file):
// lock on the same key at the same time, but no other transaction can // modify the key at the same time. A holder of a Shared lock on a key is // only permitted to read from key while the lock is held.
"read from key" really means reading the key or the value. May be worth clarifying since someone may confuse this with KEY SHARE.
pkg/storage/concurrency/lock/locking.go, line 88 at r1 (raw file):
// be aborted. // // To avoid this potential deadlock problem, an Update lock can be used in
s/Update/Upgrade/g
pkg/storage/concurrency/lock/locking.go, line 107 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
The optimistic read may also prevent the writing transaction from committing at the timestamp at which it had hoped it would commit. I don't know if this is worth mentioning, but I think we are hoping clients can use targeted SFU to avoid needing to restart, in which case you do want to avoid having your writes pushed because once your timestamp is pushed, you need to re-verify the optimistic reads. Or maybe we're assuming that all unprotected reads will refresh just fine? That's reasonable too. Anyway, it's likely that we want to comment on this all a bit.
It sounds like Upgrade locks are incompatible with other locks but are not preventing non-locking reads -- do we know what workloads would benefit from them?
pkg/storage/concurrency/lock/locking.go, line 124 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
This isn't a bitfield, so use
= iotahere.
Are the bitmasks to concisely represent that a txn holds a lock with multiple Strengths and Durability?
nvanbenschoten
force-pushed
the
nvanbenschoten/lockModes
branch
from
7200f85 to
5c7cf08
Compare
There was a problem hiding this comment.
TFTRs! I added a None variant to Strength to "represents the absence of a lock or the intention to acquire locks". Essentially it was a better place to document OCC. PTAL.
Reviewable status:
pkg/storage/concurrency/lock/locking.go, line 1 at r1 (raw file):
Previously, sumeerbhola wrote…
nit: 2020
Done.
pkg/storage/concurrency/lock/locking.go, line 24 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Compatibility
Done.
pkg/storage/concurrency/lock/locking.go, line 30 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
ditto
Done.
pkg/storage/concurrency/lock/locking.go, line 50 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
timestamp
Done.
pkg/storage/concurrency/lock/locking.go, line 51 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
lock's (twice)
Done.
pkg/storage/concurrency/lock/locking.go, line 52 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
tible
Done.
pkg/storage/concurrency/lock/locking.go, line 61 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
compatible
this happens a lot, so probably just search for "patab" across all files
Who built this language? Done.
pkg/storage/concurrency/lock/locking.go, line 64 at r1 (raw file):
I am assuming this is using the term "key" and not "row" because this is down in the kv layer, yes?
Yes.
It may be worth clarifying up top that a table row can correspond to many key-value pairs because of indices and it is up to the higher layer to decide which key-value pairs a lock is acquired for.
Done.
pkg/storage/concurrency/lock/locking.go, line 65 at r1 (raw file):
Previously, sumeerbhola wrote…
"read from key" really means reading the key or the value. May be worth clarifying since someone may confuse this with
KEY SHARE.
Done.
pkg/storage/concurrency/lock/locking.go, line 88 at r1 (raw file):
Previously, sumeerbhola wrote…
s/Update/Upgrade/g
Done.
pkg/storage/concurrency/lock/locking.go, line 107 at r1 (raw file):
The optimistic read may also prevent the writing transaction from committing at the timestamp at which it had hoped it would commit. I don't know if this is worth mentioning, but I think we are hoping clients can use targeted SFU to avoid needing to restart, in which case you do want to avoid having your writes pushed because once your timestamp is pushed, you need to re-verify the optimistic reads.
Yes, this is all true. I'm planning on running some experiments with YCSB-A and YCSB-B comparing the use of UNREPLICATED UPGRADE and UNREPLICATED EXCLUSIVE locks during the UPDATE statements' initial row fetch step. I suspect that until we get a little smarter about noticing that we don't need to refresh when converting an UPGRADE lock, we'll actually get better performance out of the latter. We'll have to see though.
Or maybe we're assuming that all unprotected reads will refresh just fine? That's reasonable too.
It's not clear to me. Certainly, in YCSB this is the case because there are no other unprotected reads, but we'll need to resolve this question.
Anyway, it's likely that we want to comment on this all a bit.
Done. Added a bit more about failed refreshes to the comment on None as well.
It sounds like Upgrade locks are incompatible with other locks but are not preventing non-locking reads -- do we know what workloads would benefit from them?
If we're smart about noticing that we don't need to refresh keys that were protected by upgrade locks then this is exactly the behavior we want for YCSB, which mixes UPDATE statements to a single column family on a row with scans over all column families in a row. Ideally, the scans could just ignore the locks acquired during the UPDATEs' initial KV fetch.
pkg/storage/concurrency/lock/locking.go, line 113 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
read from or write to
Done.
pkg/storage/concurrency/lock/locking.go, line 124 at r1 (raw file):
Are the bitmasks to concisely represent that a txn holds a lock with multiple Strengths and Durability?
No, the bitfield wasn't necessary. Done.
pkg/storage/concurrency/lock/locking.go, line 136 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
s/please/lease/
Done.
This commit introduces a new `concurrency/lock` package that provides type definitions for locking-related concepts used by concurrency control in the key-value layer. The package initially provides a lock `Strength` enumeration consisting of `Shared`, `Upgrade`, and `Exclusive` and a lock `Durability` enumeration consisting of `Unreplicated` and `Replicated`. These definitions are currently unused but will be referenced soon by cockroachdb#43740 and cockroachdb#43775. The changes around SELECT FOR UPDATE in the optimizer (cockroachdb#44015) are also approaching the point where they'll soon need the type definitions to interface with the KV API. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/lockModes
branch
from
5c7cf08 to
76f2cbe
Compare
|
bors r+ |
43862: storage/gc: create gc pkg, reverse iteration in rditer, paginate versions during GC r=tbg a=ajwerner This PR comes in 6 commits. See the commit messages for more details. The meat of the change is in: `storage: rework RunGC so it no longer buffers keys and values in memory`. 1) Fix the spanset Iterator such that SeekLT can seek to the end of the range. 2) Expose reverse iteration in rditer.ReplicaDataIterator. 3) Rearrange some code in gc_queue.go to put the RunGC() helpers below that function. 4) Remove lockableGCInfo because there was no need for it. 5) Move RunGC into a `gc` subpackage. 5) Rework RunGC to paginate versions of keys with reverse iteration. Fixes #42531. 44054: storage/concurrency/lock: define lock Strength and Durability modes r=nvanbenschoten a=nvanbenschoten This commit introduces a new `concurrency/lock` package that provides type definitions for locking-related concepts used by concurrency control in the key-value layer. The package initially provides a lock `Strength` enumeration consisting of `Shared`, `Upgrade`, and `Exclusive` and a lock `Durability` enumeration consisting of `Unreplicated` and `Replicated`. These definitions are currently unused but will be referenced soon by #43740 and #43775. The changes around SELECT FOR UPDATE in the optimizer (#44015) are also approaching the point where they'll soon need the type definitions to interface with the KV API. Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Build succeeded |
There was a problem hiding this comment.
Reviewable status:
pkg/storage/concurrency/lock/locking.go, line 75 at r2 (raw file):
// transactions are forced to restart. See the comment on txnSpanRefresher // for more. None Strength = iota
I have a bias against iota. First, it requires the code reader to have to count to figure out what value a given entry gets. Explicitly listing each value is more obvious. Second, it makes it too easy to change the values with an innocuous change, such as a reordering. This is important if the value is being persisted, but also if (more subtly) someone tries to look at old logs where the value has been printed.
(Sorry for the drive-by, I understand this is already merged, but if you happen be in here again...)
There was a problem hiding this comment.
Reviewable status:
pkg/storage/concurrency/lock/locking.go, line 75 at r2 (raw file):
Previously, danhhz (Daniel Harrison) wrote…
I have a bias against iota. First, it requires the code reader to have to count to figure out what value a given entry gets. Explicitly listing each value is more obvious. Second, it makes it too easy to change the values with an innocuous change, such as a reordering. This is important if the value is being persisted, but also if (more subtly) someone tries to look at old logs where the value has been printed.
(Sorry for the drive-by, I understand this is already merged, but if you happen be in here again...)
All fair points. I realized last night that this will need to be converted to a proto enum anyway, so I think that will address your concerns.
This commit introduces a new
concurrency/lockpackage that provides type definitions for locking-related concepts used by concurrency control in the key-value layer. The package initially provides a lockStrengthenumeration consisting ofShared,Upgrade, andExclusiveand a lockDurabilityenumeration consisting ofUnreplicatedandReplicated.These definitions are currently unused but will be referenced soon by #43740 and #43775. The changes around SELECT FOR UPDATE in the optimizer (#44015) are also approaching the point where they'll soon need the type definitions to interface with the KV API.