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

parser: add AsOfClause for START TRANSACTION READ ONLY statement #1215

Merged
merged 10 commits into from
May 18, 2021

Conversation

JmPotato
Copy link
Member

@JmPotato JmPotato commented Apr 27, 2021

Signed-off-by: JmPotato ghzpotato@gmail.com

What problem does this PR solve?

Part of pingcap/tidb#24291. Add AsOfClause for START TRANSACTION READ ONLY statement.

What is changed and how it works?

  • Extend START TRANSACTION READ ONLY statement.
  • Add the TIDB_BOUNDED_STALENESS built-in function.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Related changes: #1206

Signed-off-by: JmPotato <ghzpotato@gmail.com>
@CLAassistant
Copy link

CLAassistant commented Apr 27, 2021

CLA assistant check
All committers have signed the CLA.

@JmPotato
Copy link
Member Author

/cc @nolouch @kennytm @djshow832

Signed-off-by: JmPotato <ghzpotato@gmail.com>
Copy link
Member

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

lgtm, plz reslove the conflict.

@JmPotato
Copy link
Member Author

JmPotato commented Apr 29, 2021

lgtm, plz reslove the conflict.

#1211 introduced a change to TiDB that is not forward compatible. I'm waiting for the merging of pingcap/tidb#24196, then I will resolve the conflict.

Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato JmPotato requested a review from nolouch May 10, 2021 03:42
ast/functions.go Outdated
// TiDBBoundStaleness is used to determine the TS for a read only request.
// It will be used in the Stale Read feature.
// For more info, please see AsOfClause.
TiDBBoundStaleness = "tidb_bound_staleness"
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be bounded staleness rather than bound staleness.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's ok to use the passive voice bound here as an adjective, and it's shorter than bounded.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you google bound staleness, the results are all bounded staleness.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough. So what about tidb_bounded_staleness or tidb_staleness_bound?

Copy link
Member Author

@JmPotato JmPotato May 11, 2021

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I rename it to tidb_staleness_bound, PTAL!

Copy link
Contributor

Choose a reason for hiding this comment

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

@morgo PTAL, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer tidb_bounded_staleness . :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree with @nolouch :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

@morgo @nolouch @djshow832 I finished the renaming, PTAL.

ast/misc.go Outdated Show resolved Hide resolved
JmPotato and others added 2 commits May 10, 2021 17:46
Co-authored-by: djshow832 <zhangming@pingcap.com>
Signed-off-by: JmPotato <ghzpotato@gmail.com>
Signed-off-by: JmPotato <ghzpotato@gmail.com>
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@djshow832
Copy link
Contributor

/lgtm

@ti-srebot ti-srebot added the status/LGT1 LGT1 label May 13, 2021
@morgo
Copy link
Contributor

morgo commented May 13, 2021

/lgtm

@ti-srebot ti-srebot added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels May 13, 2021
ti-srebot
ti-srebot previously approved these changes May 13, 2021
@JmPotato
Copy link
Member Author

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@JmPotato merge failed.

Signed-off-by: JmPotato <ghzpotato@gmail.com>
@ti-srebot ti-srebot added status/LGT3 LGT3. This PR looks very good to our bot. and removed status/LGT2 LGT2 labels May 14, 2021
@nolouch nolouch merged commit 92fa6fe into pingcap:master May 18, 2021
@JmPotato JmPotato deleted the as_of_beginsmt branch May 18, 2021 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge status/LGT3 LGT3. This PR looks very good to our bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants