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: include conflicting request information in latch manager traces/logs #114601

Closed
nvanbenschoten opened this issue Nov 16, 2023 · 3 comments · Fixed by #115511
Closed

kv: include conflicting request information in latch manager traces/logs #114601

nvanbenschoten opened this issue Nov 16, 2023 · 3 comments · Fixed by #115511
Labels
A-kv-observability 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

Comments

@nvanbenschoten
Copy link
Member

nvanbenschoten commented Nov 16, 2023

The latch manager contains trace events and logs for conflicts in Manager.waitForSignal. This observability uses latch.String to present the span and the strength of the conflicting latch. However, it does not present information about the specific requests holding the latch or the requests acquiring the latch.

We should push down *BatchRequests into the spanlatch.Manager and include the output from BatchRequest.SafeFormat in latch.SafeFormat.

Jira issue: CRDB-33589

Epic CRDB-34227

@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. A-kv-observability T-kv KV Team labels Nov 16, 2023
@nvanbenschoten nvanbenschoten added this to Incoming in KV via automation Nov 16, 2023
@nvanbenschoten
Copy link
Member Author

When doing this, we should store the *BatchRequest on the Guard and change latch from storing a *signals to a *Guard. This will avoid bloating the size of the latch struct.

@nvanbenschoten
Copy link
Member Author

nvanbenschoten commented Nov 16, 2023

Will pair nicely with #114475.

@lyang24
Copy link
Contributor

lyang24 commented Dec 1, 2023

Hi Nathan, I thought about two ways of implementing the push down of *BatchRequest, which path do you recommend?

  1. Make SequenceReq that take *BatchRequest as new parameter
  2. Since majority of fields in Request struct at pkg/kv/kvserver/concurrency/concurrency_control.go are derived from batchRequest we can refactor Request struct to below this will require us changing how request is constructed in test.
type Request struct {
	BatchRequests *kvpb.BatchRequest
	MaxLockWaitQueueLength int
	PoisonPolicy poison.Policy
	LatchSpans *spanset.SpanSet
	LockSpans *lockspanset.LockSpanSet
}

lyang24 added a commit to lyang24/cockroach that referenced this issue Dec 3, 2023
This commit add request information in latch conflict logging. This is
achieved by refactor Request struct to include *kvpb.BatchRequests and pass it
down to Guard. Since the Guard struct contains the signal field, we will make
latch to store a pointer to the Guard instead to avoid latch size increase.

Fixes: cockroachdb#114601

Relase Note: None
lyang24 added a commit to lyang24/cockroach that referenced this issue Dec 12, 2023
This commit logs latch waiter and latch holder's request information when latch
conflicts. This is achieved by adding *kvpb.BatchRequests in
concurrency.Request and pass it down to latch Guard. Since the Guard struct
contains the signal field, we will make latch struct store a pointer to the
latch guard to avoid latch size increases.

Fixes: cockroachdb#114601

Relase note: None
lyang24 added a commit to lyang24/cockroach that referenced this issue Dec 21, 2023
This commit logs latch waiter and latch holder's request information when latch
conflicts. This is achieved by adding *kvpb.BatchRequests in
concurrency.Request and pass it down to latch Guard. Since the Guard struct
embedded the signal struct, we will make latch struct store a pointer to the
latch guard instead.

Fixes: cockroachdb#114601

Relase note: None
lyang24 added a commit to lyang24/cockroach that referenced this issue Dec 21, 2023
This commit logs latch waiter and latch holder's request information when latch
conflicts. This is achieved by adding *kvpb.BatchRequests in
concurrency.Request and pass it down to latch Guard. Since the Guard struct
embedded the signal struct, we will store store a pointer to the latch guard
in latch struct instead.

Fixes: cockroachdb#114601

Relase note: None
craig bot pushed a commit that referenced this issue Jan 3, 2024
116696: kv: implement BatchRequest methods on pointer receivers r=nvanbenschoten a=nvanbenschoten

The commit changes the receiver type of the BatchRequest methods to pointer receivers. This allows *BatchRequest to implement the redact.SafeFormatter interface, which is helpful now that (as of e822649) BatchRequest is always passed around by pointer.

Informs #114601.
Release note: None

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
lyang24 added a commit to lyang24/cockroach that referenced this issue Jan 3, 2024
This commit logs latch waiter and latch holder's request information when latch
conflicts. This is achieved by adding redact.SafeFormatter from batch request
into concurrency.Request and pass it down to latch Guard. Since the Guard struct
embedded the signal struct, we will store store a pointer to the latch guard
in latch struct instead.

Fixes: cockroachdb#114601

Relase note: None
lyang24 added a commit to lyang24/cockroach that referenced this issue Jan 4, 2024
This commit logs latch waiter and latch holder's request information when latch
conflicts. This is achieved by adding redact.SafeFormatter from batch request
into concurrency.Request and pass it down to latch Guard. Since the Guard struct
embedded the signal struct, we will store store a pointer to the latch guard
in latch struct instead.

Fixes: cockroachdb#114601

Relase note: None
lyang24 added a commit to lyang24/cockroach that referenced this issue Jan 6, 2024
This commit logs latch waiter and latch holder's request information when latch
conflicts. This is achieved by adding redact.SafeFormatter from batch request
into concurrency.Request and pass it down to latch Guard. Since the Guard struct
embedded the signal struct, we will store store a pointer to the latch guard
in latch struct instead.

Fixes: cockroachdb#114601

Relase note: None
craig bot pushed a commit that referenced this issue Jan 8, 2024
115511: kv: include conflicting request information in latch manager traces/logs r=nvanbenschoten a=lyang24

This commit logs latch waiter and latch holder's request information when latch
conflicts. This is achieved by adding batch request derived redact.SafeFormatter
into concurrency.Request and pass it down to latch Guard. Since the Guard struct
embedded the signal struct, we will store store a pointer to the latch guard
in latch struct instead.

Fixes: #114601

Relase note: None

Co-authored-by: lyang24 <lanqingy@usc.edu>
@craig craig bot closed this as completed in #115511 Jan 8, 2024
KV automation moved this from Incoming to Closed Jan 8, 2024
@exalate-issue-sync exalate-issue-sync bot reopened this Feb 20, 2024
KV automation moved this from Closed to Incoming Feb 20, 2024
KV automation moved this from Incoming to Closed Feb 20, 2024
@exalate-issue-sync exalate-issue-sync bot reopened this Feb 26, 2024
KV automation moved this from Closed to Incoming Feb 26, 2024
KV automation moved this from Incoming to Closed Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-observability 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
KV
Closed
Development

Successfully merging a pull request may close this issue.

2 participants