Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug Report: primaryVindexesDiffer is returning the wrong value #16565

Open
CAEL0 opened this issue Aug 9, 2024 · 1 comment
Open

Bug Report: primaryVindexesDiffer is returning the wrong value #16565

CAEL0 opened this issue Aug 9, 2024 · 1 comment

Comments

@CAEL0
Copy link

CAEL0 commented Aug 9, 2024

Overview of the Issue

In go/vt/wrangler/materializer.go and go/vt/vtctl/workflow/materializer.go, the function primaryVindexesDiffer returns true if neither source nor target have any vindexes.

// primaryVindexesDiffer returns true if, for any tables defined in the provided
// materialize settings, the source and target vschema definitions for those
// tables have different primary vindexes.
//
// The result of this function is used to determine whether to apply a source
// shard selection optimization in MoveTables.
func primaryVindexesDiffer(ms *vtctldatapb.MaterializeSettings, source, target *vschemapb.Keyspace) bool {

	...

		// If neither source nor target have any vindexes, treat the answer to
		// the question as trivially false.
		if len(sColumnVindexes) == 0 && len(tColumnVindexes) == 0 {
			return true
		}

	...

But it should return false as written in the comment.

Reproduction Steps

N/A

Binary Version

N/A

Operating System and Environment details

N/A

Log Fragments

No response

@CAEL0 CAEL0 added Needs Triage This issue needs to be correctly labelled and triaged Type: Bug labels Aug 9, 2024
@mattlord
Copy link
Contributor

Thanks, @CAEL0 ! Due to the way this feature works, true or false does not matter if the source and target are unsharded as there's no optimization to make. So while this doesn't have a practical impact, obviously this return value is wrong and we should fix it. 🙂

@mattlord mattlord added Component: VReplication and removed Needs Triage This issue needs to be correctly labelled and triaged labels Aug 12, 2024
mattlord added a commit to planetscale/vitess that referenced this issue Aug 12, 2024
This did not have a practical impact but was obviously
wrong

Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord mattlord self-assigned this Aug 12, 2024
mattlord added a commit to planetscale/vitess that referenced this issue Aug 12, 2024
This did not have a practical impact but was obviously
wrong

Signed-off-by: Matt Lord <mattalord@gmail.com>
mattlord added a commit to planetscale/vitess that referenced this issue Aug 12, 2024
This reverts commit b1e1e85.
mattlord added a commit to planetscale/vitess that referenced this issue Aug 13, 2024
Revert "Fix vitessio#16565"

This reverts commit b1e1e85.
mattlord added a commit to planetscale/vitess that referenced this issue Aug 13, 2024
Revert "Fix vitessio#16565"

Signed-off-by: Matt Lord <mattalord@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

No branches or pull requests

2 participants