fix(vdiff): preserve PK definition order in getSourcePKCols - #20603
fix(vdiff): preserve PK definition order in getSourcePKCols#20603pedroalb wants to merge 19 commits into
Conversation
vitess-bot
Bot
added
NeedsWebsiteDocsUpdate
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
There was a problem hiding this comment.
Pull request overview
Preserves source primary-key definition order when saving VDiff resume progress.
Changes:
- Maps table columns to their row indices.
- Builds
sourcePkColsinPrimaryKeyColumnsorder. - Adds regression coverage for reordered composite keys.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
go/vt/vttablet/tabletmanager/vdiff/table_differ.go |
Preserves source PK definition order. |
go/vt/vttablet/tabletmanager/vdiff/source_pk_cols_ordering_test.go |
Tests composite PK ordering scenarios. |
![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: 2cd6c3c810
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
374d828 to
0918584
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20603 +/- ##
==========================================
+ Coverage 69.67% 72.40% +2.73%
==========================================
Files 1614 905 -709
Lines 216793 161730 -55063
==========================================
- Hits 151044 117097 -33947
+ Misses 65749 44633 -21116
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:
|
mattlord
added
Type: Bug
and removed
NeedsDescriptionUpdate
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
go/vt/vttablet/tabletmanager/vdiff/framework_test.go:494
GetSchemafilters by exact string match andExecuteFetchAsAppmatches PKE results viastrings.Contains(query, tableName). Both are prone to false negatives/positives (e.g., case differences likeT2vst2, or substring collisions liket1matchingt10) which can make tests flaky or accidentally exercise the wrong path. Prefer case-insensitive equality for table-name filtering (e.g.,strings.EqualFold), and forExecuteFetchAsAppuse a more robust match (e.g., map expected results by exact query string / regex, or parse/extract the referenced table name rather than substring matching).
func (tmc *fakeTMClient) GetSchema(ctx context.Context, tablet *topodatapb.Tablet, request *tabletmanagerdatapb.GetSchemaRequest) (*tabletmanagerdatapb.SchemaDefinition, error) {
if len(request.Tables) == 0 {
return tmc.schema, nil
}
filtered := &tabletmanagerdatapb.SchemaDefinition{
TableDefinitions: make([]*tabletmanagerdatapb.TableDefinition, 0),
}
for _, td := range tmc.schema.TableDefinitions {
if slices.Contains(request.Tables, td.Name) {
filtered.TableDefinitions = append(filtered.TableDefinitions, td)
}
}
return filtered, nil
}
func (tmc *fakeTMClient) ExecuteFetchAsApp(ctx context.Context, tablet *topodatapb.Tablet, usePool bool, req *tabletmanagerdatapb.ExecuteFetchAsAppRequest) (*querypb.QueryResult, error) {
query := string(req.Query)
for tableName, result := range tmc.pkeResults {
if strings.Contains(query, tableName) {
return sqltypes.ResultToProto3(result), nil
}
}
return sqltypes.ResultToProto3(noResults), nil
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
go/vt/vttablet/tabletmanager/vdiff/table_differ.go:976
- This turns any unsupported
FROMshape (joins, derived tables, multiple tables) into a hard VDiff failure. If those query shapes are possible in existing VReplication filters, this is a breaking behavioral change. A safer approach is to treat 'cannot resolve a single physical source table' as a 'non-resumable checkpoint' case (setsourceCheckpointUnavailable = true) and proceed without persistinglastpk, instead of failing the entire diff.
sourceTableName, err := sourceTableNameFromSelect(sourceSelect)
if err != nil {
return vterrors.Wrapf(err, "failed to determine source table for target table %s", td.table.Name)
}
go/vt/vttablet/tabletmanager/vdiff/framework_test.go:494
- Using
strings.Contains(query, tableName)for routing mock PKE results can produce accidental matches (e.g., table name appearing in a column/index name or comment), making tests flaky/hard to reason about. Consider keying on a more precise match (e.g., matching the exactFROM <table>/ information_schema predicate pattern, or using a compiled regexp per table).
func (tmc *fakeTMClient) ExecuteFetchAsApp(ctx context.Context, tablet *topodatapb.Tablet, usePool bool, req *tabletmanagerdatapb.ExecuteFetchAsAppRequest) (*querypb.QueryResult, error) {
query := string(req.Query)
for tableName, result := range tmc.pkeResults {
if strings.Contains(query, tableName) {
return sqltypes.ResultToProto3(result), nil
}
}
return sqltypes.ResultToProto3(noResults), nil
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20df9ad963
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Two follow-up correctness fixes for the source-checkpoint-unavailable case: - Reset the report on restart: a table with no resumable checkpoint restarts from the beginning on every run, so diff() must not carry over the persisted partial report or mismatch flag. Doing so would double-count rows and duplicate mismatch samples across max-diff-duration restarts. Start the DiffReport fresh instead. - Clear stale checkpoints: buildPlan loads any persisted lastpk into the in-memory stream checkpoints before getSourcePKCols runs. A stale (possibly wrong-length) lastpk from before this fix, resumed after an upgrade, would otherwise be used to resume the streams. getSourcePKCols now clears the in-memory keys when flagging the table, and updateTableProgress persists lastpk = NULL so the database value is cleared for future resumes. Update the persist/reload test to assert lastpk is cleared to NULL. Signed-off-by: pedroalb <pedro.albuquerque@slack-corp.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
go/vt/vttablet/tabletmanager/vdiff/framework_test.go:26
framework_test.gonow depends on the standard-libraryslicespackage (Go 1.21+). If this repo/tests are expected to build with older Go versions, this will break compilation. Consider replacingslices.Containswith a small local loop or amap[string]struct{}lookup to avoid bumping the minimum Go toolchain requirement from within a test helper.
"slices"
go/vt/vttablet/tabletmanager/vdiff/framework_test.go:484
framework_test.gonow depends on the standard-libraryslicespackage (Go 1.21+). If this repo/tests are expected to build with older Go versions, this will break compilation. Consider replacingslices.Containswith a small local loop or amap[string]struct{}lookup to avoid bumping the minimum Go toolchain requirement from within a test helper.
func (tmc *fakeTMClient) GetSchema(ctx context.Context, tablet *topodatapb.Tablet, request *tabletmanagerdatapb.GetSchemaRequest) (*tabletmanagerdatapb.SchemaDefinition, error) {
if len(request.Tables) == 0 {
return tmc.schema, nil
}
filtered := &tabletmanagerdatapb.SchemaDefinition{
TableDefinitions: make([]*tabletmanagerdatapb.TableDefinition, 0),
}
for _, td := range tmc.schema.TableDefinitions {
if slices.Contains(request.Tables, td.Name) {
filtered.TableDefinitions = append(filtered.TableDefinitions, td)
}
}
return filtered, nil
}
go/vt/vttablet/tabletmanager/vdiff/framework_test.go:494
- Matching PKE queries via
strings.Contains(query, tableName)is brittle and can yield accidental matches (e.g., tabletmatchingt2, or matching occurrences in comments/aliases), making tests harder to reason about as schemas grow. A more robust approach is to keypkeResultsby the exact query string (or a compiled regexp that matchesFROM <table>/ the specific information_schema query shape) so the mock behavior is deterministic and less coupled to substring coincidences.
func (tmc *fakeTMClient) ExecuteFetchAsApp(ctx context.Context, tablet *topodatapb.Tablet, usePool bool, req *tabletmanagerdatapb.ExecuteFetchAsAppRequest) (*querypb.QueryResult, error) {
query := string(req.Query)
for tableName, result := range tmc.pkeResults {
if strings.Contains(query, tableName) {
return sqltypes.ResultToProto3(result), nil
}
}
return sqltypes.ResultToProto3(noResults), nil
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebfb136111
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…bles Two more follow-ups for the source-checkpoint-unavailable case: - Avoid endless duration retries: a table that cannot be checkpointed restarts from row zero every attempt, so if it exceeds max-diff-duration the retry loop would restart it forever (full scan plus a 30s delay each time) and never finish. Fail explicitly with an actionable error telling the operator to increase or unset --max-diff-duration instead of retrying. - Clear the persisted mismatch bit on a full restart: when a table with no resumable checkpoint restarts fresh, a mismatch recorded by a discarded partial attempt must not stick. Otherwise a clean full-table pass still reports has_mismatch=1 in VDiff show. Clear _vt.vdiff_table.mismatch when discarding the report so the fresh pass re-derives it. Signed-off-by: pedroalb <pedro.albuquerque@slack-corp.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (4)
go/vt/vttablet/tabletmanager/vdiff/table_differ.go:1144
sourceTableNameFromSelectreturnstableName.String(), which can include a qualifier (e.g.db.table).GetSchemaRequest.TablesandTableDefinition.Nameare typically unqualified, so this can cause schema lookup failures for qualified table references. Return the unqualified name (e.g.tableName.Name.String()) or otherwise normalize to the expected schema key.
tableName := sqlparser.GetTableName(aliased.Expr)
if tableName.IsEmpty() {
return "", vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported source query, could not resolve the source table: %s", sqlparser.String(sourceSelect))
}
return tableName.String(), nil
go/vt/vttablet/tabletmanager/vdiff/workflow_differ.go:276
- The new fail-fast behavior for
sourceCheckpointUnavailableonErrMaxDiffDurationExceededis important to prevent infinite retry loops, but it isn’t covered by a test in this diff. Add a unit test (or a small harness test) that forcesdiffTableto hitErrMaxDiffDurationExceededwithsourceCheckpointUnavailable = trueand asserts the returned error code/message and that no retry occurs.
if td.tablePlan.sourceCheckpointUnavailable {
// This table cannot be checkpointed because its filter does not
// project the full source primary key (see getSourcePKCols), so every
// retry restarts from the beginning and would hit the same timeout,
// looping forever. Fail explicitly with an actionable message instead
// of retrying.
return vterrors.Errorf(vtrpcpb.Code_FAILED_PRECONDITION,
"table %s exceeded the max-diff-duration and cannot be resumed because its filter does not project the full source primary key; increase or unset --max-diff-duration so it can complete within a single window",
td.table.Name)
}
go/vt/vttablet/tabletmanager/vdiff/table_differ.go:561
- When
sourceCheckpointUnavailableis true, the code intentionally ignores the persistedreportin memory, but it does not clear the persistedreportblob in_vt.vdiff_tableat the start of the run. If the run fails before the firstupdateTableProgress, observers may see a stale/partial report even though the run is intended to restart clean. Consider clearing/resetting the persisted report (and possibly rows_compared) alongside clearing the mismatch bit for this restart-from-beginning mode.
if td.tablePlan.sourceCheckpointUnavailable {
// This table has no resumable checkpoint and restarts from the beginning
// on every run (see getSourcePKCols). Carrying over the persisted partial
// report or mismatch flag would double-count rows and duplicate mismatch
// samples across restarts, so we start fresh instead. Also clear the
// persisted mismatch bit so a mismatch recorded by a discarded partial
// attempt does not stick after a clean full-table pass.
mismatch = false
if err = clearTableMismatch(dbClient, td.wd.ct.id, td.table.Name); err != nil {
return nil, err
}
} else if rpt := curState.AsBytes("report", []byte("{}")); json.Valid(rpt) {
go/vt/vttablet/tabletmanager/vdiff/framework_test.go:494
- The test fake matches PKE results using
strings.Contains(query, tableName), which is brittle (substring collisions liket1vst10, or table names appearing in other contexts). Make the lookup more deterministic by extracting the table name from the query (e.g., match on atable_name = '...'pattern) or by keying off the exact expected query shape used byGetPrimaryKeyEquivalentColumns.
func (tmc *fakeTMClient) ExecuteFetchAsApp(ctx context.Context, tablet *topodatapb.Tablet, usePool bool, req *tabletmanagerdatapb.ExecuteFetchAsAppRequest) (*querypb.QueryResult, error) {
query := string(req.Query)
for tableName, result := range tmc.pkeResults {
if strings.Contains(query, tableName) {
return sqltypes.ResultToProto3(result), nil
}
}
return sqltypes.ResultToProto3(noResults), nil
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ba66d230a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…etry The previous commit made an un-checkpointable table fail when it exceeded --max-diff-duration, but that only ended the in-process retry loop. With auto_retry enabled the engine reconstructs the persisted error, classifies it as ephemeral, and restarts the whole VDiff every 30s, so it still loops forever. Fix the root cause instead: --max-diff-duration works by checkpointing and resuming, which these tables cannot do, so the limit is simply ignored for them (with a warning) and they are diffed in a single uninterrupted pass. No timeout means no ErrMaxDiffDurationExceeded, no in-process retry, and no engine-level auto-retry loop, and the diff actually completes. This removes the explicit-failure path added in the prior commit. Signed-off-by: pedroalb <pedro.albuquerque@slack-corp.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
go/vt/vttablet/tabletmanager/vdiff/framework_test.go:494
strings.Contains(query, tableName)can match unintended tables when one name is a substring of another (e.g., "t" matching "t2"), returning the wrong mocked PKE result and making tests flaky/incorrect. Use a more precise match (e.g., parse the query and extract the table name, or match on a stricter pattern like backticked identifiers / FROM clause token boundaries) so only the intended table triggers a result.
func (tmc *fakeTMClient) ExecuteFetchAsApp(ctx context.Context, tablet *topodatapb.Tablet, usePool bool, req *tabletmanagerdatapb.ExecuteFetchAsAppRequest) (*querypb.QueryResult, error) {
query := string(req.Query)
for tableName, result := range tmc.pkeResults {
if strings.Contains(query, tableName) {
return sqltypes.ResultToProto3(result), nil
}
}
return sqltypes.ResultToProto3(noResults), nil
}
go/vt/vttablet/tabletmanager/vdiff/table_differ_test.go:366
- The regex is brittle because it hard-codes
lastpk = nullwith specific casing/spacing; binder output can vary (e.g.,NULL). Consider making the match case-insensitive for NULL (e.g.,(?i)null) and/or relaxing whitespace around=to reduce false negatives unrelated to the behavior under test.
persistClient.ExpectRequestRE(
`^update _vt\.vdiff_table set rows_compared = 100, lastpk = null, report = '.*' where vdiff_id = 1 and table_name = 'customer'$`,
&sqltypes.Result{}, nil)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7f8f0e64c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…arning Keep the message constant and pass the table and vdiff identifiers as slog.String fields instead of interpolating them, so the log line has stable, queryable text. Signed-off-by: pedroalb <pedro.albuquerque@slack-corp.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
go/vt/vttablet/tabletmanager/vdiff/framework_test.go:489
- Using
strings.Contains(query, tableName)for routing mock results can lead to accidental matches (e.g., table name appearing in a comment, alias, or as a substring of another identifier), which makes tests harder to reason about and potentially flaky. Consider tightening the match (e.g., regex/word-boundary match onFROM <table>/TABLE <table>patterns) so each mock response is triggered deterministically.
for tableName, result := range tmc.pkeResults {
if strings.Contains(query, tableName) {
return sqltypes.ResultToProto3(result), nil
}
}
Thanks for digging into this @mattlord , the reviews caught some real edge cases. The tricky case is a filter that doesn't project the full source PK (e.g. On main this shape either hard-failed or reported false extras on resume. And normal reshards never hit this branch (they project every column), so the PK-ordering fix is what actually unblocks them. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Suppressed comments (3)
go/vt/vttablet/tabletmanager/vdiff/table_differ.go:1143
tableName.String()can include a qualifier (e.g.,db.table).GetSchemaRequest.Tablesand schemaTableDefinition.Nameare typically unqualified table names, so this can cause schema lookups to fail for filters that use qualified names. Prefer returning the unqualified identifier (e.g.,tableName.Name.String()), and (optionally) validate/ignore the qualifier if present.
tableName := sqlparser.GetTableName(aliased.Expr)
if tableName.IsEmpty() {
return "", vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported source query, could not resolve the source table: %s", sqlparser.String(sourceSelect))
}
return tableName.String(), nil
go/vt/vttablet/tabletmanager/vdiff/framework_test.go:493
- Matching PKE results with
strings.Contains(query, tableName)is fragile (false positives if a table name is a substring of another identifier, comment, alias, etc.). To avoid test flakiness, consider keyingpkeResultsby an exact expected query (or by a stricter pattern likeFROM <table>/ backticked name), or parse the query to extract the referenced table reliably.
func (tmc *fakeTMClient) ExecuteFetchAsApp(ctx context.Context, tablet *topodatapb.Tablet, usePool bool, req *tabletmanagerdatapb.ExecuteFetchAsAppRequest) (*querypb.QueryResult, error) {
query := string(req.Query)
for tableName, result := range tmc.pkeResults {
if strings.Contains(query, tableName) {
return sqltypes.ResultToProto3(result), nil
}
}
return sqltypes.ResultToProto3(noResults), nil
go/vt/vttablet/tabletmanager/vdiff/workflow_differ_test.go:692
- The “Text column as expression” success case was removed from
TestBuildPlanSuccess, but there isn’t a corresponding assertion at the workflow/build-plan level that this scenario now fails closed (or is otherwise handled) when planning a table. SincegetSourcePKColsnow errors for computed aliases, consider adding a workflow/build-plan test that verifies the expected failure mode/message when a filter projects a computed expression aliased to the PK name, so the behavior is enforced at the same layer where plans are built.
}, {
// Multiple PK columns.
input: &binlogdatapb.Rule{
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
I think there is still one blocking issue.
The subset-projection branch in workflow_differ.go:222-236 now ignores an explicitly configured --max-diff-duration and substitutes the one-year timer. This flag is an operational safety bound: long-lived InnoDB snapshots can prevent purge, grow undo history, and create disk pressure for much longer than the operator allowed.
I understand why restarting from row zero cannot make progress and why the persisted error currently loses its non-retryable type, but I don't think either justifies overriding the bound. I think we should either preserve a resumable full physical source checkpoint, or stop at the configured duration and make that failure survive persistence as non-retryable. No?
…ail non-retryably Per review, do not override the operator's --max-diff-duration safety bound for a table whose filter does not project the full source primary key (long-lived InnoDB snapshots have real operational cost). Apply the configured duration as usual. If such a table exceeds the bound it cannot be resumed (it has no source checkpoint) and restarting from row zero would loop forever, so the differ stops with a non-retryable error. The error is a MySQL ERNotSupportedYet so its "(errno 1235)" suffix survives being persisted as a string and rebuilt by retryVDiffs; IsEphemeralError then classifies it as non-ephemeral and the engine does not auto-retry it. The operator can raise or unset --max-diff-duration to let the table complete in a single window. This replaces the previous single-pass override, and reuses the intended sqlerror round-trip rather than a plain vterror (which was classified ephemeral and retried forever, the reason the earlier explicit-failure attempt was removed). Signed-off-by: pedroalb <pedro.albuquerque@slack-corp.com>
@mattlord Agreed, the safety bound should win. Went with your second option: honor |
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
LGTM! Thanks for working through all of these edge cases. This is a nice set of improvements. ❤️
Non-blocking: I think it would be worth adding focused coverage for the timeout path in workflow_differ.go:268-281. The non-retry guarantee depends on ERNotSupportedYet surviving string persistence and reconstruction through NewSQLErrorFromError, but the current tests do not exercise that round trip. A small test verifying that the reconstructed error remains non-ephemeral would help prevent this from regressing. The implementation itself looks correct to me.
And a nit... "preserve PK definition order in getSourcePKCols" is ambiguous and IMO confusing. I say that as "definition order" to me means the order defined in the table structure. Instead, what we are really doing here is "preserving the PK select order" or "mapping PK select order to PK definition order" here, no?
| sqlUpdateTableMismatch = "update _vt.vdiff_table set mismatch = true where vdiff_id = %a and table_name = %a" | ||
| sqlClearTableMismatch = "update _vt.vdiff_table set mismatch = false where vdiff_id = %a and table_name = %a" |
There was a problem hiding this comment.
IMO we could/should use a single query and just make the boolean part variable with %a but it's not a blocker.
Description
Fixes the PK column ordering and index mapping in VDiff's
getSourcePKCols, described in #20601.The bug
getSourcePKColsbuildssourcePkColsby puttingsourceTable.PrimaryKeyColumnsinto amap[string]struct{}(losing PK definition order), then scanningtd.table.ColumnsinORDINAL_POSITIONorder. This has two problems:SEQ_IN_INDEX).td.table.Columns(DDL order of the target table) instead of the SELECT expression order of the source query. When a filter reorders columns (e.g.,select c2, c1 from t1), the stored index points to the wrong value.On VDiff resume (via auto-retry,
max-diff-durationtimeout, or explicitVDiff resume),lastPKFromRowuses the wrong-orderedsourcePkColsto buildlastpk.Source. The row streamer pairs values positionally with its ownpkColumns(in correctSEQ_IN_INDEXorder fromBaseShowPrimary), producing a corruptedWHEREclause. Each resumed pass reports the entire remaining table asExtraRowsSource, compounding with each retry.The fix
Resolve the physical source table from the filter's
FROMclause(
sourceTableNameFromSelect) and use it for all source schema, PK-equivalent,and PK-column lookups. This can differ from the VDiff target table name for
cross-table MoveTables filters (e.g.
select ... from t2for targett1), andusing the wrong table produced an incorrect or empty source checkpoint.
Then map each source PK column to its position in the source query's SELECT
expression list (the actual streamed row layout) instead of
td.table.Columns.Each PK is matched against the underlying physical column of a SELECT
expression: a plain
ColName, a renamed column (select source_id as target_id), or aCONVERT(col USING charset)rename unwrapped to its innercolumn (mirroring the row streamer's planner). The core mapping is extracted
into
sourcePKSelectIndicesfor direct testability.A source PK column that is missing from the SELECT list is handled in one of two
ways:
unrelated expression, e.g.
a + b as idorother_col as id): fail closedwith an error. The row streamer resumes on the physical source PK value, so
persisting a derived value as the checkpoint would skip or repeat rows.
(cid, typ)withselect cid, name from customer): do not error and do notbuild a partial source key. Fall back to the target
pkColsso the sourcecheckpoint stays nil. The row streamer always resumes on the full source PK
and rejects a
lastpkwhose length does not match the table's PK columncount, so a partial source key must never be emitted.
Known limitation (pre-existing, out of scope)
lastPKFromRowusescolIndexto index into both the streamed row (SELECT order) andtd.tablePlan.table.Fields(DDL order). When a filter reorders columns, the field type metadata can be wrong. This affects both the source path (this PR) and the existing target path (findPKs) equally. The correct fix (using query result fields for type metadata) is a separate change that touches both paths.Related Issue(s)
Fixes #20601
Checklist
Backport justification
This is a correctness fix for VDiff resume. Without this fix, any VDiff that resumes (via auto-retry,
max-diff-duration, or manual resume) on a table where PK column order differs from column ordinal order, or where the source query reorders/aliases columns, produces falseExtraRowsSourcereports that compound with each retry pass. This makes VDiff unusable as a resharding validation gate for affected tables. The bug is present in all versions sincesourcePkColswas introduced and has been confirmed on v22.0.4. The fix is low-risk: it changes onlygetSourcePKColsand adds tests.Deployment Notes
No migrations or flag changes required. The fix changes how
sourcePkColsindices are computed - they now reference positions in the source query's SELECT list rather than the target table's DDL column order. This only affects VDiff runs that resume on tables where:With the fix, resumed VDiff passes will no longer produce false
ExtraRowsSourcereports for these cases.