Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kv: add lock durability to Get/Scan/RevScan requests #109672

Closed
arulajmani opened this issue Aug 29, 2023 · 0 comments · Fixed by #111349
Closed

kv: add lock durability to Get/Scan/RevScan requests #109672

arulajmani opened this issue Aug 29, 2023 · 0 comments · Fixed by #111349
Assignees
Labels
A-kv-transactions Relating to MVCC and the transactional model. A-read-committed Related to the introduction of Read Committed C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team

Comments

@arulajmani
Copy link
Collaborator

arulajmani commented Aug 29, 2023

Describe the problem

As part of #100193, locking Get/Scan/RevScan will carry lock durability information to indicate whether any acquired locks need to be best-effort (read: in-memory) or they need to be guaranteed (read: replicated).

Jira issue: CRDB-31044

Epic CRDB-26544

@arulajmani arulajmani added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-kv-transactions Relating to MVCC and the transactional model. T-kv KV Team A-read-committed Related to the introduction of Read Committed labels Aug 29, 2023
@arulajmani arulajmani self-assigned this Aug 29, 2023
@nvanbenschoten nvanbenschoten changed the title kvpb: add lock durability to Get/Scan/RevScan requests kv: add lock durability to Get/Scan/RevScan requests Aug 29, 2023
arulajmani added a commit to arulajmani/cockroach that referenced this issue Sep 7, 2023
This patch adds lock durability information to Get, Scan, and
ReverseScan requests. This field is only ever meaningful in conjunction
with a locking strength that's not lock.None.

By default, all locking requests ask for best-effort locks. This
preserves the mixed version story between 23.1 <-> 23.2 nodes. However,
transactions that need them for correctness (read: read committed), will
now have the option to ask for guranteed durability locks. These will
then correspond to replicated locks. Note that the field here is named
in terms of durability guarantees, and not the specific implementation
detail we'll use to provide this -- this is intentional. It allows us to
offer a different kind of durable locks in the future, for example
schemes that selectively replicate locks because they have buy-in from
the both the kvserver and kvclient, in conjunction with some scheme to
verify locks at commit time.

Informs cockroachdb#109672

Release note: None
arulajmani added a commit to arulajmani/cockroach that referenced this issue Sep 8, 2023
This patch adds lock durability information to Get, Scan, and
ReverseScan requests. This field is only ever meaningful in conjunction
with a locking strength that's not lock.None.

By default, all locking requests ask for best-effort locks. This
preserves the mixed version story between 23.1 <-> 23.2 nodes. However,
transactions that need them for correctness (read: read committed), will
now have the option to ask for guranteed durability locks. These will
then correspond to replicated locks. Note that the field here is named
in terms of durability guarantees, and not the specific implementation
detail we'll use to provide this -- this is intentional. It allows us to
offer a different kind of durable locks in the future, for example
schemes that selectively replicate locks because they have buy-in from
the both the kvserver and kvclient, in conjunction with some scheme to
verify locks at commit time.

Informs cockroachdb#109672

Release note: None
arulajmani added a commit to arulajmani/cockroach that referenced this issue Sep 11, 2023
This patch adds lock durability information to Get, Scan, and
ReverseScan requests. This field is only ever meaningful in conjunction
with a locking strength that's not lock.None. It allows SQL to indicate
the durability with which locks, if acquired, should be held --
replicated or unreplicated.

To preserve the mixed-version story between 23.1 <-> 23.2 nodes, where
Get/Scan/ReverseScan requests can only acquire unreplicated locks, we
need switch the ordering in the lock.Durability protobuf. This is safe
for us to do -- the field is only sent over the wire when querying
`crdb_internal.cluster_locks` for observability. The only other place
it's used in, `roachpb.LockAcquisition`, is never sent over the wire.

Informs cockroachdb#109672

Release note: None
arulajmani added a commit to arulajmani/cockroach that referenced this issue Sep 12, 2023
This patch adds lock durability information to Get, Scan, and
ReverseScan requests. This field is only ever meaningful in conjunction
with a locking strength that's not lock.None. It allows SQL to indicate
the durability with which locks, if acquired, should be held --
replicated or unreplicated.

To preserve the mixed-version story between 23.1 <-> 23.2 nodes, where
Get/Scan/ReverseScan requests can only acquire unreplicated locks, we
need switch the ordering in the lock.Durability protobuf. This is safe
for us to do -- the field is only sent over the wire when querying
`crdb_internal.cluster_locks` for observability. The only other place
it's used in, `roachpb.LockAcquisition`, is never sent over the wire.

Informs cockroachdb#109672

Release note: None
arulajmani added a commit to arulajmani/cockroach that referenced this issue Sep 12, 2023
This patch adds lock durability information to Get, Scan, and
ReverseScan requests. This field is only ever meaningful in conjunction
with a locking strength that's not lock.None. It allows SQL to indicate
the durability with which locks, if acquired, should be held --
replicated or unreplicated.

To preserve the mixed-version story between 23.1 <-> 23.2 nodes, where
Get/Scan/ReverseScan requests can only acquire unreplicated locks, we
need switch the ordering in the lock.Durability protobuf. This is safe
for us to do -- the field is only sent over the wire when querying
`crdb_internal.cluster_locks` for observability. The only other place
it's used in, `roachpb.LockAcquisition`, is never sent over the wire.

Informs cockroachdb#109672

Release note: None
craig bot pushed a commit that referenced this issue Sep 12, 2023
110102: sql: do not generate index recs when they are not shown r=mgartner a=mgartner

