sql,opt: re-enable 1pc for small delRange ops #41324
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
jordanlewis
force-pushed
the
delrng-1pc
branch
from
89c757b
to
3d4a7eb
Compare
jordanlewis
commented
Oct 4, 2019
jordanlewis
force-pushed
the
delrng-1pc
branch
from
3d4a7eb
to
3767d50
Compare
|
RFAL |
nvanbenschoten
approved these changes
Oct 5, 2019
There was a problem hiding this comment.
@RaduBerinde should also sign off on the optimizer portion though.
Reviewed 3 of 6 files at r1, 2 of 3 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @nvanbenschoten, and @RaduBerinde)
pkg/sql/delete_range.go, line 30 at r2 (raw file):
// instead of many point deletes. // // Note: deleteRangeNode can't autocommit, because it has to delete in batches,
You can delete this comment now.
pkg/sql/opt/exec/execbuilder/testdata/delete_range, line 16 at r2 (raw file):
query TT SELECT operation, message FROM [SHOW KV TRACE FOR SESSION] WHERE message LIKE '%DelRange%'
Can you also test against the batches sent? OR message LIKE '%sending batch%'
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Oct 5, 2019
First commit from cockroachdb#41324. We expect a selection of simple single-statement mutations to hit the "1-phase commit" transaction fast-path. cockroachdb#41320 demonstrated how critical this is, as regressions here can cause major (> 50%) performance hits to benchmarks and user workloads. This commit adds a logic test to validate that these statements continue to hit this fast-path. Release justification: Testing only. Release note: None
This commit tells the optimizer to enable autoCommit on delRange operations if it can prove that the number of returned keys is low enough. Release justification: high-priority fix to performance regression Release note: None
jordanlewis
force-pushed
the
delrng-1pc
branch
from
3767d50
to
e92b602
Compare
jordanlewis
commented
Oct 7, 2019
There was a problem hiding this comment.
Done, TFTRs
Reviewable status:
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Oct 7, 2019
41324: sql,opt: re-enable 1pc for small delRange ops r=jordanlewis a=jordanlewis This commit tells the optimizer to enable autoCommit on delRange operations if it can prove that the number of returned keys is low enough. Closes #41320. Release justification: high-priority fix to performance regression Release note: None Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
Build succeeded |
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Oct 8, 2019
First commit from cockroachdb#41324. We expect a selection of simple single-statement mutations to hit the "1-phase commit" transaction fast-path. cockroachdb#41320 demonstrated how critical this is, as regressions here can cause major (> 50%) performance hits to benchmarks and user workloads. This commit adds a logic test to validate that these statements continue to hit this fast-path. Release justification: Testing only. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Oct 10, 2019
41356: Revert "opt: disallow mutations under union" r=RaduBerinde a=RaduBerinde This reverts commit a34d705. Release justification: recently merge fix no longer needed (thanks to #41307). Release note (sql change): Mutations under UNION or UNION ALL are allowed again. 41358: sql/logictest: add logictest for all expected 1PC txn statements r=nvanbenschoten a=nvanbenschoten First commit from #41324. We expect a selection of simple single-statement mutations to hit the "1-phase commit" transaction fast-path. #41320 demonstrated how critical this is, as regressions here can cause major (> 50%) performance hits to benchmarks and user workloads. This commit adds a logic test to validate that these statements continue to hit this fast-path. Release justification: Testing only. 41371: kv: avoid allocations in client.Txn constructor r=nvanbenschoten a=nvanbenschoten This PR contains two small wins that help speed up the client.Txn constructor, which is responsible for **2.90%** of a CPU profile when running Sysbench's `oltp_point_select` workload. The biggest win here will come from addressing #32508. #### kv: lazily create *RangeIterator in txnPipeliner This allocation was responsible for **0.34%** of a CPU profile when running Sysbench's `oltp_point_select` workload. #### kv: only re-alloc refresh spans in augmentMetaLocked if necessary This was responsible for **0.057%** of a CPU profile when running Sysbench's `oltp_point_select` workload. Release justification: None. These can wait for the branch to split. 41372: sql/pgwire: statically allocate format code slice for all text args/cols r=nvanbenschoten a=nvanbenschoten This allocation was responsible for **0.8%** of a CPU profile when running Sysbench's oltp_point_select workload. Release justification: Probably none, although this does look very safe. Release note: None 41379: pkg/sql: use ring.Buffer in StmtBuf r=nvanbenschoten a=nvanbenschoten The StmtBuf has the exact access patterns typically associated with a ring buffer. Command instances are added to the back of the buffer and trimmed from the front of the buffer. These operations are often performed in lockstep, but that is not always the case, so we need the buffer to be able to grow. `ring.Buffer` provides exactly these semantics and it allows us to avoid memory allocations on each Command in `StmtBuf.Push`. Release justification: None. Don't merge until branch is split. Co-authored-by: Radu Berinde <radu@cockroachlabs.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit tells the optimizer to enable autoCommit on delRange
operations if it can prove that the number of returned keys is low
enough.
Closes #41320.
Release justification: high-priority fix to performance regression
Release note: None