sql: add ALTER TABLE/INDEX .. UNSPLIT AT ..

[jeffrey-xiao]

Now that manual splits add a sticky bit to the range descriptor, and
the merge queue respects this sticky bit, we can expose functionality to
manually unset this sticky bit.

If the key to unsplit is not the start of a range, then the unsplit
command will throw an error. If the range was manually split (I.E. the
sticky bit is set), then the sticky bit will be unset. Otherwise, this
command is a no-op.

Syntactically, the unsplit command is identical to the split command.

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

Reviewed 26 of 27 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jeffrey-xiao, and @nvanbenschoten)

---

pkg/sql/unsplit.go, line 115 at r1 (raw file):

	if err := params.extendedEvalCtx.ExecCfg.DB.AdminUnsplit(params.ctx, rowKey); err != nil {
		return false, err

Do we need to do any special error mapping here? Is it okay to just propagate the error directly? This isn't a question to which I know the answer.

---

pkg/sql/unsplit_test.go, line 96 at r1 (raw file):

		{
			in:    "ALTER TABLE d.t UNSPLIT AT VALUES (1, 'non-existent')",
			error: "unsplit key is not the start of a range",

This is the error case I'm worried about. I mean worst case is that it's just an XX00 SQL error but is that desirable?

[ajwerner]

Reviewed 1 of 27 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jeffrey-xiao and @nvanbenschoten)

[nvanbenschoten]

Reviewed 26 of 27 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jeffrey-xiao)

---

pkg/internal/client/db.go, line 484 at r1 (raw file):

// AdminUnsplit removes the sticky bit of the range specified by splitKey.
//
// spanKey is the start key of the range whose sticky bit should be removed.

Comment about what happens if the sticky bit doesn't exist. Also, talk about what happens if the provided key is not the start key of the range.

---

pkg/roachpb/api.proto, line 672 at r1 (raw file):

}

Extra line.

---

pkg/sql/unsplit.go, line 101 at r1 (raw file):

func (n *unsplitNode) startExec(params runParams) error {
	return nil

We'll need to check the cluster version to see if VersionStickyBit is active. If not, we'll want to reject the statement.

---

pkg/sql/unsplit.go, line 124 at r1 (raw file):

func (n *unsplitNode) Values() tree.Datums {
	return tree.Datums{

I'm curious whether you think this is what a user will be interested in as the result of an UNSPLIT AT statement. Should we return the split point that we removed? The new start of the range? A boolean saying whether the split point was removed or not? I haven't thought much into the desired UX here. Having written this all up and used it in a few tests, do you have any thoughts about this?

---

pkg/sql/unsplit_test.go, line 64 at r1 (raw file):

	}

	tests := []struct {

Nice tests!

There are a few other logic_tests that we'll want to add as well. To start, I think we'll want to add some basic tests to alter_table. Next, we'll want to expand on the crdb_internal tests that we added last time. Finally, the ranges test seems like an appropriate place to add some higher-level functional tests.

---

pkg/storage/client_merge_test.go, line 3434 at r1 (raw file):

		verifyUnmerged(t)

		// TODO(jeffreyxiao): Use same mechanism as ALTER TABLE ... UNSPLIT AT

Very satisfying.

---

pkg/storage/replica_command.go, line 384 at r1 (raw file):

// AdminUnsplit removes the sticky bit of the range specified by the
// splitKey

s/splitKey/args.Key/ right?

Also, period at the end.

---

pkg/storage/replica_command.go, line 385 at r1 (raw file):

// AdminUnsplit removes the sticky bit of the range specified by the
// splitKey
func (r *Replica) AdminUnsplit(

For now, this is probably fine, but at some point we'll probably need a retry loop like we have for AdminSplit.

---

pkg/storage/replica_command.go, line 403 at r1 (raw file):

	if err := r.store.DB().Txn(ctx, func(ctx context.Context, txn *client.Txn) error {
		b := txn.NewBatch()
		desc := *r.Desc()

We already have a desc in scope, so no need to shadow it.

---

pkg/storage/replica_command.go, line 419 at r1 (raw file):

			InternalCommitTrigger: &roachpb.InternalCommitTrigger{
				StickyBitTrigger: &roachpb.StickyBitTrigger{
					StickyBit: nil,

Leave a comment noting that this removes the sticky bit. It's always a little strange seeing a field set to nil in a constructor.

[jeffrey-xiao]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jeffrey-xiao, and @nvanbenschoten)

---

pkg/sql/unsplit.go, line 115 at r1 (raw file):

Previously, ajwerner wrote…

Do we need to do any special error mapping here? Is it okay to just propagate the error directly? This isn't a question to which I know the answer.

I've now wrapped it in a pgerror.CodeDataExceptionError.

---

pkg/sql/unsplit.go, line 124 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'm curious whether you think this is what a user will be interested in as the result of an UNSPLIT AT statement. Should we return the split point that we removed? The new start of the range? A boolean saying whether the split point was removed or not? I haven't thought much into the desired UX here. Having written this all up and used it in a few tests, do you have any thoughts about this?

I think returning the split point we removed makes sense and it's symmetric with that we return with an SPLIT AT statement (even in the no-op case the unsplit/split key is returned). The new start of the range is misleading because it's not guaranteed that unsplitting the range will result in the range being merged.

---

pkg/sql/unsplit_test.go, line 96 at r1 (raw file):

Previously, ajwerner wrote…

This is the error case I'm worried about. I mean worst case is that it's just an XX00 SQL error but is that desirable?

I've now wrapped it in a pgerror.CodeDataExceptionError.

---

pkg/storage/client_merge_test.go, line 3434 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Very satisfying.

:)

[nvanbenschoten]

Reviewed 1 of 2 files at r2, 7 of 7 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @jeffrey-xiao)

---

pkg/internal/client/db.go, line 486 at r3 (raw file):

// splitKey is the start key of the range whose sticky bit should be removed.
//
// If spanKey is not the start key of a range, then this method will throw an

"If splitKey"

---

pkg/sql/unsplit.go, line 124 at r1 (raw file):

Previously, jeffrey-xiao (Jeffrey Xiao) wrote…

I think returning the split point we removed makes sense and it's symmetric with that we return with an SPLIT AT statement (even in the no-op case the unsplit/split key is returned). The new start of the range is misleading because it's not guaranteed that unsplitting the range will result in the range being merged.

Ok, that all sounds reasonable.

---

pkg/storage/replica_command.go, line 392 at r3 (raw file):

	desc := *r.Desc()
	if !bytes.Equal(desc.StartKey.AsRawKey(), args.Header().Key) {
		return reply, roachpb.NewError(fmt.Errorf("key is not the start of a range"))

Why did this change?

---

pkg/storage/replica_command.go, line 425 at r3 (raw file):

		return txn.Run(ctx, b)
	}); err != nil {
		return reply, roachpb.NewError(errors.Wrapf(err, "unsplit at key %s failed", args.Key))

Does NewErrorf not work here?

[jeffrey-xiao]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jeffrey-xiao, and @nvanbenschoten)

---

pkg/storage/replica_command.go, line 392 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why did this change?

Originally, I wanted it not to print the file and line number, but since that's fine, I've changed it back.

---

pkg/storage/replica_command.go, line 425 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Does NewErrorf not work here?

I'm wrapping another error and it doesn't seem like roachpb has a Wrapf.

[ajwerner]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jeffrey-xiao and @nvanbenschoten)

---

pkg/storage/replica_command.go, line 425 at r3 (raw file):

Previously, jeffrey-xiao (Jeffrey Xiao) wrote…

I'm wrapping another error and it doesn't seem like roachpb has a Wrapf.

This roachpb.NewError is going to serialize the error to a string so the wrapping for use with Cause doesn't matter.

[jeffrey-xiao]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @jeffrey-xiao, and @nvanbenschoten)

---

pkg/storage/replica_command.go, line 425 at r3 (raw file):

Previously, ajwerner wrote…

This roachpb.NewError is going to serialize the error to a string so the wrapping for use with Cause doesn't matter.

I see, I've changed it to a NewErrorf.

[jeffrey-xiao]

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

---

pkg/sql/unsplit_test.go, line 64 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Nice tests!

There are a few other logic_tests that we'll want to add as well. To start, I think we'll want to add some basic tests to alter_table. Next, we'll want to expand on the crdb_internal tests that we added last time. Finally, the ranges test seems like an appropriate place to add some higher-level functional tests.

I can't add tests to ranges until manual_split_time is added to SHOW EXPERIMENTAL_RANGES, but I can do this in a follow-up PR.

---

pkg/storage/replica_command.go, line 385 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

For now, this is probably fine, but at some point we'll probably need a retry loop like we have for AdminSplit.

Left a TODO

[nvanbenschoten]

Reviewed 1 of 2 files at r5, 8 of 8 files at r6.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)

[jeffrey-xiao]

bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)