Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: run deleteRange queries in batches #36728

Merged
merged 1 commit into from May 23, 2019

Conversation

Projects
None yet
4 participants
@jordanlewis
Copy link
Member

commented Apr 10, 2019

Fixes #36588.

Previously, a query like DELETE FROM t that uses a deleteRangeNode
under the hood would attempt to send a single DelRange request for
each span it was responsible for deleting. This led to unlimited memory
usage, since the DelRange request must return the keys it deleted in
this context so SQL can know how many rows were affected.

To solve this problem, we send DelRange requests with a returned-key
limit in a loop until the range is fully deleted.

Release note (bug fix): prevent unlimited memory usage during SQL range
deletions

@jordanlewis jordanlewis requested a review from andreimatei Apr 10, 2019

@jordanlewis jordanlewis requested review from cockroachdb/sql-execution-prs as code owners Apr 10, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

This change is Reviewable

@jordanlewis jordanlewis force-pushed the jordanlewis:delete-range-mem branch from bebe799 to a91c323 Apr 10, 2019

@jordanlewis jordanlewis requested a review from cockroachdb/sql-rest-prs as a code owner Apr 10, 2019

@jordanlewis jordanlewis force-pushed the jordanlewis:delete-range-mem branch from a91c323 to 10507ba Apr 10, 2019

@jordanlewis jordanlewis requested a review from BramGruneir Apr 10, 2019

@jordanlewis jordanlewis force-pushed the jordanlewis:delete-range-mem branch from 10507ba to 2385c31 Apr 11, 2019

@jordanlewis jordanlewis requested a review from cockroachdb/sql-opt-prs as a code owner Apr 11, 2019

@jordanlewis

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

I deleted a test that was testing whether or not DELETE FROM t produced a parallelized batch. It no longer does. We already test for the presence of parallelized batches from the distsender in another trace test, so deleting this test doesn't reduce coverage.

@BramGruneir
Copy link
Member

left a comment

So the technique you used here isn't used anywhere else in our code (that I could find). This is a different type of batching than what we use for other writers.

Since this is a fast path, and it's already specialized, would it make sense to using a range_delete instead of point deletes if the size is large enough to batch?

This would avoid the batching and most likely be more efficient.

Reviewed 6 of 6 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @jordanlewis)


pkg/sql/tablewriter.go, line 95 at r1 (raw file):

)

// extendedTableWriter is a temporary interface introduced

This old TODO (that is missing a todo) should have a github issue associated with it.


pkg/sql/logictest/testdata/planner_test/delete_range, line 24 at r1 (raw file):

  AND operation != 'dist sender send'
----
flow                 DelRange /Table/53/1 - /Table/53/2

Won't this test be brittle to new system tables being added?
We really need a way to put in wildcards in the results.

@andreimatei
Copy link
Member

left a comment

Bram I probably don't understand you comment, but just in case - this patch batches less than before, not more - by putting a limit and retrying. And it already uses range deletes.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @BramGruneir and @jordanlewis)


pkg/sql/delete_range.go, line 171 at r1 (raw file):

	traceKV := params.p.ExtendedEvalContext().Tracing.KVTracingEnabled()
	for len(d.spans) != 0 {
		d.tableWriter.init(params.p.txn)

why are we using a tablewriter here? Seems that we're dealing with key ranges, not rows. What's the tablewriter giving us? Let's control our own destiny if possible.


pkg/sql/delete_range.go, line 184 at r1 (raw file):

		}

		d.spans = d.spans[:0]

comment that we're going to add all the spans we haven't touched back. And also I'd operate on a copy of d.spans; there's no reason for d.spans to be updated.

But better yet, I think the code would be clearer if you iterated over the results (which should be in order) and remove from the head of d.spans items one by one while there's no ResumeSpan (instead of wiping d.spans and then reconstructing it)


