Refactor some client codepaths, switching to state machines
ClosedPublic

Press ? to show keyboard shortcuts.
Author
steaphan
Reviewers
inaam-rana
darnaut
chip
pivanof
Repository
rWEBSCALESQL WebScaleSQL
Lint
No Linters Available
Unit
Unit Tests Postponed
Show Full Unit Results (1 Postponed, 12 Passed, 12 Details)
Commits
rWEBSCALESQLab0b153ef679: Refactor some client codepaths, switching to state machines
rWEBSCALESQL096854a9edc6: Refactor some client codepaths, switching to state machines
Unknown Object (Diffusion Commit)
rWEBSCALESQL497454ec833f: Refactor some client codepaths, switching to state machines
rWEBSCALESQLbbf95b2a7627: Refactor some client codepaths, switching to state machines
Unknown Object (Diffusion Commit)
Unknown Object (Diffusion Commit)
rWEBSCALESQLedbb663693f5: Refactor some client codepaths, switching to state machines
Unknown Object (Diffusion Commit)
Unknown Object (Diffusion Commit)
rWEBSCALESQLd19ca4507bd5: Refactor some client codepaths, switching to state machines
rWEBSCALESQL20ac0714af6a: Refactor some client codepaths, switching to state machines
rWEBSCALESQL47c733101cfb: Refactor some client codepaths, switching to state machines
rWEBSCALESQLcc3b2c3902c6: Refactor some client codepaths, switching to state machines
rWEBSCALESQL326ad3e58be3: Refactor some client codepaths, switching to state machines
rWEBSCALESQLce0fb86809c0: Refactor some client codepaths, switching to state machines
rWEBSCALESQL914b4a0ffba1: Refactor some client codepaths, switching to state machines
rWEBSCALESQL9fb8abf22075: Refactor some client codepaths, switching to state machines
rWEBSCALESQLd9e831dcb3ff: Refactor some client codepaths, switching to state machines
rWEBSCALESQLa1d67e93caaf: Refactor some client codepaths, switching to state machines
rWEBSCALESQL775026afbf4e: Refactor some client codepaths, switching to state machines
rWEBSCALESQL4bfaf6e70e18: Refactor some client codepaths, switching to state machines
rWEBSCALESQL4edf4e9f2243: Refactor some client codepaths, switching to state machines
Unknown Object (Diffusion Commit)
Apply Patch
arc patch D17025
Arcanist Project
Restricted Arcanist Project
Subscribers
aran112000, methane, steaphan and 8 others
Projects
None
Summary

Feature: Async Client

This is preparatory work for making these codepaths usable in an async
context. The goal is to represent the same state machine (async or
sync) via the context structure and calling function pointers that make
use of the context's data to perform io, handle results, etc.

This splits the connect codepaths into smaller units (generally
separating at the IO boundaries). It changes the mechanism for
executing the connect sequence to mimic what the async version will do.

It also makes a state machine for the authentication flow, similar to
the connect flow -- breaking it apart at the IO points.

This doesn't change the plugin portion yet, but does make the rest of it
state machine'd.

Test Plan

mtr

Older changes are hidden. Show older changes.
steaphan added a comment.Via LegacyMar 25 2014, 1:28 AM

Clean rebase on updated millisecond client timeouts diff. Turning this one over to @chip (and @darnaut) from here on.

steaphan updated this revision.Via LegacyMar 26 2014, 1:14 AM

Clean rebase on cleaned branch history. No actual code changes.

chip commandeered this revision.Via LegacyMar 26 2014, 1:45 AM
chip added a reviewer: steaphan.

Thanks for getting these to this point, @steaphan -- taking these diffs over now as discussed. You're free, quick, run while you can!

steaphan resigned from this revision.Via LegacyMar 26 2014, 1:48 AM

Chip has taken this over now.

pivanof added a comment.Via LegacyMar 28 2014, 6:15 AM

See several comments inline. On a general note: how changes in the mysqladmin command line are related to state machine in the client code? Can we split that into separate change?

