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

Rename drop_constraints to drop_foreign_keys on MoveTables #10017

Merged
merged 3 commits into from
Apr 4, 2022
Merged

Rename drop_constraints to drop_foreign_keys on MoveTables #10017

merged 3 commits into from
Apr 4, 2022

Conversation

Phanatic
Copy link

@Phanatic Phanatic commented Mar 31, 2022

Signed-off-by: Phani Raj phani@planetscale.com

Description

As a follow-up to #9904, we're renaming drop_constraints to drop_foreign_keys on all tables when a user chooses to do so, on running MoveTables.

Related Issue(s)

N/A

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

…raints on MoveTables

Signed-off-by: Phani Raj <phani@planetscale.com>
Copy link
Member

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

LGTM overall.

go/vt/wrangler/materializer.go Outdated Show resolved Hide resolved
go/vt/vtctl/vtctl.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mattlord mattlord left a comment

Choose a reason for hiding this comment

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

I had one question but otherwise LGTM. Thanks!

createDDLAsCopy = "copy"
createDDLAsCopyDropConstraint = "copy:drop_constraint"
createDDLAsCopy = "copy"
createDDLAsCopyDropConstraint = "copy:drop_constraint"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this? Or do we want to support dropping FK and Check constraints at some point? Note that TableSpec.Constraints is aware of both and we seem to only care bout FK constraints here: https://github.com/vitessio/vitess/pull/10017/files#diff-9a8066951e861d9d5329f7cf03e5b70e60cbe19226c2a903efeb19c63781d8ceR1091-R1093

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we say authoritatively that we will never need to drop all constraints? If so, we will need to run a deprecation cycle to remove copy:drop_constraint, since it is possible there are applications using this flag at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that makes sense.

Phani Raj added 2 commits April 4, 2022 15:43
Signed-off-by: Phani Raj <phani@planetscale.com>
…-move-tables

Signed-off-by: Phani Raj <phani@planetscale.com>
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

@rohit-nayak-ps rohit-nayak-ps merged commit 32c7008 into vitessio:main Apr 4, 2022
@deepthi deepthi changed the title Rename drop_constraints to drop_fk_constraints on MoveTables Rename drop_constraints to drop_foreign_keys on MoveTables Jun 28, 2022
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.

None yet

5 participants