sql: add new system table for locality locations

[spencerkimball]

Adds a new table, system.locations, which maps a string locality
(i.e. "city=New York City") to geographic coordinates, specified as
a latitude, longitude decimal pair.

A new admin API call returns the contents of the table, for use from
the admin UI.

Fixes #19532

[cockroach-teamcity]

This change is 

[couchand]

cc @cockroachdb/admin-ui

[danhhz]

Review status: 0 of 15 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/sqlbase/system.go, line 145 at r1 (raw file):

	// locations are used to map a locality specified by a node to geographic
	// latitude, longitude coordinates.

This should be specific about whether lat/lng is degrees or radians.

---

Comments from Reviewable

[couchand]

It was suggested that an RFC might be in order here, rather than just relying on a handful of comments in an issue. Looking through the documented criteria, this change does seem to hit at least a few of them. Perhaps it would be useful to go through the RFC process for this feature?

[danhhz]

I'm strongly in favor of an RFC for every new system table, but I didn't bring it up because I didn't want to get in the way of your prototyping. If this can spare the extra time an RFC will take, please do it

[petermattis]

@couchand writes:

It was suggested that an RFC might be in order here, rather than just relying on a handful of comments in an issue. Looking through the documented criteria, this change does seem to hit at least a few of them. Perhaps it would be useful to go through the RFC process for this feature?

@Danhz writes:

I'm strongly in favor of an RFC for every new system table, but I didn't bring it up because I didn't want to get in the way of your prototyping. If this can spare the extra time an RFC will take, please do it

What do we hope to get from an RFC that can't be discussed on the source issue or in this PR? While the RFC process is lightweight, going through it in this case will likely add at least a week to merging this PR, perhaps more. Is it worth it? Certainly if there is an expectation that the design will materially change or if there are alternatives that are worth exploring further. But the alternatives listed in the linked issue seem sufficiently explored.

That said, I have a design alternative worth exploring: should we store the geo-coordinates for a locality in specially prefixed keys of the system.ui table? Doing so would make storing the lat-long a bit more awkward, but it could remove the need for a new json endpoint.

