Don't hate me. I tend to be pretty opposed to checking in tables without code that exercises them. I have a reasonably strong preference for interfaces and mock implementations of subsystems and then adding system tables as the implementation to those interfaces. I'm open to pushback in the other direction. I have a general fear that system tables checked in first can lead to the tables themselves being used as the point of collaboration rather than interfaces.

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

I'm ok with keeping this open and merging together with the next PR that utilizes it. But it's easier if we address any comments here rather than in a big PR.

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

I'm ok with keeping this open and merging together with the next PR that utilizes it. But it's easier if we address any comments here rather than in a big PR.

I guess that's sort of what I'm getting at; the only interesting thing in this commit is the table schema. It's hard to talk about the table without the queries.

The way I'd love to see it broken down is with the addition of some sort of storage interface that can be backed by some in-memory data structure and all of the integration pieces and then a separate PR that adds the table and implements the storage on top of it. That way the feedback stays isolated and it forces you into some modularity between the interfaces and the implementation.

Generally this works out pretty well when prototyping in that you just change a constructor in server somewhere to put up the mocked out integration.

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

All code that uses the table will be in a single package. I see no value in having a temporary in-memory implementation of that code just so we can check in the table later.

There are many ways to skin a cat, and there's cost to switching approaches after work has already been done in a certain direction.

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

I see no value in having a temporary in-memory implementation of that code just so we can check in the table later.

You don't anticipate wanting to test an implementation of the API that doesn't require talking to a running server with a working SQL table? I guess I'm sort of surprised to hear that. I'm not asking you to change directions, I just would have anticipated a general desire to hide the durability part behind some abstraction.

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

Long story short, feel free to move ahead here. This commit is entirely mechanical other than the table itself. The table seems fine given the RFC. I'll go read the RFC again now that it's had commentary.

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

I'd say most things in our system that use a system table are being tested against a real table.

This hasn't really been a good thing in my opinion. It tightly couples lots of things. It makes testing slow and it makes code not really reusable or decomposable. Honestly, I like being able to look at some go interfaces and say, ah, yes, this is what I'm getting out of these sql tables. Without an interface, my read is that you're saying that the token bucket logic is inextricable from the table. I don't think that's right.

Anyway, I'm done holding you up on this topic. Consider this all to be a gentle nudge for some modularity around anything that touches the internal executor.

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

There is an interface in the sense of an API, I think the contention here is whether that really needs to be an interface with multiple implementations. I have an aversion to unnecessary layers of conceptual indirection as it makes things more obtuse and the code is harder to navigate. Anyway, I'll think more on it.

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

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

pkg/migration/migrations/tenant_usage.go, line 26 at r2 (raw file):

) error {
	// Only create the table on the system tenant.
	if !d.Codec.ForSystemTenant() {

I was missing the ! here in an earlier iteration and I don't think any test failed.. How can I test this? I did look at existing tests in this package. Maybe the BinaryVersionOverride testing knob should be plumbed to addSystemDescriptorsToSchema?