sql: set default sample_rate to 0.1#61760
Conversation
|
This change is |
asubiotto
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
|
There was a problem with Another option to going around the issue that I see is introducing The problem is that we first set up the instrumentation and decide that it needs to be finished, but then we might call |
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go, line 2274 at r2 (raw file):
p.stmt = Statement{} // Note that we deliberately do not reset p.instrumentation since we need to // have more fine-grained control over resetting that field.
This is surprising.. What field exactly was problematic? I'm worried we may hang on to various things here (e.g. traceMetadata, explainPlan).
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go, line 2274 at r2 (raw file):
Previously, RaduBerinde wrote…
This is surprising.. What field exactly was problematic? I'm worried we may hang on to various things here (e.g.
traceMetadata,explainPlan).
instrumentation.sp - now in Finish we have an assumption that sp is non-nil. This is always the case after Setup is called and returns needFinish == true, yet because of the resetting behavior in resetPlanner we would overwrite sp to nil. This was introduced in #61532 as a perf optimization.
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go, line 2274 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
instrumentation.sp- now inFinishwe have an assumption thatspis non-nil. This is always the case afterSetupis called and returnsneedFinish == true, yet because of the resetting behavior inresetPlannerwe would overwritesptonil. This was introduced in #61532 as a perf optimization.
Also because of the current resetting behavior tracing of PREPARE is effectively a noop, and I'm not sure it was the intention.
|
Updated the PR based on the conversation with Radu. |
asubiotto
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor_prepare.go, line 204 at r3 (raw file):
ex.statsCollector.reset(&ex.server.sqlStats, ex.appStats, &ex.phaseTimes) p := &ex.planner if origin != PreparedStatementOriginSQL {
nit: I think this probably deserves a "why" comment
Previously, when executing a PREPARE command, we would always reset the planner. This is not necessary when the origin of the command is SQL because in that case we properly reset the planner before starting the statement's execution. This change fixes a problem when PREPARE statements are chosen to be sampled for execution statistics (without the fix, we would encounter a nil pointer). Release note: None
Release note (sql change): Default value for `sql.txn_stats.sample_rate` cluster setting has been increased from 0 to 0.1. This means that from now on every statement has 10% probability of being sampled for the purposes of execution statistics. Note that no other criteria for sampling (like the query latencies) are currently being utilized to decide whether to sample a statement or not.
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor_prepare.go, line 204 at r3 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
nit: I think this probably deserves a "why" comment
Good point, done.
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
|
The CI seems to be red because of unrelated flakes in KV, so I'll go ahead and merge this. TFTRs! bors r+ |
|
Build succeeded: |
sql: do not reset planner if PREPARE is executed as a stmt
Previously, when executing a PREPARE command, we would always reset the
planner. This is not necessary when the origin of the command is SQL
because in that case we properly reset the planner before starting the
statement's execution. This change fixes a problem when PREPARE
statements are chosen to be sampled for execution statistics (without
the fix, we would encounter a nil pointer).
Release note: None
sql: set default sample_rate to 0.1
Fixes: #59379.
Release note (sql change): Default value for
sql.txn_stats.sample_ratecluster setting has been increased from 0 to 0.1. This means that from
now on every statement has 10% probability of being sampled for the
purposes of execution statistics. Note that no other criteria for
sampling (like the query latencies) are currently being utilized to
decide whether to sample a statement or not.