sql/execstats: TestTraceAnalyzer data race #75546
Assignees
Labels
branch-master
Failures and bugs on the master branch.
C-test-failure
Broken test (automatically or manually discovered).
O-robot
Originated from a bot.
T-sql-queries
SQL Queries Team
Comments
cockroach-teamcity
added
branch-master
|
sql/execstats.TestTraceAnalyzer failed with artifacts on master @ f918e4bae4c24fc2cad3a65292a0f347c9785232: Help
See also: How To Investigate a Go Test Failure (internal)
|
|
sql/execstats.TestTraceAnalyzer failed with artifacts on master @ c2023effaad7966ace04f4d84170f96dc47ce6ba: Help
See also: How To Investigate a Go Test Failure (internal)
|
|
sql/execstats.TestTraceAnalyzer failed with artifacts on master @ 11da0cfde9f998edd871a497a54fee457cf4a943: Help
See also: How To Investigate a Go Test Failure (internal)
|
|
Comment #1 data race -- Comment #2 detected a similar race: Comment #3 is slightly different: |
rharding6373
changed the title
|
Woops, looking. |
|
Actually this is #74803 (+cc @adityamaru, @arulajmani). |
|
Haven't been able to repro over 10mins running under |
|
Looks like this method is not expecting to get called concurrently with query execution: Lines 122 to 123 in 69ba2a9 This particular test invokes it here, which at the time it was introduced didn't have to worry about query execution in the background. cockroach/pkg/sql/execstats/traceanalyzer_test.go Lines 110 to 111 in 711e228 With the span config reconciliation job however, it's a (valid) background running process that does invoke queries to retrieve protected-ts state (after ##74803). I'm not sure if #75632 is the right fix, I think this particular test is just reaching into inner state in a way that's no longer valid. We could, for this test:
I'm not familiar enough with this test to know what to do. Probably (4) is the easiest. @rharding6373, what do you think? |
|
oh interesting, this makes a lot more sense. Alternatively, could we create a new session-bound internal executor using the factory we have on the execCfg? Though I'm also not super familiar with this test, and if it is relying on using the shared internal executor hanging off the execCfg. fwiw the test passes with the following diff - https://github.com/cockroachdb/cockroach/blob/e1350814dab20e7f0e6e37ffad2820d50ebef0c4/pkg/sql/exec_util.go#L1262-L1262 |
|
Nice, didn't realize it was not a singleton (doh). I'll let someone from the SQL team rubber stamp etc. but it looks right to me. |
|
Looks right to me too. |
|
Thanks, put up a PR with this diff. |
craig bot
pushed a commit
that referenced
this issue
Jan 28, 2022
70159: kvclient: switch error in ctx cancel edge case r=andreimatei a=andreimatei This patch changes the error returned by the DistSender on a cancelled ctx. Depending on the exactly who detects the ctx as cancelled, there are a number of different possibilities - too many to enumerate here. Generally, the client is supposed to get a context.Canceled error, wrapped in different layers. The code path that this patch changes is about the cancellation being detected by sendPartialBatch() without it having been previously detected by sendToReplicas(). This path is unusual (sendToReplicas() generally detects the ctx cancelled). Depending on the exact cancellation timing, it's possible though for sendPartialBatch() to detect it instead. In this case, this patch makes it so that, if sendToReplicas() returned a sendError (indicating that a suitable replica could not be reached and that the higher layer is expected to continue trying other replicas), the error returned to the client is a cancellation error instead of the sendError. Touches #69419 Release note: None 75632: execstats: fix flakey TestTraceAnalyzer r=yuzefovich a=adityamaru Fixes: #75546 Release note: None Co-authored-by: Andrei Matei <[email protected]> Co-authored-by: Aditya Maru <[email protected]>
craig bot
pushed a commit
that referenced
this issue
Jan 28, 2022
75632: execstats: fix flakey TestTraceAnalyzer r=yuzefovich a=adityamaru Fixes: #75546 Release note: None 75662: githooks: avoid accidental branch push to origin r=dt a=dt Branches are expected to be pushed to forks instead. Release note: none. 75666: kvstreamer: fix the tracing of async requests r=yuzefovich a=yuzefovich Previously, we forgot to specify a span option when running asynchronous requests and ended up using the default (which is `FollowsFromSpan` meaning that the span won't get included into the parent). The correct option to use is `ChildSpan` so that the DistSender tracing is included into the Streamer traces which is what this commit does. Release note: None 75669: bazel: bump size of `physicalplan` test r=rail a=rickystewart Release note: None Co-authored-by: Aditya Maru <[email protected]> Co-authored-by: David Taylor <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Ricky Stewart <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
sql/execstats.TestTraceAnalyzer failed with artifacts on master @ d5ed1d9cb363379731082ed9208b81feb39ab13a:
Help
See also: How To Investigate a Go Test Failure (internal)
Parameters in this failure:
This test on roachdash | Improve this report!
The text was updated successfully, but these errors were encountered: