sql: add crdb_internal.validate_multi_region_zone_configs builtin

[otan]

In addition to the release note, added some pgcodes to validation errors
as well as minor refactoring to make it work and error nicely with the
"overriding" error message.

Release note (sql change): Add a new builtin
crdb_internal.validate_multi_region_zone_configs which validates all
zone configurations in the current database is correctly configured.

[cockroach-teamcity]

This change is 

[ajstorm]

Nit: In release note - "which validates all zone configurations in the current database are correctly configured"

Reviewed 5 of 8 files at r1, 3 of 3 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @miretskiy and @otan)

---

docs/generated/sql/functions.md, line 881 at r2 (raw file):

table’s zone configuration this will return NULL.</p>
</span></td></tr>
<tr><td><a name="crdb_internal.validate_multi_region_zone_configs"></a><code>crdb_internal.validate_multi_region_zone_configs() &rarr; <a href="bool.html">bool</a></code></td><td><span class="funcdesc"><p>Validates all multi-region zone configurations are correctly set

Nit: I believe this should be "setup", not "set up".

---

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 42 at r2 (raw file):

statement ok
SELECT crdb_internal.validate_multi_region_zone_configs()

FYI: Longer term, does this mean that we should be removing the calls to SHOW ZONE CONFIGURATION in these tests? Or do you think that there's still value in having them so that the tests are more visually self-describing?

---

pkg/sql/region_util.go, line 834 at r2 (raw file):

}

// ValidateCurrentDatabaseMultiRegionZoneConfigs is part of the tree.EvalDatabase interface.

Nit: maybe end this line with a comma?

---

pkg/sql/region_util.go, line 835 at r2 (raw file):

// ValidateCurrentDatabaseMultiRegionZoneConfigs is part of the tree.EvalDatabase interface.
// and as such is intended for DML use. It returns an empty DatabaseRegionConfig

Nit: I think this should read "It returns nil..."

---

pkg/sql/region_util.go, line 837 at r2 (raw file):

// and as such is intended for DML use. It returns an empty DatabaseRegionConfig
// if the current database is not multi-region enabled.
func (p *planner) ValidateCurrentDatabaseMultiRegionZoneConfigs(ctx context.Context) error {

Nit: Should there be a mention somewhere in the comment above that this also validates tables? Alternatively, should the function name be changed to ValidateMultRegionZoneConfigsForCurrentDatabaseAndTables?

---

pkg/sql/region_util.go, line 842 at r2 (raw file):

		p.txn,
		p.CurrentDatabase(),
		tree.DatabaseLookupFlags{Required: true},

Nit: AvoidCached? IncludeOffline?

---

pkg/sql/region_util.go, line 1172 at r2 (raw file):

					// the ID.
					indexName := fmt.Sprintf("unknown with ID = %s",
						strconv.FormatUint(uint64(s.IndexID), 10))

I have to admit, I have not idea why I wrote it this way the first time 🤦

---

pkg/sql/region_util.go, line 1185 at r2 (raw file):

// validateZoneConfigForMultiRegionDatabase validates that the zone config
//for the databases matches the exact expectations as the multi-region

Nit: space at beginning of row.

Nit: I think we can probably remove "the exact expectations as the", as it's somewhat implied.

---

pkg/sql/region_util.go, line 1212 at r2 (raw file):

	if !same {
		dbName := tree.Name(dbDesc.GetName())
		return validateZoneConfigForMultiRegionErrorHandler.newError(

Nice. I like this encapsulation of the error message.

---

pkg/sql/region_util.go, line 1300 at r2 (raw file):

					return validateZoneConfigForMultiRegionErrorHandler.newError(
						"index",
						fmt.Sprintf("%s@%s", tableName.String(), indexName),

Nit: Since we're using tableName.String() down here already, might want to use indexTreeName.String() so that we don't need indexName at all. If you do this, might want to change indexTreeName to indexName.

---

pkg/sql/sem/builtins/builtins.go, line 5075 at r2 (raw file):

					evalCtx.Context,
				); err != nil {
					return nil, err

Should this return false here?

[otan]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @miretskiy)

---

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs, line 42 at r2 (raw file):

Previously, ajstorm (Adam Storm) wrote…

FYI: Longer term, does this mean that we should be removing the calls to SHOW ZONE CONFIGURATION in these tests? Or do you think that there's still value in having them so that the tests are more visually self-describing?

i reckon there's value in having both.

---

pkg/sql/region_util.go, line 1172 at r2 (raw file):

Previously, ajstorm (Adam Storm) wrote…

I have to admit, I have not idea why I wrote it this way the first time 🤦

hehe

---

pkg/sql/region_util.go, line 1300 at r2 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: Since we're using tableName.String() down here already, might want to use indexTreeName.String() so that we don't need indexName at all. If you do this, might want to change indexTreeName to indexName.

i can't do tree.Name(name).String() in one pop because Name has a pointer receiver, so this has to be as it is.

---

pkg/sql/sem/builtins/builtins.go, line 5075 at r2 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Should this return false here?

the error will supercede all else :P

[arulajmani]

For all this validation we seem to be either reading descriptors from the store or using the DDL variants when constructing zone configs -- is this what we want for a builtin? I think it should be fine to used leased descriptors for the builtin, right?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @miretskiy, and @otan)

---

pkg/sql/region_util.go, line 842 at r2 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: AvoidCached? IncludeOffline?

I don't fully understand, why do we need these flags here?

---

pkg/sql/region_util.go, line 888 at r3 (raw file):

		tree.DatabaseLookupFlags{
			Required:       true,
			AvoidCached:    true,

Are you sure you want AvoidCached here? This method is called from the DefaultToDatabasePrimaryRegionBuiltinName, which means any insert into a Regional By Row table without an explicit value for crdb_region. A roundtrip here wouldn't fly, will it?

[otan]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, and @miretskiy)

---

pkg/sql/region_util.go, line 842 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I don't fully understand, why do we need these flags here?

Thinking about it more, yeah I don't think we do. We don't mind using the lease in the case, and no offline job code goes through this logic (yet).

---

pkg/sql/region_util.go, line 888 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Are you sure you want AvoidCached here? This method is called from the DefaultToDatabasePrimaryRegionBuiltinName, which means any insert into a Regional By Row table without an explicit value for crdb_region. A roundtrip here wouldn't fly, will it?

oops, good catch, didn't intend this to be here.

[arulajmani]

I think @ajstorm already approved this, but to me too after your changes

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, and @miretskiy)

[otan]

bors r=ajstorm,arulajmani

[craig]

Build failed:

• GitHub CI (Cockroach)

[otan]

bors r=ajstorm,arulajmani

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[ajstorm]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/region_util.go, line 888 at r3 (raw file):

Previously, otan (Oliver Tan) wrote…

oops, good catch, didn't intend this to be here.

Good catch @arulajmani!