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

The latest Docker preview image breaks MySQL that contains multiple stataments (such as set variable) #5925

Closed
myonlylonely opened this issue Mar 21, 2023 · 11 comments · Fixed by #5957

Comments

@myonlylonely
Copy link
Contributor

myonlylonely commented Mar 21, 2023

Issue Summary

The latest Docker preview image breaks MySQL that contains multiple stataments. Reverting back to the previous preview image solves the problem.

Steps to Reproduce

  1. Write a MySQL query that contains mutiple statements.
set @var_1 = 123;
select * from test_table;
  1. Execute the query, then it returns Error running query: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'select * from test_table' at line 2.

image

  1. Executing the first or the second statements alone works fine.

  2. Reverting to the old Docker preview image also works fine.

image

Technical details:

  • Redash Version: 11.0.0-dev (28b0a23)
  • Browser/OS: Chrome 111.0.5563.64/macOS 13.2.1
  • How did you install Redash: Docker
@myonlylonely
Copy link
Contributor Author

myonlylonely commented Mar 22, 2023

I have found the cause, the version string returned from _mysql.get_client_info() is different, and is lower than 4 in the latest Docker image.

import _mysql
logger.info(_mysql.get_client_info())

The above code print 3.1.20 in the latest Docker image, but 10.3.36 in the previous Docker Image.
mysqlclient 1.3.14 has the following code (https://github.com/PyMySQL/mysqlclient/blob/1.3.14/MySQLdb/connections.py)

client_flag = kwargs.get('client_flag', 0)
client_version = tuple([ numeric_part(n) for n in _mysql.get_client_info().split('.')[:2] ])
if client_version >= (4, 1):
  client_flag |= CLIENT.MULTI_STATEMENTS
if client_version >= (5, 0):
  client_flag |= CLIENT.MULTI_RESULTS

kwargs2['client_flag'] = client_flag

So it does not enable MULTI_STATEMENTS and MULTI_RESULTS flags.
Manually pass the client_flag parameter works fine.

def _connection(self):
  params = dict(
  # other param
    client_flag=MULTI_STATEMENTS | MULTI_RESULTS,
  )
  # other code
  connection = MySQLdb.connect(**params)

  return connection

But I wonder why it is different in the newest Docker image? I guess it has something to do with the dependency issues?
@arikfr

@arikfr
Copy link
Member

arikfr commented Mar 22, 2023

Did you build the Docker image locally?

@myonlylonely
Copy link
Contributor Author

myonlylonely commented Mar 22, 2023

Did you build the Docker image locally?

No, I use the Docker image from official Docker hub.I tried the latest preview image, it does not work, but previous preview image works (published 6 months ago).

@arikfr
Copy link
Member

arikfr commented Mar 22, 2023

My best guess is that something went wrong with the Docker build and it's using an old mysql client version. We're going to fix the build soon🤞, so maybe we can wait for a new image to test this again?

@myonlylonely
Copy link
Contributor Author

Sure, please let me know when the new image is ready for testing.

@myonlylonely
Copy link
Contributor Author

My best guess is that something went wrong with the Docker build and it's using an old mysql client version. We're going to fix the build soon🤞, so maybe we can wait for a new image to test this again?

It may has something to do with mariadb-client changes. https://jira.mariadb.org/browse/CONC-488.

It says

When old GPL licensed libmysql was replaced by MariaDB Connector/C (10.2 and above) we changed the version number to the corresponding server version to prevent breakage of existing applications.

In the new preview Docker image the client version is 10.3, and the old one is using 10.1. So it may be related to that.

@myonlylonely
Copy link
Contributor Author

@arikfr
Could we bump the version of mysqlclient to a newer version, as the latest version does not require check the mysql client lib version to enable MULTI_STATEMENTS flag.
Or we could manually pass the client_flag parameter to the connection to stay at current version?
Either way, I could make a pull request.

@arikfr
Copy link
Member

arikfr commented Mar 30, 2023

@myonlylonely yes, we can. The plan is to upgrade all dependencies once we have CI properly running again. But you can create a PR to update the client ahead of this.

Are there any limitations in terms of what MySQL version it can connect to with the new client?

@myonlylonely
Copy link
Contributor Author

@arikfr Not that I know of. I have already made a pull request to bump the mysqlclient version. #5957

@justinclift
Copy link
Member

... once we have CI properly running again.

@arikfr What's the plan of action for that bit?

@arikfr
Copy link
Member

arikfr commented Apr 16, 2023

@justinclift switch to GitHub Actions.

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 a pull request may close this issue.

3 participants