Feature(onlineddl): Add shard-specific completion to online ddl

[siddharth16396]

Description

This PR implements the feature request in #18335

TL;DR: While Vitess already supports launching schema changes shard by shard, there is currently no way to complete those migrations shard by shard. This PR addresses that gap.

This enhancement facilitates scenarios where teams aim to safely test the impact of schema changes, such as adding an index, on a specific shard before deploying it across the entire keyspace.

Example:
Consider a keyspace with multiple shards (e.g., X shards). The objective is to:

• Introduce a new index to a table.
• Evaluate the query performance improvements resulting from the index.
• Analyze the resource impact (CPU, memory, IO) of the index on the shard.

To limit the blast radius, the schema change should be:

• Applied and tested on a single shard.
• Monitored for metrics and performance before a broader rollout.

This feature enables shard-level control and observability, allowing for targeted testing and validation.

The code changes in this PR add shard-specific completion to online alter

• Enhance CompleteMigration to support shard-specific arguments
• Update test cases to validate shard-based online DDL migrations
• Modify query execution in tabletserver to include shard details
• Adjust SQL parser mappings to incorporate shard-specific changes
• Refactor OnlineDDL executor logic for better shard-based migration handling

This update improves the granularity of migration completion, allowing
shard-specific operations for postponed migrations.

Vitess #general slack discussion with shlomi:: https://vitess.slack.com/archives/C0PQY0PTK/p1737524865276269

Related Issue(s)

Checklist

• "Backport to:" labels have been added if this change should be back-ported to release branches
• If this change is to be back-ported to previous releases, a justification is included in the PR description
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on CI?
• Documentation was added or is not required

Deployment Notes

testing Notes

1. Ran ApplySchema with postpone-completion to add sample table called "Persons"

[root@/home/udocker/db #]vtctldclient ApplySchema --ddl-strategy="vitess --postpone-completion" --sql "CREATE TABLE Persons ( PersonID int,LastName varchar(255),FirstName varchar(255),Address varchar(255),City varchar(255));" arif_test
9d73442c_41d2_11f0_82af_02001707a39a

2. Ran OnlineDDL show on keyspace ::

[root@/home/udocker/db #]vtctldclient  OnlineDDL show arif_test 9d73442c_41d2_11f0_82af_02001707a39a --json
{
  "migrations": [
    {
      "uuid": "9d73442c_41d2_11f0_82af_02001707a39a",
      "keyspace": "arif_test",
      "shard": "-55",
      "schema": "arif_test",
      "table": "Persons",
      "migration_statement": "create table Persons (\n\tPersonID int,\n\tLastName varchar(255),\n\tFirstName varchar(255),\n\tAddress varchar(255),\n\tCity varchar(255)\n)",
      "strategy": "VITESS",
      "options": "--postpone-completion",
      "added_at": {
        "seconds": "1749103328",
        "nanoseconds": 0
      },
      "requested_at": {
        "seconds": "1749103329",
        "nanoseconds": 0
      },
      "ready_at": null,
      "started_at": null,
      "liveness_timestamp": null,
      "completed_at": null,
      "cleaned_up_at": null,
      "status": "QUEUED",
      "log_path": "",
      "artifacts": "",
      "retries": "0",
      "tablet": {
        "cell": "xxx",
        "uid": xxxx
      },
      "tablet_failure": false,
      "progress": 0,
      "migration_context": "vtctl:9c2b8bae-41d2-11f0-82af-02001707a39a",
      "ddl_action": "create",
      "message": "",
      "eta_seconds": "-1",
      "rows_copied": "0",
      "table_rows": "0",
      "added_unique_keys": 0,
      "removed_unique_keys": 0,
      "log_file": "",
      "artifact_retention": {
        "seconds": "86400",
        "nanos": 0
      },
      "postpone_completion": true,
      "removed_unique_key_names": "",
      "dropped_no_default_column_names": "",
      "expanded_column_names": "",
      "revertible_notes": "",
      "allow_concurrent": false,
      "reverted_uuid": "",
      "is_view": false,
      "ready_to_complete": true,
      "vitess_liveness_indicator": "0",
      "user_throttle_ratio": 0,
      "special_plan": "",
      "last_throttled_at": null,
      "component_throttled": "",
      "cancelled_at": null,
      "postpone_launch": false,
      "stage": "",
      "cutover_attempts": 0,
      "is_immediate_operation": true,
      "reviewed_at": {
        "seconds": "1749103330",
        "nanoseconds": 0
      },
      "ready_to_complete_at": {
        "seconds": "1749103330",
        "nanoseconds": 0
      },
      "removed_foreign_key_names": ""
    },
    {
      "uuid": "9d73442c_41d2_11f0_82af_02001707a39a",
      "keyspace": "arif_test",
      "shard": "55-aa",
      "schema": "arif_test",
      "table": "Persons",
      "migration_statement": "create table Persons (\n\tPersonID int,\n\tLastName varchar(255),\n\tFirstName varchar(255),\n\tAddress varchar(255),\n\tCity varchar(255)\n)",
      "strategy": "VITESS",
      "options": "--postpone-completion",
      "added_at": {
        "seconds": "1749103328",
        "nanoseconds": 0
      },
      "requested_at": {
        "seconds": "1749103329",
        "nanoseconds": 0
      },
      "ready_at": null,
      "started_at": null,
      "liveness_timestamp": null,
      "completed_at": null,
      "cleaned_up_at": null,
      "status": "QUEUED",
      "log_path": "",
      "artifacts": "",
      "retries": "0",
      "tablet": {
        "cell": "xxx",
        "uid": xxxx
      },
      "tablet_failure": false,
      "progress": 0,
      "migration_context": "vtctl:9c2b8bae-41d2-11f0-82af-02001707a39a",
      "ddl_action": "create",
      "message": "",
      "eta_seconds": "-1",
      "rows_copied": "0",
      "table_rows": "0",
      "added_unique_keys": 0,
      "removed_unique_keys": 0,
      "log_file": "",
      "artifact_retention": {
        "seconds": "86400",
        "nanos": 0
      },
      "postpone_completion": true,
      "removed_unique_key_names": "",
      "dropped_no_default_column_names": "",
      "expanded_column_names": "",
      "revertible_notes": "",
      "allow_concurrent": false,
      "reverted_uuid": "",
      "is_view": false,
      "ready_to_complete": true,
      "vitess_liveness_indicator": "0",
      "user_throttle_ratio": 0,
      "special_plan": "",
      "last_throttled_at": null,
      "component_throttled": "",
      "cancelled_at": null,
      "postpone_launch": false,
      "stage": "",
      "cutover_attempts": 0,
      "is_immediate_operation": true,
      "reviewed_at": {
        "seconds": "1749103330",
        "nanoseconds": 0
      },
      "ready_to_complete_at": {
        "seconds": "1749103330",
        "nanoseconds": 0
      },
      "removed_foreign_key_names": ""
    },
    {
      "uuid": "9d73442c_41d2_11f0_82af_02001707a39a",
      "keyspace": "arif_test",
      "shard": "aa-",
      "schema": "arif_test",
      "table": "Persons",
      "migration_statement": "create table Persons (\n\tPersonID int,\n\tLastName varchar(255),\n\tFirstName varchar(255),\n\tAddress varchar(255),\n\tCity varchar(255)\n)",
      "strategy": "VITESS",
      "options": "--postpone-completion",
      "added_at": {
        "seconds": "1749103328",
        "nanoseconds": 0
      },
      "requested_at": {
        "seconds": "1749103329",
        "nanoseconds": 0
      },
      "ready_at": null,
      "started_at": null,
      "liveness_timestamp": null,
      "completed_at": null,
      "cleaned_up_at": null,
      "status": "QUEUED",
      "log_path": "",
      "artifacts": "",
      "retries": "0",
      "tablet": {
        "cell": "xxx",
        "uid": xxx
      },
      "tablet_failure": false,
      "progress": 0,
      "migration_context": "vtctl:9c2b8bae-41d2-11f0-82af-02001707a39a",
      "ddl_action": "create",
      "message": "",
      "eta_seconds": "-1",
      "rows_copied": "0",
      "table_rows": "0",
      "added_unique_keys": 0,
      "removed_unique_keys": 0,
      "log_file": "",
      "artifact_retention": {
        "seconds": "86400",
        "nanos": 0
      },
      "postpone_completion": true,
      "removed_unique_key_names": "",
      "dropped_no_default_column_names": "",
      "expanded_column_names": "",
      "revertible_notes": "",
      "allow_concurrent": false,
      "reverted_uuid": "",
      "is_view": false,
      "ready_to_complete": true,
      "vitess_liveness_indicator": "0",
      "user_throttle_ratio": 0,
      "special_plan": "",
      "last_throttled_at": null,
      "component_throttled": "",
      "cancelled_at": null,
      "postpone_launch": false,
      "stage": "",
      "cutover_attempts": 0,
      "is_immediate_operation": true,
      "reviewed_at": {
        "seconds": "1749103330",
        "nanoseconds": 0
      },
      "ready_to_complete_at": {
        "seconds": "1749103330",
        "nanoseconds": 0
      },
      "removed_foreign_key_names": ""
    }
  ]
}

4. Ran command to complete migration on a specific shard only ::

[root@/home/udocker/db #]vtctldclient ApplySchema  --sql "alter vitess_migration '9d73442c_41d2_11f0_82af_02001707a39a' complete vitess_shards 'aa-'" arif_test

5. Exec'd into a different shard and verified table "Persons" does not exist ::

mysql> use arif_test;
mysql> show tables;
+----------------------------------------------------------+
| Tables_in_arif_test                                      |
+----------------------------------------------------------+
| _vt_vrp_95399e6c412f11f0948f0200173485a5_20250604104017_ |
| employees                                                |
| testTable                                                |
+----------------------------------------------------------+
3 rows in set (0.00 sec)

6. Exec'ed into the shard we applied completion on and saw table "Persons" existed. ::

mysql> show tables;
+----------------------------------------------------------+
| Tables_in_arif_test                                      |
+----------------------------------------------------------+
| Persons                                                  |
| _vt_vrp_95399e6c412f11f0948f0200173485a5_20250604104017_ |
| employees                                                |
| testTable                                                |
+----------------------------------------------------------+
4 rows in set (0.00 sec)

[vitess-bot]

Review Checklist

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

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 67.50%. Comparing base (d429368) to head (c87e90f).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vttablet/onlineddl/executor.go	0.00%	6 Missing ⚠️
go/vt/vttablet/tabletserver/query_executor.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #18331      +/-   ##
==========================================
- Coverage   67.51%   67.50%   -0.02%     
==========================================
  Files        1607     1607              
  Lines      262684   262688       +4     
==========================================
- Hits       177343   177315      -28     
- Misses      85341    85373      +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[GrahamCampbell]

Doesn't this get much more complex when planning scatter queries that use select *?

[GrahamCampbell]

I guess if we limit the class of changes that are allowed it could work out. Your use case for changing onyl indexes would fit into this.

[siddharth16396]

Thanks for the note, @GrahamCampbell

You're right that scatter queries using SELECT * can run into issues if the schema is not consistent across shards. That said, even today Vitess already allows schema changes to be launched only on specific shards using ApplySQLSchema with --postpone-launch and then selectively launching on those shard. So this PR doesn’t introduce that inconsistency — it just extends support to complete migrations at the shard level, which currently isn't possible.

At our organisation, we have an internal mandate to do safe, staged schema changes. This means:

• We always roll out schema changes shard by shard, starting with a small percentage (5%), then scaling to 25%, 50%, and finally 100%.
• Our services are built to ignore unknown fields if fetched using selects, and schema changes are always deployed before service code changes. This way, we maintain forward compatibility even if the schema is ahead of the app in some shards and i believe everyone should be doing this (if they are not).

In terms of correctness, i believe it's the responsibility of the operator or database admin to plan and execute shard-by-shard rollouts safely. Enforcing consistency or rollout strategy checks at the Vitess code level would add unnecessary complexity and may not fit all use cases.

This PR gives operators the control and flexibility, while assuming they will apply it responsibly.

[shlomi-noach]

Looks good! This is the exact same way I would have implemented this. A very minor suggestion for the endtoend test.

[shlomi-noach]

Let's add an artificial sleep here just to ensire we're not missing a would-be completion

Suggested change

	// Migration should still be in running state
	time.Sleep(2 * time.Second)
	// Migration should still be in running state

[shlomi-noach]

I would drop this file's changes. The correct place is in onlineddl_scheduler_test.go. I see the test is also failing, but in all honesty, I think we should just remove these changes rather than debug this.

[siddharth16396]

Actually this test was initially failing bcoz i couldn't figure out how to run endtoend tests on my local.
But somehow with the help of github CI pipelines i was able to debug and fix the test.

The current CI is not failing bcoz of this test, but some other tests which i haven't touched.

[siddharth16396]

with respect to correct place for this test, i can place it in onlineddl_scheduler_test.go

[siddharth16396]

WRT the website documentation, i'll create a PR soon.

[siddharth16396]

@shlomi-noach : i have incoporated your reviews.

also i think by mistake (or because i pushed another commit) it requires a re-stamp from you.

[shlomi-noach]

Thank you, looking again. Please do not force-push into reviewed PRs as this means I'll need to re-review the entire PR without understanding what changes were made since my last review 🙏

[siddharth16396]

@shlomi-noach : sorry for the force push, i messed a few things up in my local branch and had no option.

only change i made from last review was :: c87e90f

Which was to incorp your this comment:

Let's add an artificial sleep here just to ensire we're not missing a would-be completion

Nothing else is changed from previous stamp.

[shlomi-noach]

I can see why the test is failing, as the onlineddl_vrepl test has a long successive list of tests which are not isolated, and you've injected one that changes the expected state. We can make that test successful - but I still argue that we shouldn't run the test in onlineddl_vrepl. The tests you've added in onlineddl_scheduler` suffice, and that is the correct test suite for this particular test.

[shlomi-noach]

I pushed a fix and onlineddl_vrepl now passes. I'd still like to remove this entire test.

[siddharth16396]

Thanks a lot for the fix, and like you suggested i will remove the test from onlineddl_vrepl

087bdbb

Updated with new commit to remove that test entirely.

Really appreciate all the help 🙇