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

*: remove DirtyDB and DirtyTable to reduce memory usage #19042

Merged
merged 6 commits into from
Sep 4, 2020

Conversation

bobotu
Copy link
Contributor

@bobotu bobotu commented Aug 6, 2020

What problem does this PR solve?

UnionScan only need to check whether key already exists in MemBuffer before UnionScan started. We can directly use MemBuffer to provide this information.

Check List

Tests

  • Unit test

Release note

  • Reduce memory usage of large transactions.

@bobotu bobotu added the sig/transaction SIG:Transaction label Aug 6, 2020
@bobotu bobotu requested review from lysu, coocood and tiancaiamao August 6, 2020 09:23
@bobotu bobotu requested a review from a team as a code owner August 6, 2020 09:23
@bobotu bobotu requested review from wshwsh12 and removed request for a team August 6, 2020 09:23
@bobotu bobotu removed the request for review from wshwsh12 August 6, 2020 09:27
@zz-jason
Copy link
Member

zz-jason commented Aug 6, 2020

Reduce memory usage of large transactions.

Is there any test report?

@bobotu bobotu marked this pull request as draft August 6, 2020 12:42
@bobotu bobotu force-pushed the dirty-table branch 3 times, most recently from 5ffd88c to 105c4cb Compare August 10, 2020 09:44
@bobotu bobotu marked this pull request as ready for review August 10, 2020 09:50
@bobotu
Copy link
Contributor Author

bobotu commented Aug 10, 2020

/run-all-tests

@coocood
Copy link
Member

coocood commented Aug 10, 2020

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 10, 2020
tk.MustExec("drop table if exists t;")
tk.MustExec("create table t(a int primary key, b int);")
tk.MustExec("begin")
for i := 2; i < 100000; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bench test? so much data may slow the CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A correctness test. If the snapshot getter reads the newly added key, the result of this SQL will not empty. The first prototype of this PR doesn't implement snapshot getter, and it fails to pass this test.

}
tk.MustExec("commit;")

tk.MustExec("set tidb_distsql_scan_concurrency = 1;")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why set those concurrency variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To slow down the execution, otherwise, the concurrent scanner cannot scan the updated rows.

This SQL is a special case, tries to update handle + 1000 when reading headle. If the executor is slow enough, it can read some dirty data.

@github-actions github-actions bot added the sig/execution SIG execution label Aug 20, 2020
@bobotu
Copy link
Contributor Author

bobotu commented Aug 31, 2020

Add a snapshot iterator like #19276 to fix #19054

@bobotu
Copy link
Contributor Author

bobotu commented Aug 31, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Sep 4, 2020

@tiancaiamao
Copy link
Contributor

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 4, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 4, 2020
@tiancaiamao
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 4, 2020
@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 19791

@tiancaiamao
Copy link
Contributor

/rebuild

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@bobotu merge failed.

@tiancaiamao
Copy link
Contributor

/run-check_dev_2

@tiancaiamao tiancaiamao merged commit ce849c3 into pingcap:master Sep 4, 2020
@qw4990
Copy link
Contributor

qw4990 commented Sep 7, 2020

Do we need to pick this to v4.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/executor sig/execution SIG execution sig/transaction SIG:Transaction status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants