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

Prepared statements (rebase of #558) #591

Merged
merged 60 commits into from Jun 1, 2015

Conversation

Projects
None yet
8 participants
@justincase
Contributor

justincase commented Mar 6, 2015

This is a rebase of #558 against the current master (5e21bf2).

@sodabrew

This comment has been minimized.

Collaborator

sodabrew commented Mar 6, 2015

wow! and all tests pass! I will review asap.

@justincase

This comment has been minimized.

Contributor

justincase commented Mar 7, 2015

Many thanks, Aaron. There's quite a bit of code that could be refactored:

  • deeply nested if and switch statements
  • args passing from rb_mysql_result_each result.c#L1008
  • rb_mysql_result_each_nonstmt and rb_mysql_result_each_stmt are nearly identical

It works for now though.

@ak47

This comment has been minimized.

ak47 commented Mar 19, 2015

Hi there, just so happens this PR solves 100% of problems for my current task

Any idea if this PR will make it into the gem?

It would lead to much rejoicing.

thank you!

I should add, I've tested this out locally and it is working as expected with respect to parameterized prepared statements.

@sodabrew

This comment has been minimized.

Collaborator

sodabrew commented Mar 29, 2015

Was just going through this for review, yes the rb_mysql_result_each_stmt and rb_mysql_result_each_nonstmt versions jumped out at me, and so did rb_mysql_result_stmt_fetch_row and rb_mysql_result_fetch_row. I will take a stab at factoring this out a bit.

Edit: OMG I didn't realize how completely different the data structures are for prepared vs. conventional result sets. So there will have to be two different conversion functions :( ... at the very least, the same result set / type conversion tests should be executed against each type of query.

@sodabrew

This comment has been minimized.

Collaborator

sodabrew commented Apr 2, 2015

I just pushed a melding of rb_mysql_result_each_stmt and rb_mysql_result_each_nonstmt to a copy of this branch in my tree, would you review and pull the top commits from https://github.com/sodabrew/mysql2/tree/pr/591 into this PR?

case MYSQL_TYPE_SET: // char[]
case MYSQL_TYPE_ENUM: // char[]
case MYSQL_TYPE_GEOMETRY: // char[]
wrapper->result_buffers[i].buffer = malloc(fields[i].max_length);

This comment has been minimized.

@sodabrew

sodabrew Apr 2, 2015

Collaborator

Should be xmalloc or xcalloc for consistency?

This comment has been minimized.

@sodabrew

sodabrew Apr 4, 2015

Collaborator

Switched to xmalloc in a289066 for consistency, and to use xfree for all result_buffers to match.

case T_FIXNUM:
#if SIZEOF_INT < SIZEOF_LONG
bind_buffers[i].buffer_type = MYSQL_TYPE_LONGLONG;
bind_buffers[i].buffer = malloc(sizeof(long long int));

This comment has been minimized.

@sodabrew

sodabrew Apr 2, 2015

Collaborator

These are all malloc, too, but in result.c it's all xmalloc/xcalloc.

This comment has been minimized.

@sodabrew

sodabrew Apr 4, 2015

Collaborator

Actually this was explicitly chosen in 05163af.

This comment has been minimized.

@sodabrew

sodabrew Apr 11, 2015

Collaborator

Per #591 (comment) let's switch this to xmalloc unless @brianmario says he switched it for a strong reason?

@justincase

This comment has been minimized.

Contributor

justincase commented Apr 7, 2015

For future reference; why did we decide on xmalloc/xcalloc and what's the difference between those?

@sodabrew

This comment has been minimized.

Collaborator

sodabrew commented Apr 8, 2015

Thanks for pulling my commits over! I'll do another read-through this week. It's looking good!

Regular malloc/calloc will grab memory from the system. xmalloc/xcalloc will get memory from Ruby's heap, which means it can be tracked from the GC system, and can also trigger GC thresholds (i.e. do a GC run before returning the allocation).

@evanphx

This comment has been minimized.

evanphx commented Apr 8, 2015

@sodabrew That is actually not true. xmalloc simply lets the ruby GC know that some C heap memory was allocated. This goes into it's calculate of when to run the GC, allowing the GC to run when there was only a small number of ruby objects created but a large amount of heap memory allocated, allowing free functions on the ruby objects to run and hopefully free the C heap memory.

@sodabrew

This comment has been minimized.

Collaborator

sodabrew commented Apr 10, 2015

@justincase Here's another one for memory usage: c02cb6c. By the way, you can pull (fetch + merge --ff) the commits from my branch to your branch, rather than cherry-pick, and retain the same commit hashes, since my branch has the same history as your branch.

@evanphx I found my way to http://rxr.whitequark.org/mri/source/gc.c#4685 and then it got hairy! On the upside, it looks like using xmalloc means you get to use Ruby's memory information tools, which may be helpful for debugging memory leaks.

I guess the bottom line is whether it is current best practice for extensions to use Ruby's xmalloc/xfree or not? Many articles on the topic are a few years old and reference the 1.8.x memory system...

@evanphx

This comment has been minimized.

evanphx commented Apr 10, 2015

@sodabrew Using xmalloc is generally recommended so that the ruby GC can run and call your free functions properly.

@justincase

This comment has been minimized.

Contributor

justincase commented Apr 13, 2015

@sodabrew I merged the commits. However, I can't find c02cb6c on any branch. Shall I apply it as a patch?

@sodabrew

This comment has been minimized.

Collaborator

sodabrew commented Apr 13, 2015

I replaced c02cb6c with 2dbbcdc. Looks like you've got them all, thank you!

@sodabrew

This comment has been minimized.

Collaborator

sodabrew commented May 11, 2015

@rdp This is the current PR for prepared statements support. I have reviewed it and am working on a merge branch on my desktop. Sorry about going radio silent!

@justincase One issue I noticed, and was trying some ideas locally, is that the author dates on some of the rebased commits (which should be the original 2012-2013 dates) got reset to be the same as the commit date (which should be your date of work). I was looking at using git filter-branch to restore the original dates.

@justincase

This comment has been minimized.

Contributor

justincase commented May 22, 2015

@sodabrew Apologies for the delay, I had not noticed your edit.

I'll attempt to change the dates back to their original state over the weekend. There are quite a few suggestions on stackoverflow with regards to filter-branch.


Edit @sodabrew, restoring the correct dates works well. When I've gone through all of them, I can force push them to this PR/branch. Note that this will rewrite all commits including the SHA1. Any objections?

@sodabrew

This comment has been minimized.

Collaborator

sodabrew commented May 27, 2015

No objections to new SHAs, this branch has been through its share of rebases already :)

@justincase

This comment has been minimized.

Contributor

justincase commented May 27, 2015

@sodabrew The author dates have been restored. I did a rebase against current master as well.

@sodabrew

This comment has been minimized.

Collaborator

sodabrew commented Jun 1, 2015

I've branched 0.3.x at https://github.com/brianmario/mysql2/tree/0.3.x and now we can take master down the rabbit hole.

sodabrew added a commit that referenced this pull request Jun 1, 2015

Merge pull request #591 from justincase/prepared_mysql2_rebase
Prepared statements (rebase of #558, collected from #289)

@sodabrew sodabrew merged commit 435b550 into brianmario:master Jun 1, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@justincase

This comment has been minimized.

Contributor

justincase commented Jun 1, 2015

Thank you @tenderlove, @nyaxt, @sodabrew, @brianmario, @johncant.

Tenderlove's first commit was almost 5 years ago and now we're close to a released version.

Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment