sql: added indexes to system.statement_statistics, system.transaction_statistics

[ericharmeling]

Fixes #98624.

This commit adds indexes on new computed columns to the system.statement_statistics and system.transaction_statistics tables.

Epic: none

Release note: None

[cockroach-teamcity]

This change is 

[ericharmeling]

Here's some benchmark results: BenchmarkSqlStatsPersistedresults.txt

[maryliag]

Reviewed 17 of 17 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling and @msirek)

---

pkg/sql/catalog/systemschema/system.go line 124 at r1 (raw file):

	indexUsageComputeExpr           = `(statistics->'statistics':::STRING)->'indexes':::STRING`
	executionCountComputeExpr       = `((statistics->'execution_statistics':::STRING)->'cnt':::STRING)::INT8`

do you need to convert this to STRING and then to INT8? can you do INT8 right away? same things for other below

also, this should bt the count from statistics -> statistics -> cnt

---

pkg/sql/catalog/systemschema/system.go line 569 at r1 (raw file):

			plan,
			index_recommendations,
      execution_count,

nit: alignment is off

---

pkg/sql/catalog/systemschema/system.go line 615 at r1 (raw file):

			metadata,
			statistics,
      execution_count,

nit: alignment

---

pkg/upgrade/upgrades/create_computed_indexes_sql_statistics.go line 69 at r1 (raw file):

ALTER TABLE system.statement_statistics
ADD COLUMN IF NOT EXISTS "total_estimated_execution_time" FLOAT 
AS ((statistics->'execution_statistics'->>'cnt')::float * (statistics->'statistics'->'svcLat'->>'mean')::FLOAT) STORED

you should use statistics -> statistics -> cnt, because the count for exec stats is not the same and can have less that was used for the latency on stats.

---

pkg/upgrade/upgrades/create_computed_indexes_sql_statistics.go line 82 at r1 (raw file):

ALTER TABLE system.transaction_statistics
ADD COLUMN IF NOT EXISTS "execution_count" INT8 
AS ((statistics->'execution_statistics'->'cnt')::INT8) STORED

ditto

---

pkg/upgrade/upgrades/create_computed_indexes_sql_statistics.go line 126 at r1 (raw file):

ALTER TABLE system.transaction_statistics
ADD COLUMN IF NOT EXISTS "total_estimated_execution_time" FLOAT 
AS ((statistics->'execution_statistics'->>'cnt')::float * (statistics->'statistics'->'svcLat'->>'mean')::FLOAT) STORED

ditto

---

pkg/sql/sqlstats/persistedsqlstats/bench_test.go line 184 at r1 (raw file):

				// compute expressions
				executionCount := "((statistics->'execution_statistics'->'cnt')::INT8)"

you will need to update all references of count on the test to use the statistics one

[ericharmeling]

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

---

pkg/sql/catalog/systemschema/system.go line 124 at r1 (raw file):

do you need to convert this to STRING and then to INT8? can you do INT8 right away? same things for other below

Originally, I was not explicitly casting anything except the final type (e.g., INT8 here), but TestSystemTableLiterals was failing the compute expressions. The compute expressions here are what that test expects.

also, this should bt the count from statistics -> statistics -> cnt

Done.

---

pkg/sql/catalog/systemschema/system.go line 569 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: alignment is off

Done.

---

pkg/sql/catalog/systemschema/system.go line 615 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: alignment

Done.

---

pkg/upgrade/upgrades/create_computed_indexes_sql_statistics.go line 69 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

you should use statistics -> statistics -> cnt, because the count for exec stats is not the same and can have less that was used for the latency on stats.

Done.

---

pkg/upgrade/upgrades/create_computed_indexes_sql_statistics.go line 82 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

ditto

Done.

---

pkg/upgrade/upgrades/create_computed_indexes_sql_statistics.go line 126 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

ditto

Done.

---

pkg/sql/sqlstats/persistedsqlstats/bench_test.go line 184 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

you will need to update all references of count on the test to use the statistics one

Done.

[maryliag]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @msirek)

[ericharmeling]

Here are the queries that these indexes optimize:

SELECT * FROM ((SELECT
  aggregated_ts,
  fingerprint_id,
  transaction_fingerprint_id,
  app_name,
  execution_count,
  service_latency,
  cpu_sql_nanos,
  contention_time,
  total_estimated_execution_time,
  metadata,
  statistics
FROM system.statement_statistics
WHERE app_name NOT LIKE '$ internal%' AND aggregated_ts > (now() - INTERVAL '1 hour') ORDER BY execution_count DESC LIMIT 500) UNION (SELECT
  aggregated_ts,
  fingerprint_id,
  transaction_fingerprint_id,
  app_name,
  execution_count,
  service_latency,
  cpu_sql_nanos,
  contention_time,
  total_estimated_execution_time,
  metadata,
  statistics
FROM system.statement_statistics
WHERE app_name NOT LIKE '$ internal%' AND aggregated_ts > (now() - INTERVAL '1 hour')  ORDER BY service_latency DESC LIMIT 500) UNION (SELECT
  aggregated_ts,
  fingerprint_id,
  transaction_fingerprint_id,
  app_name,
  execution_count,
  service_latency,
  cpu_sql_nanos,
  contention_time,
  total_estimated_execution_time,
  metadata,
  statistics
FROM system.statement_statistics
WHERE app_name NOT LIKE '$ internal%' AND aggregated_ts > (now() - INTERVAL '1 hour')  ORDER BY cpu_sql_nanos DESC LIMIT 500) UNION (SELECT
  aggregated_ts,
  fingerprint_id,
  transaction_fingerprint_id,
  app_name,
  execution_count,
  service_latency,
  cpu_sql_nanos,
  contention_time,
  total_estimated_execution_time,
  metadata,
  statistics
FROM system.statement_statistics
WHERE app_name NOT LIKE '$ internal%' AND aggregated_ts > (now() - INTERVAL '1 hour')  ORDER BY contention_time DESC LIMIT 500) UNION (SELECT
  aggregated_ts,
  fingerprint_id,
  transaction_fingerprint_id,
  app_name,
  execution_count,
  service_latency,
  cpu_sql_nanos,
  contention_time,
  total_estimated_execution_time,
  metadata,
  statistics
FROM system.statement_statistics
WHERE app_name NOT LIKE '$ internal%' AND aggregated_ts > (now() - INTERVAL '1 hour')  ORDER BY total_estimated_execution_time DESC LIMIT 500))

SELECT * FROM ((SELECT
  aggregated_ts,
  fingerprint_id,
  app_name,
  execution_count,
  service_latency,
  cpu_sql_nanos,
  contention_time,
  total_estimated_execution_time,
  metadata,
  statistics
FROM system.transaction_statistics
WHERE app_name NOT LIKE '$ internal%' AND aggregated_ts > (now() - INTERVAL '1 hour')  ORDER BY execution_count DESC LIMIT 500) UNION (SELECT
  aggregated_ts,
  fingerprint_id,
  app_name,
  execution_count,
  service_latency,
  cpu_sql_nanos,
  contention_time,
  total_estimated_execution_time,
  metadata,
  statistics
FROM system.transaction_statistics
WHERE app_name NOT LIKE '$ internal%' AND aggregated_ts > (now() - INTERVAL '1 hour')  ORDER BY service_latency DESC LIMIT 500) UNION (SELECT
  aggregated_ts,
  fingerprint_id,
  app_name,
  execution_count,
  service_latency,
  cpu_sql_nanos,
  contention_time,
  total_estimated_execution_time,
  metadata,
  statistics
FROM system.transaction_statistics
WHERE app_name NOT LIKE '$ internal%' AND aggregated_ts > (now() - INTERVAL '1 hour')  ORDER BY cpu_sql_nanos DESC LIMIT 500) UNION (SELECT
  aggregated_ts,
  fingerprint_id,
  app_name,
  execution_count,
  service_latency,
  cpu_sql_nanos,
  contention_time,
  total_estimated_execution_time,
  metadata,
  statistics
FROM system.transaction_statistics
WHERE app_name NOT LIKE '$ internal%' AND aggregated_ts > (now() - INTERVAL '1 hour')  ORDER BY contention_time DESC LIMIT 500) UNION (SELECT
  aggregated_ts,
  fingerprint_id,
  app_name,
  execution_count,
  service_latency,
  cpu_sql_nanos,
  contention_time,
  total_estimated_execution_time,
  metadata,
  statistics
FROM system.transaction_statistics
WHERE app_name NOT LIKE '$ internal%' AND aggregated_ts > (now() - INTERVAL '1 hour')  ORDER BY total_estimated_execution_time DESC LIMIT 500))

These queries will be used to populate the tables introduced in #98885.

[rytaft]

If any of these queries are performance sensitive, I suggest adding them to your new sql_statistics_persisted file or another optimizer test.

[ericharmeling]

@rytaft Thanks for looking at this!

If any of these queries are performance sensitive, I suggest adding them to your new sql_statistics_persisted file or another optimizer test.

I tried doing this in a first revision, but the execbuilder tests were failing on non-deterministic output from EXPLAIN (VERBOSE) (the spans are over aggregated_ts values, which change each run). Any advice on how to write an optimizer test for the queries I posted above?

[rytaft]

Just change now() to some arbitrary fixed timestamp. It shouldn't change the plan.

[ericharmeling]

Just change now() to some arbitrary fixed timestamp. It shouldn't change the plan.

🤦 Of course. Thank you!

[j82w]

Reviewed all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @maryliag and @msirek)

[ericharmeling]

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)