sql: display locality in SHOW TABLES

[otan]

Also reduced the number of joins required on the comments table whilst
I was cleaning around it.

Release note (sql change): crdb_internal.tables and SHOW TABLES now
shows locality data on the tables.

[cockroach-teamcity]

This change is 

[ajstorm]

Clever join reduction!

Just one comment on handling NULLs in the locality column.

Reviewed 25 of 25 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

---

pkg/sql/crdb_internal.go, line 333 at r1 (raw file):

				}
				locality := tree.DNull
				if c := table.TableDesc().LocalityConfig; c != nil {

I think there's a problem here with us showing NULL if the LocalityConfig is nil. All tables with no specified locality will end up showing NULL but these tables have an implicit locality provided that the database is multi-region (they're REGIONAL BY TABLE IN PRIMARY REGION). Can we put in REGIONAL BY TABLE IN PRIMARY REGION if there is no LocalityConfig and the database is a multi-region database?

I'm a bit torn on whether or not to do it here, or in SHOW TABLES, but I'm leaning towards doing it here so that it can be leveraged in all cases where crdb_internal is queried.

[otan]

---

pkg/sql/crdb_internal.go, line 333 at r1 (raw file):

Previously, ajstorm wrote…

I think there's a problem here with us showing NULL if the LocalityConfig is nil. All tables with no specified locality will end up showing NULL but these tables have an implicit locality provided that the database is multi-region (they're REGIONAL BY TABLE IN PRIMARY REGION). Can we put in REGIONAL BY TABLE IN PRIMARY REGION if there is no LocalityConfig and the database is a multi-region database?

I'm a bit torn on whether or not to do it here, or in SHOW TABLES, but I'm leaning towards doing it here so that it can be leveraged in all cases where crdb_internal is queried.

It would be global, no?

[otan]

---

pkg/sql/crdb_internal.go, line 333 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

It would be global, no?

This should be an error if we always set the table to this value on first region add.

If we're choosing the still have NULL route if not implicitly set, I think we should still different here somehow that it is implicit. Any ideas?

[otan]

---

pkg/sql/crdb_internal.go, line 333 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

This should be an error if we always set the table to this value on first region add.

If we're choosing the still have NULL route if not implicitly set, I think we should still different here somehow that it is implicit. Any ideas?

As discussed offline, we're happy migrating all tables to be a separate locality when adding the first region since it should be a straightforward transaction across all table descriptors. I've added a TODO on create_table.go to set this by default.

[ajstorm]

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

---

pkg/sql/crdb_internal.go, line 333 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

As discussed offline, we're happy migrating all tables to be a separate locality when adding the first region since it should be a straightforward transaction across all table descriptors. I've added a TODO on create_table.go to set this by default.

Let's push this in for now and circle back on the right thing to do at the table level. I'm still not 100% sold on a separate locality, but that debate is not worth holding this PR up.

[otan]

sg! thanks!

bors r=ajstorm

[craig]

Build succeeded:

• GitHub CI (Cockroach)