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

Use SET syntax as specified in the MySQL documentation #1402

Merged
merged 2 commits into from
Mar 8, 2023

Conversation

beautifulentropy
Copy link
Contributor

@beautifulentropy beautifulentropy commented Mar 7, 2023

Description

PR #1099 was able save on round-trip-time by SETting multiple variables in a single transaction. However, the syntax used is out of alignment with both the MySQL and MariaDB docs. Specifically there are missing spaces around the equals (=) sign and after each comma (,).

Syntax output by go-sql-driver/mysql:

SET variable=expr[,variable=expr]...

Syntax as specified in the MySQL 8.0 documentation.

SET variable = expr [, variable = expr] ...

Syntax as specified in the MariaDB documentation:

SET variable_assignment [, variable_assignment] ...

variable_assignment:
      user_var_name = expr
    | [GLOBAL | SESSION] system_var_name = expr
    | [@@global. | @@session. | @@]system_var_name = expr

The SET syntax output by go-sql-driver/mysql is accepted by recent versions of MariaDB in our testing. Unfortunately we've encountered significant issues when these statements are parsed by ProxySQL, a tool we use for efficient routing of queries to our various MariaDB servers.

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

@beautifulentropy beautifulentropy changed the title Use mysql set syntax Use SET syntax as specified in the MySQL documentation Mar 7, 2023
@pgporada
Copy link
Contributor

pgporada commented Mar 7, 2023

An example of how go-sql-driver/mysql v1.6 and v1.7 sending the extended SET syntax to ProxySQL is mangled by regex capturing in that software.

2214 Connect  bad-key-revoker@host on database using TCP/IP
2214 Query    SET SESSION TRANSACTION ISOLATION LEVEL READ UNCOMMITTED,long_query_time
2214 Query    SET sql_mode='STRICT_ALL_TABLES'

Notice how there is a trailing ,long_query_time in SET SESSION TRANSACTION ISOLATION LEVEL READ UNCOMMITTED,long_query_time.

With the fix from @beautifulentropy this is the correct output we now see going through go-sql-driver/mysql => ProxySQL => MariaDB.

7323 Connect  bad-key-revoker@host on database using TCP/IP
7323 Query    SET SESSION TRANSACTION ISOLATION LEVEL READ UNCOMMITTED
7323 Query    SET long_query_time=2880
7323 Query    SET sql_mode='STRICT_ALL_TABLES'
7323 Query    SET long_query_time = 2880, max_statement_time = 3420, sql_mode = STRICT_ALL_TABLES, tx_isolation = 'READ-UNCOMMITTED'

@methane methane merged commit af380e9 into go-sql-driver:master Mar 8, 2023
@methane
Copy link
Member

methane commented Mar 8, 2023

thanks

@methane methane mentioned this pull request Apr 11, 2023
@dolmen
Copy link
Contributor

dolmen commented Jun 1, 2023

@beautifulentropy So this is a workaround for a bug in ProxySQL, isn't it?

If yes, could you link to the ticket you opened on the ProxySQL project?

@beautifulentropy
Copy link
Contributor Author

beautifulentropy commented Jun 1, 2023

@beautifulentropy So this is a workaround for a bug in ProxySQL, isn't it?

If yes, could you link to the ticket you opened on the ProxySQL project?

I don't believe it's fair to characterize this as a "workaround for a bug in ProxySQL". In the issue above I describe how this package passes variables using a SET syntax which isn't described in the official MySQL or MariaDB documentation. The PR simply corrects the package to pass the variables using the syntax as specified by the the maintainers of both projects. Similarly, I don't feel that it's appropriate to open an issue with ProxySQL to support a syntax which isn't documented by either project.

aarongable added a commit to letsencrypt/boulder that referenced this pull request Jul 11, 2023
This update pulls in changes that we contributed upstream, which should
smooth interactions between Boulder and ProxySQL.

