-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sql: create system.mvcc_statistics table and update job #111365
sql: create system.mvcc_statistics table and update job #111365
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rhu713 and @zachlite)
-- commits
line 23 at r1:
Please add a release note
pkg/sql/catalog/systemschema/system.go
line 1013 at r1 (raw file):
SystemMVCCStatisticsCacheSchema = ` CREATE TABLE system.mvcc_statistics_cache ( id INT8 NOT NULL DEFAULT unique_rowid(),
For the other stats tables we normally start with time. Why is the random id necessary?
pkg/sql/catalog/systemschema/system.go
line 1016 at r1 (raw file):
database_id INT8 NOT NULL, table_id INT8 NOT NULL, index_id INT8 NOT NULL,
Just confirming it's not possible to have mvcc without an index?
pkg/sql/catalog/systemschema/system.go
line 1017 at r1 (raw file):
table_id INT8 NOT NULL, index_id INT8 NOT NULL, created_at TIMESTAMP NOT NULL DEFAULT now():::TIMESTAMP,
Should this be part of the response from the remote node to get a more accurate time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 11 of 15 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rhu713 and @zachlite)
pkg/sql/catalog/systemschema/system.go
line 1017 at r1 (raw file):
table_id INT8 NOT NULL, index_id INT8 NOT NULL, created_at TIMESTAMP NOT NULL DEFAULT now():::TIMESTAMP,
Curious if it makes any difference to use TIMESTAMPZ (with time zone)?
pkg/sql/sem/catconstants/constants.go
line 101 at r1 (raw file):
SpanStatsTenantBoundaries SystemTableName = "span_stats_tenant_boundaries" RegionalLiveness SystemTableName = "region_liveness" MVCCStatisticsCache SystemTableName = "mvcc_statistics_cache"
Why table name contains _cache
?
Previously, koorosh (Andrii Vorobiov) wrote…
+1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 15 of 15 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @rhu713, and @zachlite)
pkg/sql/mvcc_statistics_update_job.go
line 1 at r1 (raw file):
package sql
nit: add copy rights section
pkg/sql/mvcc_statistics_update_job.go
line 22 at r1 (raw file):
) (jobErr error) { // TODO(zachlite):
is this going to be part of a follow up PR?
pkg/upgrade/upgrades/mvcc_statistics_cache_migration.go
line 1 at r1 (raw file):
package upgrades
nit: add copy rights section
pkg/upgrade/upgrades/mvcc_statistics_cache_migration.go
line 1 at r1 (raw file):
package upgrades
once you remove the cache
name from the table name, make sure to also update this file name to remove the cache
from it
860aaa4
to
952a66e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @j82w, @koorosh, @maryliag, and @rhu713)
Previously, j82w (Jake) wrote…
Please add a release note
I was hesitant to add a release note here, since there's nothing that is intended or expected to be used by end users.
WDYT?
pkg/sql/mvcc_statistics_update_job.go
line 1 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: add copy rights section
Done
pkg/sql/mvcc_statistics_update_job.go
line 22 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
is this going to be part of a follow up PR?
Yes. Since the job should be baked during a cluster upgrade, I needed to set the job up now. But the implementation is not as time sensitive.
pkg/sql/catalog/systemschema/system.go
line 1013 at r1 (raw file):
Previously, j82w (Jake) wrote…
For the other stats tables we normally start with time. Why is the random id necessary?
As far as I can tell, there isn't a way to configure a monotonically increasing ID for an explicitly when configuring system schema. Another idea is that we can leave out a primary key altogether, so there is an implicit rowid
primary key.
Since there will only be 1 writer at a time, I don't think a sequential id is cause for concern, but I also don't think there's harm in a unique_rowid. WDYT?
pkg/sql/catalog/systemschema/system.go
line 1016 at r1 (raw file):
Previously, j82w (Jake) wrote…
Just confirming it's not possible to have mvcc without an index?
Yeah, even for tables created without an explicit primary key, the rowid
becomes the primary index.
create table t (name string);
SELECT index_name, index_id FROM [SHOW RANGES FROM table t WITH INDEXES];
index_name | index_id
-------------+-----------
t_pkey | 1
Also, the escape hatch is that we can just set the value to 0.
pkg/sql/catalog/systemschema/system.go
line 1017 at r1 (raw file):
Previously, j82w (Jake) wrote…
Should this be part of the response from the remote node to get a more accurate time?
Maybe? I don't think it effects our choice of schema here.
pkg/sql/catalog/systemschema/system.go
line 1017 at r1 (raw file):
Previously, koorosh (Andrii Vorobiov) wrote…
Curious if it makes any difference to use TIMESTAMPZ (with time zone)?
Updated. Thanks!
pkg/sql/sem/catconstants/constants.go
line 101 at r1 (raw file):
Previously, j82w (Jake) wrote…
+1
Renamed!
pkg/upgrade/upgrades/mvcc_statistics_cache_migration.go
line 1 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
once you remove the
cache
name from the table name, make sure to also update this file name to remove thecache
from it
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 15 files at r1, 14 of 14 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @j82w, @maryliag, @rhu713, and @zachlite)
pkg/sql/catalog/systemschema/system.go
line 4135 at r2 (raw file):
{Name: "index_id", ID: 4, Type: types.Int}, {Name: "created_at", ID: 5, Type: types.TimestampTZ, DefaultExpr: &nowString},
should be also changed &nowString
to &nowTZString
952a66e
to
fc4c0dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @maryliag, @rhu713, and @zachlite)
pkg/sql/catalog/systemschema/system.go
line 1020 at r3 (raw file):
statistics JSONB NOT NULL, CONSTRAINT mvcc_statistics_pkey PRIMARY KEY (id ASC), INDEX mvcc_statistics_idx_db_table_idx_created_at (database_id ASC, table_id ASC, index_id ASC, created_at ASC),
I think we just need one index like:
INDEX mvcc_statistics_idx_db_table_idx_created_at (created_at ASC, database_id ASC, table_id ASC, index_id ASC),
ef6736b
to
fa1c527
Compare
The system.mvcc_statistics table stores historical mvcc data for a tenant's SQL objects. It's purpose it to serve mvcc data for a SQL object quickly - The span stats API is too slow to use in a hot path. Storing data over time unlocks new use cases like showing a table or index's accumulated garbage over time. The MVCCStatisticsUpdate Job is responsible for managing the contents of the table, decoupled from the read-hotpath. Both the table and job are baked when a cluster bootstraps itself, or upgrades itself from a previous version. Epic: CRDB-25491 Release note: None
fa1c527
to
541f9d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @j82w, @maryliag, @rhu713, and @zachlite)
pkg/sql/catalog/systemschema/system.go
line 4162 at r6 (raw file):
KeyColumnNames: []string{ "created_at_hash", "created_at",
Should created_at_hash
and created_at
keys be specified in column names? Having them here requires to specify them in WHERE
clause to use the index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @j82w, @maryliag, @rhu713, and @zachlite)
pkg/sql/catalog/systemschema/system.go
line 1045 at r6 (raw file):
sqlStmtHashComputeExpr = `mod(fnv32(crdb_internal.datums_to_bytes(aggregated_ts, app_name, fingerprint_id, node_id, plan_hash, transaction_fingerprint_id)), 8:::INT8)` sqlTxnHashComputeExpr = `mod(fnv32(crdb_internal.datums_to_bytes(aggregated_ts, app_name, fingerprint_id, node_id)), 8:::INT8)` timestampHashComputeExpr = `mod(fnv32(crdb_internal.datums_to_bytes(created_at)), 8:::INT8)`
Should it be 16:::INT8
to match the same number of shards as below:
ShardBuckets: 16, // Cluster setting default.
This commit is the combination of two separate work streams, brought together for resolving logic test fallout simultaneously. The first, authored by @koorosh is the creation of system.transaction_exec_insights and system.statement_exec_insights. The second, authored by @zachlite in cockroachdb#111365 is the creation of system.mvcc_statistics and the MVCCStatisticsUpdate job. Regarding persisted insights: Before, this data was kept in memory only and tracked limited number of latest insights. These tables will be used to persist this data periodically. Tables allow to store the same information as in memory insights without aggregation. To control the amount of data stored in tables, there will be follow up PR to run GC job and prune old records. To make tables flexible to changes when some columns might become obsolete, most of the columns defined as nullable. Regarding persisted MVCC Statistics: The system.mvcc_statistics table stores historical mvcc data for a tenant's SQL objects. It's purpose it to serve mvcc data for a SQL object quickly - The span stats API is too slow to use in a hot path. Storing data over time unlocks new use cases like showing a table or index's accumulated garbage over time. The MVCCStatisticsUpdate Job is responsible for managing the contents of the table, decoupled from the read-hotpath. Both the table and job are baked when a cluster bootstraps itself, or upgrades itself from a previous version. This PR supersedes cockroachdb#111365 with the following changes: - Descriptor fixes to the mvcc_statistics table. No logical changes, just housekeeping to make sure that the create table schema and descriptors produce the same table. - Fixes to the job to make sure the job system can wind down. Partially resolves: cockroachdb#104582 Epic: CRDB-25491 Release note: None
Superseded by #104714 |
This commit is the combination of two separate work streams, brought together for resolving logic test fallout simultaneously. The first, authored by @koorosh is the creation of system.transaction_exec_insights and system.statement_exec_insights. The second, authored by @zachlite in cockroachdb#111365 is the creation of system.mvcc_statistics and the MVCCStatisticsUpdate job. Regarding persisted insights: Before, this data was kept in memory only and tracked limited number of latest insights. These tables will be used to persist this data periodically. Tables allow to store the same information as in memory insights without aggregation. To control the amount of data stored in tables, there will be follow up PR to run GC job and prune old records. To make tables flexible to changes when some columns might become obsolete, most of the columns defined as nullable. Regarding persisted MVCC Statistics: The system.mvcc_statistics table stores historical mvcc data for a tenant's SQL objects. It's purpose it to serve mvcc data for a SQL object quickly - The span stats API is too slow to use in a hot path. Storing data over time unlocks new use cases like showing a table or index's accumulated garbage over time. The MVCCStatisticsUpdate Job is responsible for managing the contents of the table, decoupled from the read-hotpath. Both the table and job are baked when a cluster bootstraps itself, or upgrades itself from a previous version. This PR supersedes cockroachdb#111365 with the following changes: - Descriptor fixes to the mvcc_statistics table. No logical changes, just housekeeping to make sure that the create table schema and descriptors produce the same table. - Fixes to the job to make sure the job system can wind down. Partially resolves: cockroachdb#104582 Epic: CRDB-25491 Release note: None
104714: sql: create new system observability tables and update job r=koorosh a=koorosh This commit is the combination of two separate work streams, brought together for resolving logic test fallout simultaneously. The first, authored by `@koorosh` is the creation of system.transaction_exec_insights and system.statement_exec_insights. The second, authored by `@zachlite` in #111365 is the creation of system.mvcc_statistics and the MVCCStatisticsUpdate job. Regarding persisted insights: Before, this data was kept in memory only and tracked limited number of latest insights. These tables will be used to persist this data periodically. Tables allow to store the same information as in memory insights without aggregation. To control the amount of data stored in tables, there will be follow up PR to run GC job and prune old records. To make tables flexible to changes when some columns might become obsolete, most of the columns defined as nullable. Regarding persisted MVCC Statistics: The system.mvcc_statistics table stores historical mvcc data for a tenant's SQL objects. It's purpose it to serve mvcc data for a SQL object quickly - The span stats API is too slow to use in a hot path. Storing data over time unlocks new use cases like showing a table or index's accumulated garbage over time. The MVCCStatisticsUpdate Job is responsible for managing the contents of the table, decoupled from the read-hotpath. Both the table and job are baked when a cluster bootstraps itself, or upgrades itself from a previous version. This PR supersedes #111365 with the following changes: - Descriptor fixes to the mvcc_statistics table. No logical changes, just housekeeping to make sure that the create table schema and descriptors produce the same table. - Fixes to the job to make sure the job system can wind down. Partially resolves: #104582 Epic: [CRDB-25491](https://cockroachlabs.atlassian.net/browse/CRDB-25491) Release note: None 111660: sctest: Add a test that runs comparator testing from logictest stmts r=Xiang-Gu a=Xiang-Gu This commit introduced a new test that takes as input a path to a corpus file (of stmts collected from logic tests) and feed them into the comparator testing framework. It also edit the existing nightly such that we now collect the corpus and immediately feed them into the comparator testing framework, without having to store them in the cloud in between. Fixes #108183 Epic [CRDB-30346](https://cockroachlabs.atlassian.net/browse/CRDB-30346) Release note: None Co-authored-by: Zach Lite <zach@cockroachlabs.com> Co-authored-by: Xiang Gu <xiang@cockroachlabs.com>
The system.mvcc_statistics table stores historical mvcc data for a tenant's SQL objects. It's purpose it to serve mvcc data for a SQL object quickly - The span stats API is too slow to use in a hot path. Storing data over time unlocks new use cases like showing a table or index's accumulated garbage over time.
The MVCCStatisticsUpdate Job is responsible for managing the contents of the table, decoupled from the read-hotpath.
Both the table and job are baked when a cluster bootstraps itself, or upgrades itself from a previous version.
Ignoring test fallout for now, so that it can be made available to reviewers
Epic: CRDB-25491
Release note: None