builtins: add `tenant_span_stats` generator function #97534

THardy98 · 2023-02-22T23:38:30Z

Part of: #94332
Part of: #94330

Added new builtin function tenant_span_stats that retrieves span
statistics for the current tenant. tenant_span_stats can be called as:

crdb_internal.tenant_span_stats(): returns table span statistics for all of the tenants tables
crdb_internal.tenant_span_stats(database_id): returns table span statistics for the tenant's tables belonging to the specified database id
crdb_internal.tenant_span_stats(database_id, table_id): returns the tenant's table span statistics for the provided table id

Returned rows take the format:

[email protected]:26257/movr> select * from crdb_internal.tenant_span_stats(104);
  database_id | table_id | range_count | approximate_disk_bytes | live_bytes | total_bytes | live_percentage
--------------+----------+-------------+------------------------+------------+-------------+------------------
          104 |      106 |          27 |                  53866 |      16821 |       16821 |               1
          104 |      107 |          27 |                  45285 |      10932 |       10932 |               1
          104 |      108 |          27 |                 225645 |     528219 |      528219 |               1
          104 |      109 |           3 |                 130978 |     260826 |      260826 |               1
          104 |      110 |           3 |                 393815 |     708387 |      708387 |               1
          104 |      111 |           3 |                  30021 |       1413 |        1413 |               1

Note: This implementation is naive - it calls SpanStats for each table span. Future work will entail expanding the endpoint to be able to fetch span statistics for multiple spans at once.

Release note (sql change): new builtin function tenants_span_stats,
retrieves the span statistics for the current tenant.

cockroach-teamcity · 2023-02-22T23:38:45Z

This change is

THardy98 · 2023-02-23T19:17:28Z

Not sure how to add logic tests for this builtin. Is there a way to set deterministic values for approximate_disk_bytes, live_bytes, total_bytes?

zachlite · 2023-02-23T21:14:41Z

Not sure how to add logic tests for this builtin. Is there a way to set deterministic values for approximate_disk_bytes, live_bytes, total_bytes?

My 2-cents - Perhaps it would be sufficient to only test this function up to the boundary with SpanStats(). There's already coverage of SpanStats and tenant isolation --- and then even lower-level coverage of RangeStats and ComputeMetrics.

Another test idea would be to make sure that each overloaded function returns the correct column values with respect to database and table.

THardy98 · 2023-02-24T20:30:54Z

Not sure how to add logic tests for this builtin. Is there a way to set deterministic values for approximate_disk_bytes, live_bytes, total_bytes?

My 2-cents - Perhaps it would be sufficient to only test this function up to the boundary with SpanStats(). There's already coverage of SpanStats and tenant isolation --- and then even lower-level coverage of RangeStats and ComputeMetrics.

Another test idea would be to make sure that each overloaded function returns the correct column values with respect to database and table.

Yes, considering that SpanStats is already tested there's no need to test the values being returned by SpanStats. Rather, I've added tests ensuring that returned values are scoped correctly (to the correct database, or database + table) and a couple checks to ensure the validity of passed in arguments (no negative values).

zachlite

Reviewable status: complete! 1 of 0 LGTMs obtained

ecwall

Can you partially test the non-deterministic columns with something like SELECT ... approximate_disk_bytes > 0, live_bytes > 0, etc to make sure the columns are being populated and aren't just defaulting to a zero value to cover this part of the code

		tree.NewDInt(tree.DInt(tssi.currStatsResponse.RangeCount)),
		tree.NewDInt(tree.DInt(tssi.currStatsResponse.ApproximateDiskBytes)),
		tree.NewDInt(tree.DInt(liveBytes)),
		tree.NewDInt(tree.DInt(totalBytes)),
		tree.NewDFloat(tree.DFloat(livePercentage)),

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @THardy98)

pkg/sql/planner.go line 857 at r2 (raw file):

	ctx context.Context, dbId int, tableId int,
) (eval.InternalRows, error) {
	query := `SELECT parent_id, table_id from crdb_internal.tables`

Could you capitalize FROM/WHERE/AND/etc for consistency

pkg/sql/planner.go line 867 at r2 (raw file):

		args = append(args, dbId)
	} else {
		query += ` where parent_id != $1`

Why is this parent_id != 0 instead of no WHERE clause?

THardy98

Added a test to check for non-deterministic columns. Omitted approximate_disk_bytes, as we weren't recording any disk bytes until I generated a large series (1mil), which I didn't feel was particularly valuable. I think this its probably sufficient enough to check that our other span stats are being populated, but if there's a simple way to ensure that a table has disk bytes, than I can make the change.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ecwall)

pkg/sql/planner.go line 857 at r2 (raw file):

Previously, ecwall (Evan Wall) wrote…

Could you capitalize FROM/WHERE/AND/etc for consistency

Done.

pkg/sql/planner.go line 867 at r2 (raw file):

Previously, ecwall (Evan Wall) wrote…

Why is this parent_id != 0 instead of no WHERE clause?

Some tables belonging to crdb_internal.tables are not affiliated with a database and have a parent_id of 0 (usually crdb_internal or pg catalog tables), which aren't useful to the user.

ecwall

Looks good just a couple minor comments before merging

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @THardy98)

pkg/sql/planner.go line 867 at r2 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

Some tables belonging to crdb_internal.tables are not affiliated with a database and have a parent_id of 0 (usually crdb_internal or pg catalog tables), which aren't useful to the user.

Can you add this as a brief comment here since it was not obvious to me.

pkg/sql/planner.go line 871 at r3 (raw file):

	}

	it, err := p.QueryIteratorEx(

You can just return p.QueryIteratorEx(...) instead of doing this assignment.

pkg/sql/sem/builtins/generator_builtins.go line 2958 at r3 (raw file):

	var next bool
	var err error
	next, err = tssi.it.Next(ctx)

Remove the separate declarations and use next, err :=.

THardy98

Addressed the nits, TYFR :)

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @ecwall)

pkg/sql/planner.go line 867 at r2 (raw file):

tables belonging to crdb_internal.tables are not affiliated with a database and have a parent_id of 0 (usually crdb_internal or pg catalog tables), which aren't useful to the user.
Added a comment.

pkg/sql/planner.go line 871 at r3 (raw file):

Previously, ecwall (Evan Wall) wrote…

You can just return p.QueryIteratorEx(...) instead of doing this assignment.

Done

pkg/sql/sem/builtins/generator_builtins.go line 2958 at r3 (raw file):

Previously, ecwall (Evan Wall) wrote…

Remove the separate declarations and use next, err :=.

Done

j82w

Reviewed 2 of 7 files at r2, 1 of 2 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @ecwall and @THardy98)

pkg/sql/sem/builtins/generator_builtins.go line 3017 at r4 (raw file):

		return nil, err
	}
	if !isAdmin {

Should this also be allowed for the same roles as HasViewActivityOrViewActivityRedactedRole to match the crdb_internal view role check?

THardy98

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @ecwall and @j82w)

pkg/sql/sem/builtins/generator_builtins.go line 3017 at r4 (raw file):

Previously, j82w (Jake) wrote…

Should this also be allowed for the same roles as HasViewActivityOrViewActivityRedactedRole to match the crdb_internal view role check?

I had kept it to admin as the SpanStats endpoint is scoped to admin privileges.

j82w · 2023-03-01T15:28:13Z

pkg/sql/sem/builtins/generator_builtins.go line 3017 at r4 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

I kept it to admin as the SpanStats endpoint is scoped to admin privileges.

Does the database page work with view activity permission? Will the ui using sql-over-http still work if this requires admin?

j82w

Reviewable status: complete! 1 of 0 LGTMs obtained (and 2 stale) (waiting on @ecwall and @THardy98)

THardy98 · 2023-03-02T15:57:35Z

pkg/sql/sem/builtins/generator_builtins.go line 3017 at r4 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…
Does the database page work with view activity permission? Will the ui using sql-over-http still work if this requires admin?

After some discussion offline, opened up the builtin to anyone with VIEWACTIVITY permission (necessary for use from the console).

j82w

Reviewed 1 of 5 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 3 stale) (waiting on @ecwall and @THardy98)

pkg/sql/authorization.go line 462 at r5 (raw file):

// HasViewActivity implements the AuthorizationAccessor interface.
// Requires a valid transaction to be open.
func (p *planner) HasViewActivity(ctx context.Context) (bool, error) {

Why not use the existing HasViewActivityOrViewActivityRedactedRole which the crdb_internal views use?

THardy98 · 2023-03-02T16:03:53Z

Reviewed 1 of 5 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 3 stale) (waiting on @ecwall and @THardy98)

pkg/sql/authorization.go line 462 at r5 (raw file):
// HasViewActivity implements the AuthorizationAccessor interface.
// Requires a valid transaction to be open.
func (p *planner) HasViewActivity(ctx context.Context) (bool, error) {
Why not use the existing HasViewActivityOrViewActivityRedactedRole which the crdb_internal views use?

Not sure how I missed that, will use it instead.

THardy98 · 2023-03-02T16:38:48Z

Reviewed 1 of 5 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 3 stale) (waiting on @ecwall and @THardy98)

pkg/sql/authorization.go line 462 at r5 (raw file):
// HasViewActivity implements the AuthorizationAccessor interface.
// Requires a valid transaction to be open.
func (p *planner) HasViewActivity(ctx context.Context) (bool, error) {
Why not use the existing HasViewActivityOrViewActivityRedactedRole which the crdb_internal views use?

Done

j82w

Reviewed 2 of 2 files at r6, 5 of 5 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 3 stale) (waiting on @ecwall and @THardy98)

pkg/sql/sem/builtins/generator_builtins.go line 3018 at r7 (raw file):

	}
	if !hasViewActivity {
		return nil, pgerror.Newf(pgcode.InsufficientPrivilege, "user needs the VIEWACTIVITY or VIEWACTIVITYREDACTED permission to view span statistics")

Should this include admin?

THardy98

Reviewable status: complete! 0 of 0 LGTMs obtained (and 3 stale) (waiting on @ecwall and @j82w)

pkg/sql/sem/builtins/generator_builtins.go line 3018 at r7 (raw file):

Previously, j82w (Jake) wrote…

Should this include admin?

Added.

Part of: cockroachdb#94332 Part of: cockroachdb#94330 Added new builtin function `tenant_span_stats` that retrieves span statistics for the current tenant. `tenant_span_stats` can be called as: - `crdb_internal.tenant_span_stats()`: returns table span statistics for all of the tenants tables - `crdb_internal.tenant_span_stats(database_id)`: returns table span statistics for the tenant's tables belonging to the specified database id - `crdb_internal.tenant_span_stats(database_id, table_id)`: returns the tenant's table span statistics for the provided table id Release note (sql change): new builtin function `tenants_span_stats`, retrieves the span statistics for the current tenant.

THardy98 · 2023-03-02T19:09:38Z

TYFR :)

THardy98 · 2023-03-02T19:09:45Z

bors r+

craig · 2023-03-02T19:43:17Z

Build succeeded:

Bazel Essential CI (Cockroach)

THardy98 requested a review from a team February 22, 2023 23:38

THardy98 force-pushed the table_span_stats_builtin branch 2 times, most recently from a856f12 to 41d4292 Compare February 22, 2023 23:41

THardy98 mentioned this pull request Feb 23, 2023

sql: new functions crdb_internal.ranges_in_span and crdb_internal.tenant_ranges_per_table #96011

Closed

THardy98 force-pushed the table_span_stats_builtin branch from 41d4292 to 2bd3d8a Compare February 23, 2023 00:23

THardy98 requested review from a team February 23, 2023 01:19

THardy98 force-pushed the table_span_stats_builtin branch from 2bd3d8a to a864d4d Compare February 23, 2023 16:55

THardy98 force-pushed the table_span_stats_builtin branch 3 times, most recently from 26b82c4 to bc7d252 Compare February 24, 2023 19:07

zachlite approved these changes Feb 24, 2023

View reviewed changes

rafiss requested a review from ecwall February 27, 2023 16:18

THardy98 mentioned this pull request Feb 27, 2023

cluster-ui: move database details endpoint to use sql-over-http #93209

Merged

ecwall reviewed Feb 27, 2023

View reviewed changes

THardy98 force-pushed the table_span_stats_builtin branch from bc7d252 to 2c83a7e Compare February 27, 2023 20:52

THardy98 commented Feb 27, 2023

