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

ignore mysql-specific comment statements #3576

Merged
merged 1 commit into from
Jan 23, 2018
Merged

ignore mysql-specific comment statements #3576

merged 1 commit into from
Jan 23, 2018

Conversation

slanning
Copy link
Contributor

This is related to #3520

Ignore /*! mysql-specific */ and /*!50708 mysql-version-specific */ comments.
I'm not sure if it's a good idea. Instead of stripping leading comments,
in case it's /*! ... */ we now don't strip it, and consider it a new StmtComment statement type.
(Not sure if that should be added to ast.go . I didn't.)

I assumed this kind of comment was the only thing in the query,
so if there's something like "/*! ... */ select ..." it wouldn't work.
The handleComment in executor.go basically does nothing, returning &sqltypes.Result{}

This is related to #3520

Also ignore /*!50708 mysql-version-specific */ comments.

I'm not sure if it's a good idea. Instead of stripping leading comments,
in case it's /*! ... */ we don't strip it, and consider it a new StmtComment statement type.
(Not sure if that should be added to ast.go . I didn't.)
I assumed this kind of comment was the only thing in the query,
so if there's something like "/*! ... */ select ..." it wouldn't work.
The handleComment in executor.go basically does nothing, returning &sqltypes.Result{}
@derekperkins
Copy link
Member

I think this will help with migrations. I ran into this yesterday when trying to import a default mysqldump, but vtgate choked on all these standard statements.

/*!40101 SET @OLD_CHARACTER_SET_CLIENT=@@CHARACTER_SET_CLIENT */;
/*!40101 SET @OLD_CHARACTER_SET_RESULTS=@@CHARACTER_SET_RESULTS */;
/*!40101 SET @OLD_COLLATION_CONNECTION=@@COLLATION_CONNECTION */;
/*!40101 SET NAMES utf8 */;
/*!40103 SET @OLD_TIME_ZONE=@@TIME_ZONE */;
/*!40103 SET TIME_ZONE='+00:00' */;
/*!40014 SET @OLD_UNIQUE_CHECKS=@@UNIQUE_CHECKS, UNIQUE_CHECKS=0 */;
/*!40014 SET @OLD_FOREIGN_KEY_CHECKS=@@FOREIGN_KEY_CHECKS, FOREIGN_KEY_CHECKS=0 */;
/*!40101 SET @OLD_SQL_MODE=@@SQL_MODE, SQL_MODE='NO_AUTO_VALUE_ON_ZERO' */;
/*!40111 SET @OLD_SQL_NOTES=@@SQL_NOTES, SQL_NOTES=0 */;

@slanning
Copy link
Contributor Author

Hopefully. :) I'm not sure if what I did makes it not send the SQL to any vttablets, or if it goes through to all of them by default somehow. I think it doesn't pass it through... So I worry that in that mysqldump case, if the SET commands aren't passed through, it might cause data corruption (wrong time zone, character set).

@sougou
Copy link
Contributor

sougou commented Jan 23, 2018

LGTM

Approved with PullApprove

@sougou sougou merged commit 1b5c48f into vitessio:master Jan 23, 2018
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.

4 participants