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

VReplication (and by product, Online DDL): support GENERATED column as part of PRIMARY KEY #8335

Conversation

shlomi-noach
Copy link
Contributor

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

Description

Worked to add/fix Online DDL/VReplication suite test generated-columns57-unique, an dended up fixing something more generic to VReplication:

  1. In MySQL it is valid to have a generated column as part of the PRIMARY KEY.
  2. In VReplication, we skip reading GENERATED columns
  3. In VReplication, we validate we've read all PRIMARY KEY columns

The combination of (2+3) contradicts (1).

With this PR, VReplication does not require (3) for GENERATED columns.

Related Issue(s)

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

…s part of PRIMARY KEY

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

This is nice! Another edge case in vreplication handled :-)

The online ddl test tests both the copy and replication phase, right? So we don't need to add explicit tests for this case as part of the vcopier/vplayer testsuite, right?

@shlomi-noach
Copy link
Contributor Author

The online ddl test tests both the copy and replication phase, right?

Correct!

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 e182ce2 into vitessio:main Jun 15, 2021
@shlomi-noach shlomi-noach deleted the vreplication-generated-column-part-of-pk branch June 15, 2021 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants