Port the Async MySQL client to 5.6
ClosedPublic

Press ? to show keyboard shortcuts.
Author
steaphan
Reviewers
chip
inaam-rana
pivanof
darnaut
jeremycole
Repository
rWEBSCALESQL WebScaleSQL
Lint
No Linters Available
Unit
Unit Tests OK
Show Full Unit Results (49 Passed, 49 Details)
Commits
Unknown Object (Diffusion Commit)
rWEBSCALESQLc195c11e7566: Port the Async MySQL client to 5.6
rWEBSCALESQL52596775ef59: Port the Async MySQL client to 5.6
rWEBSCALESQLb89256ef10a0: Port the Async MySQL client to 5.6
Unknown Object (Diffusion Commit)
rWEBSCALESQLa0a4422bbae0: Port the Async MySQL client to 5.6
rWEBSCALESQL014391269e67: Port the Async MySQL client to 5.6
rWEBSCALESQL004b6b348fdf: Port the Async MySQL client to 5.6
Branch
ws_async_client
Apply Patch
arc patch D17031
Arcanist Project
Restricted Arcanist Project
Subscribers
fe, darnaut, methane and 9 others
Projects
None
Summary

WebScaleSQL Feature: Async Client

This diff builds loosely upon the 5.1 client work and brings async to
5.6. It is not fully feature complete (no SSL. no compression, only
supports the default native auth) but it is sufficient for our needs and
we can extend it.

This breaks the API slightly by cleaning it up and making it more
succinct. Code using the old async API will need to change in minor
ways (though a compatibility layer could be introduced).

This also adds a #define to indicate the nonblocking client is available.

This changes slightly the ABI and so it also renames 'libmysqlclient' to
'libwebscalesqlclient' -- however, this is compatible with almost every
use case (changes are additive only, but do include changes to some data
structures).

Test Plan

mtr, testing against internal tools

Older changes are hidden. Show older changes.
steaphan added a comment.Via ConduitDec 31 2014, 2:55 AM

Clean rebase, based on the now-pushed version of the dependent diff.

darnaut resigned from this revision.Via WebJan 1 2015, 9:26 AM
darnaut removed a reviewer: darnaut.

I'm not particularly fond of this patch, might benefit from a new reviewer.

client/mysqltest.cc
178

The fact that it is used in only a single file doesn't imply it should be in that file. These are wrappers for the client library that clearly form a separate API/module and would be better organized in a separate file whose object file could be linked to mysqltest.

Ignoring that for a moment, and pursuing your argument that this should be in a single file, so why are those functions not static, why are there STDCALL annotations? Looks like copypasta.

186

It's beside me how does this truly says that something is non-blocking. I'm usually against leaving weird debug code behind, but alas..

220

Who knows, the point is that we should strive for resilient code that doesn't push problems forward.

256

If select fails (that is, returns -1), there is no guarantee that expect is valid.

Quoting the man page: "On error, -1 is returned, and errno is set appropriately; the sets and timeout become undefined, so do not rely on their contents after an error."

This function is pretty much an example on how not to use select().

453

So what is it for? The printf seems useless. socket_event_listen already contains code like:

+ if (result < 0) {
+ perror("select");
+ }
+
+ if (FD_ISSET(fd, &except)) {
+ fprintf(stderr, "socket %d in exception set\n", fd);
+ result = -1;
+ }

457

It just looks silly and unnecessary noise given the language rules around NULL.

3911

I would buy that if this patch actually followed the "fit in with the surroundings", but it clearly does not (see coding style).

Just to clarify, when we switched to doxygen comments, the goal was to incrementally move to it as changes are made (that is, not converting everything to doxygen in step).

Anyway, don't care.

7160

The way to achieve this is to have such tests skip if async-client (see various have_*.inc).

mysql-test/t/big_packets_boundary.test
4

I'm not sure I follow. Simple example:

let $str= SELECT REPEAT('a', 16000000);
...
--eval SELECT MD5("$str")

Would both test receiving and sending large queries.

mysql-test/t/connection_timeout-mysqltest.opt
1

--connect-timeout=1200 doesn't matter anymore?

vio/viosocket.c
136

