sql: cluster setting for persisting gateway node ID #88583
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @j82w and @maryliag)
docs/generated/settings/settings.html line 183 at r1 (raw file):
<tr><td><code>sql.metrics.statement_details.dump_to_logs</code></td><td>boolean</td><td><code>false</code></td><td>dump collected statement statistics to node logs when periodically cleared</td></tr> <tr><td><code>sql.metrics.statement_details.enabled</code></td><td>boolean</td><td><code>true</code></td><td>collect per-statement query statistics</td></tr> <tr><td><code>sql.metrics.statement_details.gateway_node.enabled</code></td><td>boolean</td><td><code>true</code></td><td>save the gateway node for each statement fingerprint. If false, the value will be stored as 0.</td></tr>
Is 0 a valid node id? Just wondering if it is possible for users to confuse it being disabled and just using 1 node.
pkg/sql/sqlstats/persistedsqlstats/flush.go line 578 at r1 (raw file):
nodeID := s.GetSQLInstanceID() if !sqlstats.GatewayNodeEnabled.Get(&s.cfg.Settings.SV) {
should this just be a function to avoid duplicate code? It will also make sure everywhere uses the same default value.
maryliag
force-pushed
the
node-setting
branch
from
1036c65
to
621f836
Compare
There was a problem hiding this comment.
Reviewable status:
docs/generated/settings/settings.html line 183 at r1 (raw file):
Previously, j82w wrote…
Is 0 a valid node id? Just wondering if it is possible for users to confuse it being disabled and just using 1 node.
It's not a valid node, it starts at 1. So the 0 should indicate is disabled
pkg/sql/sqlstats/persistedsqlstats/flush.go line 578 at r1 (raw file):
Previously, j82w wrote…
should this just be a function to avoid duplicate code? It will also make sure everywhere uses the same default value.
Good idea! Done
maryliag
added
backport-22.2.x
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlstats/persistedsqlstats/provider.go line 172 at r2 (raw file):
// GetEnabledSQLInstanceID returns the SQLInstanceID when gateway node is enabled, // and zero otherwise. func (s *PersistedSQLStats) GetEnabledSQLInstanceID(gatewayNodeIDEnabled bool) base.SQLInstanceID {
Is there any reason to not just get the config in the function?
// GetEnabledSQLInstanceID returns the SQLInstanceID when gateway node is enabled,
// and zero otherwise.
func (s *PersistedSQLStats) GetEnabledSQLInstanceID() base.SQLInstanceID {
gatewayNodeIDEnabled := sqlstats.GatewayNodeEnabled.Get(&s.cfg.Settings.SV)
if gatewayNodeIDEnabled {
return s.cfg.SQLIDContainer.SQLInstanceID()
}
return 0
}
When persisting statement statistics, one of the columns is the gateway node id, but because it needs to be a unique value, users who have a large amount of data are creating a lot of rows on the system.statement_statistics table. To help decrease the amount of rows, the new cluster setting `sql.metrics.statement_details.gateway_node.enabled` define if the value on that column should be the gateway node ID or 0. We still display the node ID on the statistics column. Fixes cockroachdb#88484 Release note (sql change): New cluster setting `sql.metrics.statement_details.gateway_node.enabled` that controls if the gateway node ID should be persisted to the `system.statement_statistics` table as is or as a 0, to decrease cardinality on the table. The node id is still available on the statistics column.
maryliag
force-pushed
the
node-setting
branch
from
621f836
to
f49ec12
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlstats/persistedsqlstats/provider.go line 172 at r2 (raw file):
Previously, j82w wrote…
Is there any reason to not just get the config in the function?
// GetEnabledSQLInstanceID returns the SQLInstanceID when gateway node is enabled,
// and zero otherwise.
func (s *PersistedSQLStats) GetEnabledSQLInstanceID() base.SQLInstanceID {
gatewayNodeIDEnabled := sqlstats.GatewayNodeEnabled.Get(&s.cfg.Settings.SV)
if gatewayNodeIDEnabled {
return s.cfg.SQLIDContainer.SQLInstanceID()
}
return 0
}
Done.
There was a problem hiding this comment.
Reviewable status:
|
TFTR! |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from f49ec12 to blathers/backport-release-21.2-88583: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 21.2.x failed. See errors above. error creating merge commit from f49ec12 to blathers/backport-release-22.1-88583: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 22.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
When persisting statement statistics, one of the columns is the gateway node id, but because it needs to be a unique value, users who have a large amount of data are creating a lot of rows on the system.statement_statistics table.
To help decrease the amount of rows, the new cluster setting
sql.metrics.statement_details.gateway_node.enableddefine if the value on that column should be the gateway node ID or 0. We still display the node ID on the statistics column.Fixes #88484
Release note (sql change): New cluster setting
sql.metrics.statement_details.gateway_node.enabledthat controls if the gateway node ID should be persisted to thesystem.statement_statisticstable as is or as a 0, to decrease cardinality on the table. The node id is still available on the statistics column.