spanconfig: introduce the spanconfig.SQLWatcher

[arulajmani]

This patch introduces a few more span config interfaces/structs, namely
the SQLReconciler, SQLWatcher, and an eventBuffer helper struct for the
SQLWatcher.

The SQLReconciler is responsible for reconciling SQL descriptors and
their zone configurations to span configurations. It simply constructs
the implied span configuration state and is agnostic to the actual span
configuration state in KV.

The SQLWatcher is intended to incrementally process zone configuration
updates in the SQL layer. It does so by establishing a rangefeed on
system.zones and system.descriptors from a given checkpoint. It buffers
events until the next checkpoint. The list of buffered descriptor IDs,
along with the new checkpoint, are passed to a handler callback.

The SQLWatcher uses an eventBuffer helper struct to buffer rangefeed
updates. This thing maintains the notion of a checkpoint timestamp,
which is minimum timestamp for a {zones, descriptor} event.

This flow doesn't quite work end to end yet because rangefeeds don't
expose checkpoint events yet. These interfaces aren't hooked up to
anything yet either.

References #67679

Release justification: non-production code changes
Release note: None

[cockroach-teamcity]

This change is 

[rytaft]

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 654 at r1 (raw file):

// for any that are computed and do not yet have values provided by the input
// expression. New columns are synthesized for any missing columns using the
// computed column expression	.

nit: stray whitespace

[arulajmani]

I think the general structure here is coming together and this is ready for some eyes. I have everything in a single commit right now but I can break apart the rangefeed setup and full reconciliation into separate commits if it'll make things easier to review.

I still need to add a configuration that runs the data driven tests for secondary tenants as some of the stuff around named zones is tenant specific.

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

[arulajmani]

Friendly ping on this one now that we're all back from breather week @irfansharif @ajwerner @nvanbenschoten

[irfansharif]

Will take a look today, catching up.

[ajwerner]

I don't have a clear vision on how checkpointing is going to fit in. I was sort of expecting the event API to involve timestamps and batches of updates in some form or another.

Reviewed 4 of 23 files at r1, 5 of 23 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @irfansharif, @nvanbenschoten, and @RichardJCai)

---

-- commits, line 10 at r3:
uber-nit: "if any at all"?

---

-- commits, line 13 at r3:
abiilty

---

.github/CODEOWNERS, line 96 at r3 (raw file):

/pkg/ccl/backupccl/          @cockroachdb/bulk-prs
/pkg/ccl/importccl/          @cockroachdb/bulk-prs
/pkg/ccl/spanconfigccl/      @cockroachdb/multiregion

🤔

---

pkg/ccl/spanconfigccl/testdata/databases, line 42 at r3 (raw file):

constraints: []
voterconstraints: []
leasepreferences: []

these might be easier to read if you omitted empty values.

---

pkg/ccl/spanconfigccl/testdata/full_reconciliation_named_zones_deleted, line 44 at r3 (raw file):

rangeminbytes: 134217728
rangemaxbytes: 536870912

you know what, what if you formatted it differently so it's like a list of configuration literals and then a mapping from spans to those literals. This here is too hard to visually differentiate.

---

pkg/spanconfig/spanconfig.go, line 37 at r3 (raw file):

// SQLWatcher emits SQL span configuration updates.
type SQLWatcher interface {

I think this would be better phrased as:

SQLWatcher emits span configs that might come from updates made to zone configurations at the SQL layer.

---

pkg/spanconfig/spanconfig.go, line 38 at r3 (raw file):

// SQLWatcher emits SQL span configuration updates.
type SQLWatcher interface {
	WatchForSQLUpdates(ctx context.Context) (<-chan Update, error)

This needs commentary. How do these two methods interact? (I imagine they don't). I'm surprised to not see timestamps anywhere on here. Please fill out the description of the semantics here.

---

pkg/spanconfig/spanconfig.go, line 61 at r3 (raw file):

	Entry roachpb.SpanConfigEntry
	// Deleted is true if the span config entry has been deleted.
	Deleted bool

It feels like this might be a bit cleaner as

type Update struct {
    Span    roachpb.Span
    Config  roachpb.SpancConfig
    Deleted bool
}

---

pkg/spanconfig/spanconfigsqlwatcher/sql_watcher.go, line 312 at r3 (raw file):

Quoted 10 lines of code…

	if err := sql.DescsTxn(
		ctx,
		s.execCfg,
		func(ctx context.Context, txn *kv.Txn, descsCol *descs.Collection) error {
			var err error
			updates, err = s.handleIDUpdate(ctx, id, txn, descsCol)
			return err
		}); err != nil {
		return err
	}

extreme nit: I've taken to wrapping these like:

	if err := sql.DescsTxn(ctx, s.execCfg, func(
		ctx context.Context, txn *kv.Txn, descsCol *descs.Collection,
	) (err error) {
 		updates, err = s.handleIDUpdate(ctx, id, txn, descsCol)
 		return err
 	}); err != nil {
		return err
 	}

however, while I'm here, this feels like the perfect seam for an interface boundary. I think that I would have done something like:

type Reconciler interface {
     Reconcile(context.Context, descpb.IDs) ([]spanconfig.Update, error)
}

That should make it much easier to test this logic, which is super stateful in terms of events and what not and that logic which is really a function of the database. Consider splitting these two areas apart.

---

pkg/sql/catalog/descs/collection.go, line 286 at r3 (raw file):

// the transaction inside the database referenced by the given database ID. It
// first checks the collections cached descriptors before defaulting to a key-value scan.
func (tc *Collection) GetAllTableDescriptorsInDatabase(

Note that it does not hydrate the descriptors. The alternative here is to fetch the IDs and then the descriptors. I was thinking just today that I wished namespace had type information.

[arulajmani]

@irfansharif I rebased this thing from the SQLTranslator PR like we spoke about earlier. I think there's still a few comments left in here that I might need to address and I need to switch out the event_buffer to use the more generalized library you merged in. I'll send out a new SQLWatcher PR with all of this done very soon.

Hopefully nothing major will change in the interface definitions here, so if you end up starting on the Reconciler before I send out the cleaned up SQLWatcher PR hopefully this rebased PR can help you for your SQLWatcher needs.

[irfansharif]

(backlog grooming) Superseded by #71968 (merged) + #71359 (merged) and #71994 (unmerged).