mysql-test/r/disconnect_on_expired_password_default.result
30 ↗(On Diff #53361)

I don't see what's changed here?

sql-common/client.c
2953

It would be nice for the order of state descriptions in the comment to match order of enums, or maybe just split the comment and move it into enum definition.

2966

Space after asm_context?

3045

I believe convention in MySQL and in this file to have space before '*', not after. Will we comply with this style here and in functions below?

3136

Can we be consistent on the space after DBUG_RETURN?

3146

It seems that this will be better called asm_read_change_user_result, because it doesn't actually call change_user.

3363

Spaces around '+'.

3436

Why not this?

if (!pump_connect_state_machine(&ctx))
  DBUG_RETURN(mysql);
3451

I think this is now so far from the original placement that it doesn't make sense to keep the extra block already. Or does it have meaning that I'm missing?

3656

I guess there is tab here at the beginning of the line. Should we replace it with spaces?

4109

Probably these two lines will look more appropriate before the comment.

4148

It seems to me that these will look cleaner if we go to csm_prep_init_commands here (and above) unconditionally, bu put ifdef into csm_prep_init_commands function. What do you think?

4165

Is it appropriate to have identical comments inside two different functions? Can we specialize them?

steaphan requested changes to this revision.Via LegacyMar 28 2014, 9:13 PM

MySQLl-5.6.17 was released, so this needs to be rebased onto the webscalesql-5.6.17 branch now.

chip updated this revision.Via LegacyMar 30 2014, 7:26 AM

rebase to 5.6.17

steaphan resigned from this revision.Via LegacyMar 31 2014, 6:12 PM

Now it's on the right branch, and all tests passed, so I leave the reviews to the others.

The perf results are interesting - I am re-running just those tests to try to confirm these. They could have been just because the perf testing system was cold when I triggered these runs.

darnaut requested changes to this revision.Via LegacyApr 9 2014, 8:56 PM

Some initial comments inline, also:

  • Move mysqladmin related changes to a separate change.
  • Please try to follow MySQL’s coding style.

I haven't finished looking at the patch because I'm not following what is the goal with some of the separate states. A lot of the new "states" seem to lack a good reason. Could you expand on the reasoning?

sql-common/client.c
614

Make static.

2955

What is the significance of 1800?

3005

I would suggest rewriting this function in a more concise manner. For example:

state_machine_status status;

do
  status = ctx->state_function(ctx);
while (status == STATE_MACHINE_CONTINUE)

return status != STATE_MACHINE_DONE;

Also, the function seems to only be used from run_plugin_auth. Perhaps the loop should be inlined there.

3100

Why does this function need to exist at all? Do we really need a separate state just to call a function? This is confusing, there is no reason this should be a state.

steaphan commandeered this revision.Via WebJun 3 2014, 5:04 AM
steaphan added a reviewer: chip.

I'll take this one from here. Thanks @chip!

steaphan updated this revision to Diff 58233.Via ConduitJun 7 2014, 12:00 AM

Clean rebase to 5.6.19, in preparation for requested changes. No changes made to this diff yet.

steaphan planned changes to this revision.Via WebJun 7 2014, 12:01 AM

Just rebased to 5.6.19 in this revision, changes will be in the next diff.

steaphan removed a subscriber: WebScaleSQL.Via WebJul 31 2014, 8:15 PM
steaphan updated this revision to Diff 69669.Via ConduitJul 31 2014, 8:15 PM

Clean rebase to 5.6.20 - no changes to this diff (but some are planned).

steaphan planned changes to this revision.Via ConduitJul 31 2014, 8:15 PM
steaphan added a subscriber: WebScaleSQL.Via WebJul 31 2014, 8:16 PM
steaphan removed a subscriber: WebScaleSQL.Via WebOct 30 2014, 6:18 PM
steaphan updated this revision to Diff 149127.Via ConduitOct 30 2014, 6:19 PM

Clean rebase to 5.6.21. No changes to this diff yet.

steaphan planned changes to this revision.Via ConduitOct 30 2014, 6:19 PM
steaphan updated this revision to Diff 158313.Via ConduitDec 16 2014, 3:29 AM

Clean rebase to 5.6.22. No changes to this diff yet.

steaphan planned changes to this revision.Via WebDec 16 2014, 3:36 AM
steaphan added a comment.Via WebDec 29 2014, 10:42 PM

Preparing the update to this diff now. Here are my replies to just @pivanof's review comments.

Also, I am not sure how, nor even *if*, the MYSQLADMIN changes are related. I will confer with @chip on that.

mysql-test/r/disconnect_on_expired_password_default.result
30 ↗(On Diff #53361)

An invisible control character (BELL, I think) was removed from the beginning of the line. I am not sure where it came from before.

sql-common/client.c
2953

Agreed. Fixed.

2966

This doesn't seem consistent, even in this file. Most pointer types in parens do not seem to have a space.

3045

This is also not consistent, even in this file. But, most do agree with you, so I have changed all those touched by this diff.

3136

Yep, the convention seems to be no space, so I've fixed all of these.

3146

Agreed. Changed.

3363

Yep.

3436

Yeah, I like your version much better (Though I added braces, for extra clarity). Changed.

3451

Agreed. Changed.

3656

Yep, was a tab. Fixed.

4109

Agreed. Fixed.

4148

I'd agree with you, except that would change the logic of when the assignment to ctx->state_function happens... and I don't understand this well enough to know if it's safe to set that unprotected by the ifdef. So, I am going to leave this as-is.

Perhaps @chip will want to chime in on this?

4165

I assumed this was two different paths to do the same thing. @chip?

For now, I will leave it as-is.

steaphan added a comment.Via WebDec 29 2014, 11:00 PM

My replies to @darnaut's individual code questions are included here.

I will let @chip answer the general questions you have. I'll also get the context from him on why the MYSQLADMIN stuff is part of this diff - if there isn't some reason I don't know that requires it to be, I agree with you that it should be separated out.

sql-common/client.c
614

Agreed.

2955

I thought it was arbitrary, but I may be incorrect. @chip?

3005

Agreed, on both. I have both simplified it, and factored it into the one caller.

3100

I am not 100% sure what you are asking. This seems to make sense to me. Perhaps @chip can explain it better?

For now, I am leaving this as-is.

chip added a comment.Via WebDec 30 2014, 12:03 AM

I'd agree with you, except that would change the logic of when the assignment to ctx->state_function happens... and I don't understand this well enough to know if it's safe to set that unprotected by the ifdef. So, I am going to leave this as-is.

Perhaps @chip will want to chime in on this?

When mysql is connecting to another mysql (replication), it doesn't support init commands, so the code flow is different. Perhaps it can be rearranged but I think this is just the state machine version of what was happening before.

I assumed this was two different paths to do the same thing. @chip?

For now, I will leave it as-is.

The comments are incorrect, yes; basically the "hey we authenticated" code path is more spread out now, but the comment is in two places. Post-auth, we first (might) do select_db, then run init_commands. Since select_db is optional, it sometimes is skipped. Server mode doesn't do init_commands (but might do select_db? unclear to me; I don't know if it specified a db for replicatoin or not)

What is the significance of 1800?

Totally arbitrary. Enumbs that start at 0 are impossible to disambiguate from just inspecting data (say, if state is confused and you're looking at something with gdb). I try to pick arbitrary, distinct values for all enums.

Why does this function need to exist at all? Do we really need a separate state just to call a function? This is confusing, there is no reason this should be a state.

It doesn't strictly have to be a state; it could just be part of the one that calls it. It probably evolved this way over time as the diff was worked on and once had some kind of blocking code in it (or perhaps just separated to keep the chunks of code smaller).

steaphan updated this revision to Diff 160911.Via ConduitDec 30 2014, 1:34 AM
steaphan updated the summary for this revision. (Show Details)

Updated based on review comments. Split mysqladmin changes off into: https://reviews.facebook.net/D30753

pivanof added a comment.Via WebDec 31 2014, 12:43 AM
In D17025#43, @chip wrote:

I'd agree with you, except that would change the logic of when the assignment to ctx->state_function happens... and I don't understand this well enough to know if it's safe to set that unprotected by the ifdef. So, I am going to leave this as-is.

Perhaps @chip will want to chime in on this?

When mysql is connecting to another mysql (replication), it doesn't support init commands, so the code flow is different. Perhaps it can be rearranged but I think this is just the state machine version of what was happening before.

I understand that the code flow is different, I didn't propose to change it. My proposal is as follows. Currently code looks like this:
In csm_prep_select_database:

 if (ctx->db && !mysql->db) {
   ctx->state_function = csm_send_select_database;
 } else {
#ifdef MYSQL_SERVER
   DBUG_RETURN(STATE_MACHINE_DONE);
#else
   ctx->state_function = csm_prep_init_commands;
#endif
 }

In csm_send_select_database:

#ifndef MYSQL_SERVER
  ctx->state_function = csm_prep_init_commands;
  DBUG_RETURN(STATE_MACHINE_CONTINUE);
#else
  DBUG_RETURN(STATE_MACHINE_DONE);
#endif

And csm_prep_init_commands:

#ifndef MYSQL_SERVER
csm_prep_init_commands() {
...
}
#endif

What I think will look cleaner.
In csm_prep_select_database:

if (ctx->db && !mysql->db) {
  ctx->state_function = csm_send_select_database;
} else {
  ctx->state_function = csm_prep_init_commands;
}

In csm_send_select_database:

ctx->state_function = csm_prep_init_commands;
DBUG_RETURN(STATE_MACHINE_CONTINUE);

And csm_prep_init_commands:

csm_prep_init_commands() {
#ifndef MYSQL_SERVER
  DBUG_RETURN(STATE_MACHINE_DONE);
#else
...
#endif
}

If necessary we can even join DBUG_RETURN(DONE) with the one inside "if (!mysql->options.init_commands)" (though it may look uglier) and for more cleanliness to rename csm_prep_init_commands into csm_maybe_prep_init_commands.
Yes this will change assignments to ctx->state_function, but besides execution of couple more operators this won't affect any code flow.
WDYT?

I assumed this was two different paths to do the same thing. @chip?

For now, I will leave it as-is.

The comments are incorrect, yes; basically the "hey we authenticated" code path is more spread out now, but the comment is in two places. Post-auth, we first (might) do select_db, then run init_commands. Since select_db is optional, it sometimes is skipped. Server mode doesn't do init_commands (but might do select_db? unclear to me; I don't know if it specified a db for replicatoin or not)

So can we remove "Part 3" comment from csm_prep_select_database (it looks the least appropriate there)?

What is the significance of 1800?

Totally arbitrary. Enumbs that start at 0 are impossible to disambiguate from just inspecting data (say, if state is confused and you're looking at something with gdb). I try to pick arbitrary, distinct values for all enums.

This sounds like a very unconventional coding. I don't think I agree with it, though I won't fight against it either. But at the very least please add a comment to the enum definition explaining this.

sql-common/client.c
2981

Can you also add a small comment explaining that asm == authentication state machine, but not assembler? Or maybe even replace all "asm" with "auth"?

3005

I actually liked it as a separate function. :) Code looked more readable too me. But if you inlined it then probably pump_connect_state_machine should be inlined too.

4196

Just realized that probably a name like csm_send_one_init_command would be more appropriate for this.

steaphan added a comment.Via WebDec 31 2014, 1:57 AM

Update coming shortly. Changes have been made as described in the inline comments.

As for your suggested cleanup of csm_prep_select_database(), csm_send_select_database(), and csm_prep_init_commands() - I *think* you are right... but I am scared of making any functional changes to this code... even if they seem like they will not affect anything. So, I am *not* going to make those changes (even though I think you are right). The mileage of testing and production that's run through this code already at Facebook gives me great confidence in it, and I don't want to risk losing that confidence by changing any functionality, unless something is actually broken.

I am sure if/when this gets included upstream, they will want to refactor it anyway, so I don't think it is worth my time to catch up on how all of this works, just for this sort of refactor. Though, after this is pushed, you are more than welcome to propose these sorts of fixes as separate diffs, for @chip to review - since he actually knows this code. Then, once your update is pushed, I will be happy to squash those into this diff in the future releases. :)

sql-common/client.c
2955

I added a comment explaining the logic (or lack thereof) in choosing 1800.

2981

That confused me too. I will change this to "authsm", as "asm" is a pretty standard abbreviation for assembly (even within the mysql codebase itself).

3005

Yeah, it's really too simple to deserve to be its own function, if only called once. I now did the same to pump_connect_state_machine().

4165

I removed this comment from csm_prep_select_database(), as @pivanof suggested.

4196

Agreed. I have changed it.

steaphan updated this revision to Diff 161277.Via ConduitDec 31 2014, 1:58 AM

Updated based on review comments.

pivanof accepted this revision.Via WebDec 31 2014, 2:50 AM

Thanks.

This revision is now accepted and ready to land.Via WebDec 31 2014, 2:50 AM
steaphan closed this revision.Via DaemonsDec 31 2014, 2:54 AM
steaphan updated this revision to Diff 161313.

Closed by commit rWEBSCALESQL9fb8abf22075 (authored by @steaphan).

Revision Update History

DiffIDBaseDescriptionCreatedLintUnit
BaseBase
Diff 152743ec76488Mar 19 2014, 4:22 PM
Diff 25318771efdc7Clean 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 353361c1d98ebClean rebase on cleaned branch history. No actual code changes.Mar 26 2014, 1:14 AM
Diff 4538831ba2531rebase to 5.6.17Mar 30 2014, 7:26 AM
Diff 5582330238daeClean rebase to 5.6.19, in preparation for requested changes. No changes made to this diff yet.Jun 7 2014, 12:00 AM
Diff 6696693226fbfClean rebase to 5.6.20 - no changes to this diff (but some are planned).Jul 31 2014, 8:15 PM
Diff 7149127cdbce69Clean rebase to 5.6.21. No changes to this diff yet.Oct 30 2014, 6:19 PM
Diff 8158313dde5089Clean rebase to 5.6.22. No changes to this diff yet.Dec 16 2014, 3:29 AM
Diff 9160911520065eUpdated based on review comments. Split mysqladmin changes off into: https://reviews.facebook.net/D30753Dec 30 2014, 1:34 AM
Diff 1016127786a1f12Updated based on review comments.Dec 31 2014, 1:58 AM
Diff 1116131386a1f12Commit rWEBSCALESQL9fb8abf220750e31ff6a4e29345230f2012631e9Dec 31 2014, 2:53 AM

Local Commits

CommitTreeParentsAuthorSummaryDate
acd6c393aa749b4ccfcf62f186a1f1257bdbSteaphan Greene
Refactor some client codepaths, switching to state machines (Show More…)
Jun 6 2014, 11:57 PM

Table of Contents

PathCoverage (All)Coverage (Touched)
Msql-common/client.c (825 lines)--

Diff 161313

sql-common/client.c

Loading...

Add Comment