-
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
Fix regression where inserts into reference tables with a different name on sharded keyspaces were not routed correctly. #15796
Fix regression where inserts into reference tables with a different name on sharded keyspaces were not routed correctly. #15796
Conversation
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
… already logic for updates/deletes Signed-off-by: Rohit Nayak <rohit@planetscale.com>
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15796 +/- ##
==========================================
- Coverage 68.45% 68.43% -0.03%
==========================================
Files 1558 1559 +1
Lines 195928 196498 +570
==========================================
+ Hits 134128 134476 +348
- Misses 61800 62022 +222 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
vTbl, routing := buildVindexTableForDML(ctx, tableInfo, qt, "insert") | ||
|
||
if tableInfo.GetVindexTable().Type == vindexes.TypeReference { |
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 reference table source should be changed inside otherwise the vindex table and routing will be pointing to reference
table and not the source
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.
Updated code as per our discussion.
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
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.
Rest LGTM!
sourceTable, _, _, _, _, err := ctx.VSchema.FindTableOrVindex(vindexTable.Source.TableName) | ||
if err != nil { | ||
panic(err) | ||
} | ||
vindexTable = sourceTable | ||
refTbl := sqlparser.NewAliasedTableExpr(vindexTable.GetTableName(), "") | ||
ins.Table.Expr = refTbl.Expr | ||
// We don't need to process the alias because you cannot define aliases for inserts. |
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.
What does this mean?
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.
insert
s take a table name where as other dmls take a table_reference (where you can specify aliases).
INSERT [LOW_PRIORITY | DELAYED | HIGH_PRIORITY] [IGNORE]
[INTO] tbl_name
[PARTITION (partition_name [, partition_name] ...)]
[(col_name [, col_name] ...)]
{ {VALUES | VALUE} (value_list) [, (value_list)] ... }
[AS row_alias[(col_alias [, col_alias] ...)]]
[ON DUPLICATE KEY UPDATE assignment_list]
UPDATE [LOW_PRIORITY] [IGNORE] table_reference
SET assignment_list
[WHERE where_condition]
[ORDER BY ...]
[LIMIT row_count]
table_references:
escaped_table_reference [, escaped_table_reference] ...
escaped_table_reference: {
table_reference
| { OJ table_reference }
}
table_reference: {
table_factor
| joined_table
}
table_factor: {
tbl_name [PARTITION (partition_names)]
[[AS] alias] [index_hint_list]
| [LATERAL] table_subquery [AS] alias [(col_list)]
| ( table_references )
}
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
…ame on sharded keyspaces were not routed correctly. (#15796) Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Description
Inserts for reference tables where the name of the sharded table was different and the insert statement used the sharded table as the table to insert into, were failing because they were not being routed correctly. However delete and update queries are being handled correctly.
This PR adds the logic present in delete/update to insert. 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 PR will be backported to v19 but a separate PR (#15788) will fix the issue on v18.
Related Issue(s)
#15770
Checklist