It should only return 0 when the EOF condition is read. This is pretty much a well established standard. Reading an EOF is not the same as "timeout or other end did something wonky", this change is actually the kind of thing that makes APIs inconsistent and peculiar in their behaviors.

Again, please look at the callers of vio_read (in particular the ones expecting 0 on EOF) and tell me again how does this is needed and how it does not break such use cases?

steaphan planned changes to this revision.Via WebJan 1 2015, 11:29 PM

I will propose an updated version including our latest update to the test (once that is ready). Should take < 1 week.

client/mysqltest.cc
178

Yeah, I wasn't saying I disagreed with you, only that the time it would take to move them into a new file would not be worth it, as it wouldn't really gain us anything of much value.

Yes, you're right - these should all be static. I'll fix that.

186

@chip, if that's all this was for, we should take it out. Agreed?

220

Of course... but this is mysqltest.cc, which really only ever has to work for mtr tests, and I don't see how this could possibly fail to handle that.

256

Yep. I'll fix that.

453

Yeah... this looks like more debugging code that's not really required anymore. @chip?

457

Ok, I'll remove it. Getting rid of it can't hurt either.

7160

We actually have a diff to this effect in review (internally at Facebook) right now. I'll merge it in with this diff once it's completed.

mysql-test/t/big_packets_boundary.test
4
mysql-test/t/connection_timeout-mysqltest.opt
1

This removal looks wrong to me. @chip - does removing this make sense somehow that I don't understand?

vio/viosocket.c
136

@chip ^^^

chip added inline comments.Via WebJan 3 2015, 2:10 AM
client/mysqltest.cc
186

It is meant for visual inspection; think of it as a slightly fancy way to print DBUG statements. It proved quite helpful for ensuring there were no accidentally blocking codepaths and that API calls were taking the expected amount of time (ie, generally zero). This is a hard thing to write a perfect test for as timing variations are tricky when talking to the same host.

the comment definitely isn't clear about this, though, and could be improved to make it clear this is effectively just a way to print dbug messages to help during development on this feature (and will be used again soon when we add async ssl support)

453

agree

mysql-test/t/big_packets_boundary.test
4

I vaguely recall trying to do precisely this but it not working for some reason, but that was well over a year ago so my memory isn't precise. It seems to work fine now, though, so either something changed or I was just doing something wrong.

-pad str 16777208;
+let $str = `SELECT REPEAT('X', 16777208)`;

so, definitely no need for the pad function

mysql-test/t/connection_timeout-mysqltest.opt
1

No specific recollection; probably unintentional, assuming it passes with it back. If it doesn't, I can look more into it.

vio/viosocket.c
136

Let's try removing it and see if async and sync tests pass. It may not be necessary. EOF or error pretty much means the same thing in most (all?) codepaths inside of mysql, except for sometimes retrying, so the net difference is probably nothing. It's worth a try; this line was in the very original diff (and very possibly is from the 5.1 patch before it was mostly rewritten).

steaphan added a comment.Via WebJan 3 2015, 2:26 AM

I am running the tests on this (internally) now. Assuming all is well in the tests (both in async and sync client modes), I will update this diff shortly.

client/mysqltest.cc
186

That makes sense to me. So, I've left this in.

453

I looked through all of these, and they added no additional context, so I removed them all.

mysql-test/t/big_packets_boundary.test
4

I've removed the pad function, and used this instead.

mysql-test/t/connection_timeout-mysqltest.opt
1

I've restored this, and the test still passes.

vio/viosocket.c
136

Ok, I've removed it.

steaphan added a comment.Via WebJan 3 2015, 3:42 AM

Putting up a new revision now.

vio/viosocket.c
136

A whole bunch of tests fail in async client mode without this - mostly rpl tests, and all timing out on 900 seconds. They all work fine if I put this back in.

An example (fast) test that shows this behavior is: binlog.binlog_enforce_gtid_consistency

So, while I don't see why this is there, it *does* apparently catch an important case. So, I put it back.

steaphan updated this revision to Diff 161757.Via ConduitJan 3 2015, 3:42 AM

Updated based on review comments.

darnaut added a subscriber: darnaut.Via WebJan 3 2015, 10:25 AM
darnaut added inline comments.
client/mysqltest.cc
220

