VDiff: Copy non in_keyrange workflow filters to target tablet query

[mattlord]

Description

In #15403 we added experimental support for moving tables from a multi-tenant environment (database per tenant, each with the same schema).

You would do that by specifying a column on the table(s) representing the tenant identifier in the target VSchema and then specifying a tenant identifier value using MoveTables create --tenant-id=<val>. You can see an example here: #16305 (comment)

There is an optional --shards <shards> flag for MoveTables create that would limit the workflow streams created to only the appropriate target shard for the provided tenant identifier value. In order to know what that would be, you would need to query the vindex function to know where they should land. This can be cumbersome in some situations and introduces the potential for human error so should not be required for anything to work properly (and one could argue it's better left to Vitess to figure this out — a future optimization would be for vreplication itself to determine which streams need to be created for the provided tenant identifier). Anyhow, when this --shards flag was also specified then running VDiff on the workflow worked as expected. When it was not specified, however, there would be extra rows reported on the target if you had previously migrated any other tenants.

This PR addresses this issue by ensuring that we copy any WHERE predicates from the workflow filter to the query used on the target tablets for the diff.

I think that we should backport this fix to v20 where the multi-tenant support was added since it's a pretty small change and will eliminate potential issues for those using this new experimental feature (and we want all the usage and input we can get).

Related Issue(s)

• Fixes: Feature Request: make VDiff support multi-tenant migrations #16305

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

[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 14 lines in your changes missing coverage. Please review.

Project coverage is 68.70%. Comparing base (0f4b544) to head (c8ca7bc).
Report is 2 commits behind head on main.

Files	Patch %	Lines
go/vt/vttablet/tabletmanager/vdiff/utils.go	0.00%	12 Missing ⚠️
go/vt/vttablet/tabletmanager/vdiff/table_plan.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16307      +/-   ##
==========================================
+ Coverage   68.69%   68.70%   +0.01%     
==========================================
  Files        1544     1547       +3     
  Lines      198011   198260     +249     
==========================================
+ Hits       136021   136216     +195     
- Misses      61990    62044      +54     

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

[shlomi-noach]

If you mean to backport this to v20, please make sure to do so today, as we have a tentative plan to make a point release tomorrow (your late night tonight).

[deepthi]

This PR was reviewed and merged before I could, but here's some feedback.

• Since @mattlord identified that you can in fact run VDiff, you just need to specify --shards, the issue should be marked as an enhancement versus a feature request
• also given that, it was unnecessary to backport this to the release-20.0 branch. There is a good workaround to it, it's not as if it didn't work at all.

[rohit-nayak-ps]

Since @mattlord identified that you can in fact run VDiff, you just need to specify --shards, the issue should be marked as an enhancement versus a feature request

I don't think this is correct. The --shards works only if there is no other tenant in the target shard.

The bug this PR fixes is that vdiff was dropping the where clause. So the where tenant_id = 10 was being dropped and all rows from the source and target were used in the vdiff for a specific tenant workflow. If there is a previously migrated tenant in the same shard as the tenant in the workflow being vdiffed, we will see extra rows from that migrated tenant.

@mlord, let me know if my understanding is incorrect.