sql: allow deleteRangeNode to autoCommit transactions

[nvanbenschoten]

deleteRangeNode currently has the following restriction:

cockroach/pkg/sql/delete_range.go

Lines 29 to 31 in 445ec41

	// Note: deleteRangeNode can't autocommit, because it has to delete in batches,
	// and it won't know whether or not there is more work to do until after a batch
	// is returned. This property precludes using auto commit.

This has huge performance implications, as it means that no point DELETE statement can run as a 1PC transaction. This results in roughly a 50% performance hit for delete-heavy workloads on tables without secondary indexes.

The comment says that we delete in batches and we won't know whether or not there is more work to do until after a batch is returned. I don't fully understand this. If the comment is talking about b.Header.MaxSpanRequestKeys = TableTruncateChunkSize then I don't think this needs to preclude attempting a 1PC transaction. I think the kv implementation is smart enough to handle 1PC transactions with key limits. If it's not, we should make it work to support this case.

For context, we have heard in the past from people running Cockroach that deletes are slower than inserts, which has never made sense to me. This explains why this is the case. Sysbench adds more weight to this claim. oltp_delete without a secondary index is the only sysbench workload that we are slower on than similarly architected databases. If we could run point DELETE statements as 1PC transactions then we would immediately close this gap.

@jordanlewis I noticed that some of this changed in #36728. Do you have context on this? Did that cause a regression in this behavior?

[nvanbenschoten]

It does look like this is new as of v19.2. This is a pretty bad regression. I think it's probably a release blocker.

# v19.1

root@localhost:26257/defaultdb> create table a (k int primary key);
CREATE TABLE

root@localhost:26257/defaultdb> set tracing = on; delete from a where k = 1; set tracing = off;
SET TRACING

root@localhost:26257/defaultdb> show kv trace for session;
             timestamp             |       age       |                      message                       |                        tag                         | location |    operation     | span
+----------------------------------+-----------------+----------------------------------------------------+----------------------------------------------------+----------+------------------+------+
  2019-10-04 06:28:10.893029+00:00 | 00:00:00.000217 | querying next range at /Table/52/1/1               | [n1,client=127.0.0.1:65018,user=root,txn=6f8307c2] |          | dist sender send |    5
  2019-10-04 06:28:10.89305+00:00  | 00:00:00.000238 | r21: sending batch 1 DelRng, 1 EndTxn to (n1,s1):1 | [n1,client=127.0.0.1:65018,user=root,txn=6f8307c2] |          | dist sender send |    5
  2019-10-04 06:28:10.893198+00:00 | 00:00:00.000386 | fast path completed                                | [n1,client=127.0.0.1:65018,user=root]              |          | flow             |    3
  2019-10-04 06:28:10.893215+00:00 | 00:00:00.000403 | rows affected: 0                                   | [n1,client=127.0.0.1:65018,user=root]              |          | sql txn          |    1
(4 rows)


# v19.2

root@localhost:26257/defaultdb> set tracing = on; delete from a where k = 1; set tracing = off;
SET TRACING

root@localhost:26257/defaultdb> show kv trace for session;
             timestamp             |       age       |                 message                  |                        tag                         | location |    operation     | span
+----------------------------------+-----------------+------------------------------------------+----------------------------------------------------+----------+------------------+------+
  2019-10-04 06:29:58.780724+00:00 | 00:00:00.000212 | querying next range at /Table/52/1/1     | [n1,client=127.0.0.1:65040,user=root,txn=b7f909e0] |          | dist sender send |    8
  2019-10-04 06:29:58.780744+00:00 | 00:00:00.000232 | r21: sending batch 1 DelRng to (n1,s1):1 | [n1,client=127.0.0.1:65040,user=root,txn=b7f909e0] |          | dist sender send |    8
  2019-10-04 06:29:58.780882+00:00 | 00:00:00.00037  | fast path completed                      | [n1,client=127.0.0.1:65040,user=root]              |          | flow             |    5
  2019-10-04 06:29:58.780897+00:00 | 00:00:00.000385 | rows affected: 0                         | [n1,client=127.0.0.1:65040,user=root]              |          | exec stmt        |    3
  2019-10-04 06:29:58.780921+00:00 | 00:00:00.000409 | querying next range at /Table/52/1/1     | [n1,client=127.0.0.1:65040,user=root,txn=b7f909e0] |          | dist sender send |   11
  2019-10-04 06:29:58.780931+00:00 | 00:00:00.000419 | r21: sending batch 1 EndTxn to (n1,s1):1 | [n1,client=127.0.0.1:65040,user=root,txn=b7f909e0] |          | dist sender send |   11
(6 rows)

[jordanlewis]

Wow, good catch, glad you noticed this. We kind of had tests for this, but I overwrote them in my patch because it seemed like they were specific to interleaved tables (the tests were in files named interleaved). I'll try to fix it.

[jordanlewis]

Actually, I don't see what you mean - how could you avoid doing a 2pc transaction in this case?

If the comment is talking about b.Header.MaxSpanRequestKeys = TableTruncateChunkSize then I don't think this needs to preclude attempting a 1PC transaction. I think the kv implementation is smart enough to handle 1PC transactions with key limits. If it's not, we should make it work to support this case.

What would happen if you ran CommitInBatch on a batch that didn't finish?

[jordanlewis]

I put up a strawman PR that doesn't work for the reason that I mentioned. Maybe I'm just missing something about what you said.