While diagnosing a bug with index recommendations, I noticed that index
recommendations were being generated for `EXPLAIN` modes that never show
the recommendations: `OPT`, `DDL`, and `VEC`. This commit eliminates
that unnecessary computation.

Epic: None

Release note: None


110201: roachpb: add lock durability info to Get/Scan/ReverseScan requests r=nvanbenschoten a=arulajmani

This patch adds lock durability information to Get, Scan, and ReverseScan requests. This field is only ever meaningful in conjunction with a locking strength that's not lock.None.

By default, all locking requests ask for best-effort locks. This preserves the mixed version story between 23.1 <-> 23.2 nodes. However, transactions that need them for correctness (read: read committed), will now have the option to ask for guranteed durability locks. These will then correspond to replicated locks. Note that the field here is named in terms of durability guarantees, and not the specific implementation detail we'll use to provide this -- this is intentional. It allows us to offer a different kind of durable locks in the future, for example schemes that selectively replicate locks because they have buy-in from the both the kvserver and kvclient, in conjunction with some scheme to verify locks at commit time.

Informs #109672

Release note: None

110238: skip: add Unimplemented skip reason r=knz a=stevendanna

Some test cases are skipped because we intend to work on a feature in the future and have written the test to show the desired end-state once written.

Having a special method for this case allows us to semantically distinguish these kinds of skips from skips that are the results of bugs or unknown issues.

Epic: none

Release note: None

110389: concurrency: correctly defer to higher lock strength during dup access r=nvanbenschoten a=arulajmani

Burns down a TODO which only did so for lock.None lock strength. This was left over from a time when the lock table only expected lock.None and lock.Exclusive lock strengths.

When a key is being accessed with more than one lock strengths in a batch, we want to defer conflict resolution to the higher lock strength, as the lower lock strength access has sufficient protection from it. This means that if a lock table scan is resumed somewhere in the middle, and we've already checked for conflicts at the key before, we can simply ignore it and move on. Note that it doesn't matter if there is a conflict at the key -- we won't proceed to evaluation before coming back to this key, while holding latches and restarting our scan from the begining, so it'll be caught then.

Epic: none

Release note: None

110391: ui: use MAX downsampler for sql connection rate r=sean- a=dhartunian

Epic: None

Release note (ui change): The "SQL Connection Rate" metric on the SQL Dashboard is downsampled using the MAX function instead of SUM. This improves situations where zooming out would cause the connection rate to increase for downsampled data.

110429: ui: fix generated `DROP INDEX` statement on table details page r=zachlite a=zachlite

This commit does two things:
1. Fixes the generated `DROP INDEX` statement by considering already quoted fully-qualified names.

2. Adds unit tests for the `QuoteIdentifier` utility to clarify its intended usage and limitations.

Epic: none
Release note (bug fix): Fixed a UI issue where the DROP_UNUSED index recommendations produced by the
table details page produced an invalid `DROP INDEX` statement.

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>
Co-authored-by: Steven Danna <[email protected]>
Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: Zach Lite <[email protected]>
arulajmani added a commit to arulajmani/cockroach that referenced this issue Sep 27, 2023
Now that both the storage layer and the concurrency manager have been
taught about {shared, exclusive} replicated locks, we can hook things
up end to end. This patch does so by:

- Checking for conflicts with replicated locks during lock acquisition
of an unreplicated lock.
- Checking for conflicts and acquiring a replicated lock during lock
acquisition of a replicated lock.

Closes cockroachdb#109672
Informs cockroachdb#100193

Release note: None
arulajmani added a commit to arulajmani/cockroach that referenced this issue Sep 27, 2023
Now that both the storage layer and the concurrency manager have been
taught about {shared, exclusive} replicated locks, we can hook things
up end to end. This patch does so by:

- Checking for conflicts with replicated locks during lock acquisition
of an unreplicated lock.
- Checking for conflicts and acquiring a replicated lock during lock
acquisition of a replicated lock.

Closes cockroachdb#109672
Informs cockroachdb#100193

Release note: None
craig bot pushed a commit that referenced this issue Sep 27, 2023
111347: sql: fix an exported gist out of bounds error r=cucaroach a=cucaroach

Fixes: #111346
Release note: None
Epic: None


111349: kvserver: hook up {shared, exclusive} replicated locks end to end r=nvanbenschoten a=arulajmani

Now that both the storage layer and the concurrency manager have been taught about {shared, exclusive} replicated locks, we can hook things up end to end. This patch does so by:

- Checking for conflicts with replicated locks during lock acquisition of an unreplicated lock.
- Checking for conflicts and acquiring a replicated lock during lock acquisition of a replicated lock.

Closes #109672
Informs #100193

Release note: None

Co-authored-by: Tommy Reilly <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>
@craig craig bot closed this as completed in 2487571 Sep 28, 2023
THardy98 pushed a commit to THardy98/cockroach that referenced this issue Oct 6, 2023
Now that both the storage layer and the concurrency manager have been
taught about {shared, exclusive} replicated locks, we can hook things
up end to end. This patch does so by:

- Checking for conflicts with replicated locks during lock acquisition
of an unreplicated lock.
- Checking for conflicts and acquiring a replicated lock during lock
acquisition of a replicated lock.

Closes cockroachdb#109672
Informs cockroachdb#100193

Release note: None
@github-project-automation github-project-automation bot moved this to Closed in KV Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-transactions Relating to MVCC and the transactional model. A-read-committed Related to the introduction of Read Committed C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant