apiutil, roachpb: create utilities to map descriptors to ranges #133840

angles-n-daemons · 2024-10-30T14:23:54Z

apiutil, roachpb: create utilities to map descriptors to ranges

Previously each range correlated to a single table, or even a single
index in a database, so all that was required to identify which tables,
indexes were in the range were to look at the start key of the range and
map it accordingly.

With range coalescing however, it's possible for one, or many tables,
indexes and the like to reside within the same range. To properly
identify the contents of a range, this PR adds the following utilities:

A utility function which turns a range into a span, and clamps it
to its tenant's table space.
A utility function which takes the above spans and uses the catalog
and new descriptor by span utility to turn those spans into a set of
table descriptors ordered by id.
A utility function which transforms those table descriptors into a
set of (database, table, index) names which deduplicate and identify
each index uniquely.
A utility function, which merges the ranges and indexes into a map
keyed by RangeID whose values are the above index names.
A primary entrypoint for consumers from which a set of ranges can be
passed in and a mapping from those ranges to indexes can be
returned.

A variety of caveats come with this approach. It attempts to scan the
desrciptors all at once, but it still will scan a sizable portion of the
descriptors table if the request is large enough. This makes no attempt
to describe system information which does not have a descriptor. It will
describe system tables which appear in the descriptors table, but it
will not try to explain "tables" which do not have descriptors (example
tsdb), or any other information stored in the keyspace without a
descriptor (PseudoTableIDs, GossipKeys for example).

Throughout this work, many existing utilities were duplicated, and then
un-duplicated (keys.TableDataMin, roachpb.Span.Overlap, etc). If you
see anything that seems to already exist, feel free to point it out
accordingly.

Epic: CRDB-43151
Fixes: #130997

Release note: None

blathers-crl · 2024-10-30T14:23:58Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2024-10-30T14:24:11Z

This change is

angles-n-daemons · 2024-10-30T14:24:22Z

pkg/server/apiutil/rangeutil.go

