ssmemstorage: improve lock contention on RecordStatement

[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.

[cockroach-teamcity]

This change is 

[maryliag]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w)

---

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 569 at r1 (raw file):

	}

	// Key does

key does....? (also add period to your comments, including above)

---

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 581 at r1 (raw file):

	// doesn't exist yet.
	stats, ok := s.mu.stmts[key]
	if ok {

can you combine the two things? so ok || !createIfNonexistent ?

---

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 630 at r1 (raw file):

	// Avoid taking the full lock
	if !createIfNonexistent {

same comment as above

---

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 651 at r1 (raw file):

	}

	if !createIfNonexistent {

same comment as above

[j82w]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

---

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 569 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

key does....? (also add period to your comments, including above)

Done.

---

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 581 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

can you combine the two things? so ok || !createIfNonexistent ?

Done.

---

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 630 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

same comment as above

Done.

---

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 651 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

same comment as above

Done.

[xinhaoz]

pkg/sql/sqlstats/insights/detector.go line 97 at r2 (raw file):

	id appstatspb.StmtFingerprintID, shouldFlush bool,
) PercentileValues {
	// latencySummary.Query might modify its own state (Stream.flush), so a read-write lock is necessary.

Is this comment no longer true? If so it looks like it needs to be updated.

[xinhaoz]

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 982 at r2 (raw file):

	key sampledPlanKey,
) (lastSampled time.Time, found bool) {
	s.mu.RLock()

Should this be the muPlanCache Rlock?

[j82w]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @xinhaoz)

---

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 982 at r2 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Should this be the muPlanCache Rlock?

Fixed

---

pkg/sql/sqlstats/insights/detector.go line 97 at r2 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Is this comment no longer true? If so it looks like it needs to be updated.

I'll revert it for now. In a future PR the latencySummary should have it's own lock.

[maryliag]

Thank you for looking into this! Really nice changes!

Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @xinhaoz)

[j82w]

bors r+

[craig]

This PR was included in a batch that timed out, it will be automatically retried

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)