Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @zachlite)

-- commits line 19 at r1:
I have studied your change and even tested the change locally, and I am facing the following observation:

• your code does, indeed, do what you wanted it to do and what is written here. 💯
• sadly, the resulting UX is not yet valuable.

(This is a shortcoming in the original spec of the feature - and I think we wouldn't have noticed prior to you putting this PR out to try it in practice. So the work here is valuable in any case.)

Okay so what went wrong. SHOW RANGES is meant to emphasize ranges primarily.

• without WITH INDEXES/TABLES, we want DETAILS to indicate the total stats for the range. You got this right already - nothing else to say here.
• when WITH INDEXES/TABLES is specified, we want DETAILS to report stats about the intersection between the SQL objects and each range.

Intersection means that:

1. we don't want repeated information across multiple rows -- each row should report details about a "segment" of the SQL object
2. each row should pertain to the data just in the range mentioned in the range_id column.

The code as currently written works fine in the case where a SQL object is fully contained in 1 range. However, when the SQL object spans across 2 or more ranges, we see the span stats computed across the entire SQL object (i.e. across all enclosing ranges), not per range.

There are two "criteria" to identify when success will be achieved:

• in any combination of SHOW RANGES options, the span stats range_count must always be = 1.
• when WITH TABLES/WITH INDEXES is specified, it should be possible to aggregate the span stats with SUM across all the rows for 1 SQL object to obtain an estimate of the size of that SQL object.

Currently, when the code is run over an index which spans, say, 3 ranges, we see 1) range_count=3 2) the same span stats repeated over 3 different rows, such that the SUM will report 3x the actual size of the index.

How to achieve this?

• The code will need to add an IF() or CASE expression to decide which span start/end key to use. See some sketch below:
• While trying to do this myself, I also noticed that the columns span_start_key / span_end_key as currently computed in the code also have ... the same bug. This is a mistake I made in the initial implementation. I'll send a separate PR, which you can use here.

Sketch of what I mean above (this assumes WITH INDEXES was specified)

WITH ranges AS (
  SELECT
    r.range_id,
    r.start_key, r.end_key, -- SAME AS CURRENTLY / UNCHANGED
    ...
    s.start_key AS span_start_key, s.end_key AS span_end_key, -- SAME AS CURRENTLY / UNCHANGED
    -- NEW (I WILL SEND A NEW PR)
    IF(s.start_key<r.start_key,r.start_key,s.start_key) AS per_range_start_key,
    IF(s.end_key<r.end_key,s.end_key,r.end_key) AS per_range_end_key
  FROM crdb_internal.ranges_no_leases r
  INNER JOIN defaultdb.crdb_internal.index_spans s
   ON ...
  ),
 named_ranges AS ( ... )
-- CHANGE IN YOUR CODE:
  all_span_stats AS (SELECT * FROM crdb_internal.tenant_span_stats(ARRAY(SELECT (per_range_start_key, per_range_end_key) FROM named_ranges))),
  intermediate AS (
     SELECT r.*, sps.stats AS span_stats
       FROM named_ranges r
 INNER JOIN all_span_stats sps ON r.per_range_start_key = sps.start_key AND r.per_range_end_key = sps.end_key)

-- main query
SELECT ... 
-- CHANGE (I WILL SEND SEPARATE PR)
    '…'||crdb_internal.pretty_key(per_range_start_key, 1) AS index_start_key,
    '…'||crdb_internal.pretty_key(per_range_end_key, 1) AS index_end_key,
    ...
 -- as in your PR - unchanged:
    span_stats

If you want to test it for yourself before you change your PR, here's a full query that works:

Move the computation of all_span_stats below this point, and use named_ranges instead as input which has already computed the DISTINCT span bounds. This will avoid duplicate work.

pkg/sql/sem/builtins/generator_builtins.go line 608 at r1 (raw file):

			volatility.Stable,
		),
		// Span overload

Either expand this comment (and make it a sentence with a final period) or you can remove it entirely.

pkg/sql/sem/builtins/generator_builtins.go line 615 at r1 (raw file):

			spanStatsGeneratorType,
			makeSpanStatsGenerator,
			"foo bar",

This description would benefit from more words.

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

pkg/sql/delegate/show_ranges.go line 381 at r1 (raw file):
Reviewable ate my comment in the previous review. here it is:

Move the computation of all_span_stats below this point, and use named_ranges instead as input which has already computed the DISTINCT span bounds. This will avoid duplicate work.

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

-- commits line 19 at r1:

Here's the patch you'll need as starting point: #103667

This is not in-scope for this PR, and we can work on that separately afterwards.

Ack!

@knz, I've rebased on top of your work in #103667.

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

-- commits line 19 at r1:

Done.

pkg/sql/delegate/show_ranges.go line 381 at r1 (raw file):

Done.

pkg/sql/delegate/show_ranges.go line 381 at r1 (raw file):

Done.

pkg/sql/sem/builtins/generator_builtins.go line 608 at r1 (raw file):

Done.

pkg/sql/sem/builtins/generator_builtins.go line 615 at r1 (raw file):

☠️ missed this!
Done.

This PR has the potential to fill me with joy, but the joy will only appear after there's a unit test for the new functionality.

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @zachlite)

pkg/sql/delegate/show_ranges.go line 388 at r3 (raw file):

		}

		fmt.Fprintf(&buf, "all_span_stats AS (\n"+

nit: use backticks for the literal string and avoid the string concatenation.

I have found this bug during the span stats fan-out: #103809
While working on a patch, I've found some other curious behavior in span_stats_test.go. I will know more soon.

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

The overall structure of the code LGTM.

After trying the feature out however, I found myself raising eyebrows at the meaning of the various JSON fields.
I have two opinions here, one i feel strongly about and one that i feel less strongly about:

• strongly held opinion: regardless of the fields we choose to include in the JSON stats, they need to be part of a documented public interface for SHOW RANGES. That is, we need to document the list of fields (if only in the release note), and there should be some rudimentary testing that all the fields we guarantee to provide are populated in a way that makes sense.

• less strongly held opinion: some of the fields are kinda "internal" and are hard to reason about for an end-user. I would personally leave them out. For example, "contains_estimates", "last_update_nanos", "gc_bytes_age". Some are confusing by being separated, I'm not sure we need to separate them (can we combine them perhaps), namely the "range_key_X" and "key_X" fields, and "separated_intent_X" vs "intent_X". I also think that "range_count" should be omitted entirely in SHOW RANGES because it's always going to be =1 so it's just noise.

The two opinions combine together: we could select a subset of the stats to present using a fixed, documented interface; and tell users "if you want to know more, you can use that reserved/private span_stats() function. What would be a good subset to expose via SHOW RANGES? I think the following would make the cut: key/val/sys/live/intent count and bytes. They are all useful and straightforward to explain.

Finally, a question for you: is there a feature somewhere that gives us the total on-disk size of a range? Users probably want to use SHOW RANGES for that (in the 1 row per range output mode) and I don't think this can be inferred from the MVCCStats payload. I also asked this on slack here.

Reviewed 5 of 11 files at r4, 7 of 7 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @zachlite)

pkg/sql/sem/builtins/generator_builtins.go line 579 at r5 (raw file):

	),
	"crdb_internal.tenant_span_stats": makeBuiltin(genProps(),
		// Tenant overload

When you look at these comments again, maybe help here by adding a period after each of the comments from here.

pkg/sql/sem/builtins/generator_builtins.go line 608 at r5 (raw file):

			volatility.Stable,
		),
		// Span overload

comment?

pkg/sql/sem/builtins/generator_builtins.go line 615 at r5 (raw file):

			spanStatsGeneratorType,
			makeSpanStatsGenerator,
			"foo bar",

foo bar?

I think the following would make the cut: key/val/sys/live/intent count and bytes.

I like it. This was easy enough to implement. This is documented in the release note and I've updated the test to include assertions that these values are present.

is there a feature somewhere that gives us the total on-disk size of a range?

approximate_disk_bytes has always been a part of the span stats response but has been omitted by the marshaler when it has a value of 0 (empty). The value comes from pebble's EstimateDiskUsage, and has a "warmup time" where it returns 0 for a minute or so after the cluster starts up.

For this PR, I've adjusted the proto definition with an explicit json tag so the value is not emitted when empty. As much as this feature should be authoritative, I suppose since the thing has 'approximate' in the name, it's ok.

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

pkg/sql/delegate/show_ranges.go line 388 at r3 (raw file):

Done.

pkg/sql/sem/builtins/generator_builtins.go line 579 at r5 (raw file):

Done. My bad - I had resolved these comment issues previously but they got lost in the shuffle as I changed branches.

Amazing work.

(with a nit on release note)

Reviewed 10 of 10 files at r6.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @zachlite)

-- commits line 31 at r6:
nit: the doc writer knows nothing about enginepb.MVCCStats. To help them it would be useful to copy-paste the description strings for those fields into the release note.

Thank you for all of your help and for your thorough review!
I see. will do.

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