Allow per-query read_timeout

[kirs]

For most of the queries in our Rails app, we allow a higher read timeout since they may legitimately take longer. But for health check queries like SHOW VARIABLES or SHOW MASTER STATUS we don't want to want that long before they time out due whatever reasons. It'd be nice to have a per-query read_timeout option - that's what this PR implements!

Implementation

I was looking at how waiting for a read timeout works and it was interesting to learn that it's simply waiting for a file descriptor to become available:

for(;;) {
  retval = rb_wait_for_single_fd(async_args->fd, RB_WAITFD_IN, tvp); // tvp is the timeout

This makes me believe that we can leverage the second argument to query which sets current_query_options and allow read_timeout as a supported option of current_query_options.

[kirs]

See https://stackoverflow.com/questions/40934345/rubocop-25-line-block-size-and-rspec-tests for background.

[sodabrew]

Thanks! This number could be reduced then: https://github.com/kirs/mysql2/blob/b50e1178fe1f8db5fc252a3697b94b42a0f34db2/.rubocop_todo.yml#L24

[jacobbednarz]

I think you're after the instance variable here 😄

[kirs]

Thanks @jacobbednarz!

[sodabrew]

Thank you!

Feedback 1: I think you should also change the MYSQL_OPT_READ_TIMEOUT client option as well (accessible from the C function set_read_timeout and exposed to Ruby space as Mysql2::Client#read_timeout=).

The comment from when this was added (see 82aa624 and d48846f for the major history of this timeout) suggests it was only useful for blocking functions like mysql_ping, and not any different in the query case. Need to do a little more reading on this.

Way under the hood, this MySQL option turns into a setsockopt(... SO_RCVTIMEO ...) https://linux.die.net/man/7/socket so that the read operation will time out and return control into the MySQL library up from the socket level.

Feedback 2: If you're running MySQL 5.7 you can pass an optimizer hint to set a different maximum query time. https://dev.mysql.com/doc/refman/5.7/en/optimizer-hints.html

Example with a timeout of 1 second (1000 milliseconds):
SELECT /*+ MAX_EXECUTION_TIME(1000) */ * FROM t1 INNER JOIN t2 WHERE ...

The benefit of putting the timeout on the server side is that it actually cancels the query, so you get to continue using the client connection without risk that it's still in a query state.

[kirs]

Thanks for feedback and for the context in timeouts.

1. I'd be in favour of updating MYSQL_OPT_CONNECT_TIMEOUT as well, but calling _mysql_client_options requires that the client is not connected - while I'd like to have per-query timeout control on the live connection. Do you think it would be a problem?

2. Optimizer hits are great and I'd be happy to use them instead of changing the client, but I'm afraid that they only work for SELECT. I also guess that they wouldn't work of the problem is on the proxy side.

[sodabrew]

You're right, I forgot that mysql_options has to be called before the connection is established. https://dev.mysql.com/doc/refman/8.0/en/mysql-options.html

The inconsistency I want to avoid is someone trying to set a longer read timeout on a per-query basis, and reporting the next bug :) . Maybe just a simple warning if the timeout value on the query is longer than the value on the connection, and then clamping the effective value to the smaller of the two.