sql: add locality to system.sql_instances table #82915

rharding6373 · 2022-06-15T01:07:49Z

This PR adds the column locality to the system.sql_instances table
that contains the locality (e.g., region) of a SQL instance. The encoded
locality is a string representing the roachpb.Locality that may have
been provided when the instance was created.

This change also pipes the locality through InstanceInfo. This will
allow us to determine and use locality information of other SQL
instances, e.g. in DistSQL for multi-tenant locality-awareness
distribution planning.

Informs: #80678

Release note (sql change): Table system.sql_instances has a new
column, locality, that stores the locality of a SQL instance if it was
provided when the instance was started. This exposes a SQL instance's
locality to other instances in the cluster for query planning.

cockroach-teamcity · 2022-06-15T01:07:57Z

This change is

rharding6373

First commit is #82844

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

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.

rharding6373

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):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Looks like a merge conflict resolution in the wrong commit.

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):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: is the TODO still relevant?

No, thanks for the catch.

yuzefovich

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)

postamar

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.

rharding6373

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)

knz · 2022-06-23T17:19:51Z

What about a SQL string array []STRING?

cucaroach · 2022-06-23T20:22:41Z

pkg/clusterversion/cockroach_versions.go

@@ -381,6 +381,10 @@ const (
 	// Previously, SSTs containing these could error.
 	AddSSTableTombstones



nit: empty line not conventional maybe?

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.

rharding6373

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):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

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

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.

rharding6373 · 2022-06-29T19:43:51Z

bors r+

craig · 2022-06-29T21:36:08Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-06-30T01:36:23Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-06-30T03:54:19Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-06-30T05:32:16Z

Build failed:

GitHub CI (Cockroach)

rharding6373 · 2022-06-30T15:34:23Z

bors r-

This PR adds the column `locality` to the `system.sql_instances` table that contains the locality (e.g., region) of a SQL instance. The encoded locality is a JSONB representing the `roachpb.Locality` that may have been provided when the instance was created. This change also pipes the locality through `InstanceInfo`. This will allow us to determine and use locality information of other SQL instances, e.g. in DistSQL for multi-tenant locality-awareness distribution planning. Informs: cockroachdb#80678 Release note (sql change): Table `system.sql_instances` has a new column, `locality`, that stores the locality of a SQL instance if it was provided when the instance was started. This exposes a SQL instance's locality to other instances in the cluster for query planning.

rharding6373 · 2022-06-30T16:07:59Z

bors r+

craig · 2022-06-30T18:05:22Z

Build succeeded:

GitHub CI (Cockroach)

…flag set Historically, offline tables were excluded from backups (e.g. restoring or importing tables) because incremental backups could not reason correctly about their MVCC history. Because all within tenant operations are now MVCC, BACKUP can begin incrementally backing up offline tables. Independent of the MVCC history thread, cockroachdb#83915 allowed backup to implicitly target _all_ offline descriptors (e.g. database, schema, table, type) to account for a class of compound online schema changes. Note with this PR, the user can't _explicitly_ target an offline descriptor (e.g. BACKUP DATABASE my_offline_database). This patch reverts part of cockroachdb#82915 such that BACKUP targets offline tables iff it contains an in-progress import and when an external flag is set. Since the class of online schema changes described in cockroachdb#82915 will never occur in 22.2, there isn't actually a need to back up a larget set of offline tables. This patch merely refactors the internal targeting logic in backupresolver.ResolveTargetsToDescriptors, and does not change how backups behave. In other words, after this patch, no offline descriptors will get targeted, as all callers set the function's backupImports flag to false. A future PR will change these callsites. Informs cockroachdb#76722 Release note: none

rharding6373 added the do-not-merge bors won't merge a PR with this label. label Jun 15, 2022

rharding6373 force-pushed the distsql_tenant_sql_pod branch 7 times, most recently from ff94903 to 405dab2 Compare June 16, 2022 02:36

rharding6373 requested review from yuzefovich and cucaroach June 16, 2022 02:37

rharding6373 commented Jun 16, 2022

View reviewed changes

rharding6373 removed the do-not-merge bors won't merge a PR with this label. label Jun 16, 2022

rharding6373 marked this pull request as ready for review June 16, 2022 02:38

rharding6373 requested a review from a team June 16, 2022 02:38

rharding6373 requested a review from a team as a code owner June 16, 2022 02:38

yuzefovich reviewed Jun 16, 2022

View reviewed changes

rharding6373 force-pushed the distsql_tenant_sql_pod branch from 405dab2 to 3b93e7b Compare June 16, 2022 19:01

rharding6373 commented Jun 16, 2022

View reviewed changes

yuzefovich approved these changes Jun 21, 2022

View reviewed changes

rharding6373 force-pushed the distsql_tenant_sql_pod branch 4 times, most recently from 9e6d30c to 27142e9 Compare June 21, 2022 20:00

rharding6373 requested a review from postamar June 21, 2022 20:43

postamar approved these changes Jun 22, 2022

View reviewed changes

rharding6373 commented Jun 23, 2022

View reviewed changes

rharding6373 force-pushed the distsql_tenant_sql_pod branch from 27142e9 to 93ebccc Compare June 23, 2022 20:18

cucaroach reviewed Jun 23, 2022

View reviewed changes

yuzefovich reviewed Jun 27, 2022

View reviewed changes

rharding6373 force-pushed the distsql_tenant_sql_pod branch 3 times, most recently from 83d0eaa to 4fe1071 Compare June 29, 2022 17:44

rharding6373 commented Jun 29, 2022

View reviewed changes

rharding6373 force-pushed the distsql_tenant_sql_pod branch from 4fe1071 to 282a232 Compare June 29, 2022 19:43

rharding6373 force-pushed the distsql_tenant_sql_pod branch from 282a232 to f7bb118 Compare June 30, 2022 16:07

craig bot merged commit 05ca68a into cockroachdb:master Jun 30, 2022

cockroach-teamcity mentioned this pull request Jun 30, 2022

sql: add locality to system.sql_instances table cockroachdb/docs#14418

Open

msbutler mentioned this pull request Aug 11, 2022

backupccl: target offline importing tables if flag is set #85979

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add locality to system.sql_instances table #82915

sql: add locality to system.sql_instances table #82915

rharding6373 commented Jun 15, 2022 •

edited

Loading

cockroach-teamcity commented Jun 15, 2022

rharding6373 left a comment

yuzefovich left a comment

rharding6373 left a comment

yuzefovich left a comment

postamar left a comment

rharding6373 left a comment

knz commented Jun 23, 2022

cucaroach Jun 23, 2022

yuzefovich left a comment

rharding6373 left a comment

rharding6373 commented Jun 29, 2022

craig bot commented Jun 29, 2022

craig bot commented Jun 30, 2022

craig bot commented Jun 30, 2022

craig bot commented Jun 30, 2022

rharding6373 commented Jun 30, 2022

rharding6373 commented Jun 30, 2022

craig bot commented Jun 30, 2022

		@@ -381,6 +381,10 @@ const (
		// Previously, SSTs containing these could error.
		AddSSTableTombstones

sql: add locality to system.sql_instances table #82915

sql: add locality to system.sql_instances table #82915

Conversation

rharding6373 commented Jun 15, 2022 • edited Loading

cockroach-teamcity commented Jun 15, 2022

rharding6373 left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

postamar left a comment

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

knz commented Jun 23, 2022

cucaroach Jun 23, 2022

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

rharding6373 commented Jun 29, 2022

craig bot commented Jun 29, 2022

craig bot commented Jun 30, 2022

craig bot commented Jun 30, 2022

craig bot commented Jun 30, 2022

rharding6373 commented Jun 30, 2022

rharding6373 commented Jun 30, 2022

craig bot commented Jun 30, 2022

rharding6373 commented Jun 15, 2022 •

edited

Loading