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

*: fix lost index bug of insert on duplicate key update #16672

Merged
merged 7 commits into from
Apr 22, 2020

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Apr 21, 2020

Signed-off-by: crazycs crazycs520@gmail.com

What problem does this PR solve?

Fix issue #16669

What is changed and how it works?

When check the untouched index, should also check the memory-buffer in the session too.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Side effects

  • No

Release note

Signed-off-by: crazycs <crazycs520@gmail.com>
@crazycs520 crazycs520 requested a review from a team as a code owner April 21, 2020 10:17
@ghost ghost requested review from XuHuaiyu and removed request for a team April 21, 2020 10:18
@crazycs520 crazycs520 added sig/transaction SIG:Transaction needs-cherry-pick-3.0 require-LGT3 Indicates that the PR requires three LGTM. type/bugfix This PR fixes a bug. labels Apr 21, 2020
Signed-off-by: crazycs <crazycs520@gmail.com>
Signed-off-by: crazycs <crazycs520@gmail.com>
@@ -293,7 +293,11 @@ func (c *index) Create(sctx sessionctx.Context, rm kv.RetrieverMutator, indexedV
// If the index kv was untouched(unchanged), and the key/value already exists in mem-buffer,
// should not overwrite the key with un-commit flag.
// So if the key exists, just do nothing and return.
_, err = txn.GetMemBuffer().Get(ctx, key)
if memTxn, ok := txn.(kv.MemBufferRetriever); ok {
_, err = memTxn.GetFromMemBuffer(ctx, key)
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to change TxnState.GetMembuffer implementation, so we will not misuse it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

The buffer level is too complex to get things right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great

@crazycs520
Copy link
Contributor Author

/rebuild

@github-actions github-actions bot added the sig/execution SIG execution label Apr 21, 2020
Signed-off-by: crazycs <crazycs520@gmail.com>
@coocood
Copy link
Member

coocood commented Apr 21, 2020

/run-all-test

Signed-off-by: crazycs <crazycs520@gmail.com>
@crazycs520
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented Apr 21, 2020

Codecov Report

Merging #16672 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #16672   +/-   ##
===========================================
  Coverage   80.4305%   80.4305%           
===========================================
  Files           506        506           
  Lines        136958     136958           
===========================================
  Hits         110156     110156           
  Misses        18219      18219           
  Partials       8583       8583           

@crazycs520
Copy link
Contributor Author

/rebuild

@coocood
Copy link
Member

coocood commented Apr 21, 2020

LGTM

@coocood
Copy link
Member

coocood commented Apr 21, 2020

/run-unit-test

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp
Copy link
Member

jackysp commented Apr 22, 2020

PTAL @tiancaiamao

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT3 The PR has already had 3 LGTM. label Apr 22, 2020
@tiancaiamao
Copy link
Contributor

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 22, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Apr 22, 2020

Your auto merge job has been accepted, waiting for:

  • 16656

@sre-bot
Copy link
Contributor

sre-bot commented Apr 22, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Apr 22, 2020

@crazycs520 merge failed.

@bb7133
Copy link
Member

bb7133 commented Apr 22, 2020

Data Race case in test:

 ==================
[2020-04-22T02:18:01.641Z] WARNING: DATA RACE

[2020-04-22T02:18:01.641Z] Write at 0x0000055cace0 by goroutine 126:

[2020-04-22T02:18:01.641Z]   github.com/pingcap/tidb/executor/seqtest_test.(*testOOMSuite).registerHook()
[2020-04-22T02:18:01.641Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/log@v0.0.0-20200117041106-d28c14d3b1cd/log.go:110 +0x271

[2020-04-22T02:18:01.641Z]   github.com/pingcap/tidb/executor/seqtest_test.(*testOOMSuite).SetUpSuite()
[2020-04-22T02:18:01.641Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/seqtest/seq_executor_test.go:1402 +0x50
[2020-04-22T02:18:01.641Z]   runtime.call32()
[2020-04-22T02:18:01.641Z]       /usr/local/go/src/runtime/asm_amd64.s:539 +0x3a
[2020-04-22T02:18:01.641Z]   reflect.Value.Call()
[2020-04-22T02:18:01.641Z]       /usr/local/go/src/reflect/value.go:321 +0xd3
[2020-04-22T02:18:01.641Z]   github.com/pingcap/check.(*suiteRunner).runFixture.func1()
[2020-04-22T02:18:01.641Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/check@v0.0.0-20200212061837-5e12011dc712/check.go:799 +0x195
[2020-04-22T02:18:01.641Z]   github.com/pingcap/check.(*suiteRunner).forkCall.func1()
[2020-04-22T02:18:01.641Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/check@v0.0.0-20200212061837-5e12011dc712/check.go:739 +0x113
[2020-04-22T02:18:01.641Z] 
[2020-04-22T02:18:01.641Z] Previous read at 0x0000055cace0 by goroutine 8:
[2020-04-22T02:18:01.641Z]   github.com/pingcap/tidb/domain.(*Domain).topologySyncerKeeper.func1()
[2020-04-22T02:18:01.641Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/util/logutil/log.go:344 +0x72
[2020-04-22T02:18:01.641Z]   github.com/pingcap/tidb/domain.(*Domain).topologySyncerKeeper()
[2020-04-22T02:18:01.641Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/domain/domain.go:488 +0x644
[2020-04-22T02:18:01.641Z] 
[2020-04-22T02:18:01.641Z] Goroutine 126 (running) created at:
[2020-04-22T02:18:01.641Z]   github.com/pingcap/check.(*suiteRunner).forkCall()
[2020-04-22T02:18:01.641Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/check@v0.0.0-20200212061837-5e12011dc712/check.go:734 +0x4a3

[2020-0422T02:18:01.641Z]   github.com/pingcap/check.(*suiteRunner).runFixture()
[2020-04-22T02:18:01.641Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/check@v0.0.0-20200212061837-5e12011dc712/check.go:751 +0x83
[2020-04-22T02:18:01.641Z]   github.com/pingcap/check.(*suiteRunner).run()
[2020-04-22T02:18:01.641Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/check@v0.0.0-20200212061837-5e12011dc712/check.go:695 +0x127
[2020-04-22T02:18:01.641Z]   github.com/pingcap/check.Run()
[2020-04-22T02:18:01.641Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/check@v0.0.0-20200212061837-5e12011dc712/run.go:150 +0x5a
[2020-04-22T02:18:01.641Z]   github.com/pingcap/check.RunAll()
[2020-04-22T02:18:01.641Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/check@v0.0.0-20200212061837-5e12011dc712/run.go:117 +0x13db
[2020-04-22T02:18:01.641Z]   github.com/pingcap/check.TestingT()
[2020-04-22T02:18:01.641Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/check@v0.0.0-20200212061837-5e12011dc712/run.go:99 +0x751
[2020-04-22T02:18:01.641Z]   github.com/pingcap/tidb/executor/seqtest_test.TestT()
[2020-04-22T02:18:01.641Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/seqtest/seq_executor_test.go:73 +0x163
[2020-04-22T02:18:01.641Z]   testing.tRunner()
[2020-04-22T02:18:01.641Z]       /usr/local/go/src/testing/testing.go:909 +0x199
[2020-04-22T02:18:01.641Z] 
[2020-04-22T02:18:01.642Z] Goroutine 8 (finished) created at:
[2020-04-22T02:18:01.642Z]   github.com/pingcap/tidb/domain.(*Domain).Init()
[2020-04-22T02:18:01.642Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/domain/domain.go:747 +0x796
[2020-04-22T02:18:01.642Z]   github.com/pingcap/tidb/session.(*domainMap).Get.func1()

https://internal.pingcap.net/idc-jenkins/blue/organizations/jenkins/tidb_ghpr_unit_test/detail/tidb_ghpr_unit_test/33522/pipeline

@bb7133
Copy link
Member

bb7133 commented Apr 22, 2020

/run-unit-test

2 similar comments
@coocood
Copy link
Member

coocood commented Apr 22, 2020

/run-unit-test

@bb7133
Copy link
Member

bb7133 commented Apr 22, 2020

/run-unit-test

bb7133 added a commit that referenced this pull request Apr 22, 2020
@coocood coocood merged commit a909102 into pingcap:master Apr 22, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 22, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Apr 22, 2020

cherry pick to release-3.0 in PR #16687

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 22, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Apr 22, 2020

cherry pick to release-3.1 in PR #16688

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 22, 2020
Fix issue pingcap#16669

When check the untouched index, we should also check the memory-buffer in the session too.
@sre-bot
Copy link
Contributor

sre-bot commented Apr 22, 2020

cherry pick to release-4.0 in PR #16689

crazycs520 added a commit to crazycs520/tidb that referenced this pull request Apr 22, 2020
Fix issue pingcap#16669

When check the untouched index, we should also check the memory-buffer in the session too.
crazycs520 added a commit to crazycs520/tidb that referenced this pull request Apr 22, 2020
Fix issue pingcap#16669

When check the untouched index, we should also check the memory-buffer in the session too.
coocood pushed a commit that referenced this pull request Apr 22, 2020
)

Fix issue #16669

When check the untouched index, we should also check the memory-buffer in the session too.
bb7133 pushed a commit that referenced this pull request Apr 22, 2020
)

Fix issue #16669

When check the untouched index, we should also check the memory-buffer in the session too.
crazycs520 added a commit to crazycs520/tidb that referenced this pull request Apr 23, 2020
Fix issue pingcap#16669

When check the untouched index, we should also check the memory-buffer in the session too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
require-LGT3 Indicates that the PR requires three LGTM. sig/execution SIG execution sig/transaction SIG:Transaction status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants