parser: support create and alter placement policy #1313
Conversation
Signed-off-by: ailinkid <[email protected]>
Signed-off-by: ailinkid <[email protected]>
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Signed-off-by: ailinkid <[email protected]>
| {"create placement policy x learner_constraints='ww'", true, "CREATE PLACEMENT POLICY `x` LEARNER_CONSTRAINTS = 'ww'"}, | ||
| {"create placement policy x primary_region='cn' regions='us' schedule='even'", true, "CREATE PLACEMENT POLICY `x` PRIMARY_REGION = 'cn' REGIONS = 'us' SCHEDULE = 'even'"}, | ||
| {"create placement policy x primary_region='cn', leader_constraints='ww', leader_constraints='yy'", true, "CREATE PLACEMENT POLICY `x` PRIMARY_REGION = 'cn' LEADER_CONSTRAINTS = 'ww' LEADER_CONSTRAINTS = 'yy'"}, | ||
| {"create placement policy if not exists x regions = 'us', follower_constraints='yy'", true, "CREATE PLACEMENT POLICY IF NOT EXISTS `x` REGIONS = 'us' FOLLOWER_CONSTRAINTS = 'yy'"}, |
There was a problem hiding this comment.
What happens if we execute "create placement policy x placement policy y"? I think it is better to make it as a syntax error instead of handling it in planner.
There was a problem hiding this comment.
PlacementOption2 is not a good name, what about DirectPlacementOption or somethine similar?
Signed-off-by: ailinkid <[email protected]>
Signed-off-by: ailinkid <[email protected]>
kennytm
changed the title
Signed-off-by: ailinkid <[email protected]>
|
/cc @lcwangchao @xhebox |
| @@ -1568,6 +1586,9 @@ PlacementOption: | |||
| { | |||
| $$ = &ast.PlacementOption{Tp: ast.PlacementOptionLearnerConstraints, StrValue: $3} | |||
| } | |||
|
|
|||
| PlacementOption: | |||
| DirectPlacementOption | |||
| | "PLACEMENT" "POLICY" EqOpt stringLit | |||
There was a problem hiding this comment.
Is PlacementOption used now? May be below is better because a database or table can not assign a policy and set the direct placement option at the same time.
PlacementPolicyOrOptionList:
PlacementOptionList
| "PLACEMENT" "POLICY" EqOpt stringLit
There was a problem hiding this comment.
DirectPlacementOption is a basic option set for create policy usage.
PlacementOption is super set of DirectPlacementOption, plus placement policy for create table usage.
a database or table can't assign a policy and set the direct placement option at the same time, @morgo Is this true?
I think alter table t placement policy = x, follower_constraints='yy', other placement_opt is tolerant, the application will check whether these options together are compatible like old placement rules.
There was a problem hiding this comment.
Yes that's true. For simplification we decided setting a placement policy unsets all placement options (and vice versa)
There was a problem hiding this comment.
After code trying. we couldn't convert PlacementPolicyOrOptionList to a single database option since databaseOptList assume it's element is every single database option, no a list here.
for example
placement policy = x character set='utf8' follower_constraints='yy'
parser will treat every token individually, every element is a database option, we couldn't divide/tell the placement_options list and placement_policy apart at the first beginning. For this case character_set is not a placement option.
We should handle this in the TiDB side, what's your option? @lcwangchao
There was a problem hiding this comment.
I think you are right, we should at least guarantee follower=1 character set='utf8' follower_constraints='yy' can be accepted by parser.
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: 4aedcff
|
What problem does this PR solve?
support create and alter placement policy
What is changed and how it works?
add syntax for
1:
create placement policy if not exists x placement_opts2:
alter placement policy if exists x placement_optsthis is based on #1299, wait for it merged first.
Check List
Tests
Related changes