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

ddl : support alter table xx cache operations #29022

Merged
merged 24 commits into from
Oct 25, 2021
Merged

ddl : support alter table xx cache operations #29022

merged 24 commits into from
Oct 25, 2021

Conversation

JayLZhou
Copy link
Contributor

@JayLZhou JayLZhou commented Oct 21, 2021

What problem does this PR solve?

handle DDL schema changes when a table is set to cachable , we use synatx alter table xx cache to switch a table to cachable
Problem Summary: See Optimizing hotspot small tables #25293

What is changed and how it works?

support switch a table to cachable operation. and do some compatibility check with other feature.
ALTER TABLE t CACHE is a DDL operation. for consistency under distributed conditions. an intermediate Switching state is introduced, the schema change process is similar to the table lock implementation:
Disabled => Switching => Enabled

work

Check List

Tests

  • Unit test

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Oct 21, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • djshow832
  • tiancaiamao

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 21, 2021
@tiancaiamao tiancaiamao changed the title DDL : support alter table xx cache operations ddl: support alter table xx cache operations Oct 21, 2021
@JayLZhou JayLZhou changed the title ddl: support alter table xx cache operations ddl : support alter table xx cache operations Oct 21, 2021
@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 21, 2021
@JayLZhou
Copy link
Contributor Author

JayLZhou commented Oct 22, 2021

./rebuild

Copy link
Contributor

@tiancaiamao tiancaiamao 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

parser/model/ddl.go Outdated Show resolved Hide resolved
parser/model/model.go Outdated Show resolved Hide resolved
@JayLZhou
Copy link
Contributor Author

./rebuild

@@ -6587,3 +6589,20 @@ func (d *ddl) AlterPlacementPolicy(ctx sessionctx.Context, stmt *ast.AlterPlacem
err = d.callHookOnChanged(err)
return errors.Trace(err)
}
func (d *ddl) AlterTableCache(ctx sessionctx.Context, ti ast.Ident) (err error) {
schema, t, err := d.getSchemaAndTableByIdent(ctx, ti)
Copy link
Contributor

Choose a reason for hiding this comment

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

Check if it is already cached. If it is, we don't even need to add a job.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is checking the type of the table (is it a view) one of the feature compatibilities you mentioned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i check if is a already cached in onAlterTableCache function. if needed i can change its position

Copy link
Contributor

Choose a reason for hiding this comment

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

Typically, we check it twice. One before creating the job and another in executing the job.
Also, please notice the other question I asked above.

ddl/ddl_api.go Outdated
Comment on lines 6598 to 6599
SchemaID: schema.ID,
TableID: t.Meta().ID,
Copy link
Contributor

Choose a reason for hiding this comment

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

What about SchemaName?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...i not use it. so i am not transfer it

Copy link
Contributor

Choose a reason for hiding this comment

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

We use it in some places, such as error reporting and show DDL jobs. It's better to follow the convention.

@tiancaiamao tiancaiamao mentioned this pull request Oct 25, 2021
32 tasks
@JayLZhou
Copy link
Contributor Author

@djshow832 please PTAL again

@JayLZhou
Copy link
Contributor Author

./rebuild

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 25, 2021
@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 25, 2021
@tiancaiamao
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: f44f76d

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 25, 2021
@ti-chi-bot ti-chi-bot merged commit 1777bd0 into pingcap:master Oct 25, 2021
@tiancaiamao
Copy link
Contributor

close #29085

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants