First commit is #82844

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)

Reviewed 24 of 25 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @rharding6373)

pkg/sql/logictest/logic.go line 2250 at r3 (raw file):

		// Enumerate all the blocked configs if the blocked config is a default
		// config list.
		<<<<<<< HEAD

Looks like a merge conflict resolution in the wrong commit.

pkg/sql/sqlinstance/instancestorage/instancestorage.go line 72 at r4 (raw file):

// CreateInstance allocates a unique instance identifier for the SQL pod and
// associates it with its SQL address and session information.
// TODO(harding): add locality info here? This seems to be where the system.sql_instances

nit: is the TODO still relevant?

pkg/upgrade/upgrades/alter_sql_instances_locality.go line 1 at r4 (raw file):

// Copyright 2021 The Cockroach Authors.

nit: s/2021/2022/.

pkg/sql/sqlinstance/instancestorage/instancereader_test.go line 100 at r4 (raw file):

				}
				if !locality.Equals(instanceInfo.Locality) {
					return errors.Newf("expected instance locality %s != actual instance locality %s", locality.String(), instanceInfo.Locality.String())

nit: I think there is no need to call String() when using %s format directive.

pkg/upgrade/upgrades/alter_sql_instances_locality_test.go line 1 at r4 (raw file):

// Copyright 2021 The Cockroach Authors.

nit: ditto

pkg/upgrade/upgrades/alter_sql_instances_locality_test.go line 68 at r4 (raw file):

	// Inject the old copy of the descriptor.
	upgrades.InjectLegacyTable(ctx, t, s, systemschema.SQLInstancesTable, getDeprecatedSqlInstancesDescriptor)
	// Validate that the table statistics table has the old schema.

nit: comment needs an update.

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)

pkg/sql/logictest/logic.go line 2250 at r3 (raw file):

Ah, thought I fixed this botched merge, but I guess I only fixed it in the last commit. Looks like it's resolved now.

pkg/sql/sqlinstance/instancestorage/instancestorage.go line 72 at r4 (raw file):

No, thanks for the catch.

Reviewed 26 of 29 files at r1, 1 of 25 files at r4, 24 of 24 files at r5, 24 of 24 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach)

I realize I was added as a reviewer mainly to vet the upgrade logic (which LGTM!), though I was a bit surprised at the use of a STRING column type and a corresponding pseudo-CSV string encoding for roachpb.Locality, instead of JSONB, or perhaps BYTES containing the proto binary encoding. I'm guessing that that choice is deliberate & it's not bad per se in my view; I just want to raise awareness that this encoding choice effectively prevents adding further levels of nesting to this roachpb.Locality type. It also forces the use of regexp functions should we want to extract a specific key in a SQL query.

I have no idea whether this is a valid concern or not since I don't know how this type is effectively used. Feel free to disregard.

That's a good point, I didn't consider the long-term implications of using a string type when I was pushing this proto around. However, json doesn't preserve order which is a required property for roachpb.Locality. We seem to use our own json library, so maybe we can make an ordered version, but I'm not sure that would work well, either.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @cucaroach and @yuzefovich)

Reviewed 1 of 38 files at r7, 4 of 6 files at r8, 12 of 12 files at r9, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rharding6373 and @yuzefovich)

pkg/sql/sqlinstance/instancestorage/row_codec.go line 134 at r9 (raw file):

		sessionID = sqlliveness.SessionID(tree.MustBeDBytes(sessionIDVal))
	}
	locality = roachpb.Locality{}

nit: this line seems unnecessary.

pkg/sql/sqlinstance/instancestorage/row_codec.go line 139 at r9 (raw file):

		v, err := localityJ.FetchValKey("Tiers")
		if err != nil {
			return instanceID, "", "", roachpb.Locality{}, hlc.Timestamp{}, false, err

nit: maybe add some context to this error (like "failed to find Tiers attribute in Locality JSON") ?

pkg/util/json/json.go line 394 at r9 (raw file):

// BuildUnsorted creates a JSON object that uniqueifies the keys, but leaves
// the keys in their original order instead of sorting them by key.
func (b *ObjectBuilder) BuildUnsorted() JSON {

Is this used anywhere? I can't seem to find references.

TFTR! I think I finally fixed all the tests, but I'm going to make sure the tests are green before merging.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @cucaroach and @yuzefovich)

pkg/util/json/json.go line 394 at r9 (raw file):

Thanks for catching this. It was part of an initial draft to try to get JSON to work for roachpb.locality, but I forgot to remove it.