-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[release-18.0] Direct PR. Fix regression where reference tables with a different name on sharded keyspaces were not routed correctly. #15788
Conversation
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
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
|
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
1c4d590
to
237cdfc
Compare
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
tableName.Qualifier = vschemaTable.GetTableName().Qualifier | ||
tableName.Name = sqlparser.NewIdentifierCS(vschemaTable.Name.String()) | ||
newAliasTbl := sqlparser.NewAliasedTableExpr(tableName, "") | ||
aliasTbl.Expr = newAliasTbl.Expr | ||
aliasTbl.As = newAliasTbl.As |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alias should be either an existing one or the reference table name. Should not be source table name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to add some tests around it in dml_cases.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you have created a new test file for it. So, those can be added there.
DML with alias.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the test cases into reference_cases.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alias should be either an existing one or the reference table name. Should not be source table name.
So I should only change the qualifier and retain the existing alias' table name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harshit-gangal, updated the code as we discussed and added more test cases for queries where aliases are specified.
…ove additional test Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@vitessio/query-serving does this need to be back ported to v17? |
…d test cases for aliases Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
According to me, this DML routing of the reference table was added in v17. I am not sure about the regression referred to here. |
@@ -463,7 +463,6 @@ func validateDryRunResults(t *testing.T, output string, want []string) { | |||
w = strings.TrimSpace(w[1:]) | |||
result := strings.HasPrefix(g, w) | |||
match = result | |||
//t.Logf("Partial match |%v|%v|%v\n", w, g, match) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should be removed or uncommented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove in the next vrep. PR.
if vindexTbl != nil { | ||
// vindex cannot be present in a dml statement. | ||
return false, vterrors.VT09014() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this will happen? do you have a test case for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean the nil vindexTbl check? The same check is made above in the same function. So I assumed it was required. I can remove it, but is there any problem adding the extra check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging as is for now: assuming an additional check is safe. Can remove later on if required.
Description
If a reference table is named differently in the sharded keyspace, inserts and updates to that table behave differently than if they had the same name. Looks like there was a regression somewhere between v16 and v18.
The only code change is in the dml planning, where tables are rerouted. Additionally unit tests and an e2e test has been added to validate the rerouting and protect against regression.
Note: since there has been significant refactors in the vtgate planner between v18 and the current main, this is being implemented as a standalone PR instead of following the backport route. #15796 will fix the issue in v19 and v20.
Test cases have been added for dmls with reference tables have been added:
Related Issue(s)
#15770
Checklist
Deployment Notes