+// RangeToTableSpans is a simple utility function which converts a set of ranges
+// to a set of spans bound to the codec's SQL table space, and removed if the bound
+// span is zero length.
+func RangesToTableSpans(codec keys.SQLCodec, ranges []roachpb.RangeDescriptor) []roachpb.Span {


Utility 1: A function which turns a range into a span, and clamps it
to its tenant's table space.

angles-n-daemons · 2024-10-30T14:25:05Z

pkg/server/apiutil/rangeutil.go

+
+// SpansToOrderedTableDescriptors uses the transaction's collection to turn a set of
+// spans to a set of descriptors which describe the table space in which those spans lie.
+func SpansToOrderedTableDescriptors(


Utility 2: A function which takes the above spans and uses the catalog
and new descriptor by span utility to turn those spans into a set of
table descriptors ordered by id (no tests for this func).

angles-n-daemons · 2024-10-30T14:25:19Z

pkg/server/apiutil/rangeutil.go

+// database, table, index combinations within. It assumes that every table
+// has at least one index, the descriptors input are ordered, and that
+// there can be duplicates of the descriptors.
+func TableDescriptorsToIndexNames(


Utility 3: A function which transforms those table descriptors into a
set of (database, table, index) names which deduplicate and identify
each index uniquely.

angles-n-daemons · 2024-10-30T14:25:35Z

pkg/server/apiutil/rangeutil.go

+// MapRangesToIndexes is a utility function which iterates over two lists,
+// one consisting of ordered ranges, and the other consisting of ordered index names
+// and outputs a mapping from range to index.
+func MapRangesToIndexes(


Utility 4: A function, which merges the ranges and indexes into a map
keyed by RangeID whose values are the above index names.

angles-n-daemons · 2024-10-30T14:25:58Z

pkg/server/apiutil/rangeutil.go

+//  2. Get the table descriptors that fall within the given spans.
+//  3. Get the database, table and index name for all indexes found in the descriptors.
+//  4. Return a mapping of the indexes which appear in each range.
+func GetRangeIndexMapping(


Primary Entrypoint for callers: A function in which a set of ranges can be
passed in and a mapping from those ranges to indexes can be
returned.

angles-n-daemons · 2024-10-30T14:26:33Z

pkg/sql/catalog/internal/catkv/catalog_reader.go

+// into an upper bound descriptor ID. It does this by turning the key into a
+// descriptor ID, and then moving it upwards if and only if it is not the prefix
+// of the current index / table span.
+func getDescriptorIDFromExclusiveKey(codec keys.SQLCodec, key roachpb.Key) (uint32, error) {


Not a part of this PR

angles-n-daemons · 2024-10-30T14:26:42Z

pkg/sql/catalog/internal/catkv/catalog_reader_test.go

@@ -203,6 +205,24 @@ func TestDataDriven(t *testing.T) {
 						return cr.GetByNames(ctx, txn, nis)
 					}
 					return h.doCatalogQuery(ctx, q)
+				case "scan_descriptors_in_span":


Not a part of this PR

angles-n-daemons · 2024-10-30T14:26:46Z

pkg/sql/catalog/internal/catkv/catalog_reader_test.go

@@ -217,6 +237,58 @@ type testHelper struct {
 	ucr, ccr catkv.CatalogReader
 }

+func (h testHelper) parseSpanFromArgKey(argkey string) roachpb.Span {


Not a part of this PR

angles-n-daemons · 2024-10-30T14:26:50Z

pkg/sql/catalog/internal/catkv/testdata/testdata_app

@@ -16,6 +16,18 @@ COMMENT ON TABLE kv IS 'this is a table';
 COMMENT ON INDEX mv@idx IS 'this is an index';
 COMMENT ON CONSTRAINT ck ON kv IS 'this is a check constraint';
 COMMENT ON CONSTRAINT kv_pkey ON kv IS 'this is a primary key constraint';
+


Not a part of this PR

angles-n-daemons · 2024-10-30T14:26:55Z

pkg/sql/catalog/internal/catkv/testdata/testdata_system

@@ -16,6 +16,18 @@ COMMENT ON TABLE kv IS 'this is a table';
 COMMENT ON INDEX mv@idx IS 'this is an index';
 COMMENT ON CONSTRAINT ck ON kv IS 'this is a check constraint';
 COMMENT ON CONSTRAINT kv_pkey ON kv IS 'this is a primary key constraint';
+


Not a part of this PR

angles-n-daemons · 2024-10-30T14:27:03Z

pkg/sql/catalog/descs/collection.go

@@ -743,6 +743,13 @@ func (tc *Collection) GetAll(ctx context.Context, txn *kv.Txn) (nstree.Catalog,
 	return ret.Catalog, nil
 }

+// GetDescriptorsInSpans returns all descriptors within a given span.


Not a part of this PR

dhartunian

Reviewed 7 of 7 files at r1, 9 of 9 files at r3, 10 of 11 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angles-n-daemons and @kyle-a-wong)

-- commits line 134 at r4:
nit: replace tabs with spaces.

-- commits line 145 at r4:
💯 thank you for this description.

-- commits line 147 at r4:
nit: "caveat"

nit: what is "it" here?

-- commits line 154 at r4:
doesn't need to be part of the PR, but should we return some list of "unexplained spans" when someone asks about a range?

pkg/roachpb/data.go line 224 at r4 (raw file):

func (k Key) Less(b Key) bool {
	return bytes.Compare(k, b) == -1
}

You have a compare on keys here:

cockroach/pkg/roachpb/data.go

Lines 215 to 219 in 07423f4

    
           // Compare compares the two Keys. 
        
           func (k Key) Compare(b Key) int { 
        
           	return bytes.Compare(k, b) 
        
           }

pkg/roachpb/metadata.go line 1010 at r4 (raw file):

}

type RangeDescriptorsByStartKey []RangeDescriptor

I don't see where this is used.

pkg/roachpb/metadata.go line 1022 at r4 (raw file):

	tmp := r[i]
	r[i] = r[j]
	r[j] = tmp

you can swap on one line via r[i], r[j] = r[j], r[i]

pkg/server/apiutil/rangeutil.go line 20 at r4 (raw file):

)

type IndexNames struct {

nit: docstring. I'm mostly curious why there's no construct already that combines the db/table/index names together.

pkg/server/apiutil/rangeutil.go line 44 at r4 (raw file):

	txn descs.Txn,
	codec keys.SQLCodec,
	databases map[descpb.ID]catalog.DatabaseDescriptor,

it's expected that the caller can produce this map?

pkg/server/apiutil/rangeutil.go line 54 at r4 (raw file):

	}

	indexes, err := TableDescriptorsToIndexNames(codec, databases, tables)

This part's a bit confusing to me. If a span maps to a table descriptor then we'll grab all the indexes from that table? We can't definitively state whether one or the other is in the span?

AHH I'm keeping this comment because I think I understand now: we produce the Span field on every index and then intersect it with the spans from the ranges. Is that right?

pkg/server/apiutil/rangeutil.go line 72 at r4 (raw file):

	flushToResults := func(rangeID roachpb.RangeID) {
		results[rangeID] = contents
		contents = []IndexNames{}

There's a new clear builtin you can use to do this without allocating a new slice: https://pkg.go.dev/builtin#clear

pkg/server/apiutil/rangeutil.go line 124 at r4 (raw file):

// spans to a set of descriptors which describe the table space in which those spans lie.
func SpansToOrderedTableDescriptors(
	ctx context.Context, txn descs.Txn, codec keys.SQLCodec, spans []roachpb.Span,

It appears that codec is unused in this function.

pkg/server/apiutil/rangeutil.go line 151 at r4 (raw file):

	tables []catalog.TableDescriptor,
) ([]IndexNames, error) {
	seen := map[string]bool{}

you can use struct{} as the value type in the map to avoid allocating booleans, since you never read them.

pkg/server/apiutil/rangeutil.go line 178 at r4 (raw file):

func spanFromRange(rangeDesc roachpb.RangeDescriptor) roachpb.Span {
	return rangeDesc.KeySpan().AsRawSpanWithNoLocals()

I think one-liner functions like this make the code harder to read. I'd just inline this.

pkg/sql/catalog/internal/catkv/catalog_reader.go line 168 at r2 (raw file):

Previously, angles-n-daemons (Brian Dillmann) wrote…

Not a part of this PR

No need for per-file comments like this. You can just have a note in the PR description if you want the reviewer to skip a commit because it's part of a stack that's being reviewed separately. In Reviewable, I can easily filter the diffs by commit.

pkg/roachpb/key_test.go line 31 at r4 (raw file):

		{"low tenant codec gets clamped to lower bound", lowTp(5), tp(1), tp(10), tp(1)},
		{"high tenant codec gets clamped to upper bound", highTp(5), tp(1), tp(10), tp(10)},
		{"system codec im not sure", sysTp(5), tp(1), tp(10), tp(1)},

I think system tenant should be unchanged if the clamp a/b are also in system tenant, otherwise clamp to the start key.

pkg/roachpb/span_test.go line 4 at r4 (raw file):

//
// Use of this software is governed by the CockroachDB Software License
// included in the /LICENSE file.

Any reason why you're not using the keys_test.go file? I think if the code is in X the tests should be in X_test.

pkg/server/apiutil/rangeutil_test.go line 76 at r4 (raw file):

			}
		}
	}

there might be some utils in the require package that can make these checks easier.x

angles-n-daemons

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @kyle-a-wong)

-- commits line 134 at r4: