VReplication TableStreamer: Only stream tables in tablestreamer (ignore views) #14646
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Rohit Nayak <[email protected]>
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
|
vitess-bot
bot
added
NeedsDescriptionUpdate
rohit-nayak-ps
removed
NeedsDescriptionUpdate
Signed-off-by: Rohit Nayak <[email protected]>
rohit-nayak-ps
requested review from
harshit-gangal,
systay,
shlomi-noach and
deepthi
as code owners
mattlord
approved these changes
Nov 30, 2023
Comment on lines
125
to
+126
| tableName := row[0].ToString() | ||
| tableType := row[1].ToString() |
There was a problem hiding this comment.
I think that we should either switch to using named rows or add a quick len(row) check just to be sure we don't crash with index out of bounds in some odd cases.
There was a problem hiding this comment.
Named rows is a no-go here, because the column name is dynamically constructed by the schema name (e.g. Tables_in_mydb) which is a bit of a nightmare with UTF and escaping.
I'd say an output of SHOW FULL TABLES can be trusted to have two columns, much like SHOW VARIABLES can be trusted to have two columns.
At any case, if we choose to validate number of columns, we should return some outrageous error like Code_INTERNAL or something likewise dramatic.
shlomi-noach
approved these changes
Nov 30, 2023
vitess-bot
pushed a commit
that referenced
this pull request
Nov 30, 2023
…re views) (#14646) Signed-off-by: Rohit Nayak <[email protected]>
rohit-nayak-ps
added a commit
that referenced
this pull request
Nov 30, 2023
…estreamer (ignore views) (#14646) (#14649) Signed-off-by: Rohit Nayak <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-by: Rohit Nayak <[email protected]>
ejortegau
pushed a commit
to slackhq/vitess
that referenced
this pull request
Dec 13, 2023
…re views) (vitessio#14646) Signed-off-by: Rohit Nayak <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When we do an atomic copy the
tablestreamerstreams all tables. The correspondingMoveTablesgets its own list of tables, for which it adds filters for each table as part of the vreplication stream'sbinlogsource. For non-atomic copy workflows, we stream each table in thebinlogsourcelist of tables. On the target vreplication creates one table plan for each of these tables.However we get a full list of tables again after we have taken a snapshot (after locking all tables) for atomic copy and stream those tables.
There was a bug where we were not excluding views from this list. Hence the target was seeing a stream from the view resulting in the target stream erroring out since it doesn't have this view in its list of table plans.
This PR fixes that bug. It also adds a view to the e2e test: that test fails without the fix in the PR.
Related Issue(s)
Fixes #14648
Checklist