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.

Sign up for GitHub

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

Jump to bottom

kv: requests waiting on locks should listen to local state transitions when pushing #111596

Closed
nvanbenschoten opened this issue Oct 2, 2023 · 0 comments · Fixed by #112768
Closed

kv: requests waiting on locks should listen to local state transitions when pushing #111596

nvanbenschoten opened this issue Oct 2, 2023 · 0 comments · Fixed by #112768
Labels
A-kv-transactions Relating to MVCC and the transactional model. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team

Comments

@nvanbenschoten
Copy link
Member

nvanbenschoten commented Oct 2, 2023 •
edited by cockroach-jira-scripts
Loading

The lockTableWaiter draws a distinction between requests waiting on locks and requests waiting on other requests. In the latter case, it pushes the other request's transaction while continuing to listen to local state transitions on the lock wait-queue:

cockroach/pkg/kv/kvserver/concurrency/lock_table_waiter.go

Lines 379 to 382 in 0be8311

pushCtx, pushCancel := context.WithCancel(ctx)
defer pushCancel()
go watchForNotifications(pushCtx, pushCancel, newStateC)
err := w.pushRequestTxn(pushCtx, req, timerWaitingState)

However, in the former case, it pushes the lock's transaction without continuing to listen to local state transitions:

cockroach/pkg/kv/kvserver/concurrency/lock_table_waiter.go

Line 370 in 0be8311

return w.pushLockTxn(ctx, req, timerWaitingState)

The reasoning for this is spelled out in this comment:

cockroach/pkg/kv/kvserver/concurrency/lock_table_waiter.go

Lines 354 to 359 in 0be8311

// If the request is conflicting with a held lock then it pushes its
// holder synchronously - there is no way it will be able to proceed
// until the lock's transaction undergoes a state transition (either
// completing or being pushed) and then updates the lock's state
// through intent resolution. The request has a dependency on the
// entire conflicting transaction.

The comment is mistaken. There are cases where the request can proceed without the lock holder's entire transaction being finalized. Namely, the lock may be rolled back even if the other transaction does not immediately complete. In such cases, the lockTableWaiter will be informed of the rollback, but it won't be listening on the channel, so it will continue to block on the push. If the timerWaitingState.held case also used watchForNotifications, it would properly handle this case.

Jira issue: CRDB-31983
@nvanbenschoten nvanbenschoten 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. labels Oct 2, 2023
@blathers-crl blathers-crl bot added the T-kv KV Team label Oct 2, 2023
@nvanbenschoten nvanbenschoten mentioned this issue Oct 2, 2023
Merged
@arulajmani arulajmani mentioned this issue Oct 20, 2023
Merged
arulajmani added a commit to arulajmani/cockroach that referenced this issue Oct 20, 2023
@arulajmani
concurrency: always list for lock state transitions when pushing
eb33878 
Prior to this patch, the lockTableWaiter would only listen for lock
state transitions if it was pushing a transaction while waiting for
an unheld lock. The reasoning was that if the lock was held, the pusher
would not be able to proceed until the push returned. This isn't quite
true -- there's a few cases where the request may no longer conflict
with what's being tracked in the lock table:

- The lock may have been rolled back because of savepoints.
- The lock may have been forgotten by the lock table (replicated locks
are forgotten when they're updated).

This patch changes the lockTableWaiter to also listen for state
transitions when pushing the a held lock's transaction. Cases where the
pusher no longer conflicts with the lock state are detected and the push
is cancelled.

Conveniently, the updates to `resolve_pushed_intents` show the effect
of making this change.

Fixes cockroachdb#111596

Release note: None
@arulajmani arulajmani mentioned this issue Oct 20, 2023
Merged
arulajmani added a commit to arulajmani/cockroach that referenced this issue Oct 30, 2023
@arulajmani
concurrency: always listen for lock state transitions when pushing
9629393 
Prior to this patch, the lockTableWaiter would only listen for lock
state transitions if it was pushing a transaction while waiting for
an unheld lock. The reasoning was that if the lock was held, the pusher
would not be able to proceed until the push returned. This isn't quite
true -- there's a few cases where the request may no longer conflict
with what's being tracked in the lock table:

- A waiting request may not conflict with the lock holder, but a waiting
request instead. In such cases, it'll push the lock holder even though
it is compatible with it. However, if the conflicting waiting request
drops out of the lock's wait queues, the pusher may proceed.
- The lock may have been rolled back because of savepoints.
- The lock may have been forgotten by the lock table (replicated locks
are forgotten when they're updated).

This patch changes the lockTableWaiter to also listen for state
transitions when pushing a held lock's transaction. Cases where the
pusher no longer conflicts with the lock state are detected and the push
is cancelled.

Conveniently, the updates to `resolve_pushed_intents` show the effect
of making this change.

Fixes cockroachdb#111596

Release note: None
craig bot pushed a commit that referenced this issue Oct 31, 2023
@arulajmani
Merge #112768
8b34755 
112768: concurrency: always listen for lock state transitions when pushing r=nvanbenschoten a=arulajmani

Prior to this patch, the lockTableWaiter would only listen for lock state transitions if it was pushing a transaction while waiting for an unheld lock. The reasoning was that if the lock was held, the pusher would not be able to proceed until the push returned. This isn't quite true -- there's a few cases where the request may no longer conflict with what's being tracked in the lock table:

- The lock may have been rolled back because of savepoints.
- The lock may have been forgotten by the lock table (replicated locks are forgotten when they're updated).

This patch changes the lockTableWaiter to also listen for state transitions when pushing the a held lock's transaction. Cases where the pusher no longer conflicts with the lock state are detected and the push is cancelled.

Conveniently, the updates to `resolve_pushed_intents` show the effect of making this change.

Fixes #111596

Release note: None


Co-authored-by: Arul Ajmani <[email protected]>
@craig craig bot closed this as completed in 51a3ce4 Oct 31, 2023
blathers-crl bot pushed a commit that referenced this issue Oct 31, 2023
@arulajmani
concurrency: always listen for lock state transitions when pushing
0f4aff4 
Prior to this patch, the lockTableWaiter would only listen for lock
state transitions if it was pushing a transaction while waiting for
an unheld lock. The reasoning was that if the lock was held, the pusher
would not be able to proceed until the push returned. This isn't quite
true -- there's a few cases where the request may no longer conflict
with what's being tracked in the lock table:

- A waiting request may not conflict with the lock holder, but a waiting
request instead. In such cases, it'll push the lock holder even though
it is compatible with it. However, if the conflicting waiting request
drops out of the lock's wait queues, the pusher may proceed.
- The lock may have been rolled back because of savepoints.
- The lock may have been forgotten by the lock table (replicated locks
are forgotten when they're updated).

This patch changes the lockTableWaiter to also listen for state
transitions when pushing a held lock's transaction. Cases where the
pusher no longer conflicts with the lock state are detected and the push
is cancelled.

Conveniently, the updates to `resolve_pushed_intents` show the effect
of making this change.

Fixes #111596

Release note: None
@blathers-crl blathers-crl bot mentioned this issue Oct 31, 2023
Merged
@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
Assignees
No one assigned
Labels
A-kv-transactions Relating to MVCC and the transactional model. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team
Projects
No open projects
KV
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

concurrency: always listen for lock state transitions when pushing arulajmani/cockroach
1 participant
@nvanbenschoten