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/Vreplication suite: support ENUM->VARCHAR/TEXT type change #8275

Merged

Conversation

shlomi-noach
Copy link
Contributor

@shlomi-noach shlomi-noach commented Jun 7, 2021

Description

This is a difficult one to solve and this PR is not ready yet. solved.

See github/gh-ost#642 for issue description in gh-ost and see openark/gh-ost#18 for gh-ost solution.

When the user converts a column's type from from ENUM to VARCHAR they expect the new column to contain the textual representation of the enum value. And that's what happens for rows copied via VCopier. But VPlayer copies the numeric value.

Problem is VPlayer (or table plan builder) do not have the information about what the enum looks like. WIP.

Related Issue(s)

#8181

Checklist

  • 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>
…mapping

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

This is now ready to review. Noteworthy:

  • converting ENUM to TEXT requires changes to VReplication logic. This is because VReplication needs to know how to interpret an enum valu in the binary logs.
  • OnlineDDL/vrepl is responsible for identifying an Online DDL which converts an enum to varchar/text
  • The, it parses the enum definition
  • And stores it in VReplication's Rule: a mapping between column name to its enum values. The existence of the mapping will indicate to VReplication that it must use the mapping.
  • To that effect, I've added enum_text in rule's proto. This has to happen, because VReplicatio nitself needs to be able to tell what to do with an enum value.
  • We go the way down to TablePlan (replicator_plan.go). In applyChange, TablePlan looks at the field it read from the binary log. If it is an enum and is non-NULL and there is a mapping for this column, then instead of binding the enum's numeric value, it binds the textual value.
  • This PR does not change behavior for enums otherwise. e.g. if there's a table with enum and there's Online DDL or MoveTables or whatever, without modifying the enum, then behavior is unaffected by this PR.

@shlomi-noach shlomi-noach marked this pull request as ready for review June 13, 2021 08:40
@shlomi-noach shlomi-noach added Component: Build/CI Component: VReplication Type: CI/Build Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Jun 13, 2021
@shlomi-noach shlomi-noach changed the title WIP, Online DDL/Vreplication suite: support ENUM->VARCHAR/TEXT type change Online DDL/Vreplication suite: support ENUM->VARCHAR/TEXT type change Jun 13, 2021
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

Hmmm, I just added a new complex test: enum-to-varchar-rename which both converts a column from enum to test and renames it. This fails. Looking into.

@shlomi-noach shlomi-noach marked this pull request as draft June 14, 2021 05:13
@shlomi-noach
Copy link
Contributor Author

Converting back to draft while checking the new test

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>
@shlomi-noach shlomi-noach marked this pull request as ready for review June 14, 2021 09:03
@shlomi-noach
Copy link
Contributor Author

Ready for review, final.

Fixed the issue failing the added test.
Also, refactored code to match changes in another PR, #8322. Both PRs touch replicator plan in similar way.
Whiever PR gets merged first, will cause merge conflicts on the other, but now they're much more aligned.

Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps left a comment

Choose a reason for hiding this comment

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

lgtm.

@shlomi-noach shlomi-noach merged commit c5bdbc6 into vitessio:main Jun 15, 2021
@shlomi-noach shlomi-noach deleted the online-ddl-vrepl-enum-to-varchar branch June 15, 2021 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Build/CI Component: VReplication Type: CI/Build 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

2 participants