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

ApplySchema: -skip_preflight #7587

Merged
merged 1 commit into from Mar 4, 2021

Conversation

shlomi-noach
Copy link
Contributor

Description

This PR introduces -skip_preflight flag to vtctl ApplySchema. Preflight checks test the sanity of the schema change. As example, if we add a column c to a table that already has column c, preflight checks will intercept the error and the query never gets sent to the backend tables.

However, preflight checks don't always make sense:

  • With online DDL, I could submit migration#1: add column c, and migration#2 add key c_idx(c). If migration#1 still runs when I submit migration#2, preflight rejects migration#2. This is counter productive to the online DDL concept.
  • On unmanaged setups, preflight may not have the privileges to run, thus becoming impossible to execute.

The new flag -skip_preflight, defaults false for backwards compatibility, completely disables the use of preflight checks. When enabled, the query moves directly to the relevant shards.

Related Issue(s)

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required
    f work. These should note any db migrations, etc. -->

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build/CI
  • VTAdmin

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

The code itself looks fine; but we should probably have a way to do the same for the SQL version of an online DDL (i.e. set @@ddl_strategy='gh-ost';... etc.)? It doesn't seem to be quite as straightforward, unless we introduce a fake argument for the ddl_strategy value, which we strip before passing to gh-ost (or pt-osc)?

@shlomi-noach
Copy link
Contributor Author

The code itself looks fine; but we should probably have a way to do the same for the SQL version of an online DDL

I'm not sure what you're asking. There is no preflight check in the SQL version of Online DDL.

@aquarapid
Copy link
Contributor

I'm not sure what you're asking. There is no preflight check in the SQL version of Online DDL.

Got it; I was under the impression it followed the same flow as ApplySchema with -ddl_strategy.

@shlomi-noach
Copy link
Contributor Author

I was under the impression it followed the same flow as ApplySchema

One more reason to remove preflight from ApplySchema. the code paths are really very different.

@derekperkins
Copy link
Member

ApplySchema is in a pretty bad state as is. It should either be deprecated and removed since it gets in the way as is, or promoted to be the preferred way of making changes and functionality added to incentivize people to use it. I lean towards deprecating it.

@deepthi
Copy link
Member

deepthi commented Mar 4, 2021

ApplySchema is in a pretty bad state as is. It should either be deprecated and removed since it gets in the way as is, or promoted to be the preferred way of making changes and functionality added to incentivize people to use it. I lean towards deprecating it.

I also vote for deprecation.

@deepthi deepthi merged commit fb9bc3d into vitessio:master Mar 4, 2021
@deepthi deepthi deleted the apply-schema-skip-preflight branch March 4, 2021 19:15
@askdba askdba added this to the v10.0 milestone Mar 10, 2021
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