Release notes:
https://github.com/go-sql-driver/mysql/releases/tag/v1.7.1
Changelog:
go-sql-driver/mysql@v1.5.0...v1.7.1
Relevant change: go-sql-driver/mysql#1402
@aarongable aarongable deleted the use-mysql-set-syntax branch September 8, 2023 18:24
@dolmen
Copy link
Contributor

dolmen commented Sep 13, 2023

@beautifulentropy Here is the reference documentation:

That documentation doesn't mention anywhere that spaces are mandatory around = or ,. Could you point me to the doc I missed?

@pgporada
Copy link
Contributor

pgporada commented Sep 13, 2023

You linked to the correct docs just as the PR description linked to prior versions of mariadb/mysql docs. While the docs do not explicitly go on to explain why spaces are mandatory around = and ,, the example code block at the top of each of the linked doc page is quite clear. The code block shows spaces surrounding the = and a space surrounding the ,.

SET variable = expr [, variable = expr] ...

@beautifulentropy
Copy link
Contributor Author

You linked to the correct docs just as the PR description linked to prior versions of mariadb/mysql docs. While the docs do not explicitly go on to explain that spaces are mandatory around = and ,, the example code block at the top of each of the linked doc page is quite clear. The code block shows spaces surrounding the = and a space surrounding the ,.

SET variable = expr [, variable = expr] ...

Thanks @pgporada 👍🏻. I'd also like to highlight that the link to the SET syntax outlined in the MariaDB documentation, which was provided in the PR description, aligns with the syntax found in the MySQL documentation links you've presented.

@dolmen
Copy link
Contributor

dolmen commented Sep 28, 2023

Spaces in examples are for readability for humans.

Here we are discussing about machine-to-machine interactions.

Spaces are NOT mandatory around commas.

@dolmen dolmen mentioned this pull request Sep 28, 2023
5 tasks
@evanelias
Copy link
Contributor

evanelias commented Apr 8, 2024

The root cause here was a ProxySQL bug, see sysown/proxysql#4268 (comment). It appears to have been fixed sometime last year.

fwiw, to the best of my knowledge, the MySQL manual and MariaDB manual both use spaces in the grammar strictly for human readability. The server's lexer treats spaces as entirely optional in situations where the division between tokens is unambiguous. For a couple extreme examples, these horribly-formatted queries are actually completely legal:

SELECT'foo'AS'lol'FROM(SELECT'ok')x;

select*from`information_schema`.`tables`where`engine`='innodb';

Ambiguous cases do require spaces, for example you must use limit 1 and never limit1, because the latter could be an identifier name.

I've been using MySQL continuously since 2003, and to the best of my recollection it has always worked this way. The manual doesn't explicitly state this one way or another, to be fair. But it does provide detailed documentation on the rare edge cases in which whitespace is meaningful. Off the top of my head, I can think of only two of those cases:

Since there's no corresponding long discussion in the manual around SET args, I would interpret that as meaning whitespace is optional there. In any case that's how the server actually behaves in that situation too.

Anyway, sorry for rambling on a year-old PR. Personally for my two cents, I don't think this PR should be reverted anytime soon. ProxySQL is widely used (it's great software) and it will be a long while until every ProxySQL user upgrades to a new enough version that fixes this bug. Paying a measly two extra bytes per session variable is a low cost to help with compatibility here.

That said, for future situations like this, I would strongly urge opening an issue with ProxySQL. Regardless of how one interprets the MySQL manual, this situation is technically a parser differential, and (in general) parser differentials between proxies and backends are like catnip to clever attackers looking for crazy ways to build SQL injection attacks or find information leaks. I doubt this one is exploitable, but who knows, and there's no harm in reporting it.

@beautifulentropy
Copy link
Contributor Author

beautifulentropy commented Apr 8, 2024

@evanelias as much as you might feel awkward for "rambling on a year-old PR". Thanks for taking the time to write this, it was super informative.

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.

5 participants