sql: fix the sql.trace.stmt.enable_threshold cluster setting #75739
sql: fix the sql.trace.stmt.enable_threshold cluster setting #75739craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
rafiss
left a comment
There was a problem hiding this comment.
looks good overall, just had small comments
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @rafiss)
pkg/sql/conn_executor_exec.go, line 722 at r1 (raw file):
} ctx = origCtx
instead of needing to remember to reset back to origCtx (and add a new {} block), can we replace the whole block with
spanCtx := ctx
if !alreadyRecording && stmtTraceThreshold > 0 {
spanCtx, stmtThresholdSpan = createRootOrChildSpan(ctx, "trace-stmt-threshold", ex.transitionCtx.tracer, tracing.WithRecording(tracing.RecordingVerbose))
}
if err := ex.dispatchToExecutionEngine(spanCtx, p, res); err != nil {
stmtThresholdSpan.Finish()
return nil, nil, err
}
pkg/sql/trace_test.go, line 646 at r1 (raw file):
ctx := context.Background() settings := cluster.MakeTestingClusterSettings() sql.TraceStmtThreshold.Override(ctx, &settings.SV, 1*time.Nanosecond)
if this test were in sql instead of sql_test, could we avoid exporting TraceStmtThreshold? (is it better to keep that private?)
e43fb09 to
095e3a5
Compare
|
Thank you for updating your pull request. Before a member of our team reviews your PR, I have some potential action items for you:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
andreimatei
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor_exec.go, line 722 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
instead of needing to remember to reset back to origCtx (and add a new
{}block), can we replace the whole block withspanCtx := ctx if !alreadyRecording && stmtTraceThreshold > 0 { spanCtx, stmtThresholdSpan = createRootOrChildSpan(ctx, "trace-stmt-threshold", ex.transitionCtx.tracer, tracing.WithRecording(tracing.RecordingVerbose)) } if err := ex.dispatchToExecutionEngine(spanCtx, p, res); err != nil { stmtThresholdSpan.Finish() return nil, nil, err }
good idea, done
pkg/sql/trace_test.go, line 646 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
if this test were in
sqlinstead ofsql_test, could we avoid exportingTraceStmtThreshold? (is it better to keep that private?)
I thought about doing that, but I think it's fine for cluster settings to be exported, to benefit tests that want to set them through this mechanism. Just like anyone can run the set cluster setting command, similarly I think anyone should be able to do what we're doing here.
rafiss
left a comment
There was a problem hiding this comment.
lgtm
Reviewable status:
pkg/sql/conn_executor_exec.go, line 710 at r3 (raw file):
alreadyRecording := ex.transitionCtx.sessionTracing.Enabled() stmtTraceThreshold := TraceStmtThreshold.Get(&ex.planner.execCfg.Settings.SV) var stmtCtx context.Context
could initialize as stmtCtx := ctx to avoid the hanging else
pkg/sql/trace_test.go, line 646 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
I thought about doing that, but I think it's fine for cluster settings to be exported, to benefit tests that want to set them through this mechanism. Just like anyone can run the
set cluster settingcommand, similarly I think anyone should be able to do what we're doing here.
makes sense
This setting had rotted, leading to a span use-after-Finish. This was caught by the recently-unskipped follower reads roachtests (roachtests panic on use-after-finish), accounting for all the issues below. The last entry in these issues refers to the nodes crashing because of the bug fixed here. Some of these issues have been open for a long time for more relevant failures - those have been fixed in cockroachdb#72296. Fixes cockroachdb#75716 Fixes cockroachdb#75715 Fixes cockroachdb#75714 Fixes cockroachdb#71244 Fixes cockroachdb#71126 Fixes cockroachdb#70818 Fixes cockroachdb#70191 Fixes cockroachdb#70011 Fixes cockroachdb#70010 Fixes cockroachdb#69952 Fixes cockroachdb#69951 Release note: None
Release note: None
095e3a5 to
c8069ad
Compare
andreimatei
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor_exec.go, line 710 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
could initialize as
stmtCtx := ctxto avoid the hanging else
in this case, I preferred this more explicit way of writing it. By not assigning an initial value to stmtCtx, the code suggests that something funky. Whenever mucking with contexts, I found it good to highlight it.
andreimatei
left a comment
There was a problem hiding this comment.
bors r+
Reviewable status:
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
This setting had rotted, leading to a span use-after-Finish.
This was caught by the recently-unskipped follower reads roachtests
(roachtests panic on use-after-finish), accounting for all the issues
below. The last entry in these issues refers to the nodes crashing
because of the bug fixed here. Some of these issues have been open for a
long time for more relevant failures - those have been fixed in #72296.
Fixes #75716
Fixes #75715
Fixes #75714
Fixes #71244
Fixes #71126
Fixes #70818
Fixes #70191
Fixes #70011
Fixes #70010
Fixes #69952
Fixes #69951
Release note: None