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

only rewrite database() against dual #5793

Merged
merged 8 commits into from
Feb 6, 2020

Conversation

deepthi
Copy link
Member

@deepthi deepthi commented Feb 6, 2020

When we implemented handling of database() to map to keyspace_name, we didn't consider cases where the expression is part of a query that is being sent to a tablet instead of being handled in vtgate.

So a query like select database() would return the keyspace name (which is correct), but a query like select table_name from information_schema.tables where table_schema = database() would not work as expected because the table_schema doesn't match the keyspace name.

This is a stopgap fix until we fully implement information_schema.

Adapted from #5787 with unit test fixes and more tests.
cc @systay

systay and others added 3 commits February 4, 2020 20:01
Signed-off-by: Andres Taylor <andres@planetscale.com>
…systay-fix-database

Signed-off-by: deepthi <deepthi@planetscale.com>
Signed-off-by: deepthi <deepthi@planetscale.com>
@deepthi deepthi requested a review from sougou as a code owner February 6, 2020 01:20
@deepthi deepthi changed the title Systay fix database fix database() rewriting to not rewrite if it is in a where clause Feb 6, 2020
@deepthi deepthi changed the title fix database() rewriting to not rewrite if it is in a where clause only rewrite database() against dual Feb 6, 2020
systay and others added 3 commits February 6, 2020 07:02
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: deepthi <deepthi@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay
Copy link
Collaborator

systay commented Feb 6, 2020

@deepthi, You are correctly pointing out inconsistencies with the illusion we are creating when doing this query rewriting. The way I see it, we have a few choices:

  1. Don't rewrite database() calls. This is what we did before, and the inconsistency was that USE and database() become disconnected from each other:
mysql> use test;
Database changed
mysql> select database();
+------------+
| database() |
+------------+
| vt_test    |
+------------+
1 row in set (0.00 sec)
  1. Rewrite all database() uses. One of the inconsistencies introduced is the mismatch with the information in information_schema.

  2. Try to be clever and rewrite when we think that makes sense. This PR rewrites all queries where the query is a SELECT either without a FROM clause, or one with only dual in it. The thinking is that if the query is SELECT database() or something like that, we use the keyspace name. But if you pull in data from any other source than dual, we'll not handle it by rewriting and lean on mysqld to evaluate it for us.

Another heuristic we could use is to only rewrite database() in SELECT projections, but not in WHERE predicates.

Input is most welcome.

@deepthi
Copy link
Member Author

deepthi commented Feb 6, 2020

@deepthi, You are correctly pointing out inconsistencies with the illusion we are creating when doing this query rewriting. The way I see it, we have a few choices:

  1. Don't rewrite database() calls. This is what we did before, and the inconsistency was that USE and database() become disconnected from each other:
mysql> use test;
Database changed
mysql> select database();
+------------+
| database() |
+------------+
| vt_test    |
+------------+
1 row in set (0.00 sec)
  1. Rewrite all database() uses. One of the inconsistencies introduced is the mismatch with the information in information_schema.
  2. Try to be clever and rewrite when we think that makes sense. This PR rewrites all queries where the query is a SELECT either without a FROM clause, or one with only dual in it. The thinking is that if the query is SELECT database() or something like that, we use the keyspace name. But if you pull in data from any other source than dual, we'll not handle it by rewriting and lean on mysqld to evaluate it for us.

Another heuristic we could use is to only rewrite database() in SELECT projections, but not in WHERE predicates.

Input is most welcome.

Thank you for the detailed analysis. The current implementation seems to be the most sensible option. Even though it is inconsistent, it gives us "working" behavior for all the cases you enumerated.

…nto systay-fix-database

Signed-off-by: deepthi <deepthi@planetscale.com>
…r db name

Signed-off-by: deepthi <deepthi@planetscale.com>
@morgo morgo self-requested a review February 6, 2020 17:20
Copy link
Contributor

@morgo morgo left a comment

Choose a reason for hiding this comment

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

LGTM

@deepthi deepthi merged commit 261d498 into vitessio:master Feb 6, 2020
@deepthi deepthi deleted the systay-fix-database branch February 6, 2020 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants