sql: Add new builtin generator function crdb_internal.scan_storage_internal_keys()
#107743
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. |
|
This change is |
raggar
force-pushed
the
rahul_advanced_sstable_sql
branch
9 times, most recently
from
9f47020
to
dd6c324
Compare
pkg/storage/pebble.go
Outdated
| func (p *Pebble) GetAdvancedPebbleMetrics( | ||
| start, end roachpb.Key, | ||
| ) ([]enginepb.AdvancedPebbleMetrics, error) { | ||
| return []enginepb.AdvancedPebbleMetrics{}, nil |
There was a problem hiding this comment.
once cockroachdb/pebble#2713 is merged, I will call ScanStatistics to retrieve the metrics.
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @bananabrick, @RahulAggarwal1016, and @rharding6373)
pkg/sql/sem/builtins/generator_builtins.go line 687 at r2 (raw file):
), ), "crdb_internal.pebble_metrics": makeBuiltin(
@jbowens any opinions about the name? pebble_key_metrics or sstable_key_metrics are possible alternatives
pkg/sql/sem/builtins/generator_builtins.go line 701 at r2 (raw file):
makeAdvancedPebbleMetricsGenerator, "Returns statistics for a pebble store in the range start_key and end_key for the provided node id.", volatility.Stable,
This (and sstable_metrics) should be volatility.Volatile. Stable means it returns the same result when executed multiple times in the same txn (not a big deal though given its typical usage).
There was a problem hiding this comment.
Reviewed 1 of 14 files at r1.
Reviewable status:
-- commits line 2 at r2:
nit: prefix with sql,kv: and use imperative voice in the commit title: "Add new builtin ..."
pkg/kv/kvserver/stores_server.go line 176 at r2 (raw file):
} func (is Server) GetAdvancedPebbleMetrics(
Throughout let's remain consistent with the SQL function name we decide, eg ScanStorageInternalKeys
pkg/sql/sem/builtins/generator_builtins.go line 687 at r2 (raw file):
Previously, RaduBerinde wrote…
@jbowens any opinions about the name?
pebble_key_metricsorsstable_key_metricsare possible alternatives
How about scan_storage_internal_keys — I think including "scan" is appropriate to convey the cost of the operation.
pkg/sql/sem/builtins/generator_builtins.go line 700 at r2 (raw file):
advancedPebbleMetricsGeneratorType, makeAdvancedPebbleMetricsGenerator, "Returns statistics for a pebble store in the range start_key and end_key for the provided node id.",
How about: "Scans a store's storage engine, computing statistics describing the internal keys within the span [start_key, end_key)."
pkg/storage/pebble.go line 2070 at r2 (raw file):
Previously, RahulAggarwal1016 (Rahul Aggarwal) wrote…
once cockroachdb/pebble#2713 is merged, I will call ScanStatistics to retrieve the metrics.
Let's hold off merging this PR until that's in place in case the statistics we compute change.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/builtins/generator_builtins.go line 687 at r2 (raw file):
Previously, jbowens (Jackson Owens) wrote…
How about
scan_storage_internal_keys— I think including "scan" is appropriate to convey the cost of the operation.
yeah that sounds good
There was a problem hiding this comment.
Reviewable status:
pkg/storage/pebble.go line 2070 at r2 (raw file):
Previously, jbowens (Jackson Owens) wrote…
Let's hold off merging this PR until that's in place in case the statistics we compute change.
yup thats the plan
raggar
force-pushed
the
rahul_advanced_sstable_sql
branch
from
dd6c324
to
39a4962
Compare
raggar
changed the title
crdb_internal.pebble_metrics()crdb_internal.scan_storage_internal_keys()
There was a problem hiding this comment.
Reviewable status:
Previously, jbowens (Jackson Owens) wrote…
nit: prefix with
sql,kv:and use imperative voice in the commit title: "Add new builtin ..."
done
pkg/kv/kvserver/stores_server.go line 176 at r2 (raw file):
Previously, jbowens (Jackson Owens) wrote…
Throughout let's remain consistent with the SQL function name we decide, eg ScanStorageInternalKeys
fixed
pkg/sql/sem/builtins/generator_builtins.go line 700 at r2 (raw file):
Previously, jbowens (Jackson Owens) wrote…
How about: "Scans a store's storage engine, computing statistics describing the internal keys within the span [start_key, end_key)."
that sounds good
pkg/sql/sem/builtins/generator_builtins.go line 701 at r2 (raw file):
Previously, RaduBerinde wrote…
This (and sstable_metrics) should be
volatility.Volatile. Stable means it returns the same result when executed multiple times in the same txn (not a big deal though given its typical usage).
fixed
raggar
force-pushed
the
rahul_advanced_sstable_sql
branch
from
39a4962
to
0b19e7b
Compare
There was a problem hiding this comment.
Reviewable status:
|
I know there's a plan to add an argument for rate limiting. We should have two overloads, one with and one without the argument (essentially making that argument optional). The argument should be in megabytes / second for convenience. The one without the argument should use a value on the low side, say 10MB/s - it should be "safe" to use in all cases, and the user can increase the rate if it's too slow. |
raggar
force-pushed
the
rahul_advanced_sstable_sql
branch
4 times, most recently
from
cbeccc3
to
1abf955
Compare
raggar
force-pushed
the
rahul_advanced_sstable_sql
branch
from
1abf955
to
21e31f1
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/builtins/generator_builtins.go line 713 at r5 (raw file):
storageInternalKeysGeneratorType, makeStorageInternalKeysGenerator, "Scans a store's storage engine, computing statistics describing the internal keys within the span [start_key, end_key). This funct",
[nit] unfinished description
raggar
force-pushed
the
rahul_advanced_sstable_sql
branch
5 times, most recently
from
9e67ae5
to
21d1322
Compare
| } | ||
|
|
||
| metrics = append(metrics, enginepb.StorageInternalKeysMetrics{ | ||
| Level: -1, |
There was a problem hiding this comment.
not sure what value this should be (is there a way to have a column be blank?)
|
Previously, RahulAggarwal1016 (Rahul Aggarwal) wrote…
You can convert -1 to DNull when you generate the datums |
raggar
force-pushed
the
rahul_advanced_sstable_sql
branch
from
21d1322
to
a2bf900
Compare
oh nice yeah i'll do that |
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/api.proto line 92 at r7 (raw file):
} message ScanStorageInternalKeysRequest {
[nit] would be good to have a brief comment here
|
Build failed (retrying...): |
raggar
force-pushed
the
rahul_advanced_sstable_sql
branch
from
a2bf900
to
5b41a6b
Compare
|
Canceled. |
This new builtin is used to gather specific pebble metrics for a node and store id (within an given keyspan). The builtin returns information about the different types of keys (including snapshot pinned keys) as well as bytes. Informs: cockroachdb#94659 Release note: None
raggar
force-pushed
the
rahul_advanced_sstable_sql
branch
from
5b41a6b
to
08dbd7e
Compare
|
bors r+ |
|
Build succeeded: |
This new builtin is used to gather specific pebble metrics for a node and store id (within an given keyspan). The builtin returns information about the different types of keys (including snapshot pinned keys) and bytes.
Informs: #94659
Release-note: None