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

txn: update table delta map when really lock keys #21996

Merged

Conversation

blacktear23
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #20975

Problem Summary:

mysql> drop table t2, t1;
Query OK, 0 rows affected (0.04 sec)

mysql> create table t1(a int);
Query OK, 0 rows affected (0.01 sec)

mysql> insert into t1 values (1);
Query OK, 1 row affected (0.00 sec)

mysql> begin pessimistic;
Query OK, 0 rows affected (0.00 sec)

mysql> update t1 set a=a;
Query OK, 0 rows affected (0.00 sec)
Rows matched: 1  Changed: 0  Warnings: 0

/* session 2 */ mysql> create table t2(a int);
Query OK, 0 rows affected (0.00 sec)

mysql> commit;
ERROR 8028 (HY000): Information schema is changed during the execution of the statement(for example, table definition may be updated by other DDL ran in parallel). If you see this error often, try increasing `tidb_max_delta_schema_count`. [try again later]

What is changed and how it works?

What's Changed:
Update missing table ID to table delta map when there has key locked. And skip SchemaChecker when table delta map is empty.

Related changes

  • No

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression
    • Consumes more CPU

Release note

  • txn: Fix missing table delta map track when query has locks.

@blacktear23
Copy link
Contributor Author

@cfzjywxk PTAL

@github-actions github-actions bot added the sig/execution SIG execution label Dec 24, 2020
@cfzjywxk
Copy link
Contributor

@you06 @MyonKeminta PTAL

@cfzjywxk cfzjywxk requested review from jackysp and lysu December 24, 2020 06:14
@blacktear23
Copy link
Contributor Author

@cfzjywxk Shall we cherry-pick it to master?

tk1.MustExec("commit")
}

func (s *testSuite) TestIssue20975SelectForUpdateBatchPointGet(c *C) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to add some cases for pointGetExec and batchPointGetExec

  • Isolation is default RR(SI).
  • Using non-handle unique keys.
  • The correspond handle keys do not exist.
    These unique keys should be locked and thus these lock only transactions will get schema check during 2pc.

@blacktear23
Copy link
Contributor Author

@cfzjywxk PTAL

@you06
Copy link
Contributor

you06 commented Dec 24, 2020

The schema check will be skipped here because the ddl lease is 0s.

if s.lease == 0 {
return nil, ResultSucc
}

@blacktear23
Copy link
Contributor Author

The schema check will be skipped here because the ddl lease is 0s.

if s.lease == 0 {
return nil, ResultSucc
}

Is this has some problem? For tidb server the lease time should be 45s in configuration file. So seems this check always fails.

@you06
Copy link
Contributor

you06 commented Dec 24, 2020

The schema check will be skipped here because the ddl lease is 0s.

if s.lease == 0 {
return nil, ResultSucc
}

Is this has some problem? For tidb server the lease time should be 45s in configuration file. So seems this check always fails.

I think the problem comes here

session.SetSchemaLease(0)

@cfzjywxk
Copy link
Contributor

Maybe we could move the test cases to pessimistic_test.go and the lease is not zero.

@blacktear23
Copy link
Contributor Author

Maybe we could move the test cases to pessimistic_test.go and the lease is not zero.

Good idea.

@blacktear23
Copy link
Contributor Author

The schema check will be skipped here because the ddl lease is 0s.

if s.lease == 0 {
return nil, ResultSucc
}

Is this has some problem? For tidb server the lease time should be 45s in configuration file. So seems this check always fails.

I think the problem comes here

session.SetSchemaLease(0)

Why those test case set lease to zero?

@you06
Copy link
Contributor

you06 commented Dec 24, 2020

No idea why changing default lease value, it does pollutes the test environment.

@you06
Copy link
Contributor

you06 commented Dec 24, 2020

LGTM

@cfzjywxk cfzjywxk added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Dec 24, 2020
@cfzjywxk
Copy link
Contributor

@lysu @jackysp PTAL

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

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 25, 2020
@jackysp jackysp added the sig/transaction SIG:Transaction label Dec 25, 2020
@cfzjywxk cfzjywxk added the require-LGT3 Indicates that the PR requires three LGTM. label Dec 25, 2020
@lysu
Copy link
Contributor

lysu commented Dec 25, 2020

LGTM

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 25, 2020
@lysu
Copy link
Contributor

lysu commented Dec 25, 2020

@you06 PTAL need another LGTM

@cfzjywxk
Copy link
Contributor

LGTM

@ti-srebot ti-srebot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Dec 25, 2020
@cfzjywxk
Copy link
Contributor

/merge

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

/run-all-tests

@ti-srebot ti-srebot merged commit a59dabf into pingcap:release-4.0 Dec 25, 2020
@blacktear23
Copy link
Contributor Author

@cfzjywxk shall we cherry-pick it to master?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/release-blocker This issue blocks a release. Please solve it ASAP. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants