-
Notifications
You must be signed in to change notification settings - Fork 489
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
*: support parsing BACKUP and RESTORE statements #746
Conversation
e62fb52
to
f9e078c
Compare
Codecov Report
@@ Coverage Diff @@
## master #746 +/- ##
==========================================
+ Coverage 77.82% 78.03% +0.21%
==========================================
Files 40 40
Lines 14421 14632 +211
==========================================
+ Hits 11223 11418 +195
- Misses 2514 2535 +21
+ Partials 684 679 -5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
} | ||
| "RATE_LIMIT" EqOpt LengthNum "MB" '/' "SECOND" | ||
{ | ||
// TODO: check overflow? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can it overflow ❔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tangenta RATE_LIMIT = 9223372036854775807 MB/SECOND
... admittedly this is not a real issue 😅
{"backup database * to 'noop://' rate_limit 500 MB/second snapshot 5 minute ago", true, "BACKUP DATABASE * TO 'noop://' RATE_LIMIT = 500 MB/SECOND SNAPSHOT = 300000000 MICROSECOND AGO"}, | ||
{"restore table g from 'noop://' concurrency 40 checksum 0 online 1", true, "RESTORE TABLE `g` FROM 'noop://' CONCURRENCY = 40 CHECKSUM = 0 ONLINE = 1"}, | ||
{ | ||
// FIXME: should we really include the access key in the Restore() text??? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also handle SetPwdStmt.Restore()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😱. Perhaps deal with this later (e.g. add a WriteSecureString()
method). This needs to be considered together with how TiDB uses Restore()
.
LGTM |
@kennytm please resolve conflicts, it seems to be merged soon |
PTAL again @tangenta @tiancaiamao thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM |
* *: support parsing BACKUP and RESTORE statements * parser: add SNAPSHOT = 'str' option to BACKUP statement
* *: support parsing BACKUP and RESTORE statements * parser: add SNAPSHOT = 'str' option to BACKUP statement
What problem does this PR solve?
Added the BACKUP, RESTORE and SHOW BACKUP/RESTORE statements to parser.
These are needed for integrating BR into TiDB (PR will come later).
(Note: because the new unresolved keyword
S3_USE_ACCELERATE_ENDPOINT
is longer than all existing tokens, forcinggofmt
to replace the entire token map, I've taken advantage of this to sort the entire tokenMap)What is changed and how it works?
↑
Check List
Tests
Code changes
Side effects
Related changes