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

Routing Rules: Improved escaping of reserved and invalid names for tables and keyspaces #9522

Merged
merged 2 commits into from
Jan 21, 2022

Conversation

rohit-nayak-ps
Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps commented Jan 17, 2022

Description

This PR has the following changes:

Routing Rules

The target of a routing rule during MoveTables can contain the name of keyspaces and/or tables which are reserved or invalid mysql identifiers. These need to be backticked while generating the sql. https://github.com/vitessio/vitess/blob/b514ef94b56414751d230f86b9e966b84c303677/go/vt/vtgate/vindexes/vschema.go#L492

Reshard

Copying the schema from one shard to another, if a keyspace has special characters, causes errors while running Create Database because the database name is invalid and needs backticking.
https://github.com/vitessio/vitess/blob/b514ef94b56414751d230f86b9e966b84c303677/go/vt/mysqlctl/tmutils/schema.go#L236

Test changes

The e2e tests for vreplication are updated to:

  • Rename one of the tables Lead so as to name it with a reserved mysql keyword, and to test with mixed case
  • Rename keyspace as merchant-type so as to use - to test the backticks while running create database during Reshard
  • Exec the mysql client within the test to confirm backticking of tables

Related Issue(s)

Checklist

  • Should this PR be backported? No, for now
  • Tests were added or are not required
  • Documentation is not required

@@ -57,7 +57,7 @@ ENGINE=InnoDB`, mysql.MaximumPositionSize)}
// This is to support in-place upgrades from 13.0.x to 14.0.x
func AlterReparentJournal() []string {
return []string{
"`ALTER TABLE _vt.reparent_journal CHANGE COLUMN primary_alias master_alias VARBINARY(32) NOT NULL`",
"ALTER TABLE _vt.reparent_journal CHANGE COLUMN primary_alias master_alias VARBINARY(32) NOT NULL",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deepthi, FYI: this sql query had extra backticks which was causing failures I noticed in the logs.

@mattlord mattlord added this to the v13.0 milestone Jan 17, 2022
@mattlord
Copy link
Contributor

Nice work, @rohit-nayak-ps ! I think you addressed the two unexpected issues I noticed before when I tried to fix this quickly.

…te. Escape keyspace while creating database during reshard. Update e2e tests to validate these scenarios

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@mattlord
Copy link
Contributor

mattlord commented Jan 18, 2022

FWIW, manual tests against this branch are looking good. I used this:

$ git diff
diff --git a/examples/local/202_move_tables.sh b/examples/local/202_move_tables.sh
index 4cb25390e0..0bb63906bd 100755
--- a/examples/local/202_move_tables.sh
+++ b/examples/local/202_move_tables.sh
@@ -19,4 +19,4 @@

 source ./env.sh

-vtctlclient MoveTables -source commerce -tables 'customer,corder' Create customer.commerce2customer
+vtctlclient MoveTables -source commerce -tables 'Customer-test,corder' Create customer.commerce2customer
diff --git a/examples/local/create_commerce_schema.sql b/examples/local/create_commerce_schema.sql
index e62e7d2e8b..adff44040a 100644
--- a/examples/local/create_commerce_schema.sql
+++ b/examples/local/create_commerce_schema.sql
@@ -4,7 +4,7 @@ create table product(
   price bigint,
   primary key(sku)
 ) ENGINE=InnoDB;
-create table customer(
+create table `Customer-test`(
   customer_id bigint not null auto_increment,
   email varbinary(128),
   primary key(customer_id)
diff --git a/examples/local/create_customer_sharded.sql b/examples/local/create_customer_sharded.sql
index 9d3931c7c9..1df0630961 100644
--- a/examples/local/create_customer_sharded.sql
+++ b/examples/local/create_customer_sharded.sql
@@ -1,2 +1,2 @@
-alter table customer change customer_id customer_id bigint not null;
+alter table `Customer-test` change customer_id customer_id bigint not null;
 alter table corder change order_id order_id bigint not null;

Then executing these steps in the docker_local container:

$ ./201_customer_tablets.sh ; ./202_move_tables.sh

$ mysql commerce -e 'insert into `Customer-test` values (1, "mlord@planetscale.com")'
$ mysql customer -e 'select * from `Customer-test`'
+-------------+-----------------------+
| customer_id | email                 |
+-------------+-----------------------+
|           1 | mlord@planetscale.com |
+-------------+-----------------------+

And address my own minor review nits

Signed-off-by: Matt Lord <mattalord@gmail.com>
Copy link
Contributor

@mattlord mattlord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had previously created a draft PR where I was adding the backticks directly to the routing rules and that got messy. I like how this was addressed here — simple and clean. Thanks, @rohit-nayak-ps !

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rohit-nayak-ps rohit-nayak-ps merged commit 857ed58 into vitessio:main Jan 21, 2022
@rohit-nayak-ps rohit-nayak-ps deleted the rn-escape-routing-rules branch January 21, 2022 11:01
mattlord added a commit to planetscale/vitess that referenced this pull request Jan 26, 2022
This is a follow-up to vitessio#9522

We missed that and thus MoveTables Cancel can still fail when
tables have special characters in them or use reserved words.

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
None yet
3 participants