pkg/sql/delete_range.go, line 187 at r1 (raw file):

		for _, r := range d.tableWriter.b.Results {
			var prev []byte
			for _, i := range r.Keys {

the name i here seems particularly bad. Mind changing it?


pkg/sql/tablewriter.go, line 129 at r1 (raw file):

	txn *client.Txn
	// is autoCommit turned on.
	autoCommit autoCommitOpt

this is lolo. Make it a bool :)


pkg/sql/logictest/testdata/planner_test/delete_range, line 13 at r1 (raw file):

SET tracing = on,kv; DELETE FROM a; SET tracing = off

# Ensure that DelRange requests are chunked for DELETE FROM...

If all you care about are the DelRange messages, delete all that filtering crap and say message like 'DelRange%'.
If you care about more than those messages, I for one can't support writing more tests like this - versus writing the tests in go and using a more proper library for asserting whatever.


pkg/sql/logictest/testdata/planner_test/delete_range, line 24 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Won't this test be brittle to new system tables being added?
We really need a way to put in wildcards in the results.

there's room for new system tables below id 50.

@jordanlewis jordanlewis force-pushed the jordanlewis:delete-range-mem branch from 2385c31 to 7a267bc Apr 20, 2019

@jordanlewis
Copy link
Member Author

left a comment

Bram, I also don't really understand your comment. The purpose of this patch is to break up a single enormous range delete into many smaller ones by imposing a limit on the number of keys that the range delete can delete. This batching method is used elsewhere in the codebase, namely in scans - we might ask for a huge span of keys, but we set a limit on the number of keys returned from the scan to prevent the KV layer from allocating unlimited memory and killing itself.

If you're suggesting we use a clear range, we can't, because this has to be transactional and respect MVCC.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @BramGruneir)


pkg/sql/delete_range.go, line 171 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

why are we using a tablewriter here? Seems that we're dealing with key ranges, not rows. What's the tablewriter giving us? Let's control our own destiny if possible.

It's convenient, provides error handling, and is the same as what we do everywhere else. I don't see a good reason to make this code less DRY.


pkg/sql/delete_range.go, line 184 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

comment that we're going to add all the spans we haven't touched back. And also I'd operate on a copy of d.spans; there's no reason for d.spans to be updated.

But better yet, I think the code would be clearer if you iterated over the results (which should be in order) and remove from the head of d.spans items one by one while there's no ResumeSpan (instead of wiping d.spans and then reconstructing it)

There's still the issue that you have to write the resume span back to the spans array. I've tried something like your approach, what do you think?


pkg/sql/delete_range.go, line 187 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

the name i here seems particularly bad. Mind changing it?

Done.


pkg/sql/tablewriter.go, line 95 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

This old TODO (that is missing a todo) should have a github issue associated with it.

Really? Most TODOs in our codebase don't have github issues associated with it, not sure why this one would be special


pkg/sql/tablewriter.go, line 129 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

this is lolo. Make it a bool :)

I'd rather not do that in this commit.


pkg/sql/logictest/testdata/planner_test/delete_range, line 13 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

If all you care about are the DelRange messages, delete all that filtering crap and say message like 'DelRange%'.
If you care about more than those messages, I for one can't support writing more tests like this - versus writing the tests in go and using a more proper library for asserting whatever.

Good point, done.

@jordanlewis
Copy link
Member Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @BramGruneir)


pkg/sql/delete_range.go, line 184 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

There's still the issue that you have to write the resume span back to the spans array. I've tried something like your approach, what do you think?

Eh, the approach that I took is subtly buggy. I think the way that I had originally is simpler - going to revert to it.

@jordanlewis jordanlewis force-pushed the jordanlewis:delete-range-mem branch from 7a267bc to 06c960e Apr 20, 2019

@jordanlewis
Copy link
Member Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @BramGruneir)


pkg/sql/delete_range.go, line 184 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Eh, the approach that I took is subtly buggy. I think the way that I had originally is simpler - going to revert to it.

Kept your suggestion to copy the spans though.

@andreimatei
Copy link
Member

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @BramGruneir, and @jordanlewis)


pkg/sql/delete_range.go, line 171 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

It's convenient, provides error handling, and is the same as what we do everywhere else. I don't see a good reason to make this code less DRY.

what convenience? You're calling init repeatedly, finalize (at least can you call flushAndStartNewBatch?) repeatedly, you're manually reaching into d.tableWriter.b.Header, you're reaching into autoCommit.
What error handling? row.ConvertBatchError deals with uniqueness constraint violations, which are not applicable here. Also note how that function is in the row package and we're not operating on rows here.
Tablewriters, as I understand them, have something to do with rows, and we don't seem to be using that here. I'm not completely sure why not, btw - we seem to be doing our own counting of rows; can't the tw do that? Cause if we used it for that, then I'd be for it.

So basically as far as I can see, what you want here is simply to use the KV API. Using the KV API directly (which means txn.Batch(), txn.Run()) is not "repeating yourself".

@jordanlewis jordanlewis force-pushed the jordanlewis:delete-range-mem branch from 06c960e to b1dc692 Apr 20, 2019

@jordanlewis jordanlewis force-pushed the jordanlewis:delete-range-mem branch from b1dc692 to 1338bca May 22, 2019

@jordanlewis

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

@andreimatei RFAL, I've removed the tableWriter - you were right.

@jordanlewis jordanlewis requested review from andreimatei and BramGruneir May 22, 2019

@jordanlewis jordanlewis force-pushed the jordanlewis:delete-range-mem branch 2 times, most recently from fc93e8b to 7c66864 May 22, 2019

@andreimatei
Copy link
Member

left a comment

:LGTM:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @BramGruneir, and @jordanlewis)


pkg/sql/delete_range.go, line 231 at r2 (raw file):

func (d *deleteRangeNode) enableAutoCommit() {
	// DeleteRange 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 the first

nit: it's not about the first range in particular, is it?

sql: run deleteRange queries in batches
Previously, a query like `DELETE FROM t` that uses a deleteRangeNode
under the hood would attempt to send a single `DelRange` request for
each span it was responsible for deleting. This led to unlimited memory
usage, since the `DelRange` request must return the keys it deleted in
this context so SQL can know how many rows were affected.

To solve this problem, we send `DelRange` requests with a returned-key
limit in a loop until the range is fully deleted.

Release note (bug fix): prevent unlimited memory usage during SQL range
deletions

@jordanlewis jordanlewis force-pushed the jordanlewis:delete-range-mem branch from 7c66864 to 807c1f2 May 23, 2019

@jordanlewis

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

TFTR!

bors r+

craig bot pushed a commit that referenced this pull request May 23, 2019

Merge #36728
36728: sql: run deleteRange queries in batches r=jordanlewis a=jordanlewis

Fixes #36588.

Previously, a query like `DELETE FROM t` that uses a deleteRangeNode
under the hood would attempt to send a single `DelRange` request for
each span it was responsible for deleting. This led to unlimited memory
usage, since the `DelRange` request must return the keys it deleted in
this context so SQL can know how many rows were affected.

To solve this problem, we send `DelRange` requests with a returned-key
limit in a loop until the range is fully deleted.

Release note (bug fix): prevent unlimited memory usage during SQL range
deletions

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
@craig

This comment has been minimized.

Copy link

commented May 23, 2019

Build failed

@jordanlewis

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

bors r+

craig bot pushed a commit that referenced this pull request May 23, 2019

Merge #36728
36728: sql: run deleteRange queries in batches r=jordanlewis a=jordanlewis

Fixes #36588.

Previously, a query like `DELETE FROM t` that uses a deleteRangeNode
under the hood would attempt to send a single `DelRange` request for
each span it was responsible for deleting. This led to unlimited memory
usage, since the `DelRange` request must return the keys it deleted in
this context so SQL can know how many rows were affected.

To solve this problem, we send `DelRange` requests with a returned-key
limit in a loop until the range is fully deleted.

Release note (bug fix): prevent unlimited memory usage during SQL range
deletions

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
@craig

This comment has been minimized.

Copy link

commented May 23, 2019

Build succeeded

@craig craig bot merged commit 807c1f2 into cockroachdb:master May 23, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@jordanlewis jordanlewis deleted the jordanlewis:delete-range-mem branch May 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.