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

session,executor: fix point get under @@tidb_snapshot (#22460) #22527

Merged
merged 8 commits into from
Jan 27, 2021

Conversation

ti-srebot
Copy link
Contributor

@ti-srebot ti-srebot commented Jan 26, 2021

cherry-pick #22460 to release-4.0
You can switch your code base to this Pull Request by using git-extras:

# In tidb repo:
git pr https://github.com/pingcap/tidb/pull/22527

After apply modifications, you can push your change to this PR via:

git push git@github.com:ti-srebot/tidb.git pr/22527:release-4.0-8d6ddedfb101

What problem does this PR solve?

Issue Number: close #22436

Problem Summary:

When the @@tidb_snapshot variable is set, a point get request still gets the latest data rather than a snapshot.

What is changed and how it works?

What's Changed:

  • In PrepareTSFuture, if sessionVars.SnapshotTS is not 0, do nothing
  • In buildExecutor, check sessionVars.SnapshotTS before useMaxTS

How it Works:

In buildExecutor, the useMaxTS logic have a higher priority than sessionVars.SnapshotTS,
so even the @@tidb_snapshot variable is set, the MaxTS is used by point get query.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Release note

  • Fix a bug that point get query does not get the snapshot data when the @@tidb_snapshot variable is set.

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@tiancaiamao you're already a collaborator in bot's repo.

@tiancaiamao
Copy link
Contributor

The original test case is OK on 4.0, but we still need to cherry-pick the PR for this bug:

mysql> create table t (id int key);
Query OK, 0 rows affected (0.02 sec)

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

mysql> select now();
+---------------------+
| now()               |
+---------------------+
| 2021-01-26 11:51:06 |
+---------------------+
1 row in set (0.00 sec)

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

mysql> set @@tidb_snapshot = '2021-01-26 11:51:06';
Query OK, 0 rows affected (0.01 sec)

// This one is OK
mysql> select * from t where id = 7;
Empty set (0.00 sec)

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

// But this one is not correct.
mysql> select * from t where id = 7;
+----+
| id |
+----+
|  7 |
+----+
1 row in set (0.00 sec)

PTAL @lysu @AilinKid

Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

LGTM

@lysu
Copy link
Contributor

lysu commented Jan 26, 2021

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 26, 2021
@lysu
Copy link
Contributor

lysu commented Jan 26, 2021

/approve

qw4990
qw4990 previously approved these changes Jan 26, 2021
Copy link
Contributor

@qw4990 qw4990 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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 26, 2021
@qw4990
Copy link
Contributor

qw4990 commented Jan 26, 2021

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 26, 2021
@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@ti-srebot merge failed.

@qw4990
Copy link
Contributor

qw4990 commented Jan 26, 2021

/merge

@ti-srebot
Copy link
Contributor Author

Your auto merge job has been accepted, waiting for:

  • 22481
  • 21628
  • 21142
  • 21936
  • 21874
  • 20981
  • 21464
  • 21483
  • 21525
  • 21590
  • 21604
  • 21614
  • 21673
  • 21697
  • 21810

@ti-srebot
Copy link
Contributor Author

@ti-srebot merge failed.

@qw4990
Copy link
Contributor

qw4990 commented Jan 26, 2021

/merge

@ti-srebot
Copy link
Contributor Author

Your auto merge job has been accepted, waiting for:

  • 21945
  • 22136
  • 22119
  • 21853
  • 22174
  • 21881

@qw4990
Copy link
Contributor

qw4990 commented Jan 26, 2021

/merge

@ti-srebot
Copy link
Contributor Author

Your auto merge job has been accepted, waiting for:

  • 21945
  • 22136
  • 22119
  • 21853
  • 22174
  • 21881

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@ti-srebot merge failed.

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@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 Jan 26, 2021
@qw4990
Copy link
Contributor

qw4990 commented Jan 27, 2021

/run-all-tests

@qw4990 qw4990 merged commit 67a531f into pingcap:release-4.0 Jan 27, 2021
@tiancaiamao tiancaiamao deleted the release-4.0-8d6ddedfb101 branch January 27, 2021 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/sql-infra SIG: SQL Infra 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. type/4.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants