sql: add SPLIT AT

[maddyblue]

This enables easy splitting of the KV space by specifying a table and
values of its primary index.

Fixes #8860

---

This change is

[maddyblue]

Does this need some code that ensures SPLIT is only being run outside a BEGIN?

[knz]

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

sql/split.go, line 52 [r1] (raw file):

          return nil, err
      }
      d, err := te.Eval(&p.evalCtx)

You need to check for aggregate functions here. Use AggregateInExpr().

If you arbitrarily decide to not support sub-queries nor placeholders for prepare/execute, then you must reject them explicitly in the code (and confirm via a test that they are rejected properly. Right now I fear your code would cause a panic). For this, use the same logic as in MakeColumnDefDescs (sqlbase/table.go) calling SanitizeVarFreeExpr().

Or you could decide to support them, and move the Eval call to a new splitNode's Start() method, add a startSubQueries() somewhere and populate expandPlan() accordingly.

---

sql/split.go, line 68 [r1] (raw file):

  }
  key = keys.MakeRowSentinelKey(key)
  err = p.execCfg.DB.AdminSplit(key)

This AdminSplit() call hould go to Start() and not stay in the constructor.

---

Comments from Reviewable

[knz]

I don't think it's an issue if you run a split inside a transaction.

---

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[knz]

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

sql/split_at_test.go, line 44 [r1] (raw file):

  // Without this, this test regularly fails with: "conflict updating range
  // descriptors".
  time.Sleep(time.Millisecond * 100)

Woah no that's the short way to a flaky test. I'm not sure what the better way is though.

---

sql/split_at_test.go, line 85 [r1] (raw file):

          }
      }
  }

There needs to be more logic in here to actually validate the split is taking place.

---

Comments from Reviewable

[petermattis]

Nice. Should this be an ALTER TABLE variant? For example:

ALTER TABLE foo SPLIT AT a, b, c

This could also be extended to splitting an index:

ALTER TABLE foo@index SPLIT AT a, b, c

[bdarnell]

Nice, that was fast!

That should be ALTER INDEX index SPLIT AT ..., right? Don't we always have a non-@ syntax for index operations except for the hints to the query planner?

I kind of like the ALTER TABLE formulation; it generalizes nicely to things like ALTER TABLE foo DROP SPLIT AT a, b, c. (Dropping a split is different from a merge: manually requested splits should be marked specially in the KV layer so the system will not automatically merge across a split boundary that was created manually; dropping the split would clear the "manual" flag on the split). But the grammar for ALTER TABLE is already unwieldy and if SPLIT has different properties (like transaction restrictions) then it might get tricky.

The values should be parenthesized (similar to the INSERT statement) so that several split points can be created at once (at least syntactically, even if we don't support this at first).

I'm fine with the fact that the split occurs outside of any current transaction. An alternate implementation would be to write the proposed split point into a system table somewhere and have the split queue consult this table (similar to what it does for splitting at table boundaries today). However, at least for our current usage splitting synchronously is better.

What permissions are required for SPLIT AT?

---

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

sql/split_at_test.go, line 85 [r1] (raw file):

Previously, knz (kena) wrote…

There needs to be more logic in here to actually validate the split is taking place.

How about a `SHOW SPLITS` command (or something like that)?

---

Comments from Reviewable

[RaduBerinde]

Nice, this will be very useful for writing tests!

---

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

sql/split_at_test.go, line 35 [r1] (raw file):

  defer s.Stopper().Stop()

  if _, err := db.Exec(`

can use sqlrunner to make these shorter, e.g.

cockroach/sql/distsql/server_test.go

Line 43 in 5498634

r := sqlutils.MakeSQLRunner(t, sqlDB)

---

sql/split_at_test.go, line 44 [r1] (raw file):

Previously, knz (kena) wrote…

Woah no that's the short way to a flaky test. I'm not sure what the better way is though.

I agree, things can occasionally be much slower than the average run, especially on CircleCI. Also, this means that a "normal" client that creates a table and then runs SPLIT might encounter the same error? Seems like SPLIT needs to wait for some update to complete as part of its implementation..

Assuming this error comes from AdminSplit, maybe we should run that in a retry loop?

---

Comments from Reviewable

[maddyblue]

Our existing ALTER INDEX uses the table@index syntax: https://www.cockroachlabs.com/docs/rename-index.html.

Permissions are INSERT on the table unless someone has a better idea.

Added parens to values.

I also added two result columns with the raw key bytes and pretty version. This was to help in testing to verify that the range is split, but I think is just useful in general when using this feature.

---

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

sql/split.go, line 52 [r1] (raw file):

Previously, knz (kena) wrote…

You need to check for aggregate functions here. Use AggregateInExpr().

If you arbitrarily decide to not support sub-queries nor placeholders for prepare/execute, then you must reject them explicitly in the code (and confirm via a test that they are rejected properly. Right now I fear your code would cause a panic). For this, use the same logic as in MakeColumnDefDescs (sqlbase/table.go) calling SanitizeVarFreeExpr().

Or you could decide to support them, and move the Eval call to a new splitNode's Start() method, add a startSubQueries() somewhere and populate expandPlan() accordingly.

I've added tests for aggregate functions. I've added tests and support for placeholders and subqueries.

---

sql/split.go, line 68 [r1] (raw file):

Previously, knz (kena) wrote…

This AdminSplit() call hould go to Start() and not stay in the constructor.

Done.

---

sql/split_at_test.go, line 35 [r1] (raw file):

Previously, RaduBerinde wrote…

can use sqlrunner to make these shorter, e.g.

cockroach/sql/distsql/server_test.go

Line 43 in 5498634

r := sqlutils.MakeSQLRunner(t, sqlDB)

Done.

---

sql/split_at_test.go, line 44 [r1] (raw file):

Previously, RaduBerinde wrote…

I agree, things can occasionally be much slower than the average run, especially on CircleCI. Also, this means that a "normal" client that creates a table and then runs SPLIT might encounter the same error? Seems like SPLIT needs to wait for some update to complete as part of its implementation..

Assuming this error comes from AdminSplit, maybe we should run that in a retry loop?

The error does come from `AdminSplit`. But the wait is only required after the table is created. I've added a retry loop just for the first SPLIT, which seems to work well.

---

sql/split_at_test.go, line 85 [r1] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

How about a SHOW SPLITS command (or something like that)?

Done. `SHOW SPLITS` can come later.

---

Comments from Reviewable

[RaduBerinde]

Review status: 1 of 12 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

sql/split_at_test.go, line 44 [r1] (raw file):

Previously, mjibson (Matt Jibson) wrote…

The error does come from AdminSplit. But the wait is only required after the table is created. I've added a retry loop just for the first SPLIT, which seems to work well.

But that means that any client that runs `CREATE TABLE` and then `SPLIT AT` will potentially get the same error, no? I think we should retry in the implementation of SPLIT AT rather than in the test.

---

Comments from Reviewable

[knz]

So now I like this code even more than earlier.
Mechanism-wise this is now all good (from my perspective), but your latest change brought a new concern in my mind.
It's not a blocker but still bear with me.

My concern is that making the statement return result rows now conflicts with making it a variant of ALTER.

The conflict comes from the general SQL principle that DDL statements do not return results, and ALTER (in all its variants) is supposed to be a DDL statement. From the documentation / teaching perspective, having to stay "ALTER is a DDL statement, except if you use SPLIT" feels wrong: it's very asymmetric and rather arbitrary.

Mind that I do not know technical reasons why a variant of ALTER could not be a non-DDL statement while the others are, so my argument at this point could be deemed "academic". Yet I am concerned about consistency and the ease for users to understand what we're doing. An unknown risk could be be also that external SQL tools/drivers decide whether to parse return results or not whether on which statement was sent, instead of using the pgwire statement tag properly. Of course it's theoretical, but there is really no precedent whatsoever for ALTER not always being a DDL statement, so we're sailing in uncharted territory by changing this.

Meanwhile I understand you implemented this to ease testing. There are really two positions to take:

1. either we deem this result useful and not just for testing, in which case I might want to push again for having it a different statement from ALTER, and we could reuse the SPLIT syntax you defined initially.
2. or we really want this to be a sub-feature of ALTER and then I'd like to suggest to not have it return results, keep its DDL tag, and perhaps create a SQL builtin function to compute the key encoding from values separately from this statement, so that users can use that (e.g. via SELECT cockroachdb_encode_pkey(a,b,c)) should they ever want to investigate their K/V storage from the SQL perspective. If we go this way I am OK with changing the test to not check for the statement's results for now.

My own preference goes for option 2.
We could even open a follow-up issue to define a builtin function that returns the current bounds for the range that contain a given key. This, combined with the new function above and the new ALTER statement, would then enable proper testing of splits.

What do you think?

---

Reviewed 11 of 11 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

sql/split.go, line 74 [r2] (raw file):

      }
      desired := c.Type.ToDatumType()
      typedExpr, err := p.analyzeExpr(expr, nil, nil, desired, false, "")

You can change this to analyzeExpr(expr, nil, nil, desired, true, "SPLIT AT") and drop the TypeEqual check below. analyzeExpr can enforce the type this way.

---

sql/parser/sql.y, line 1483 [r2] (raw file):

  ALTER TABLE qualified_name SPLIT AT '(' expr_list ')'
  {
    $$.val = &Split{Table: $3.normalizableTableName(), Exprs: $7.exprs()}

Ok here I don't have a very strong feeling but for consistency I'd like to suggest to prefix the node name with "AlterTable" like "AlterTableSplit" for consistency with the other Alter node. Same with the planNode btw, perhaps "alterTableSplitNode" instead of just "splitNode". Don't overthink it too much though, your call. But please also consider my comment at the top of the review.

---

Comments from Reviewable

[knz]

By the way regardless of which direction you go, I may want to suggest to define this builtin function to compute the key encoding anyway, and simplify your split implementation by requiring just 1 expression of type string as argument to SPLIT (And let the user specify either an already-encoded constant string literal argument, or a function call to the builtin). If you don't want to do this right away please let me know, I can then file a separate issue to do this in a follow-up.

---

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[maddyblue]

INSERT, DELETE, and UPDATE statements currently have their return type dependent on whether or not they have a RETURNING clause, which changes the return type from a RowsAffected to a Rows. I understand your concern about having a non-DDL ALTER, but I just don't think it matters that much. Any driver that parses the SQL and uses neither the returned type or tag to determine what to do is crazy, and I'm not worried about supporting non-spec compliant crazy drivers.

I don't care what the syntax is, though, if we wanted to make it a non ALTER for that reason. But both Ben and Peter appear to think ALTER is the best. However maybe they have new opinions now because of the return data. My vote is to leave things as they are even in the face of your arguments.

One problem with adding a SQL function that does what you describe is a user must also specify a table and optional index. I'm not sure how to do that in a function without adding some special syntax (for example similar to the EXTRACT function which supports arbitrary keywords in it). I think a family of functions exposing lower-level information about cockroach would be neat, but I'm not sure how it would work in this case.

---

Review status: 10 of 12 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

sql/split.go, line 74 [r2] (raw file):

Previously, knz (kena) wrote…

You can change this to analyzeExpr(expr, nil, nil, desired, true, "SPLIT AT") and drop the TypeEqual check below. analyzeExpr can enforce the type this way.

Done.

---

sql/split_at_test.go, line 44 [r1] (raw file):

Previously, RaduBerinde wrote…

But that means that any client that runs CREATE TABLE and then SPLIT AT will potentially get the same error, no? I think we should retry in the implementation of SPLIT AT rather than in the test.

I really don't like the idea of retrying within the implementation. We would have to switch to sentinel errors so we would know which ones we can retry on, figure out how many or for how long to attempt a retry. I think the specific case of CREATE then SPLIT causing this bug will be rare enough that users can just retry themselves.

How about this: leave it as is, and if people complain about it then we revisit retrying at that time.

---

sql/parser/sql.y, line 1483 [r2] (raw file):

Previously, knz (kena) wrote…

Ok here I don't have a very strong feeling but for consistency I'd like to suggest to prefix the node name with "AlterTable" like "AlterTableSplit" for consistency with the other Alter node. Same with the planNode btw, perhaps "alterTableSplitNode" instead of just "splitNode". Don't overthink it too much though, your call. But please also consider my comment at the top of the review.

See `rename_stmt` which uses this same ALTER syntax, and is typed as a `RenameDatabase`, `RenameColumn`, or `RenameIndex`. I think what I have here is already consistent with current usage. https://github.com/cockroachdb/cockroach/blob/544b13f638083995e0dc78c9da5056f68953cf3a/sql/parser/sql.y#L1884

---

Comments from Reviewable

[knz]

cc @sploiselle @jesse: for docs I'd like to see a section about which statements are DDL and which are not, with special mention of ALTER TABLE .. SPLIT in there.

See #8990 for the followup with special functions.

---

Reviewed 2 of 2 files at r3.
Review status: 6 of 12 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

---

sql/parser/sql.y, line 1483 [r2] (raw file):

Previously, mjibson (Matt Jibson) wrote…

See rename_stmt which uses this same ALTER syntax, and is typed as a RenameDatabase, RenameColumn, or RenameIndex. I think what I have here is already consistent with current usage.

cockroach/sql/parser/sql.y

Line 1884 in 544b13f

rename_stmt:

YEs you are right. My bad.

---

Comments from Reviewable

[knz]

Reviewed 6 of 6 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[tamird]

why is there both DistSender() and GetDistSender?

[maddyblue]

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

server/testserver.go, line 175 [r4] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why is there both DistSender() and GetDistSender?

`serverutils.StartServer` returns a `TestServerInterface`. In order to support the range lookup I needed access to a kv.DistSender, so I added a method to TestServerInterface. In order to be consistent with the existing methods on that type, I omitted the word `Get` since the other methods don't have it. It is unfortunate that TestCluster uses Get but not a huge deal.

---

Comments from Reviewable

[tamird]

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

server/testserver.go, line 175 [r4] (raw file):

Previously, mjibson (Matt Jibson) wrote…

serverutils.StartServer returns a TestServerInterface. In order to support the range lookup I needed access to a kv.DistSender, so I added a method to TestServerInterface. In order to be consistent with the existing methods on that type, I omitted the word Get since the other methods don't have it. It is unfortunate that TestCluster uses Get but not a huge deal.

There are single digit callers to `Get...` - why not take the opportunity to clean it up?

---

Comments from Reviewable

[maddyblue]

Review status: 10 of 13 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

---

server/testserver.go, line 175 [r4] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

There are single digit callers to Get... - why not take the opportunity to clean it up?

Oh I misunderstood. I thought you were talking about TestCluster vs TestServer. I didn't realize TestServer already has GetDistSender. I've made them all just `DistSender` to match the interface conventions better.

---

Comments from Reviewable

[tamird]

Review status: 10 of 13 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

---

server/testserver.go, line 175 [r4] (raw file):

Previously, mjibson (Matt Jibson) wrote…

Oh I misunderstood. I thought you were talking about TestCluster vs TestServer. I didn't realize TestServer already has GetDistSender. I've made them all just DistSender to match the interface conventions better.

Thanks!

---

Comments from Reviewable

[maddyblue]

@bdarnell @petermattis Would one of you like to take a final look at this?

[petermattis]

The syntax looks good to me now. I'll trust the other reviewers on the rest of the change.

---

Review status: 10 of 13 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

Comments from Reviewable