Unify PKE selection semantics across schemadiff consumers - #19602
varundeepsaini wants to merge 1 commit into
Conversation
8f8af7e to
1dcb6f4
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #19602 +/- ##
===========================================
+ Coverage 69.67% 74.49% +4.82%
===========================================
Files 1614 238 -1376
Lines 216793 41159 -175634
===========================================
- Hits 151044 30662 -120382
+ Misses 65749 10497 -55252
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR consolidates Primary Key Equivalent (PKE) selection so OnlineDDL and VReplication workflows share a single canonical implementation in schemadiff, and migrates VReplication-related call sites away from mysqlctl.GetPrimaryKeyEquivalentColumns.
Changes:
- Added
schemadiff.GetPrimaryKeyEquivalent(plus supporting type-cost ranking utilities) and updated OnlineDDL key prioritization to use the same ranking approach. - Migrated VReplication consumers (vstreamer, vdiff, vreplicator) to derive PKE from
SHOW CREATE TABLE+ SQL parsing instead of queryinginformation_schema. - Deprecated
mysqlctl.GetPrimaryKeyEquivalentColumnsand expanded tests to validate parity.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| go/vt/wrangler/fake_tablet_test.go | Updates test wiring for new vdiff.NewEngine signature using vtenv.Environment. |
| go/vt/vttablet/tabletserver/vstreamer/engine.go | Switches PKE discovery to SHOW CREATE TABLE + schemadiff.GetPrimaryKeyEquivalent. |
| go/vt/vttablet/tabletmanager/vreplication/vreplicator.go | Uses schemadiff.GetPrimaryKeyEquivalent based on td.Schema instead of mysqlctl PKE selection. |
| go/vt/vttablet/tabletmanager/vreplication/vreplicator_test.go | Adds parity checks between deprecated mysqlctl function and schemadiff.GetPrimaryKeyEquivalent. |
| go/vt/vttablet/tabletmanager/vdiff/workflow_differ_test.go | Updates mocked DB expectations to SHOW CREATE TABLE flow. |
| go/vt/vttablet/tabletmanager/vdiff/table_plan.go | Changes PKE lookup to parse SHOW CREATE TABLE output via schemadiff. |
| go/vt/vttablet/tabletmanager/vdiff/table_differ.go | Uses schemadiff parsing of sourceTable.Schema to determine PKE columns. |
| go/vt/vttablet/tabletmanager/vdiff/engine.go | Simplifies engine construction to accept a single *vtenv.Environment. |
| go/vt/schemadiff/onlineddl.go | Introduces IsValidPKEquivalent and GetPrimaryKeyEquivalent; updates key ranking to use type-cost sum. |
| go/vt/schemadiff/onlineddl_test.go | Adds coverage for GetPrimaryKeyEquivalent and updates ordering expectations. |
| go/vt/schemadiff/mysql.go | Adds shared type-cost table and TypeCost() helper for PKE ranking. |
| go/vt/schemadiff/key.go | Adds index-level TypeCost() aggregation for ranking. |
| go/vt/schemadiff/column.go | Adds column-level TypeCost() used by index ranking. |
| go/vt/schemadiff/env.go | Adds NewEnvWithDefaults for consistent schemadiff environment creation. |
| go/vt/mysqlctl/schema.go | Deprecates GetPrimaryKeyEquivalentColumns in favor of schemadiff API. |
| go/cmd/vttablet/cli/cli.go | Updates vttablet initialization to use new vdiff.NewEngine(ts, tablet, env) signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
1dcb6f4 to
f8e5411
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
I dug through the
Tests I think are missing before this can safely merge:
The call-site cleanup itself looks reasonable to me. The risky part is that the canonical ranking logic is not actually canonical with the legacy behavior yet. |
f8e5411 to
f75ad13
Compare
varundeepsaini
changed the title
|
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
github-actions
Bot
added
the
Stale
|
Don't close |
github-actions
Bot
removed
the
Stale
f75ad13 to
a8897c6
Compare
|
@codex-maintainers please review |
mattlord
added
Type: Enhancement
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e6d2d1aac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
6e6d2d1 to
c653caf
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
We should not treat an unparseable schema as having no PKE. It seems like this fallback can silently skip rows. For example, given a nullable column before a non-null unique key:
(payload TEXT NULL, id BIGINT NOT NULL, UNIQUE KEY (id))an unsupported table option makes parsing fail, so RowStreamer uses every column instead of id. If a copy cycle checkpoints on a row where payload IS NULL, the resume predicate contains payload = NULL / payload > NULL, which never matches, and subsequent rows can be omitted. It also replaces an indexed traversal with a full filesort.
I think we should distinguish “schema could not be parsed” from “schema has no PKE”: either use a safe metadata-based fallback or fail closed. Could we add a multi-cycle resume test with a nullable column, a valid PKE, and an unsupported table option?
c653caf to
091b10a
Compare
Replace duplicated primary-key-equivalent selection: OnlineDDL's first-column heuristics and mysqlctl's information_schema query are superseded by schemadiff.GetPrimaryKeyEquivalent, which parses CREATE TABLE and ranks unique non-NULLable keys by total type cost, then column count, then index name. vdiff, vreplicator, and vstreamer now use it. A CREATE TABLE statement the parser cannot handle (e.g. one using the SECONDARY_ENGINE option) is not treated as having no PKE: that would key the stream on every column, where resuming a copy from a checkpoint holding a NULL value builds a resume predicate that never matches and silently skips the remaining rows. Such tables fall back to the retained mysqlctl information_schema lookup, now hardened to reject unique keys containing functional (expression) key parts -- previously it could return such a key as a corrupt, non-unique column list. - Online DDL key prioritization now ranks by whole-key type-cost sum; behavior change documented in the v25 release notes. VReplication PKE selection semantics are unchanged. - Resumed lastpk values are validated against the currently selected key columns to fail closed if the key changes across an upgrade. - Warn when no CREATE TABLE schema is available to derive a PKE. Closes vitessio#10259 Signed-off-by: Varun Deep Saini <deepsainivarun@gmail.com>
091b10a to
58f0c28
Compare
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
-
Blocking: I think VDiff needs to persist the physical source key field names.
rowstreamer.go:276-284now compareslastpk.Fieldswith the physical source key columns, buttable_differ.go:877-899builds both the target and source checkpoints from the target table fields. For a supported transform such asselect c0 as c1, a resumed VDiff suppliesc1while the source key is stillc0, so the new check rejects a valid resume. I think we should capture the source stream’sPkfieldsor translate aliases back to source names, with a stop/resume regression test using a renamed key column. -
Blocking: I think the key-selection change needs a mixed-version-safe rollout. A new source tablet validates the stored field names, but an older source tablet discards that metadata and only checks the number of values. During a rolling upgrade, VReplication can checkpoint against a newly selected PKE, reconnect to an older eligible source tablet, and have that tablet resume the values against a different same-width key. That seems like it can silently skip or duplicate rows. I think we should either stage the selection change across releases or otherwise preserve the legacy selection while old source tablets may still serve streams, and add an upgrade/downgrade copy-resume test.
-
Blocking: I think the fallback query should use the same deterministic tie-break as the parser path.
GetPrimaryKeyEquivalent()uses the index name after cost and column count, butmysqlctl/schema.go:626still orders only bytype_cost, col_count. Equal-cost keys can therefore resolve differently when the parser fallback is used, causing a later tablet switch or parser upgrade to stop the copy under the new validation. AddingINDEX_NAMEto the fallback ordering and a real-MySQL parity case with tied keys should keep both implementations aligned.
|
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
github-actions
Bot
added
the
Stale
Description
Unifies Primary Key Equivalent (PKE) selection for OnlineDDL and VReplication on a single
schemadiffimplementation, replacing the legacyinformation_schemaquery inmysqlctl.schemadiff.GetPrimaryKeyEquivalent()parsesCREATE TABLEand ranks keys by total type cost, then column count — same semantics as the legacy SQL. Ties broken by index name.mysqlctl.GetPrimaryKeyEquivalentColumnsis deprecated. A parity test runs both against a real MySQL.PrioritizedUniqueKeysnow uses the same whole-key cost ranking (behavior change, see release notes). VReplication selection unchanged.lastpkgenerated with different key columns instead of resuming from the wrong position.Related Issue(s)
Checklist
Behavior change; not intended for backporting.
Deployment Notes
See v25 release notes (
changelog/25.0/25.0.0/summary.md): OnlineDDL may pick a different (still valid) unique key; expression keys are no longer PKE candidates; unparseable no-PK schemas fall back to all columns with a warning; in-flight no-PK copies may need a restart across the upgrade.AI Disclosure
Took help of opus 4.8