sqlstats: move sql stats recording out of execution path #141024

xinhaoz · 2025-02-10T20:41:33Z

Currently, we write sql stats on every execution (stmt and txn). This is not a triival operation as it often involves lock acquisitions and the merging of stmt/txn stats objects. We should move this process to be outside of the execution path.

Related:
Issue noting perf impact of sql stats collection: #135996
Issue noting mutex impact: #140590
WIP POC: #140393

Jira issue: CRDB-47554

blathers-crl · 2025-02-10T20:41:35Z

Hi @xinhaoz, please add branch-* labels to identify which branch(es) this C-bug affects.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

Previously, when attempting to get a stmt or txn entry in the sql stats containers, we pass a flag to the get* functions specifying whether or not to create the entry if it does not exist. This commit cleans up this interface by splitting the function up into a pure get or tryCreate functions. Note that originally we were pursuing a double checked lock approach, however Golang's RWMutex does not scale well with the number of CPUs. Instead this patch just simplifies the code here a bit to make some incoming changes that will eventually move this stats recording step out of the execution path. Once that happens we can investigate further improvment or removal of these mutexes. Epic: none Release note: None Part of: cockroachdb#141024

Previously, when attempting to get a stmt or txn entry in the sql stats containers, we pass a flag to the get* functions specifying whether or not to create the entry if it does not exist. This commit cleans up this interface by splitting the function up into a pure get or tryCreate functions. Note that originally we were pursuing a double checked lock approach, however Golang's RWMutex does not scale well with the number of CPUs. Instead this patch just simplifies the code here a bit to make some incoming changes that will eventually move this stats recording step out of the execution path easier. Once that happens we can investigate further improvment or removal of these mutexes. Epic: none Release note: None Part of: cockroachdb#141024

… container Previously, each stats collector for a sql session would buffer a transaciton's stmt stats in a ssmemstorage.Container struct which has a lot of additional overhead when reading and writing as it manages synchronization logic. For this per-session writing, this writing is synchronous so there's no need to use such a container. We should just buffer the stmts in a simple slice. Note that this is simply a temporary simplified state. Eventually as part of cockroachdb#141024 we will move the writing to the shared container to be async from query execution. Epic: none Fixes: cockroachdb#77376 Release note: None

Previously, when attempting to get a stmt or txn entry in the sql stats containers, we pass a flag to the get* functions specifying whether or not to create the entry if it does not exist. This commit cleans up this interface by splitting the function up into a pure get or tryCreate functions. Note that originally we were pursuing a double checked lock approach, however Golang's RWMutex does not scale well with the number of CPUs. Instead this patch just simplifies the code here a bit to make some incoming changes that will eventually move this stats recording step out of the execution path easier. Once that happens we can investigate further improvment or removal of these mutexes. Epic: none Release note: None Part of: cockroachdb#141024

… container Previously, each stats collector for a sql session would buffer a transaciton's stmt stats in a ssmemstorage.Container struct which has a lot of additional overhead when reading and writing as it manages synchronization logic. For this per-session writing, this writing is synchronous so there's no need to use such a container. We should just buffer the stmts in a simple slice. Note that this is simply a temporary simplified state. Eventually as part of cockroachdb#141024 we will move the writing to the shared container to be async from query execution. Epic: none Fixes: cockroachdb#77376 Release note: None

Previously, when attempting to get a stmt or txn entry in the sql stats containers, we pass a flag to the get* functions specifying whether or not to create the entry if it does not exist. This commit cleans up this interface by splitting the function up into a pure get or tryCreate functions. Note that originally we were pursuing a double checked lock approach, however Golang's RWMutex does not scale well with the number of CPUs. Instead this patch just simplifies the code here a bit to make some incoming changes that will eventually move this stats recording step out of the execution path easier. Once that happens we can investigate further improvment or removal of these mutexes. Epic: none Release note: None Part of: cockroachdb#141024

140604: roachtest: use atomic pointer for logger r=srosenberg,herkolategan a=DarrylWong The test runner swaps out the test logger when running post test artifacts collection and checks. However, in the case of a timeout, the test goroutine may still be running and have access to the test logger. This leads to a race condition where the logger is replaced as it's being used by the test. This change switches the test logger to use an atomic pointer instead. Fixes: none Epic: none Release note: none 141044: physicalplan: minor fixes to pooling of FlowSpecs r=yuzefovich a=yuzefovich **physicalplan: ensure that FlowSpecs are released** We pool `FlowSpec` allocations in `GenerateFlowSpecs`, but previously we would only release them back to the pool on the main query path. This commit fixes that oversight. **physicalplan: fix minor leak with pooling of FlowSpecs** The only thing that we reuse when pooling `FlowSpec` objects is the capacity of `Processors` slice. Previously, we forgot to unset each element of that slice, so we could have hold onto to some processor specs which in turn could have hold onto some large objects (like `roachpb.Span`s in the TableReader) until a particular index of the slice is overwritten. This oversight is now fixed. Found while looking at #140326. Epic: None Release note: None 141045: sqlstats: mutex improvements r=xinhaoz a=xinhaoz Previously, when attempting to get a stmt or txn entry in the sql stats containers, we pass a flag to the get* functions specifying whether or not to create the entry if it does not exist. This commit cleans up this interface by splitting the function up into a pure get or tryCreate functions. Note that originally we were pursuing a double checked lock approach, however Golang's RWMutex does not scale well with the number of CPUs. Instead this patch just simplifies the code here a bit to make some incoming changes that will eventually move this stats recording step out of the execution path easier. Once that happens we can investigate further improvment or removal of these mutexes. Epic: none Release note: None Part of: #141024 ### sqlstats: convert RWMutex to Mutex This commit changes the RWMutex on the sql stats containers to the regular exclusive Mutex struct. The motivation behind this change is that RWMutex scales poorly with CPU count. See benchmarks below. Epic: none Part of: #140590 141146: sqlstats: delete unused iterators r=kyle-a-wong a=kyle-a-wong deletes unused iterators in persistedsqlstats Epic: None Release note: None Co-authored-by: DarrylWong <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Xin Hao Zhang <[email protected]> Co-authored-by: Kyle Wong <[email protected]>

… container Previously, each stats collector for a sql session would buffer a transaciton's stmt stats in a ssmemstorage.Container struct which has a lot of additional overhead when reading and writing as it manages synchronization logic. For this per-session writing, this writing is synchronous so there's no need to use such a container. We should just buffer the stmts in a simple slice. Note that this is simply a temporary simplified state. Eventually as part of cockroachdb#141024 we will move the writing to the shared container to be async from query execution. Epic: none Fixes: cockroachdb#77376 Release note: None

xinhaoz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-observability labels Feb 10, 2025

xinhaoz self-assigned this Feb 10, 2025

xinhaoz mentioned this issue Feb 10, 2025

sqlstats: mutex improvements #141045

Merged

xinhaoz mentioned this issue Feb 11, 2025

sqlstats: buffer statement stats in regular slice instead of sqlstats container #141134

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sqlstats: move sql stats recording out of execution path #141024

sqlstats: move sql stats recording out of execution path #141024

xinhaoz commented Feb 10, 2025 •

edited by cockroach-jira-scripts

Loading

blathers-crl bot commented Feb 10, 2025

sqlstats: move sql stats recording out of execution path #141024

sqlstats: move sql stats recording out of execution path #141024

Comments

xinhaoz commented Feb 10, 2025 • edited by cockroach-jira-scripts Loading

blathers-crl bot commented Feb 10, 2025

xinhaoz commented Feb 10, 2025 •

edited by cockroach-jira-scripts

Loading