Skip to content

VDiff: save a sample for every drained extra row so reconciliation can match them (#20855) - #930

Open
pedroalb wants to merge 1 commit into
slack-22.0from
pedroalb/vdiff-drain-sample-22
Open

VDiff: save a sample for every drained extra row so reconciliation can match them (#20855)#930
pedroalb wants to merge 1 commit into
slack-22.0from
pedroalb/vdiff-drain-sample-22

Conversation

@pedroalb

@pedroalb pedroalb commented Sep 1, 2026

Copy link
Copy Markdown

Backport of upstream vitessio#20855 to slack-22.0

Description

When one side's stream ends before the other, the remaining rows on the other side are drained: they are counted as extra rows, but only the first of them gets a row-diff sample saved. reconcileExtraRows() (vitessio#18585) matches extra rows by comparing the saved samples, so drained rows beyond the first can never be reconciled. When the source and target streams return the same rows in a different order (e.g. because of PK collation differences between the sides), the diff ends with extra rows on both sides that are actually identical, and the sample-less drained rows survive reconciliation as false-positive extras — the vdiff reports HasMismatch: true on matching data.

This PR makes the drain path save a sample for each drained row, subject to the usual --max-extra-rows-to-compare cap, the same as for extra rows found in the main compare loop. It also:

  • merges the drained rows into the diff report only after the full drain succeeds, so a stream error mid-drain persists nothing and a resumed diff cannot count drained rows twice;
  • generates drained target row samples with the target query (they were previously generated with the source query);
  • bounds reconcileExtraRows()'s maxRows by the number of saved samples rather than the extra-row counts;
  • removes the now-unused primitiveExecutor.drain();

Test coverage: TestDiffDrainedRowSampling drives the full diff() loop for both drain directions plus the sampling cap, and TestDiffDrainStreamError injects a stream error mid-drain, verifies the deferred progress update persists neither counts nor samples for the partially drained rows, then resumes and verifies each drained row is counted exactly once. Both tests fail on the previous code.

Backport notes

Cherry-picked from upstream e5b3f9a6ec, rebased onto slack-22.0 (after #927). The table_differ.go drain rewrite and workflow_differ.go reconcile logic are identical to upstream. The one adaptation is in report.go: slack-22.0 predates an unrelated upstream change that renders binary sample values as hex, so only the LosslessValues logic was applied onto the existing addVal, without that hex rendering (it is orthogonal to this fix — both sides render identically, so sample matching still works). For the same missing dependency, upstream merged this to release-24.0 (vitessio#20943) but closed the release-23.0 backport (vitessio#20942); slack-22.0 is in release-23.0's position.

@pedroalb
pedroalb requested a review from a team as a code owner September 1, 2026 15:06
@github-actions github-actions Bot added this to the v22.0.4 milestone Sep 1, 2026
…n match them (vitessio#20855)

Signed-off-by: Matthias Crauwels <matthias.crauwels@planetscale.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Matt Lord <mattalord@gmail.com>
(cherry picked from commit e5b3f9a)
Signed-off-by: Pedro Albuquerque <pedro.albuquerque@slack-corp.com>
@pedroalb
pedroalb force-pushed the pedroalb/vdiff-drain-sample-22 branch from ce3f507 to 5488456 Compare September 2, 2026 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant