Experimental: Add missing tables to globally routed list in schema tracker only if they are not already present in a VSchema

[rohit-nayak-ps]

Description

Follows up on the idea in #16517 by not considering tables already present in the globally routable list of tables.

Related Issue(s)

#16516

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

[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

All modified and coverable lines are covered by tests ✅

Project coverage is 67.51%. Comparing base (747a61c) to head (271e95f).
Report is 22 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17371      +/-   ##
==========================================
+ Coverage   67.48%   67.51%   +0.02%     
==========================================
  Files        1577     1581       +4     
  Lines      253424   253955     +531     
==========================================
+ Hits       171033   171467     +434     
- Misses      82391    82488      +97     

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

[GuptaManan100]

Just a nomenclature nit, otherwise looks good to me!

[GuptaManan100]

The logic is fine, but doesn't the name of the variable seem exactly opposite of what it is doing? Like we are skipping adding to the global table if is already present when skipIfAlreadyGlobal is false? Am I misunderstanding this?

[rohit-nayak-ps]

You are right! I changed the name of the variable from override to this, which changed its meaning. So I should reverse the values passed in. Will do that.

[rohit-nayak-ps]

Fixed

[harshit-gangal]

Suggested change

	if !skipIfAlreadyGlobal {
	// Called when updating from schema tracking
	continue
	}
	if skipAlreadyGlobal {
	// Called when updating from schema tracking
	continue
	}

[harshit-gangal]

! has to go I believe

[rohit-nayak-ps]

Duh, yes, thanks, needed to change this as well since I switched the logic around. Fixed and tests are now passing.

[harshit-gangal]

The issue with this approach arises when there are multiple unsharded keyspaces. This makes adding the table to global routing non-deterministic, failing to fully achieve the intended purpose of this PR.

[rohit-nayak-ps]

The issue with this approach arises when there are multiple unsharded keyspaces. This makes adding the table to global routing non-deterministic, failing to fully achieve the intended purpose of this PR.

You mean if there are tables of the same name in multiple unsharded keyspaces (and which are not in the VSchemas).

We could collect all tables names which will be added like this (not in VSchema, but found by schema tracking) first and validate that there are no duplicates, marking ones with duplicates as ambiguous, while others get globally routed.

[rohit-nayak-ps]

Discussed with @harshit-gangal . His point is that:

1. We start with a sharded keyspace Sharded1 and one unsharded keyspace Unsharded1. Unsharded1 has no vschema defined yet.
2. After this PR, schema tracking finds a table ut1 in Unsharded1 that is not in the current VSchema. It then adds it to the global tables list. User starts depending on select * from ut1.
3. Later user adds another unsharded keyspace Unsharded2 which also has table named ut1.
4. Now we will mark ut1 as ambiguous (this logic is not yet part of this PR, but is the enhancement I referred to in Experimental: Add missing tables to globally routed list in schema tracker only if they are not already present in a VSchema #17371 (comment))
5. Suddenly the user's select * from ut1 query will start erroring out. User won't have visibility as to why this has happened since they have not done anything obvious in Vitess like modifying/adding a VSchema and will have downtime until they drop the other table, which could be difficult to diagnose/fix.

Options here are:

1. Close this PR and not do anything
2. Merge this PR (with enhanced logic to detect ambiguous tables outside of current VSchemas), which will solve the problem for X% of customers who currently have downtime because we don't do this

I guess the question is what is X. Is it high enough to merge this. Or even if X is high, do we want to do this from a principled viewpoint.