I don't see how this could possibly fail to handle that.

All that has to happen is for a file descriptor (a socket in this case) in mysqltest.cc to have a number equal to or greater than FD_SETSIZE. For example, a test that causes mysqltest.cc to open FD_SETSIZE file descriptors through a large number of connections. There is a lot of ways a test may commandeer mysqltest.cc to open a lot of file descriptors.

This might be a difference in perspective, I'm usually against pushing forward things that are obviously broken (technical debt). In particular, this is not the first time I've seen this same bug in MySQL, it has been repeated many times with similar arguments. Last time was that the socket for binding on 3306 would never get that high, which later proved to be wrong. So excuse me if I'm grumpy.

sql/net_serv.cc
1003

Code path only invoked on net_realloc failure, which already propagates an error. This masks any errors thrown by net_realloc (e.g. ER_NET_PACKET_TOO_LARGE) as an error reading from the socket.

vio/viosocket.c
136

EOF or error pretty much means the same thing in most (all?) codepaths inside of mysql, except for sometimes retrying

Not only that, it's also used to provide different error codes (for example, there is a specific error code for timeouts, which is different from other socket related errors). In other contexts, it's used to interrupt waits (e.g. SLEEP(n)) on client connection EOF.

A whole bunch of tests fail in async client mode without this - mostly rpl tests

Not surprising if the async code wasn't also fixed to gracefully handle EOF...

steaphan planned changes to this revision.Via WebJan 3 2015, 6:18 PM

Will update again soon - please feel free to continue to review and give more feedback, if you see anything else wrong.

client/mysqltest.cc
220

I assure you, I share your perspective. The only thing that makes me hesitate here is that this is mysqltest.cc - not anythnig that matters for anything other than testing.

But, just because *I* can't see how this could ever use that many FDs doesn't mean it really couldn't. You know this system better than I do, so I will trust you on this, and I will re-write this to not use select().

sql/net_serv.cc
1003

This would also be hit if vio_read() returns VIO_SOCKET_ERROR, but socket_errno isn't SOCKET_EAGAIN or SOCKET_EWOULDBLOCK (which, judging by other code, is also possible).

There are at least 5 different error status variables here. I am really not sure how this all works. @chip?

vio/viosocket.c
136

I see. So, this *is* incorrect, as we thought, but it is actually masking a different problem. @chip and I will look into this. Thanks.

steaphan updated this revision to Diff 162243.Via ConduitJan 6 2015, 8:40 PM

Updated after a full round of testing, based on review feedback.

steaphan added a reviewer: darnaut.Via WebJan 6 2015, 8:42 PM

Davi,

I do hope you would be willing to continue reviewing this diff. I know you dislike it in general, but it is easily the change I get the most questions about, and requests for. And, you are clearly the best one to do this review. Your feedback has already made this significantly better.

steaphan added a comment.Via WebJan 6 2015, 9:10 PM

Note: The test failures of main.log_errchk are expected, and are due to the upstream bug fixed here:

https://reviews.facebook.net/D31023

steaphan updated this revision to Diff 162537.Via ConduitJan 7 2015, 11:24 PM

Clean rebase, no changes. Just re-running tests based upon a branch with the main.log_errchk fix.

fe added a subscriber: fe.Via WebJan 15 2015, 5:16 PM

bunnyping

jeremycole added a reviewer: jeremycole.Via WebJan 30 2015, 10:16 PM
steaphan updated this revision to Diff 170193.Via ConduitFeb 4 2015, 6:34 PM

Rebased onto 5.6.23. One minor conflict resolved. In sql-common/client.c, upstream commit a9b61b002971 reordered code around this change. All seems well with this resolution.

jeremycole accepted this revision.Via WebFeb 5 2015, 10:43 PM

Relatively minor comments, but the main ones I have would be about not making generically-named additions to exported symbols or header files. None of that has to stop pushing this patch as-is though, clearly it works for you and should work for others. I would like to see it cleaned up re. API a bit in the future.

client/mysqltest.cc
180

This comment seems out of place... ?

include/mysql.h
284

Need mysql_ prefix.

292

Need mysql_ prefix.

302

Need mysql_ prefix.

303

Need mysql_ prefix.

371

Need mysql_ prefix.

397

Need mysql_ prefix.

398

Need mysql_ prefix.

sql-common/client.c
1880

Tabs here and below?

This revision is now accepted and ready to land.Via WebFeb 5 2015, 10:43 PM
steaphan added a comment.Via WebFeb 5 2015, 11:26 PM

Updating now, for a final test run before push.

client/mysqltest.cc
180

Yep. Removed it.

include/mysql.h
284

Agreed. Fixed.

292

Agreed. Fixed.

302

Agreed. Fixed.

303

Agreed. Fixed.

371

Agreed. Fixed.

397

Agreed. Fixed.

398

Agreed. Fixed.

sql-common/client.c
1880

Indeed. Fixed.

steaphan updated this revision to Diff 170727.Via ConduitFeb 5 2015, 11:29 PM

Made requested changes. Updating so I can run the full set of tests one more time before I push.

steaphan closed this revision.Via DaemonsFeb 6 2015, 12:12 AM
steaphan updated this revision to Diff 170733.

Closed by commit rWEBSCALESQL004b6b348fdf (authored by @steaphan).

steaphan updated the summary for this revision. (Show Details)Via WebFeb 8 2015, 6:10 AM
jtolmer mentioned this in Unknown Object (Diffusion Commit).Via DaemonsAug 27 2015, 11:59 PM
jtolmer mentioned this in Unknown Object (Diffusion Commit).

Revision Update History

DiffIDBaseDescriptionCreatedLintUnit
BaseBase
Diff 152749f903a93Mar 19 2014, 4:22 PM
Diff 25319314a4ac4Clean rebase on updated millisecond client timeouts diff. Turning this one over to @chip (and @darnaut) from here on.Mar 25 2014, 1:28 AM
Diff 353367f0badcfClean rebase on cleaned branch history. No actual code changes.Mar 26 2014, 1:14 AM
Diff 453391d9f46b4mark tests that require auth features that async doesn't support as not to be run with async clientMar 26 2014, 2:25 AM
Diff 553511d9f46b4change name of our shared library to libwebscalesqlclient.so to avoid conflicting with system client libraryMar 27 2014, 5:19 PM
Diff 653889c35fb8erebase to 5.6.17Mar 30 2014, 7:27 AM
Diff 758239fb8e2a6Clean rebase to 5.6.19, in preparation for requested changes. No changes made to this diff yet.Jun 7 2014, 12:00 AM
Diff 869675225862cClean rebase to 5.6.20 - no changes to this diff (but some are planned).Jul 31 2014, 8:17 PM
Diff 9149133552d711Clean rebase to 5.6.21. No changes to this diff yet.Oct 30 2014, 6:19 PM
Diff 1015831916208bdClean rebase to 5.6.22. No changes to this diff yet.Dec 16 2014, 3:30 AM
Diff 1115832516208bdFix ssl timeouts.Dec 16 2014, 3:33 AM
Diff 1216091761f7e27Updated based on review comments.Dec 30 2014, 1:35 AM
Diff 13161283acd6c39Rebased, and updated, based on update to dependent refactor diff.Dec 31 2014, 1:59 AM
Diff 141613199fb8abfClean rebase, based on the now-pushed version of the dependent diff.Dec 31 2014, 2:55 AM
Diff 151617579f58effUpdated based on review comments.Jan 3 2015, 3:42 AM
Diff 161622439f58effUpdated after a full round of testing, based on review feedback.Jan 6 2015, 8:40 PM
Diff 1716253754b07ebClean rebase, no changes. Just re-running tests based upon a branch with the main.log_errchk fix.Jan 7 2015, 11:24 PM
Diff 18170193cc3b2c3Rebased onto 5.6.23. One minor conflict resolved. In sql-common/client.c, upstream commit a9b61b002971 reordered code around this change. All seems well with this resolution.Feb 4 2015, 6:34 PM
Diff 1917072732d1ca8Made requested changes. Updating so I can run the full set of tests one more time before I push.Feb 5 2015, 11:29 PM
Diff 2017073332d1ca8Commit rWEBSCALESQL004b6b348fdf48f0aa4e3fe1010b891d4fdb9f70Feb 6 2015, 12:10 AM

Local Commits

CommitTreeParentsAuthorSummaryDate
cb515711a51fa4b2f8ca752f32d1ca88734dSteaphan Greene
Port the Async MySQL client to 5.6 (Show More…)
Dec 31 2014, 1:47 AM

Table of Contents

PathCoverage (All)Coverage (Touched)
Mclient/CMakeLists.txt (24 lines)--
Mclient/mysqltest.cc (590 lines)--
Mdbug/dbug.c (3 lines)--
Minclude/mysql.h (137 lines)--
Minclude/mysql.h.pp (149 lines)--
Minclude/mysql/client_plugin.h (4 lines)--
Minclude/mysql/client_plugin.h.pp (13 lines)--
Minclude/mysql/plugin_auth.h.pp (10 lines)--
Minclude/mysql/plugin_auth_common.h (18 lines)--
Minclude/mysql_com.h (124 lines)--
Minclude/sql_common.h (20 lines)--
Minclude/violite.h (9 lines)--
Mlibmysql/CMakeLists.txt (28 lines)--
Mlibmysql/libmysql.c (23 lines)--
Mlibmysqld/lib_sql.cc (7 lines)--
Mmysql-test/lib/mtr_cases.pm (5 lines)--
Mmysql-test/mysql-test-run.pl (25 lines)--
AMmysql-test/r/big_packets.result (6 lines)--
AMmysql-test/r/big_packets_boundary.result (96 lines)--
AMmysql-test/suite/auth_sec/t/access_credential_control-mysqltest.opt (1 line)--
AMmysql-test/suite/auth_sec/t/key_value_auth-mysqltest.opt (1 line)--
AMmysql-test/suite/auth_sec/t/mysql_old_passwords-mysqltest.opt (1 line)--
AMmysql-test/suite/auth_sec/t/mysql_old_plugin-mysqltest.opt (1 line)--
AMmysql-test/suite/auth_sec/t/mysql_sha256_plugin-mysqltest.opt (1 line)--
AMmysql-test/suite/auth_sec/t/password_expired-mysqltest.opt (1 line)--
Mmysql-test/suite/perfschema/t/hostcache_ipv4_auth_plugin.test (1 line)--
Mmysql-test/suite/perfschema/t/hostcache_ipv6_auth_plugin.test (1 line)--
Mmysql-test/suite/rpl/t/rpl_loaddatalocal.test (2 lines)--
AMmysql-test/suite/sys_vars/t/old_passwords_func-mysqltest.opt (1 line)--
AMmysql-test/suite/sys_vars/t/secure_auth_func-mysqltest.opt (1 line)--
AMmysql-test/t/big_packets-master.opt (1 line)--
AMmysql-test/t/big_packets.test (13 lines)--
AMmysql-test/t/big_packets_boundary-master.opt (1 line)--
AMmysql-test/t/big_packets_boundary.test (18 lines)--
Mmysql-test/t/change_user.test (1 line)--
AMmysql-test/t/connect-mysqltest.opt (1 line)--
Mmysql-test/t/connect.test (2 lines)--
Mmysql-test/t/connection_timeout-mysqltest.opt (1 line)--
AMmysql-test/t/plugin_auth-mysqltest.opt (1 line)--
Mmysql-test/t/plugin_auth.test (1 line)--
AMmysql-test/t/plugin_auth_qa_1-mysqltest.opt (1 line)--
Mmysql-test/t/plugin_auth_qa_1.test (1 line)--
AMmysql-test/t/plugin_auth_sha256-mysqltest.opt (1 line)--
AMmysql-test/t/plugin_auth_sha256_2-mysqltest.opt (1 line)--
AMmysql-test/t/plugin_auth_sha256_server_default-mysqltest.opt (1 line)--
AMmysql-test/t/plugin_auth_sha256_server_default_tls-mysqltest.opt (1 line)--
Mplugin/auth/auth_socket.c (2 lines)--
Mplugin/auth/dialog.c (3 lines)--
Mplugin/auth/qa_auth_client.c (3 lines)--
Mplugin/auth/qa_auth_interface.c (3 lines)--
Mplugin/auth/test_plugin.c (3 lines)--
Mscripts/CMakeLists.txt (2 lines)--
Mscripts/mysql_config.pl.in (6 lines)--
Mscripts/mysql_config.sh (4 lines)--
Msql-common/client.c (1739 lines)--
Msql/net_serv.cc (497 lines)--
Mtests/CMakeLists.txt (4 lines)--
Mvio/vio.c (10 lines)--
Mvio/viosocket.c (65 lines)--
Mvio/viossl.c (6 lines)--

