sql: refactor appStats to have a RWMutex instead of a Mutex #55285
Assignees
Labels
C-cleanup
Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior.
Comments
arulajmani
added
the
C-cleanup
|
@arulajmani Are you planning to do this? I was interested in taking a stab unless you already have something |
craig bot
pushed a commit
that referenced
this issue
Jul 19, 2023
106860: ssmemstorage: improve lock contention on RecordStatement r=j82w a=j82w 1. The stats object lock was being held even during the insights event creation and insert which has it's own lock. The insights logic is now moved above the stats lock. 2. Moved latency calculation outside the stats lock. 3. Moved error code calculation outside the stats lock. 4. ss_mem_storage converted to use a RWMutex. Each object in the map has it's own lock. Read lock allows mulitple threads to read from the map at the same time which is the common case. Writes are only done the first time a statement is seen. Fixes: #106789, Fixes: #55285 ``` ./dev bench pkg/sql/sqlstats/sslocal -f BenchmarkRecordStatement --ignore-cache --verbose --test-args='-test.mutexprofile=mutex_pr.out -test.cpuprofile=cpu_pr.out -test.benchtime=30s' --timeout "10m" ``` ``` // Master no changes BenchmarkRecordStatement/RecordStatement:_Parallelism_10-10 64775088 558.8 ns/op sql_stats_test.go:655: -- test log scope end -- PASS ================================================================================ INFO: Elapsed time: 37.529s, Critical Path: 37.26s INFO: 2 processes: 1 internal, 1 darwin-sandbox. INFO: Build completed successfully, 2 total actions //pkg/sql/sqlstats/sslocal:sslocal_test PASSED in 37.2s ``` ``` // Changes after the PR BenchmarkRecordStatement/RecordStatement:_Parallelism_10-10 67960656 523.8 ns/op sql_stats_test.go:655: -- test log scope end -- PASS ================================================================================ INFO: Elapsed time: 37.031s, Critical Path: 36.69s INFO: 2 processes: 1 internal, 1 darwin-sandbox. INFO: Build completed successfully, 2 total actions //pkg/sql/sqlstats/sslocal:sslocal_test PASSED in 36.7s ``` Release note (performance improvement): Reduced lock contention on `ssmemstorage.RecordStatement`. This is useful for workloads that execute the same statement concurrently on the same SQL instance. 106912: telemetryccl: Split `TestTelemetry` into smaller sub-tests r=Santamaura a=Santamaura This change breaks up `TestTelemetry` and the `multiregion` test into four tests due to the test being quite large. The tests are: `multiregion`, `multiregion_db`, `multiregion_table`, and `multiregion_row`. Informs: #103004 Release note: None 107099: sql: fix `CREATE TABLE AS` sourcing `SHOW <show_subcmd> <table>` job failures r=chengxiong-ruan a=ecwall Fixes #106260 Previously `CREATE TABLE AS`/`CREATE MATERIALIZED VIEW AS` sourcing from `SHOW <show_subcmd> <table>` generated a failing schema change job with a `relation "tbl" does not exist error` because the SHOW source table was not fully qualified. This PR fixes this by respecting the `Builder.qualifyDataSourceNamesInAST` flag in `delegate.TryDelegate()` which implements the failing SHOW commands. Release note (bug fix): Fix failing schema change job when CREATE TABLE AS or CREATE MATERAILIZED VIEW AS sources from a SHOW command: 1. CREATE TABLE t AS SELECT * FROM [SHOW CREATE TABLE tbl]; 2. CREATE TABLE t AS SELECT * FROM [SHOW INDEXES FROM tbl]; 3. CREATE TABLE t AS SELECT * FROM [SHOW COLUMNS FROM tbl]; 4. CREATE TABLE t AS SELECT * FROM [SHOW CONSTRAINTS FROM tbl]; 5. CREATE TABLE t AS SELECT * FROM [SHOW PARTITIONS FROM TABLE tbl]; 6. CREATE TABLE t AS SELECT * FROM [SHOW PARTITIONS FROM INDEX tbl@tbl_pkey]; 107170: ui: fix creation index syntax on console r=maryliag a=maryliag With an update to make the table name fully qualified, the index name was also using the fully qualified name, which contains ".", and that causes an invalid syntax since index name can't have periods. Having the qualified name is not important for the index name itself, so this commit fixes by ignoring that part and using just the table name, how it was doing previously. It also fixes an invalid syntax when there were spaces on the name generate. It also add a little more observability into indexes being created with STORING clause, adding that to the suffix of the index creation. Fixes #107168 Before <img width="583" alt="Screenshot 2023-07-19 at 10 23 21 AM" src="https://github.com/cockroachdb/cockroach/assets/1017486/e5792419-31af-4e5f-994e-13ddb716009c"> After https://www.loom.com/share/33da8b46fc9e4f72944c1d0ab943dea0 Release note (bug fix): Index recommendation on the UI no longer uses the full qualified name of a table to create an index name, allowing the creating of indexes directly from the Console to work. 107189: tests: fix stories files r=maryliag a=maryliag Stories files used for testing were missing some fixtures. This commit adds all missing parameters. Epic: none Release note: None 107197: schemachanger: skip flakey test r=rafiss a=DrewKimball Skips `TestConcurrentDeclarativeSchemaChanges`. Informs #106732 Release note: None 107207: bazel: make `fastbuild` the default `-c` again; never strip for `dev` builds r=rail a=rickystewart This is a partial revert of `60e8bcec61269c6648dc40c8094fbf303b8a02aa`. In that commit I made a change to make un-stripped binaries the default by making `dbg` the default `--compilation_mode`. This had unexpected consequences as this actually disables inlining and optimization, thereby breaking some tests, and overall it is surprising that a Go binary built by Bazel would be un-optimized by default since this is the opposite of `go build`'s default behavior. Instead, we make `fastbuild` the default (again), and to solve the problem of binaries being unstripped we set `--strip never` for `dev` builds. (`dev doctor` makes you clarify that your build is a `dev` build on setup so no one can miss this.) Now we have the following behavior for each `compilation_mode`: * `dbg`: disable optimizations and inlining. * `fastbuild` and `opt`: will produce the same binary, except `fastbuild` defaults to stripping in non-`dev` configurations. If you want an un-optimized binary for some reason, it still works to build with `-c dbg`. Epic: CRDB-17171 Release note: None Co-authored-by: j82w <jwilley@cockroachlabs.com> Co-authored-by: Alex Santamaura <alexsantamaura@crlMBP-TGJCQGWGPCMzIx.local> Co-authored-by: Evan Wall <wall@cockroachlabs.com> Co-authored-by: maryliag <marylia@cockroachlabs.com> Co-authored-by: Drew Kimball <drewk@cockroachlabs.com> Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>
srosenberg
pushed a commit
to srosenberg/cockroach
that referenced
this issue
Aug 27, 2023
1. The stats object lock was being held even during the insights event creation and insert which has it's own lock. The insights logic is now moved above the stats lock. 2. Moved latency calculation outside the stats lock. 3. Moved error code calculation outside the stats lock. 4. ss_mem_storage converted to use a RWMutex. Each object in the map has it's own lock. Read lock allows mulitple threads to read from the map at the same time which is the common case. Writes are only done the first time a statement is seen. Fixes: cockroachdb#106789, Fixes: cockroachdb#55285 Release note (performance improvement): Reduced lock contention on `ssmemstorage.RecordStatement`. This is useful for workloads that execute the same statement concurrently on the same SQL instance.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
There are various places in the code where appStats can benefit from acquiring a read lock (such as
cockroach/pkg/sql/app_stats.go
Line 251 in 5965426
Jira issue: CRDB-3680
The text was updated successfully, but these errors were encountered: