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

Online DDL: fast (and correct) RANGE PARTITION operations, 1st iteration #10315

Merged
merged 17 commits into from
May 28, 2022

Conversation

shlomi-noach
Copy link
Contributor

@shlomi-noach shlomi-noach commented May 16, 2022

Description

This PR introduces support for fast ADD PARTITION and DROP PARTITION in RANGE partitioned tables. At this time these fast migrations are not revertible, but a future iteration will make them revertible.

Why, what's the problem with existing implementation?

For both speed and correctness.

  • Speed: adding a new range partition is a simple operation, which does not require a full blown online schema change that copies the entire table over. The MySQL implementation for adding a partition is creating an empty table.
  • Correctness: in a range partitioned table, dropping the oldest partition should also delete all data in that partition. However, the way all online-schema-change tools are built, including vitess's vitess migrations, handles this incorrectly. When you DROP PARTITION in an online schema change tool, the tool creates a shadow table with that partition removed. It then copies over all table rows from the original table. What used to be the 2nd partition (now the first after dropping the oldest partition) turns out to be a good host for all the old partition's rows. The end result is that no rows are deleted in the process. Which not what the user wanted.

How

Confronted with an ALTER TABLE statement, and before sending it over to the appropriate handler (vitess, gh-ost, pt-osc), the executor checks if it can execute the migration using a "special plan". By parsing and analyzing the exact semantics of the query, vitess is able to know a statement is a ADD|DROP of a RANGE PARTITION; and if that is the case, it executes the query directly on the backend MySQL server, rather then invoke an OSC.

Limitations

  • At this time, these migrations are not revertible. There is a path and plan to make them revertible in a next iteration.
  • In anticipation of making these operations revertible, a ALTER TABLE ... DROP PARTITION may only drop one partition at a time (the MySQL syntax allows dropping multiple partitions at once).
  • Right now and while this is a new and in progress behavior, it is protected by a new and undocumented --fast-over-revertible --fast-range-rotation flag in ddl_strategy. This flag will be useful to additional migration types, but we expect it to eventually be irrelevant to ADD|DROP PARTITION queries, as we do intend to make them revertible.

There are no release notes in the meantime as I want to see how next iterations make progress. No documentation for this new behavior yet until we consider it stable.

Related Issue(s)

Checklist

  • "Backport me!" label has been added if this change should be backported
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
…olumn including full partition spec

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach shlomi-noach added Type: Bug Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving release notes none labels May 16, 2022
@shlomi-noach shlomi-noach requested review from rohit-nayak-ps, a team and dbussink May 16, 2022 08:20
@github-actions
Copy link
Contributor

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has the correct release notes label. release notes none should only be used for PRs that are so trivial that they need not be included.
  • If a new flag is being introduced, review whether it is really needed. The flag names should be clear and intuitive (as far as possible), and the flag's help should be descriptive.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should either include a link to an issue that describes the bug OR an actual description of the bug and how to reproduce, along with a description of the fix.

Non-trivial changes

  • There should be some code comments as to why things are implemented the way they are.

New/Existing features

  • Should be documented, either by modifying the existing documentation or creating new documentation.
  • New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • vtctl command output order should be stable and awk-able.

@@ -150,6 +151,11 @@ func (setting *DDLStrategySetting) IsAllowConcurrent() bool {
return setting.hasFlag(allowConcurrentFlag)
}

// IsFastOverRevertibleFlag checks if strategy options include -fast-ver-revertible
Copy link
Contributor

Choose a reason for hiding this comment

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

fast-ver-revertible -> fast-over-revertible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}

// analyzeDropFirstOrLastRangePartition sees if the online DDL drops the first or last partition
// in a range partitioned table
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this only checking if it's the first partition?

And isn't adding checking if it's adding the last? So is this comment here correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed. The code was refactored into a more general form, and the original comment was left behind.

}

// analyzeDropFirstOrLastRangePartition sees if the online DDL drops the first or last partition
// in a range partitioned table
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, I think the comment is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

partitionName := partitionDefinition.Name.String()
// OK then!

// Now, is this query adding a parititon in a RANGE partitioned table?
Copy link
Contributor

Choose a reason for hiding this comment

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

parititon -> partition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


// analyzeSpecialAlterScenarios checks if the given ALTER onlineDDL, and for the current state of affected table,
// can be executed in a special way
func (e *Executor) analyzeSpecialAlterPlan(ctx context.Context, onlineDDL *schema.OnlineDDL) (*SpecialAlterPlan, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment is wrong for the function name here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach
Copy link
Contributor Author

shlomi-noach commented May 16, 2022

Ugh edited online and that lost the signature. Gonna force-push shortly.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach
Copy link
Contributor Author

shlomi-noach commented May 16, 2022

Related, design proposal: #10317

This PR implements the forward migration aspect.

@dbussink
Copy link
Contributor

  • Right now and while this is a new and in progress behavior, it is protected by a new and undocumented --fast-over-revertible flag in ddl_strategy. This flag will be useful to additional migration types, but we expect it to eventually be irrelevant to ADD|DROP PARTITION queries, as we do intend to make them revertible.

This was a thing I had most questions over reading the code, but I think this makes sense. My concern was mostly on whether it's needed to control revertible or not strategies at a finer level or not, say, wanting to use it for partitions but not for an INSTANT ADD COLUMN style DDL operation.

But if this flag becomes eventually irrelevant for partitions, I think most of that concern disappears. That's also because partitions seem to be quite a different beast in general but with a separate strategy for dealing with them as discussed in #10317 I think we're ok.

So tl;dr, I think that if we would keep partitions as is without #10317, we would want to have a separate flag to control partitions vs. other fast over revertible cases, but given #10317 I don't think we need that (so one flag is better then I think).

@shlomi-noach
Copy link
Contributor Author

if we would keep partitions as is without #10317, we would want to have a separate flag to control partitions vs. other fast over revertible cases,

My thought as well, these probably deserve different flags. Let's see how we make progress.

partitionName := spec.Names[0].String()
// OK then!

// Now, is this query dropping the first parititon in a RANGE partitioned table?
Copy link
Contributor

Choose a reason for hiding this comment

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

parititon -> partition.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach
Copy link
Contributor Author

Out of caution, added a new ddl_strategy flag, --fast-range-rotation, which is now what the new fast ADD/DROP PARTITION functionality requires.

--fast-over-revertible remains for now, unused. I see a future for --fast-over-revertible for other types of migrations.

Both flags remain undocumented.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach
Copy link
Contributor Author

Ping for review from the Vitess team

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Query Serving Type: Bug Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants