server,roachpb: introduce span config rpcs

[irfansharif]

Part of #67679; these RPCs are exposed through the kvtenant.Connector
interface for tenants and also sit on pkg/server.(*Node) for the host
tenant. The basic type in these RPCs is the SpanConfig proto, which is
the same as our existing ZoneConfig proto but without any inheritance
business.

// GetSpanConfigsRequest is used to fetch the span configurations
// over the specified keyspans.
message GetSpanConfigsRequest {
  // Spans to request the configurations for. The spans listed here
  // are not allowed to overlap with one another.
  repeated Span spans = 1 [(gogoproto.nullable) = false];
};

// GetSpanConfigsResponse lists out the span configurations that
// overlap with the requested spans.
message GetSpanConfigsResponse {
  // SpanConfigEntries capture the span configurations over the
  // requested spans.  The results for each Span in the matching
  // GetSpanConfigsRequest are flattened out into a single slice. It's
  // possible for there to be no configurations for a given span;
  // there'll simply be no entries for it.
  repeated SpanConfigEntry span_config_entries = 1 [(gogoproto.nullable) = false];
};

// UpdateSpanConfigsRequest is used to update the span
// configurations over the given spans.
//
// This is a "targeted" API: the spans being deleted are expected to
// have been present with the same bounds (same start/end key); the
// same is true for spans being updated with new configs. If spans
// are being added, they're expected to not overlap with any
// existing spans. When divvying up an existing span into multiple
// others, callers are expected to delete the old and upsert the new
// ones. This can happen as part of the same request; we delete the
// spans marked for deletion before upserting whatever was
// requested.
//
// Spans are not allowed to overlap with other spans in the same
// list but can across lists. This is necessary to support the
// delete+upsert semantics described above.
message UpdateSpanConfigsRequest {
  // ToDelete lists out the spans we want to delete span configs
  // for.
  repeated Span to_delete = 1 [(gogoproto.nullable) = false];

  // ToUpsert lists out the spans we want to upsert and the configs
  // we want to upsert with.
  repeated SpanConfigEntry to_upsert = 2 [(gogoproto.nullable) = false];
};

The RPCs are backed by a new host-tenant only
system.span_configurations table. Future PRs will wire a view of this
table into KV with an eye towards replacing our use of
config.SystemConfig.

CREATE TABLE system.span_configurations (
    start_key    BYTES NOT NULL PRIMARY KEY,
    end_key      BYTES NOT NULL,
    config       BYTES NOT NULL,
    CONSTRAINT check_bounds CHECK (start_key < end_key),
    FAMILY "primary" (start_key, end_key, config)
)

---

While here, we also introduce a crdb_internal.pretty_span builtin to
help with the readability of this table. In future PRs we'll make use of
this built-in for datadriven tests asserting on the state of the table.

Release note (sql change): We've added a system.span_configurations
table. This will later be used to store authoritative span configs that
KV has decided to apply.

Release justification: non-production code changes

[cockroach-teamcity]

This change is 

[irfansharif]

Haven't pushed out the tests yet, but if you want an early look.

[irfansharif]

RFAL.

[ajwerner]

Here's a first pass.

Some of the schema design stuff is scary to me. It's really not okay for any of these queries to be O(span configs) and, it's not great to have so many synchronous round trips per entry in the batch.

My other concerns are around the API semantics. How do the two sets interact. Please spell this out in excruciating detail in the api proto.

Reviewed 6 of 37 files at r6, 90 of 90 files at r7, 34 of 34 files at r8, 31 of 31 files at r9, 2 of 39 files at r10.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, @irfansharif, and @nvanbenschoten)

---

pkg/ccl/kvccl/kvtenantccl/connector.go, line 533 at r10 (raw file):

// interface.
func (c *Connector) UpdateSpanConfigEntries(
	ctx context.Context, update []roachpb.SpanConfigEntry, delete []roachpb.Span,

nit: I don't like shadowing of the delete builtin function. Maybe toDelete and toUpdate? Also, my sense is the deletes conceptually precede the updates, can you put them first in the arg list?

---

pkg/config/zonepb/zone.go, line 1161 at r6 (raw file):

// fully hydrated. A fully hydrated zone configuration must have all required
// fields set, which are RangeMaxBytes, RangeMinBytes, GC, and NumReplicas.
func (z *ZoneConfig) EnsureFullyHydrated() error {

nit: you could accumulate all of the unset fields to make for a better error.nnn

---

pkg/config/zonepb/zone.go, line 1181 at r6 (raw file):

// SpanConfig. It fatals if the zone config hasn't been fully hydrated (fields
// are expected to have been cascaded through parent zone configs).
func (z *ZoneConfig) AsSpanConfig() roachpb.SpanConfig {

this is only used in testing, how about TestingAsSpanConfig?

---

pkg/migration/migrations/migrations.go, line 119 at r10 (raw file):

		retryJobsWithExponentialBackoff),
	migration.NewTenantMigration(
		"add the system.span_configurations table",

maybe add to system tenant?

---

pkg/roachpb/api.go, line 1633 at r10 (raw file):

// Unnest is a convenience method to return the slice-of-slices form of
// GetSpanConfigsResponse.

nit: Note here that this is not a copy of the underlying []SpanConfigEntry slices

---

pkg/roachpb/api.go, line 1635 at r10 (raw file):

// GetSpanConfigsResponse.
func (r *GetSpanConfigsResponse) Unnest() [][]SpanConfigEntry {
	var ret [][]SpanConfigEntry

nit: allocate this using make so it's the right size

---

pkg/roachpb/api.go, line 1668 at r10 (raw file):

		}

		for i := range list {

the runtime of this is unfortunate. Given these things really should not overlap, can we just make this an O(n log n) thing by using O(n) extra memory? At least leave a TODO

---

pkg/roachpb/api.go, line 1675 at r10 (raw file):

				if list[i].Overlaps(list[j]) {
					return errors.AssertionFailedf("overlapping spans %s and %s in same list",

I sort of envisioned that if you've got an existing span that you want to split then we'd delete the old one and upsert the new ones.

---

pkg/roachpb/api.proto, line 2452 at r10 (raw file):

  Value  content = 2 [(gogoproto.nullable) = false];
  // Which pattern does this gossip information match?
  string pattern_matched = 3; // If non-nil, the other fields will be empty and this will be the final event

Accidental removal of a newline?

---

pkg/roachpb/api.proto, line 2569 at r10 (raw file):

// SpanConfig holds the configuration that applies to a given keyspan. It
// parallels the definition found in zonepb/zone.proto.
message SpanConfig {

Can you move all of this to a new proto file in this package?

---

pkg/roachpb/api.proto, line 2661 at r10 (raw file):

  // SpanConfigsToUpdate lists out the spans we want to update and the configs
  // we want to update with.
  repeated SpanConfigEntry span_configs_to_update = 1 [(gogoproto.nullable) = false];

nit: I'd just call this to_update (or to_upsert assuming the semantics are what I think they are) as the type makes it clear what is being updated.

---

pkg/roachpb/api.proto, line 2661 at r10 (raw file):

  // SpanConfigsToUpdate lists out the spans we want to update and the configs
  // we want to update with.
  repeated SpanConfigEntry span_configs_to_update = 1 [(gogoproto.nullable) = false];

Is it an update or is it an insert? Maybe we should call this span_configs_to_upsert?

Can you describe the semantics of what happens in terms of overlap between entries in these slices and existing state in the store?

---

pkg/server/node.go, line 1487 at r10 (raw file):

				sessiondata.InternalExecutorOverride{User: security.RootUserName()},
				`SELECT start_key, end_key, config FROM system.span_configurations
			WHERE $1 < end_key AND start_key < $2`,

Can you look at this query plan? The query planner won't know anything about our non-overlapping invariant. It would be bad if this is a table scan.

Honestly, it's not good that this is so many round-trips anyway.

---

pkg/server/node.go, line 1577 at r10 (raw file):

			datums, err := n.sqlExec.QueryRowEx(ctx, "validate-span-cfgs", txn,
				sessiondata.InternalExecutorOverride{User: security.RootUserName()},
				`SELECT count(*) FROM system.span_configurations WHERE $1 < end_key AND start_key < $2`,

What's the query plan on this? This is going to be a table scan, no? That seems unfortunate and sort of not okay, it would mean that updating a zone configs is O(N^2) where N is the number of span configs in the entire cluster.

---

pkg/spanconfig/spanconfig.go, line 45 at r10 (raw file):

	// callers are to issue a delete for the previous span and updates for the
	// new ones.
	UpdateSpanConfigEntries(ctx context.Context, update []roachpb.SpanConfigEntry, delete []roachpb.Span) error

same nit on the name here, please don't call it delete

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, @irfansharif, and @nvanbenschoten)

---

pkg/server/node.go, line 1442 at r12 (raw file):

// GetSpanConfigEntriesFor implements the spanconfig.KVAccessor interface. It's a
// convenience wrapper around the RPC implementation.
func (n *Node) GetSpanConfigEntriesFor(

It feels more or less insane to me that this functionality is sitting right here directly on Node. Can you please implement these methods on an interface that Node constructs?

Another thing I'll say is that I've found that it's useful to allow that interface to take a table name or, alternatively, not specify a database when writing out the sql queries. Then, without starting up a new server, you can trivially test operations and not have to worry about using the real table.

Like, I'd expect a:

package spanconfig

type Reader interface {
   GetOverlapping(ctx context.Context, overlapping []roachpb.Span) ([][]roachpb.SpanConfigEntry, error)
}

type Writer interface {
   Update(/*...*/) /*(...)*/
}

type Storage interface {
    Reader
    Writer
}

// then something that takes a txn and gives you storage

---

pkg/sql/catalog/systemschema/system.go, line 538 at r12 (raw file):

CREATE TABLE system.span_configurations (
    start_key    BYTES NOT NULL PRIMARY KEY,
    end_key      BYTES NOT NULL,

How do you feel about CHECK end_key >= start_key?

[irfansharif]

Reviewable is down for so can't respond inline to comments, but addressed everything; PTAL. Thanks for helping shepherd this through -- the queries no longer have round trips == # of spans in request and all use constrained index scans. I suspect this KVAccessor interface could benefit with some form of randomized testing that I haven't typed out -- perhaps something we can get to during stability.

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[otan]

Five different merge conflicts in a single day, I should buy a lottery ticket.

i'm glad you managed to pass it on haha, i'm on 4 now!

[nvanbenschoten]

Reviewed 5 of 7 files at r21, 5 of 11 files at r22, 1 of 1 files at r24, 7 of 7 files at r25, 1 of 1 files at r26, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/roachpb/span_config.proto, line 19 at r20 (raw file):

Previously, irfansharif (irfan sharif) wrote…

I'll send a PR for this shortly, I tried appending it here but it got a bit hairy so I'd like to decouple it for now.

👍