Diff 170733

client/CMakeLists.txt

Loading...

client/mysqltest.cc

Loading...

dbug/dbug.c

Loading...

include/mysql.h

Loading...

include/mysql.h.pp

Loading...

include/mysql/client_plugin.h

Loading...

include/mysql/client_plugin.h.pp

Loading...

include/mysql/plugin_auth.h.pp

Loading...

include/mysql/plugin_auth_common.h

Loading...

include/mysql_com.h

Loading...

include/sql_common.h

Loading...

include/violite.h

Loading...

libmysql/CMakeLists.txt

Loading...

libmysql/libmysql.c

Loading...

libmysqld/lib_sql.cc

Loading...

mysql-test/lib/mtr_cases.pm

Loading...

mysql-test/mysql-test-run.pl

Loading...

mysql-test/r/big_packets.result

Loading...

mysql-test/r/big_packets_boundary.result

Loading...

mysql-test/suite/auth_sec/t/access_credential_control-mysqltest.opt

Loading...

mysql-test/suite/auth_sec/t/key_value_auth-mysqltest.opt

Loading...

mysql-test/suite/auth_sec/t/mysql_old_passwords-mysqltest.opt

Loading...

mysql-test/suite/auth_sec/t/mysql_old_plugin-mysqltest.opt

Loading...

mysql-test/suite/auth_sec/t/mysql_sha256_plugin-mysqltest.opt

Loading...

mysql-test/suite/auth_sec/t/password_expired-mysqltest.opt

Loading...

mysql-test/suite/perfschema/t/hostcache_ipv4_auth_plugin.test

Loading...

mysql-test/suite/perfschema/t/hostcache_ipv6_auth_plugin.test

Loading...

mysql-test/suite/rpl/t/rpl_loaddatalocal.test

Loading...

mysql-test/suite/sys_vars/t/old_passwords_func-mysqltest.opt

Loading...

mysql-test/suite/sys_vars/t/secure_auth_func-mysqltest.opt

Loading...

mysql-test/t/big_packets-master.opt

Loading...

mysql-test/t/big_packets.test

Loading...

mysql-test/t/big_packets_boundary-master.opt

Loading...

mysql-test/t/big_packets_boundary.test

Loading...

mysql-test/t/change_user.test

Loading...

mysql-test/t/connect-mysqltest.opt

Loading...

mysql-test/t/connect.test

Loading...

mysql-test/t/connection_timeout-mysqltest.opt

Loading...

mysql-test/t/plugin_auth-mysqltest.opt

Loading...

mysql-test/t/plugin_auth.test

Loading...

mysql-test/t/plugin_auth_qa_1-mysqltest.opt

Loading...

mysql-test/t/plugin_auth_qa_1.test

Loading...

mysql-test/t/plugin_auth_sha256-mysqltest.opt

Loading...

mysql-test/t/plugin_auth_sha256_2-mysqltest.opt

Loading...

mysql-test/t/plugin_auth_sha256_server_default-mysqltest.opt

Loading...

mysql-test/t/plugin_auth_sha256_server_default_tls-mysqltest.opt

Loading...

plugin/auth/auth_socket.c

Loading...

plugin/auth/dialog.c

Loading...

plugin/auth/qa_auth_client.c

Loading...

plugin/auth/qa_auth_interface.c

Loading...

plugin/auth/test_plugin.c

Loading...

scripts/CMakeLists.txt

Loading...

scripts/mysql_config.pl.in

Loading...

scripts/mysql_config.sh

Loading...

sql-common/client.c

Loading...

sql/net_serv.cc

Loading...

tests/CMakeLists.txt

Loading...

vio/vio.c

Loading...

vio/viosocket.c

Loading...

vio/viossl.c

Loading...

Add Comment