Should we go to the RFC process to discuss my alternative? In this instance, given the size of the PR and the time it took to author (which I'm guestimating), I think discussing in the PR is appropriate.

[danhhz]

Speaking generally, I think the RFC process is appropriate for new system tables because backwards compatibility issues means that getting the schema wrong is more costly than it would be otherwise. Also, I only happened to find this PR via the daily emails, whereas the RFC process includes a post to the mailing list for FCP.

Speaking specifically about this PR, I didn't see any reference in the linked issue to whether this needs to represent hierarchies of localities or if there's any interplay between it and partitioning.

I said above that I don't want to get in the way of prototyping and I won't (we have months of alphas to get the schema right). But my opinion remains that the tradeoffs support this being cherry-picked in people's local branches while an RFC makes sure we do it right the first time.

[danhhz]

being cherry-picked in people's local branches while an RFC makes sure we do it right the first time

Oh, or merged, RFC'd, and then the implementation changed to match if the RFC process discovers something significant.

[spencerkimball]

None of the guidelines in the RFC readme suggest this change merits an RFC; that should be updated to explicitly include system tables if that is now a requirement. I notice that traditionally system tables were added without the benefit of an RFC. I tend to agree with the RFC process as currently documented: they're meant for "major" changes.

As you point out, adding this system table is weighty in the sense that if we get the schema wrong, and it stays wrong until March, we'll have to do a migration of some sort. But to me, that doesn't argue for an RFC over just discussing suggestions and objections in the issue for this case. This is a simple schema for a straightforward feature. I will keep this as a branch which people can cherry pick in the short term though.

[danhhz]

Correct, it is not a requirement. All 3 system tables added in the last year have been RFC'd though: system.jobs, system.settings, system.web_sessions.

Anyway, I was mostly floating the idea of making it required and it sounds like there isn't consensus for doing that, so carry on.

[petermattis]

I said above that I don't want to get in the way of prototyping and I won't (we have months of alphas to get the schema right). But my opinion remains that the tradeoffs support this being cherry-picked in people's local branches while an RFC makes sure we do it right the first time.

Perhaps a long-lived shared branch would be more appropriate. Or perhaps this can be merged to master but guarded by an env var or something like that so that creation of the system table has to be explicitly requested. I'd generally like to reduce barriers to prototyping while balancing against painting ourselves into a corner.

[bdarnell]

As you point out, adding this system table is weighty in the sense that if we get the schema wrong, and it stays wrong until March, we'll have to do a migration of some sort.

I think we should try to do migrations for anything that makes it to master, even if it never makes it to a non-alpha release. If we want the option to make changes without migration headaches, we should keep this on a branch until we've sorted out the schema.

[dianasaur323]

Overall though, this approach seems straightforward to me. It would be great to eventually see / test out what it would be like for a user to put in these metrics (what happens when they first put in localities, then decide to put in lat / long. how do we error check that)? But that's probably further down the road.

[spencerkimball]

@vilterp this is the branch I was referring to. It should have everything we need from the backend to support assigning coordinates to localities.

[spencerkimball]

@vilterp this was a real PITA to merge after a new system table was added. Best to get this reviewed and merged asap.

[vilterp]

this schema should be fine, and if it's not, adding another migration doesn't seem bad (esp. if we catch it before a major release)

---

Reviewed 18 of 18 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[couchand]

Well, it looks like this was merged while I was validating my question below. Hopefully we can figure out a fix for that which doesn't require too much work, but it seems like it might be a somewhat significant change.

---

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/sql/sqlbase/system.go, line 170 at r2 (raw file):

CREATE TABLE system.locations (
  "localityKey"   STRING,
  "localityValue" STRING,

I suspect that this isn't actually sufficient to uniquely identify localities. As far as I can tell there's nothing stopping a user from starting node in a cluster with, for instance, --locality=region=west,datacenter=1 on one node, and --locality=region=east,datacenter=1 on another. It seems we need to either validate that a given pair of locality key and value is globally unique when configuring nodes (probably a pretty hard problem), or make this schema keep the entire hierarchy up to that point.

---

Comments from Reviewable

[a-robinson]

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/sql/sqlbase/system.go, line 170 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

I suspect that this isn't actually sufficient to uniquely identify localities. As far as I can tell there's nothing stopping a user from starting node in a cluster with, for instance, --locality=region=west,datacenter=1 on one node, and --locality=region=east,datacenter=1 on another. It seems we need to either validate that a given pair of locality key and value is globally unique when configuring nodes (probably a pretty hard problem), or make this schema keep the entire hierarchy up to that point.

Without having read anything else related this PR, I can say yes, that is allowed.

---

Comments from Reviewable

[spencerkimball]

pkg/sql/sqlbase/system.go, line 170 at r2 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Without having read anything else related this PR, I can say yes, that is allowed.

Yes, and that is by design because specifying the full hierarchical locality for a location felt too heavyweight. The expectation here is that facilities will have different names, as they do seem to have in the platforms I've worked with in the past. I've never seen non-unique names for facilities, and they usually specify some aspect of the geography ("us-east-1a", "washburn", etc.)

There are also other ways that localities can be specified which make no sense as well. I think we’re going to need to build a verifier on the server side for localities and locations. At a minimum, we should verify that locality hierarchies for each node don't contradict each other, and that any locality which matches a location has the same locality hierarchy.

---

Comments from Reviewable

[vilterp]

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/sql/sqlbase/system.go, line 170 at r2 (raw file):

Previously, spencerkimball (Spencer Kimball) wrote…

Yes, and that is by design because specifying the full hierarchical locality for a location felt too heavyweight. The expectation here is that facilities will have different names, as they do seem to have in the platforms I've worked with in the past. I've never seen non-unique names for facilities, and they usually specify some aspect of the geography ("us-east-1a", "washburn", etc.)

There are also other ways that localities can be specified which make no sense as well. I think we’re going to need to build a verifier on the server side for localities and locations. At a minimum, we should verify that locality hierarchies for each node don't contradict each other, and that any locality which matches a location has the same locality hierarchy.

Yeah, I guess users kind of have to repeat themselves as they go down the hierarchy, e.g. country=US, datacenter=us-east, AZ=us-east-1a. Not great, but not terrible.

This is a question for a later PR, but where would the validator live? Would it be an endpoint which is called by the Admin UI (when we have user auth) and writes to the system table?

---

Comments from Reviewable

[couchand]

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/sql/sqlbase/system.go, line 170 at r2 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Yeah, I guess users kind of have to repeat themselves as they go down the hierarchy, e.g. country=US, datacenter=us-east, AZ=us-east-1a. Not great, but not terrible.

This is a question for a later PR, but where would the validator live? Would it be an endpoint which is called by the Admin UI (when we have user auth) and writes to the system table?

Ok, it sounds like this might be around for a bit, so I've opened #20653 so we can keep track of it.

---

Comments from Reviewable