sql: add ALTER RANGE RELOCATE #72305

lunevalex · 2021-11-01T22:42:43Z

This commit introduces a new ALTER RANGE RELOCATE
command, which will allow an admin to move a lease
or replica for a specific range. Unlike ALTER TABLE RELOCATE
this command works on a range_id, which makes it a lot easier
to use since the user does not have to worry about range keys, which
are difficult to deal with in an emergncy situation.

Release note (sql change): Introduce new SQL syntax ALTER RANGE RELOCATE
to move a lease or replica between stores. This is helpful in an emergency
situation to relocate data in the cluster.

cockroach-teamcity · 2021-11-01T22:42:51Z

This change is

pkg/sql/parser/sql.y

lunevalex · 2021-11-18T21:05:54Z

@tbg @otan this is ready for review.

knz

Reviewed 1 of 12 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @lunevalex, @nvanbenschoten, @otan, and @tbg)

pkg/sql/parser/sql.y, line 1994 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

nit: indentation

specifically, use spaces not tabs

pkg/sql/parser/sql.y, line 1981 at r3 (raw file):

alter_range_relocate_lease_stmt:
  ALTER RANGE relocate_kw LEASE TO iconst64 FOR select_stmt

Can you either extend that iconst64 to support expressions and/or placeholders, or file a followup issue to do just this.

In particular the placeholders will be important for tests.

lunevalex

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @knz, @nvanbenschoten, @otan, and @tbg)

pkg/sql/parser/sql.y, line 844 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

you'll probably want to add some parsing tests in pkg/sql/parser/testdata/ (make test PKG=./pkg/sql/parser TESTS=TestParse TESTFLAGS='-rewrite' to rewrite)

Done.

pkg/sql/parser/sql.y, line 1981 at r3 (raw file):

Previously, knz (kena) wrote…

Can you either extend that iconst64 to support expressions and/or placeholders, or file a followup issue to do just this.

In particular the placeholders will be important for tests.

I will add a follow up once this lands.

tbg

LGTM from kv. I assume you'll be waiting for an LGTM on the syntax plumbing as well, since I am not a good reviewer for those.

tbg · 2021-11-19T16:45:53Z

pkg/kv/kvserver/client_lease_test.go

+			return errors.Errorf("Expected lease to transfer to node 4")
+		}
+		// Do this to avoid snapshot problems below when we do another replica move.
+		if repl != tc.GetRaftLeader(t, rhsDesc.StartKey) {


Wouldn't be surprised if this test ended up flaky, mind giving this a bit of extra love before merge? I know CI stresses it but sometimes not enough.

tbg · 2021-11-19T16:47:04Z

pkg/sql/opt/optbuilder/alter_range.go

+		panic(err)
+	}
+
+	b.DisableMemoReuse = true


I don't know what this is about, but it might be worth adding a comment.

lunevalex

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @knz, @lunevalex, @nvanbenschoten, @otan, and @tbg)

pkg/kv/kvserver/client_lease_test.go, line 984 at r4 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Wouldn't be surprised if this test ended up flaky, mind giving this a bit of extra love before merge? I know CI stresses it but sometimes not enough.

will do

pkg/sql/opt/optbuilder/alter_range.go, line 30 at r4 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I don't know what this is about, but it might be worth adding a comment.

ack

arulajmani

This is pretty cool! 💯

Only a few nits from me, otherwise

Reviewed 4 of 29 files at r1, 18 of 30 files at r2, 11 of 12 files at r3, 2 of 2 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @knz, @lunevalex, @nvanbenschoten, @otan, and @tbg)

pkg/sql/relocate_range.go, line 63 at r5 (raw file):

	}
	if !n.relocateLease {
		if n.fromStoreID <= 0 {

nit: you could collapse these two conditionals into a single one

pkg/sql/relocate_range.go, line 82 at r5 (raw file):

	return nil
}
func (n *relocateRange) Next(params runParams) (bool, error) {

nit: new line above

pkg/sql/relocate_range.go, line 130 at r5 (raw file):

func relocate(params runParams, req relocateRequest) (*roachpb.RangeDescriptor, error) {
	rangeDesc, err := lookupRangeDescriptorByRangeID(params.ctx, params.extendedEvalCtx.ExecCfg.DB, req.rangeID)

nit: wrap line at 100 chars (this and a couple more below)

pkg/sql/relocate_range.go, line 136 at r5 (raw file):

	if req.relocateLease {
		if err := params.p.ExecCfg().DB.AdminTransferLease(params.ctx, rangeDesc.StartKey, req.toStoreDesc.StoreID); err != nil {

nit: We could simply return without checking if the error is nil here.

pkg/sql/relocate_range.go, line 139 at r5 (raw file):

			return rangeDesc, err
		}
	} else {

nit: If you accept the suggestion above you could inline everything in this else block and buy a bit more indentation.

pkg/sql/relocate_range.go, line 180 at r5 (raw file):

					}
					// In small enough clusters it's possible for the same range
					// descriptor to be stored in both meta1 and meta2. This

This is probably because I'm unfamiliar with the area, but could you explain the effect of a range descriptor being stored in both meta1 and meta2 for my edification?

lunevalex · 2021-11-22T21:29:00Z

pkg/sql/relocate_range.go, line 180 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

This is probably because I'm unfamiliar with the area, but could you explain the effect of a range descriptor being stored in both meta1 and meta2 for my edification?

This has to do with

cockroach/pkg/kv/kvserver/addressing.go

Line 92 in 35109ac

// 3a. the range spans meta2 and user keys, update the meta1

. We store the same range descriptor in meta1 and meta2 in cases where the range spans the meta and data keyspaces so that a meta1 lookup is always sufficient to find all meta2 entries and a meta2 lookup is always sufficient to find all data entries. As pointed out by Nathan on an internal chat thread.

lunevalex · 2021-11-23T14:40:23Z

TFTR!
bors r=tbg,arulajmani

craig · 2021-11-23T15:08:22Z

Build failed:

GitHub CI (Cockroach)

arulajmani

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, @knz, @lunevalex, @nvanbenschoten, @otan, and @tbg)

pkg/sql/relocate_range.go, line 180 at r5 (raw file):

Previously, lunevalex wrote…

This has to do with

cockroach/pkg/kv/kvserver/addressing.go

Line 92 in 35109ac

// 3a. the range spans meta2 and user keys, update the meta1

. We store the same range descriptor in meta1 and meta2 in cases where the range spans the meta and data keyspaces so that a meta1 lookup is always sufficient to find all meta2 entries and a meta2 lookup is always sufficient to find all data entries. As pointed out by Nathan on an internal chat thread.

Looks like we're iterating over both meta1 and meta2 here, and returning the first range descriptor entry that we find. Maybe we should add a bit to this comment to say that even if two copies of the range descriptor are present, we don't care, and simply return the first one we find?

lunevalex · 2021-11-23T17:52:45Z

bors r+

craig · 2021-11-23T18:25:37Z

Build failed:

GitHub CI (Cockroach)

lunevalex

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, @knz, @lunevalex, @nvanbenschoten, @otan, and @tbg)

pkg/sql/relocate_range.go, line 180 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Looks like we're iterating over both meta1 and meta2 here, and returning the first range descriptor entry that we find. Maybe we should add a bit to this comment to say that even if two copies of the range descriptor are present, we don't care, and simply return the first one we find?

sure

Fixes cockroachdb#54971 This commit introduces a new ALTER RANGE RELOCATE command, which will allow an admin to move a lease or replica for a specific range. Unlike ALTER TABLE RELOCATE this command works on a range_id, which makes it a lot easier to use since the user does not have to worry about range keys, which are difficult to deal with in an emergncy situation. Release note (sql change): Introduce new SQL syntax ALTER RANGE RELOCATE to move a lease or replica between stores. This is helpful in an emergency situation to relocate data in the cluster.

lunevalex · 2021-11-23T20:04:23Z

bors r+

craig · 2021-11-23T21:55:43Z

Build succeeded:

GitHub CI (Cockroach)

lunevalex · 2021-12-14T14:53:01Z

blathers backport 21.2

blathers-crl · 2021-12-14T14:53:08Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 5487f8b to blathers/backport-release-21.2-72305: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2 failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

lunevalex requested review from ajwerner, tbg and nvanbenschoten November 1, 2021 22:42

otan reviewed Nov 10, 2021

View reviewed changes

pkg/sql/parser/sql.y Outdated Show resolved Hide resolved

pkg/sql/parser/sql.y Outdated Show resolved Hide resolved

pkg/sql/parser/sql.y Show resolved Hide resolved

lunevalex force-pushed the alex/relocate branch from 9843c0d to eb15f7f Compare November 18, 2021 21:04

blathers-crl bot requested a review from otan November 18, 2021 21:04

lunevalex marked this pull request as ready for review November 18, 2021 21:05

lunevalex requested a review from a team as a code owner November 18, 2021 21:05

lunevalex requested a review from a team November 18, 2021 21:05

lunevalex requested a review from a team as a code owner November 18, 2021 21:05

postamar removed the request for review from a team November 18, 2021 21:48

lunevalex force-pushed the alex/relocate branch 3 times, most recently from 68ff36a to 5ee93af Compare November 18, 2021 23:03

knz reviewed Nov 19, 2021

View reviewed changes

lunevalex commented Nov 19, 2021

View reviewed changes

lunevalex force-pushed the alex/relocate branch from 5ee93af to 9b73251 Compare November 19, 2021 15:15

tbg approved these changes Nov 19, 2021

View reviewed changes

lunevalex commented Nov 19, 2021

View reviewed changes

lunevalex force-pushed the alex/relocate branch from 9b73251 to 849ae3b Compare November 19, 2021 17:47

lunevalex requested a review from a team November 19, 2021 18:04

arulajmani self-requested a review November 19, 2021 18:45

lunevalex mentioned this pull request Nov 19, 2021

debug: add capability to "force" a load-based split #55116

Closed

arulajmani reviewed Nov 19, 2021

View reviewed changes

lunevalex force-pushed the alex/relocate branch from 849ae3b to 81704da Compare November 22, 2021 19:48

otan removed their request for review November 22, 2021 20:08

lunevalex force-pushed the alex/relocate branch from 81704da to da69e5f Compare November 22, 2021 20:43

blathers-crl bot requested a review from otan November 22, 2021 20:43

lunevalex force-pushed the alex/relocate branch from da69e5f to 494c520 Compare November 22, 2021 21:58

arulajmani approved these changes Nov 23, 2021

View reviewed changes

lunevalex commented Nov 23, 2021

View reviewed changes

lunevalex force-pushed the alex/relocate branch from 494c520 to 5487f8b Compare November 23, 2021 18:30

craig bot merged commit a965191 into cockroachdb:master Nov 23, 2021

cockroach-teamcity mentioned this pull request Nov 23, 2021

sql: add ALTER RANGE RELOCATE cockroachdb/docs#12453

Closed

lunevalex deleted the alex/relocate branch November 29, 2021 15:06

lunevalex mentioned this pull request Nov 30, 2021

kv: add placeholders to ALTER RANGE RELOCATE #73315

Closed

ajstorm mentioned this pull request May 25, 2022

kv: Dead end link for ALTER RANGE RELOCATE cockroachdb/docs#13949

Open

exalate-issue-sync bot mentioned this pull request Jun 8, 2022

sql: add ALTER RANGE RELOCATE cockroachdb/docs#14161

Closed

lunevalex mentioned this pull request Jun 13, 2022

release-21.2: sql: add ALTER RANGE RELOCATE #82863

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add ALTER RANGE RELOCATE #72305

sql: add ALTER RANGE RELOCATE #72305

lunevalex commented Nov 1, 2021

cockroach-teamcity commented Nov 1, 2021

lunevalex commented Nov 18, 2021

knz left a comment

lunevalex left a comment

tbg left a comment

tbg Nov 19, 2021

tbg Nov 19, 2021

lunevalex left a comment

arulajmani left a comment

lunevalex commented Nov 22, 2021

lunevalex commented Nov 23, 2021

craig bot commented Nov 23, 2021

arulajmani left a comment

lunevalex commented Nov 23, 2021

craig bot commented Nov 23, 2021

lunevalex left a comment

lunevalex commented Nov 23, 2021

craig bot commented Nov 23, 2021

lunevalex commented Dec 14, 2021

blathers-crl bot commented Dec 14, 2021

sql: add ALTER RANGE RELOCATE #72305

sql: add ALTER RANGE RELOCATE #72305

Conversation

lunevalex commented Nov 1, 2021

cockroach-teamcity commented Nov 1, 2021

lunevalex commented Nov 18, 2021

knz left a comment

Choose a reason for hiding this comment

lunevalex left a comment

Choose a reason for hiding this comment

tbg left a comment

Choose a reason for hiding this comment

tbg Nov 19, 2021

Choose a reason for hiding this comment

tbg Nov 19, 2021

Choose a reason for hiding this comment

lunevalex left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

lunevalex commented Nov 22, 2021

lunevalex commented Nov 23, 2021

craig bot commented Nov 23, 2021

arulajmani left a comment

Choose a reason for hiding this comment

lunevalex commented Nov 23, 2021

craig bot commented Nov 23, 2021

lunevalex left a comment

Choose a reason for hiding this comment

lunevalex commented Nov 23, 2021

craig bot commented Nov 23, 2021

lunevalex commented Dec 14, 2021

blathers-crl bot commented Dec 14, 2021