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

Add functionality for setting server options #52

Merged

Conversation

paarthmadan
Copy link
Contributor

@paarthmadan paarthmadan commented Mar 7, 2023

What?

This PR brings support for setting server options on a connection after it's already been established.

This requires building support for the COM_SET_OPTION command, building the respective client (block and non-blocking) flows, and offering a matching API in the Ruby binding.

It brings support for something like:

    client = new_tcp_client(multi_statement: false)
    create_test_table(client)

    # Fixture insertion flow (which benefits from MULTI statement)     
    client.set_server_option(Trilogy::SET_SERVER_MULTI_STATEMENTS_ON)

    client.insert(...) # Use multi
    client.next_result while client.more_results_exist?
ensure
    client.set_server_option(Trilogy::SET_SERVER_MULTI_STATEMENTS_OFF)

Why?

It will be immediately useful for trilogy-libraries/activerecord-trilogy-adapter#38

The context for a specific use case is summarized in this issue: trilogy-libraries/activerecord-trilogy-adapter#37. The TL;DR is that the adapter requires modifying the status of multi statement after a connection has already been initialized with the user-defined config. This unblocks that flow.

Implementation

To build the packet, I relied mostly on the MySQL doc for the COM_SET_OPTION packet. Unfortunately, the documentation was somewhat misleading, and digging into the MySQL sourcecode revealed there was some notable typos.

  1. The command identifier is 0x1b (not 0x1a as suggested in the docs). I fact checked that this was the case in MySQL's source, 0x1a appears to be reserved for COM_RESET.
  2. The packet type returned by MySQL for trilogy_set_server_option_send comes with an EOF header (but is structured like the newer OK packets). This deviates from the other flows, but I confirmed in source that this seems to be a one-off (perhaps missed when they migrated).

Currently, the only server options MySQL offer for configuration is MULTI_STATEMENT_ON and MULTI_STATEMENT_OFF.

Commits

I've separated this change into more commits than I normally would, mostly because some of the commit messages contain helpful debugging/historical context I'd like to track. If reviewers feel strongly about entering this as one commit, I can squash 👍

@casperisfine
Copy link
Contributor

We'll likely need CLIENT_MULTI_RESULTS as well.

/**
  Enable/disable multi-results

  Server
  ------

  Can send multiple resultsets for COM_QUERY.
  Error if the server needs to send them and client
  does not support them.

  Client
  -------

  Can handle multiple resultsets for COM_QUERY.

  Requires
  --------

  ::CLIENT_PROTOCOL_41

  @sa mysql_execute_command(), sp_head::MULTI_RESULTS
*/
#define CLIENT_MULTI_RESULTS (1UL << 17)

@paarthmadan paarthmadan force-pushed the pm/set-server-option-multi branch 3 times, most recently from 5b8c869 to b6ec8a6 Compare March 10, 2023 22:51
@paarthmadan
Copy link
Contributor Author

We'll likely need CLIENT_MULTI_RESULTS as well.

I've updated this PR with b6ec8a6, which brings it closer to Mysql2 in the implications it makes.

I can work on a separate patch which enables CLIENT_MULTI_RESULTS as part of the default Trilogy capabilities.

Enabling CLIENT_MULTI_RESULTS by default might require #3, because it's only use-case standalone from multi statement is in prepared statements. I suppose we could spoof it by enabling without that, but I'm curious how others feel about that?

@paarthmadan paarthmadan marked this pull request as ready for review March 10, 2023 22:55
@paarthmadan
Copy link
Contributor Author

This is ready for review, I tested it with trilogy-libraries/activerecord-trilogy-adapter#38, and it seems to work nicely.

@brianmario
Copy link
Contributor

Enabling CLIENT_MULTI_RESULTS by default might require #3, because it's only use-case standalone from multi statement is in prepared statements. I suppose we could spoof it by enabling without that, but I'm curious how others feel about that?

fwiw #3 should basically be ready to take out of draft, I just wanted to add a few more tests but haven't had the time to get to it. If anyone would like to take a stab at that I can add you to my fork. I'm still unsure when I'll have the time to pick it back up, but it's basically done.

@byroot
Copy link
Contributor

byroot commented Mar 11, 2023

I fell like we need to give context here. This PR was initially to support MULTI_STATEMENT as a nice to have for later.

However in the meantime, we discovered that ProxySQL, if clients have different connection options, has to constantly change user which is very costly on the system.

Since the official libmysqlclient set CLIENT_MULTI_RESULTS by default, we'd like to set it to make our rollout easier, but that's purely for appearing similar to mysql2 from ProxySQL point of view.

@adrianna-chang-shopify
Copy link
Collaborator

I opened a PR for MULTI_RESULTS here: #57

Enabling CLIENT_MULTI_RESULTS by default might require #3, because it's only use-case standalone from multi statement is in prepared statements.

@paarthmadan and I discussed this IRL -- I think there are use cases outside of prepared statements such as stored procedures. Makes sense to me to move forward with supporting the flag and enabling it by default.

This will be particularly useful for enabling/disabling multi statements
on the fly, after a connection has already been initialized.
After looking through MySQL sourcecode, it appears 0x1a is already reserved for COM_RESET

See, https://github.com/mysql/mysql-server/blob/1bfe02bdad6604d54913c62614bde57a055c8332/router/src/mysql_protocol/tests/test_classic_protocol.cc#L168-L173,
which highlights that 0x1A is reserved for COM_RESET, while 0x1B behaves
as expected for COM_SET_OPTION.
See, https://github.com/mysql/mysql-server/blob/1bfe02bdad6604d54913c62614bde57a055c8332/sql/sql_parse.cc#L2377-L2397,
which clarifies that the packet type returned is an eof. We should use
the EOF identifer to discriminate the packet, but parse it as an OK
because that's the information it contains.
@HParker HParker merged commit 05a9e46 into trilogy-libraries:main Mar 16, 2023
@paarthmadan paarthmadan deleted the pm/set-server-option-multi branch March 16, 2023 17:28
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.

None yet

6 participants