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

Better parsing of USE queries sent with COM_QUERY #4598 #4605

Merged
merged 4 commits into from
Aug 13, 2024
Merged

Conversation

renecannao
Copy link
Contributor

@renecannao renecannao commented Aug 12, 2024

Closes #4598

USE queries sent via COM_QUERY are not processed in a better way, improving their parsing:

  • it is implemented into SetParser (even if it is not a SET statement !! , to minimize code changes)
  • comments are removed
  • it handles schemaname with and without backtick
  • in case backtick is used, a space between USE and dbname is optional

This new parser passes the tests in existing reg_test_3493-USE_with_comment-t TAP test, and it also introduced a new set of tests that are automatically executed in debug build.

@renecannao
Copy link
Contributor Author

Some of the test in reg_test_3493-USE_with_comment-t are failing.

List of all tests: https://github.com/sysown/proxysql/blob/v2.6.3/test/tap/tests/reg_test_3493-USE_with_comment-t.cpp#L171-L185
For reference:

... ("test_use_comment", "/*+ placeholder_comment */ USE test_use_comment", false));
... ("`test_use_comment-a1`", "USE /*+ placeholder_comment */ `test_use_comment-a1`", true));
... ("test_use_comment_1", "  USE /*+ placeholder_comment */   `test_use_comment_1`", false));
... ("test_use_comment_2", "USE/*+ placeholder_comment */ `test_use_comment_2`", false));
... ("test_use_comment_3", "USE /*+ placeholder_comment */`test_use_comment_3`", true));
... ("test_use_comment_4", "  USE /*+ placeholder_comment */   test_use_comment_4", false));
... ("test_use_comment_5", "USE/*+ placeholder_comment */ test_use_comment_5", false));
... ("test_use_comment_6", "USE /*+ placeholder_comment */test_use_comment_6", true));
... ("`test_use_comment-1`", "  USE /*+ placeholder_comment */   `test_use_comment-1`", false));
... ("`test_use_comment-2`", "USE/*+ placeholder_comment */ `test_use_comment-2`", false));
... ("`test_use_comment-3`", "USE /*+ placeholder_comment */`test_use_comment-3`", true));
... ("`test_use_comment-4`", "/*+ placeholder_comment */USE          `test_use_comment-4`", false));
... ("`test_use_comment-5`", "USE/*+ placeholder_comment */`test_use_comment-5`", false));
... ("`test_use_comment-6`", "/* comment */USE`test_use_comment-6`", false));
... ("`test_use_comment-7`", "USE`test_use_comment-7`", false));

@mirostauder
Copy link
Collaborator

retest this please

@renecannao renecannao merged commit 050907e into v2.x Aug 13, 2024
50 of 51 checks passed
@TomaszKorwel
Copy link

This seems to have introduced problem with laravel's doctrine:

[2024-08-29 08:52:28] developmnet.ERROR: SQLSTATE[42000]: Syntax error or access violation: 1148 Unable to parse: use app; (Connection: mysql, SQL: ....) {"exception":"[object] (Illuminate\Database\QueryException(code: 42000): SQLSTATE[42000]: Syntax error or access violation: 1148 Unable to parse: use app; (Connection: mysql, SQL: ... ) at /var/share/nginx/html/current/vendor/laravel/framework/src/Illuminate/Database/Connection.php:829)"

Reverting proxysql to 2.6.3 solved our problem.

Laravel config:

    'connections' => [

        'mysql' => [
            'driver'      => 'mysql',
            'host'        => env('DB_HOST',''),
            'port'        => env('DB_PORT',''),
            'database'    => env('DB_DATABASE', 'app'),
            'username'    => env('DB_USERNAME',''),
            'password'    => env('DB_PASSWORD',''),
            'unix_socket' => env('DB_SOCKET', ''),
            'charset'     => 'latin1',
            'collation'   => 'latin1_swedish_ci',
            'prefix'      => '',
            'strict'      => true,
            'engine'      => null,
        ],

@renecannao
Copy link
Contributor Author

@TomaszKorwel , yes, we are aware.
Laravel seems to do 2 unusual things at the same time:

  • it uses COM_QUERY to send USE command instead of using COM_INIT_DB
  • it adds a semicolon at the end of the database name

On our end, we missed removing the semicolon from queries sent by application like Laraval .
PR #4629 fixed this.
Expect 2.6.5 to be released within hours.

@TomaszKorwel
Copy link

Thanks!

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.

Not ignoring comments on "use database" statements
3 participants