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

Add Cloud Spanner style read only transaction #610

Merged

Conversation

sunxiaoguang
Copy link
Contributor

@sunxiaoguang sunxiaoguang commented Nov 2, 2019

What is changed and how it works?

Add extra syntax for Cloud Spanner style read only transaction

Tests

  • Unit test
    parser_test.go was modified to include tests for new syntax

Code changes

  • Has exported function/method change
    No
  • Has exported variable/fields change
    Yes
  • Has interface methods change
    No

@sunxiaoguang sunxiaoguang requested a review from a team November 2, 2019 13:31
@ghost ghost requested review from kennytm and removed request for a team November 2, 2019 13:31
@codecov
Copy link

codecov bot commented Nov 2, 2019

Codecov Report

Merging #610 into master will decrease coverage by 0.06%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #610      +/-   ##
==========================================
- Coverage    79.9%   79.84%   -0.07%     
==========================================
  Files          32       32              
  Lines       12613    12631      +18     
==========================================
+ Hits        10079    10085       +6     
- Misses       1933     1949      +16     
+ Partials      601      597       -4
Impacted Files Coverage Δ
parser.go 92.72% <ø> (-0.16%) ⬇️
misc.go 96.49% <ø> (ø) ⬆️
ast/dml.go 73.96% <ø> (ø) ⬆️
ast/expressions.go 72.79% <100%> (+0.04%) ⬆️
ast/misc.go 75.17% <92%> (-0.23%) ⬇️
charset/charset.go 87.01% <0%> (ø) ⬆️
yy_parser.go 83.46% <0%> (ø) ⬆️
model/ddl.go 76.99% <0%> (ø) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d7cdc1...76311c5. Read the comment docs.

ast/dml.go Outdated Show resolved Hide resolved
ast/misc.go Show resolved Hide resolved
parser.y Outdated Show resolved Hide resolved
parser.y Outdated Show resolved Hide resolved
parser.y Outdated Show resolved Hide resolved
parser.y Outdated Show resolved Hide resolved
| "START" "TRANSACTION" "WITH" "CONSISTENT" "SNAPSHOT"
{
$$ = &ast.BeginStmt{}
}
| "START" "TRANSACTION" "READ" "ONLY"
{
$$ = &ast.BeginStmt{
Copy link
Collaborator

Choose a reason for hiding this comment

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

TiDB should handle this later on, because MySQL also has "start transaction read only" and we should keep the compatibility.

Copy link
Contributor Author

@sunxiaoguang sunxiaoguang Nov 4, 2019

Choose a reason for hiding this comment

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

START TRANSACTION READ ONLY is parsed separately which doesn't specify any Bound therefore can have different execution plan on TiDB.

In addition it would have the same semantic as START TRANSACTION READ ONLY WITH TIMESTAMP BOUND STRONG. Therefore we can add them together in this PR.

parser.y Outdated Show resolved Hide resolved
{"START TRANSACTION READ ONLY", true, "START TRANSACTION READ ONLY"},
{"START TRANSACTION READ ONLY WITH TIMESTAMP BOUND", false, ""},
{"START TRANSACTION READ ONLY WITH TIMESTAMP BOUND STRONG", true, "START TRANSACTION READ ONLY WITH TIMESTAMP BOUND STRONG"},
{"START TRANSACTION READ ONLY WITH TIMESTAMP BOUND MAX STALENESS '00:00:10'", true, "START TRANSACTION READ ONLY WITH TIMESTAMP BOUND MAX STALENESS '00:00:10'"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this SQL a valid one?

START TRANSACTION READ ONLY WITH TIMESTAMP BOUND MAX STALENESS 1 + 1

Copy link
Contributor Author

@sunxiaoguang sunxiaoguang Nov 4, 2019

Choose a reason for hiding this comment

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

STALENESS should take an expression that can be evaluated or converted to TIME.
TIMESTAMP would take an expression that can be evaluated or converted to DATETIME or TIMESTAMP on the other hand.

@tiancaiamao
Copy link
Collaborator

LGTM

Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

Rest LGTM

ast/misc.go Outdated Show resolved Hide resolved
…_spanner_style_read_only_transaction

Signed-off-by: Xiaoguang Sun <[email protected]>
…_spanner_style_read_only_transaction

Signed-off-by: Xiaoguang Sun <[email protected]>
Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

LGTM

@kennytm kennytm added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels Nov 10, 2019
@kennytm kennytm merged commit 83794f7 into pingcap:master Nov 10, 2019
@sunxiaoguang sunxiaoguang deleted the cloud_spanner_style_read_only_transaction branch November 11, 2019 00:52
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
* Add Cloud Spanner style read only transaction

* Put non keywords to the right place

Signed-off-by: Xiaoguang Sun <[email protected]>
lyonzhi pushed a commit to lyonzhi/parser that referenced this pull request Apr 25, 2024
* Add Cloud Spanner style read only transaction

* Put non keywords to the right place

Signed-off-by: Xiaoguang Sun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants