sql: create system tables for SQL stats #65374
Conversation
|
This change is |
Azhng
changed the title
Azhng
force-pushed
the
stats-sys-table
branch
2 times, most recently
from
b9638ca to
285f2e5
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @Azhng)
pkg/sql/catalog/systemschema/system.go, line 370 at r1 (raw file):
SQLStatementStatsTableSchema = ` CREATE TABLE system.sql_statement_stats (
nit: I think those two tables should use plural, otherwise looks like is several stats for one statement/transaction. So sql_statements_stats and sql_transactions_stats
Also update in all references, e.g. SQLStatementsStatsTableSchema
pkg/sql/catalog/systemschema/system.go, line 386 at r1 (raw file):
PRIMARY KEY (aggregated_ts, fingerprint_id, plan_hash, app_name, node_id) USING HASH WITH BUCKET_COUNT = 8,
You mentioned on the RFC that the size of bucket as 8 was arbitrary and that would require some benchmark testing to choose the value. Was that done? If not, should we add a comment here explaining and that this value could be replaced?
pkg/sql/catalog/systemschema/system.go, line 1852 at r1 (raw file):
}) // SQLStatementStatsTable is the descriptor for the SQL statement stats table. // It stores statistics for statistics for statement fingerprints.
nit: It stores statistics for statements fingerprints.
pkg/sql/catalog/systemschema/system.go, line 1952 at r1 (raw file):
// SQLTransactionStatsTable is the descriptor for the SQL transaction stats // table. It stores statistics for statistics for transaction fingerprints.
nit: It stores statistics for transactions fingerprints.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/systemschema/system.go, line 370 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: I think those two tables should use plural, otherwise looks like is several stats for one statement/transaction. So
sql_statements_statsandsql_transactions_stats
Also update in all references, e.g.SQLStatementsStatsTableSchema
I think currently our system view is named crdb_internal.node_statement_statistics. So it's slightly breaking the convention here if we named it plural. @kevin-v-ngo What do you think?
pkg/sql/catalog/systemschema/system.go, line 386 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
You mentioned on the RFC that the size of bucket as 8 was arbitrary and that would require some benchmark testing to choose the value. Was that done? If not, should we add a comment here explaining and that this value could be replaced?
Done.
nihalpednekar
requested review from
pbardea
and removed request for
nihalpednekar
|
(Bulk IO changes look like mainly plumbing which LGTM, so going to untag myself) |
There was a problem hiding this comment.
Thank you. Who should I tag for changes related to system table schema?
Reviewable status:
@rytaft might be a good reviewer to add, as a contributor to the file and |
There was a problem hiding this comment.
Reviewed 8 of 11 files at r1, 29 of 29 files at r2.
Reviewable status:
pkg/sql/catalog/bootstrap/metadata.go, line 352 at r2 (raw file):
// Tables introduced in 21.2. target.AddDescriptor(keys.SystemDatabaseID, systemschema.SQLStatementStatsTable)
nit: add a blank line above to be consistent with previous versions
pkg/sql/catalog/systemschema/system.go, line 456 at r2 (raw file):
singleID1 = []descpb.ColumnID{1} sqlStmtHashComputeExpr = "mod(fnv32(COALESCE(CAST(aggregated_ts AS STRING), '':::STRING)) + (fnv32(COALESCE(CAST(app_name AS STRING), '':::STRING)) + (fnv32(COALESCE(CAST(fingerprint_id AS STRING), '':::STRING)) + (fnv32(COALESCE(CAST(node_id AS STRING), '':::STRING)) + fnv32(COALESCE(CAST(plan_hash AS STRING), '':::STRING))))), 8:::INT8)" sqlTxnHashComputeExpr = "mod(fnv32(COALESCE(CAST(aggregated_ts AS STRING), '':::STRING)) + (fnv32(COALESCE(CAST(app_name AS STRING), '':::STRING)) + (fnv32(COALESCE(CAST(fingerprint_id AS STRING), '':::STRING)) + fnv32(COALESCE(CAST(node_id AS STRING), '':::STRING)))), 8:::INT8)"
where did these expressions come from? A comment would help
pkg/sql/tests/system_table_test.go, line 172 at r2 (raw file):
LocalOnlySessionData: sessiondata.LocalOnlySessionData{ EnableUniqueWithoutIndexConstraints: true, HashShardedIndexesEnabled: true,
instead of adding the sessionDataOverride, is there a reason you can't just update the session data in the test file and set this to true? Does it break anything?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/systemschema/system.go, line 456 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
where did these expressions come from? A comment would help
Done.
pkg/sql/tests/system_table_test.go, line 172 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
instead of adding the sessionDataOverride, is there a reason you can't just update the session data in the test file and set this to true? Does it break anything?
Done.
There was a problem hiding this comment.
Table names have been renamed to "system.statement_statistics" and "system.transaction_statistics" per feedback on the RFC.
Reviewable status:
Azhng
force-pushed
the
stats-sys-table
branch
2 times, most recently
from
3d53ca3 to
88026aa
Compare
There was a problem hiding this comment.
Reviewed 27 of 27 files at r3.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/systemschema/system.go, line 422 at r4 (raw file):
PRIMARY KEY (aggregated_ts, fingerprint_id, app_name, node_id) USING HASH WITH BUCKET_COUNT = 8,
I wonder if we're going to be horribly sad about this soon. @mgartner, IIUC, is going to look at switching hash sharded indexes to no longer be physically stored columns. It's a bummer that we need to create this extra hash column and makes the implementation somewhat messy. I worry we'll carry this vestige of the original implementation with us forever if we choose to use it now.
What was the back-of-the-envelope for write throughput of a cluster that convinced us that we needed to hash this index?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/systemschema/system.go, line 422 at r4 (raw file):
Previously, ajwerner wrote…
I wonder if we're going to be horribly sad about this soon. @mgartner, IIUC, is going to look at switching hash sharded indexes to no longer be physically stored columns. It's a bummer that we need to create this extra hash column and makes the implementation somewhat messy. I worry we'll carry this vestige of the original implementation with us forever if we choose to use it now.
What was the back-of-the-envelope for write throughput of a cluster that convinced us that we needed to hash this index?
Is the switch to no longer physically storing hash column going to happen this release? IIUC, we can always change the system table schema without worrying about migrations before the 21.2 happens.
I think the back-of-the-envelope for write throughput based on the data we have in snowflake for CC, the worst case would be 14 GB/hr for a 32 node cluster. Though it is one of the extreme cases. Most of the other clusters would generate tens to hundreds of MB of writes.
|
pkg/sql/catalog/systemschema/system.go, line 422 at r4 (raw file):
We're exploring simplifying the hash-sharded index implementation this release by using virtual computed columns. But there are still some unanswered questions, like, does this even make sense given that the hash-sharded column has to be park of the PK. But I don't expect the syntax for creating a hash-sharded index to change. What problems would arise if the underlying hash-sharded implementation changes if there's already this existing system table in a cluster? |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/systemschema/system.go, line 422 at r4 (raw file):
But there are still some unanswered questions, like, does this even make sense given that the hash-sharded column has to be park of the PK.
In this case, you mean? Generally, the hash-sharded column does not need to be part of the primary key. You are right that if we keep it in the primary key, then it's a total wash.
I think the back-of-the-envelope for write throughput based on the data we have in snowflake for CC, the worst case would be 14 GB/hr for a 32 node cluster. Though it is one of the extreme cases.
That is a lot.
pkg/sql/catalog/systemschema/system.go, line 423 at r4 (raw file):
PRIMARY KEY (aggregated_ts, fingerprint_id, app_name, node_id) USING HASH WITH BUCKET_COUNT = 8, INDEX "fingerprint_stats_idx" (fingerprint_id, aggregated_ts, app_name, node_id),
I'm somewhat suspect of this choice of primary and secondary index. It'd be nice to have some data generation and then some benchmarking. Changing this schema may be somewhat painful after it merges, at least to some extent. I worry that, in the long term, we'll wish that the hash-sharded index had small values and that the primary index were more scattered. Did you consider using fingerprint_stats_idx as the primary index and the current primary index as a secondary index? I think that'll allow for fast loading of a page for a given fingerprint and reduce the size of data that needs to be replicated to the hash sharded index by several orders of magnitude.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/systemschema/system.go, line 422 at r4 (raw file):
You are right that if we keep it in the primary key, then it's a total wash.
I'm not quite following this. Can you elaborate more?
That is a lot.
The write throughput scales based on unique number of fingerprints (~1 KiB per fingerprint) and number of nodes in the cluster. During the RFC discussion, knz pointed out that the performance sensitive clusters tend to tune their workload to have small number of unique number of fingerprints. This means in these clusters, we will be seeing a lot less unique fingerprints.
pkg/sql/catalog/systemschema/system.go, line 423 at r4 (raw file):
Did you consider using fingerprint_stats_idx as the primary index and the current primary index as a secondary index?
We did consider this as the original design. However, this table also will power the loading for the statement page, which displays aggregated execution statistics for all statement fingerprints for a given time interval. Since we considered (grep for 'statement page') loading statement page to be a more important use case compared to loading statistics for a single fingerprint, we chose the design of using hash sharded PK.
Changing this schema may be somewhat painful after it merges, at least to some extent
Even if we change prior to the 21.2 release ?
|
pkg/sql/catalog/systemschema/system.go, line 422 at r4 (raw file):
For a hash-sharded primary index, the hash-sharded column is the first column in the PK, no? |
There was a problem hiding this comment.
Can I make a procedural recommendation here that may derail this stuff a tad? I just went through the RFC a bit and I think this project could really benefit from some go interfaces. I'm increasingly on board with this layout etc but think everything will fall into place better with some interface structure that we put in place first rather that last. I don't want to get too in your way here, I know this is way more than you bargained for.
I think we're in a bit of a mess right now with the in-memory implementation and how we're just injecting the entire *sql.Server into the status APIs and calling arbitrary methods.
A pattern that has been working well from my perspective lately is a set of low-dependency interfaces sitting in a package and some implementations sitting in separate subpackages. This way we can trivially do things like compose the retrieval and combination of persisted stats with in-memory stats without changing any clients. We can also test things about the implementation via the interface.
How do you feel about something like a stmtstats package which has interfaces like Reader and Writer that provide access to the various layers? Then we can compose some of these implementations, simplify clients, and dramatically simplify testing of different layers.
Maybe you've got a code structure plan in mind. I'd love to reach some alignment before we just glom more arbitrary functionality onto gigantic objects like *sql.Server.
Reviewable status:
pkg/sql/catalog/systemschema/system.go, line 422 at r4 (raw file):
In this case, you mean? Generally, the hash-sharded column does not need to be part of the primary key. You are right that if we keep it in the primary key, then it's a total wash.
For a hash-sharded primary index, the hash-sharded column is the first column in the PK, no?
Correct
pkg/sql/catalog/systemschema/system.go, line 423 at r4 (raw file):
Changing this schema may be somewhat painful after it merges, at least to some extent
Even if we change prior to the 21.2 release ?
Not too painful, just non-zero.
There was a problem hiding this comment.
👍 on Andrew's suggestion.
Reviewed 2 of 27 files at r3, 7 of 7 files at r4.
Reviewable status:
There was a problem hiding this comment.
Discussed with @ajwerner offline and decided to move forward with the refactoring. Also filed a tracking issue
Reviewable status:
This PR creates two new system tables: system.statement_stats and system.transaction_stats per RFC cockroachdb#63752. This is the initial step that addresses cockroachdb#64743. Release note: None
There was a problem hiding this comment.
Reviewed 3 of 31 files at r5.
Reviewable status:
|
TFTR! bors r+ |
|
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
|
GitHub status checks took too long to complete, so bors is giving up. You can adjust bors configuration to have it wait longer if you like. |
|
bors r+ |
|
Build succeeded: |
This PR creates two new system tables: system.sql_statement_stats
and system.sql_transaction_stats per RFC #63752.
This is the initial step that addresses #64743.
Release note: None