View reviewed changes

THardy98 requested a review from ecwall February 27, 2023 21:13

ecwall approved these changes Feb 28, 2023

View reviewed changes

THardy98 force-pushed the table_span_stats_builtin branch from 2c83a7e to 96c4eb0 Compare February 28, 2023 16:14

THardy98 commented Feb 28, 2023

View reviewed changes

THardy98 force-pushed the table_span_stats_builtin branch from 96c4eb0 to 0f826d8 Compare February 28, 2023 20:24

j82w reviewed Mar 1, 2023

View reviewed changes

THardy98 commented Mar 1, 2023

View reviewed changes

THardy98 requested a review from j82w March 1, 2023 14:29

j82w approved these changes Mar 1, 2023

View reviewed changes

THardy98 force-pushed the table_span_stats_builtin branch from 0f826d8 to 10fdf97 Compare March 2, 2023 15:56

THardy98 requested a review from j82w March 2, 2023 15:56

THardy98 force-pushed the table_span_stats_builtin branch from 10fdf97 to eedcc27 Compare March 2, 2023 15:59

j82w reviewed Mar 2, 2023

View reviewed changes

THardy98 force-pushed the table_span_stats_builtin branch from eedcc27 to be984c2 Compare March 2, 2023 16:38

THardy98 requested a review from j82w March 2, 2023 16:38

j82w approved these changes Mar 2, 2023

View reviewed changes

THardy98 commented Mar 2, 2023

View reviewed changes

THardy98 force-pushed the table_span_stats_builtin branch 3 times, most recently from 305ce5d to 5d38cb0 Compare March 2, 2023 18:21

craig bot merged commit 5bae6a0 into cockroachdb:master Mar 2, 2023

cockroach-teamcity mentioned this pull request Mar 3, 2023

PR #97534 - builtins: add tenant_span_stats generator function cockroachdb/docs#16409

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

builtins: add `tenant_span_stats` generator function #97534

builtins: add `tenant_span_stats` generator function #97534

THardy98 commented Feb 22, 2023 •

edited

Loading

cockroach-teamcity commented Feb 22, 2023

THardy98 commented Feb 23, 2023

zachlite commented Feb 23, 2023

THardy98 commented Feb 24, 2023

zachlite left a comment

ecwall left a comment

THardy98 left a comment

ecwall left a comment

THardy98 left a comment

j82w left a comment

THardy98 left a comment •

edited

Loading

j82w commented Mar 1, 2023

j82w left a comment

THardy98 commented Mar 2, 2023 •

edited

Loading

j82w left a comment

THardy98 commented Mar 2, 2023

THardy98 commented Mar 2, 2023

j82w left a comment

THardy98 left a comment

THardy98 commented Mar 2, 2023

THardy98 commented Mar 2, 2023

craig bot commented Mar 2, 2023

builtins: add tenant_span_stats generator function #97534

builtins: add tenant_span_stats generator function #97534

Conversation

THardy98 commented Feb 22, 2023 • edited Loading

cockroach-teamcity commented Feb 22, 2023

THardy98 commented Feb 23, 2023

zachlite commented Feb 23, 2023

THardy98 commented Feb 24, 2023

zachlite left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

THardy98 left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

THardy98 left a comment

Choose a reason for hiding this comment

j82w left a comment

Choose a reason for hiding this comment

THardy98 left a comment • edited Loading

Choose a reason for hiding this comment

j82w commented Mar 1, 2023

j82w left a comment

Choose a reason for hiding this comment

THardy98 commented Mar 2, 2023 • edited Loading

j82w left a comment

Choose a reason for hiding this comment

THardy98 commented Mar 2, 2023

THardy98 commented Mar 2, 2023

j82w left a comment

Choose a reason for hiding this comment

THardy98 left a comment

Choose a reason for hiding this comment

THardy98 commented Mar 2, 2023

THardy98 commented Mar 2, 2023

craig bot commented Mar 2, 2023

builtins: add `tenant_span_stats` generator function #97534

builtins: add `tenant_span_stats` generator function #97534

THardy98 commented Feb 22, 2023 •

edited

Loading

THardy98 left a comment •

edited

Loading

THardy98 commented Mar 2, 2023 •

edited

Loading