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

Make server_status available #755

Closed

Conversation

cdegroot
Copy link
Contributor

This patch sets server_status in MySql2::Client whenever it's relevant. These flags contain important information about how the server viewed the query; it's useful to log this (or even throw errors ;-)) during development so slow queries can be prevented from reaching production.

First patch, first time I'm doing Ruby extensions, so flame away. If the code is good in principle, I'll look into adding some test coverage.

@sodabrew
Copy link
Collaborator

Thanks, I need to do a bit of reading since the MYSQL structure is semi-opaque per http://dev.mysql.com/doc/refman/5.7/en/c-api-data-structures.html -- I think we reach into it in just one other place.

Would you also take a look at the spec/ directory to add a test?

@cdegroot
Copy link
Contributor Author

If you look positively at this patch, I'm absolutely going to be creative and add a test, no worries :-). We have a long weekend up here in Canada, will whip something up on Tuesday.

@sodabrew
Copy link
Collaborator

Thinking some more about this, it might make sense to break these out as separate true/false properties of the Result object rather than the Client object.

@cdegroot
Copy link
Contributor Author

Either works for me - your call. The reason I took this approach is mainly that it stays "in the same object" as the MySQL C client, and it just sets a single value minimizing overhead - I have no idea how many instructions are involved in setting Ruby properties from C, so I wanted to stay on the safe side.

@cdegroot cdegroot force-pushed the make-server-status-available branch from 4385ff5 to 591409c Compare May 24, 2016 16:01
@cdegroot
Copy link
Contributor Author

Update: moved to three individual flags in a @server_flags hash and added a quick test.

@@ -65,9 +65,11 @@ void rb_mysql_client_set_active_thread(VALUE self);

void init_mysql2_client(void);
void decr_mysql2_client(mysql_client_wrapper *wrapper);
void set_server_query_flags(MYSQL *client, VALUE result);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is only called from within client.c, so you can make it static and leave it out of the header file.

Copy link
Collaborator

@sodabrew sodabrew May 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it's also called from statement.c, sorry. Since the function name is exposed in the shared module, please prefix the function name - something like rb_mysql_set_result_server_flags, and move it up a bit, so it's under the rb_mysql_client_set_active_thread definition.

@sodabrew
Copy link
Collaborator

I like the second version a lot! Just a nit about the function name.

I have half an idea to add accessors like these:

Mysql2::Result#no_index_used?
Mysql2::Result#no_good_index_used?
Mysql2::Result#query_was_slow?

The test failures are unrelated, I will look into those as I have time. Your new test passes.

@cdegroot cdegroot force-pushed the make-server-status-available branch from 591409c to 57baabb Compare May 24, 2016 19:41
@cdegroot
Copy link
Contributor Author

(Updated function name and location in header file)

VALUE server_flags = rb_hash_new();

#ifdef SERVER_QUERY_NO_GOOD_INDEX_USED
rb_hash_aset(server_flags, ID2SYM(rb_intern("no_good_index_used")), flag_to_bool(SERVER_QUERY_NO_GOOD_INDEX_USED));
Copy link
Collaborator

@sodabrew sodabrew May 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add these symbols as static VALUEs at the top of the file and in the module init? You should see a laundry list of sym_foo values initialized around line 1350.

@sodabrew sodabrew added this to the 0.5.0 milestone May 24, 2016
cdegroot pushed a commit to cdegroot/mysql2 that referenced this pull request May 25, 2016
sodabrew pushed a commit to sodabrew/mysql2 that referenced this pull request Nov 25, 2017
sodabrew pushed a commit to sodabrew/mysql2 that referenced this pull request Nov 25, 2017
sodabrew pushed a commit to sodabrew/mysql2 that referenced this pull request Nov 25, 2017
@sodabrew
Copy link
Collaborator

Merged as 811a57a

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

2 participants