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 override session.TargetString if it has not been set #5136

Conversation

tirsen
Copy link
Collaborator

@tirsen tirsen commented Aug 26, 2019

This fixes #5132

Signed-off-by: Jon Tirsen [email protected]

@tirsen tirsen requested a review from sougou as a code owner August 26, 2019 04:33
@tirsen tirsen requested a review from demmer August 26, 2019 04:41
@tirsen tirsen force-pushed the jontirsen/2019-08-26/fix-use-for-mysql-with-dbname branch 3 times, most recently from 167334b to 70360eb Compare August 26, 2019 05:25
Copy link
Member

@demmer demmer left a comment

Choose a reason for hiding this comment

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

I'm confused why you still need to set c.SchemaName and also I think a few more tests would be good to verify the behavior matches what we expect in that the use target actually affects the query planner, not just to show the value of the vitess_target.

go/vt/vtgate/mysql_protocol_test.go Show resolved Hide resolved
go/vt/vtgate/plugin_mysql_server.go Outdated Show resolved Hide resolved
go/vt/vtgate/plugin_mysql_server.go Outdated Show resolved Hide resolved
@tirsen tirsen force-pushed the jontirsen/2019-08-26/fix-use-for-mysql-with-dbname branch 2 times, most recently from 950b778 to 095dbd9 Compare August 26, 2019 21:53
@tirsen tirsen force-pushed the jontirsen/2019-08-26/fix-use-for-mysql-with-dbname branch from 095dbd9 to ad420aa Compare August 26, 2019 21:53
Copy link
Member

@demmer demmer left a comment

Choose a reason for hiding this comment

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

Too bad the sandbox doesn't make it easier to properly test the keyspace-specific routing, but I agree with you this does most of what you wanted to test for the use statement.

For completeness you could test that the DbName can be overridden if invalid and cover the various other cases (prepare, etc). But I think this change is safe and straightforward enough that I wouldn't insist on the additional tests.

@tirsen
Copy link
Collaborator Author

tirsen commented Aug 27, 2019

Yeah I agree. I spent a few hours digging into the sandbox and it doesn't look straightforward and certainly not worth the effort for this particular change. It would be nice with a more useful sandbox though!

@tirsen tirsen merged commit dbdae96 into vitessio:master Aug 27, 2019
@tirsen tirsen deleted the jontirsen/2019-08-26/fix-use-for-mysql-with-dbname branch August 27, 2019 04:37
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.

USE statement not respected with JDBC MySQL driver and dbname in connection params
4 participants