Prepared statement support #289

Closed
wants to merge 94 commits into
from

Conversation

Projects
None yet
Contributor

nyaxt commented Aug 10, 2012

Using prepared statement is an effective way to prevent injection attacks and improve performance.
I tried to continue the work on the 'stmt' branch.

Now, I think its basically done but for two caveats:

  1. Query results are cached expect for streaming queries.
  2. There are no syntax to pass query_options when PreparedStatement#execute. I don't have an idea for nice syntax.

Sweeeeet. Any idea when this might get merged into master?

Collaborator

tenderlove replied Jul 12, 2010

After it fully functions. ;-)

I don't want to merge this in to master until I have it working better.

Owner

brianmario commented on d20bc8d Jul 31, 2010

Heh, just noticed I meant to say "split out of Mysql2::Client"...

brianmario and others added some commits Aug 2, 2010

Merge branch 'master' into stmt
* master:
  remove Sequel adapter as it's now in Sequel core :)
  move -Wextra to development flags area
  update AR adapter to reflect timezone setting update
  application_timezone is allowed to be nil
  default application_timezone to nil
  sync up with sequel adapter from my Sequel fork until it's officially merged in
  convert :timezone option into two new ones :database_timezone - the timezone (:utc or :local) Mysql2 will assume time/datetime fields are stored in the db. This modifies what initial timezone your Time objects will be in when creating them from libmysql in C and :application_timezone - the timezone (:utc or :local) you'd finally like the Time objects converted to before you get them
  can't call literal here because it'll try to join it's own thread
  Mysql2::Client uses the :username key, set it to :user if that was used instead
  heh
  fix typo in comment
  major refactor of Sequel adapter - it's now green in Sequel
  add :cast_booleans option for automatically casting tinyint(1) fields into true/false for ruby
  move most previously global symbols to static to prevent conflicts (thanks for catching this Eric)
  respect :symbolize_keys option for Mysql2::Result#fields if it's called before the first row is built
  initialize @active early on to prevent warnings later
  let's try that again - libmysql only allows one query be sent at a time per connection, bail early if that's attempted
  Revert "libmysql only allows one query be sent at a time per connection, bail early if that's attempted"
  libmysql only allows one query be sent at a time per connection, bail early if that's attempted
  no need to carry over options twice as we're already doing it up in rb_mysql_client_async_result
Merge branch 'master' into stmt
* master:
  Detach before executing callbacks.
  make sure we don't hit a race condition if this EM spec is taking longer to run than normal
  was in a hurry earlier
  whoops, lost this line in a previous patch
  Dry windows configuration options
  Inject 1.8/1.9 pure-ruby entry point during xcompile
  Use MySQL 5.1.51 now from available mirror
Merge branch 'master' into stmt
* master:
  avoid potential race-condition with closing a connection
  add option for setting the wait_timeout in the AR adapter (this can be done in database.yml)
  add some more defaults to the connect flags
  add connect_flags to default options and add REMEMBER_OPTIONS to that list. fix NUM2INT to be NUM2ULONG as it should be for flags
  free the client after close if we can
  forgot to remove this
  get rid of double-pointer casting
  a couple of minor updates to connection management with some specs
  check for error from mysql_affected_rows call
  change connection check symantecs
Merge branch 'master' into stmt
* master:
  only use wait_timeout if it's a Fixnum
  update gemspec from file updates
  no need to invalidate the FD now that we're only modifying it once, in one place
  no need to check if the FD is >= 0 now that we centralized the close/free logic. Once closed, none of that code will run again
  wording fix in readme

This pull request fails (merged f1f90d7 into 2ca69b1).

This pull request fails (merged 20d72bb into 2ca69b1).

This pull request fails (merged 8bd9725 into 2ca69b1).

@nyaxt nyaxt closed this Aug 10, 2012

Contributor

nyaxt commented Aug 10, 2012

sorry, it seems to need more work on 1.8.7 support. I would like to request again when its done.

@nyaxt nyaxt reopened this Aug 10, 2012

Contributor

nyaxt commented Aug 10, 2012

should work on 1.8.7

This pull request passes (merged bb32c56 into 2ca69b1).

This pull request passes (merged f481190 into 2ca69b1).

Owner

brianmario commented Aug 10, 2012

Would you mind giving us a brief API breakdown of what's introduced here?

At first blush it looks like prepare takes s single SQL string argument then execute is where you pass in the query parameters and a Mysql2::Result is returned.
What do you think about allowing execute to take an optional hash of query options as it's last parameter as well?
Also I saw in the tests that the current implementation will ignore query options passed to Mysql2::Result#each after execute is called because of the rows being cached. I think I ran into that issue as well back when I'd last worked on this. I'd definitely like to figure out a solution where we could lazily build the rows via Mysql2::Result#each. Maybe we need to set a flag telling the result class what kind of result it is (regular vs prepared statement)?

Contributor

nyaxt commented Aug 10, 2012

APIs introduced:

Mysql2::Client#prepare(sql) => Mysql2::Statement

This creates and prepares a server-side prepared statement from a single sql given in sql. sql may contain placeholders ? to be filled on Statement#execute

Mysql2::Statement#execute(*params) # => Mysql2::Result

This binds parameters given in params and execute the prepared statement. It returns a Mysql2::Result which can be used to retrieve results just like Mysql2::Client#query.

Mysql2::Statement#param_count # => Integer

This returns number of placeholders (?) in the prepared statement.

Mysql2::Statement#field_count # => Integer

This returns the number of fields of the result set the prepared statement will create when executed.

Mysql2::Statement#fields # => Array[String]

This returns the name of fields of the result set the prepared statement will create when executed.

Contributor

nyaxt commented Aug 10, 2012

What do you think about allowing execute to take an optional hash of query options as it's last parameter as well?

Like this? stmt.execute("param1", "param2", "param3", :as => :array)
Maybe we can see in Statement#execute if argv[argc-1] is a Hash and take that as query options.

Also I saw in the tests that the current implementation will ignore query options passed to Mysql2::Result#each after execute is called because of the rows being cached. I think I ran into that issue as well back when I'd last worked on this. I'd definitely like to figure out a solution where we could lazily build the rows via Mysql2::Result#each. Maybe we need to set a flag telling the result class what kind of result it is (regular vs prepared statement)?

Yes. The current implementation forces to create row cache on Statement#execute by calling Result#each internally. This disallows specifying options in Mysql2::Result#each.

The underlying problem is a difference in mysql_fetch_row and mysql_stmt_fetch. mysql_fetch_row takes MYSQL_RES* and this allows the result to be acquired lazily if mysql_store_result has been called. However, mysql_stmt_fetch only takes MYSQL_STMT as an argument and provides no way to specify which invocation of #execute we want to create result set from.

I was thinking of binding all results to a temporary buffer when #execute is invoked, and delay conversion to Ruby types until Result#each, but this will really make the code complicated in rb_mysql_result_stmt_fetch_row.

Collaborator

sodabrew commented Nov 18, 2012

If do a version 0.4.0, we should include this branch! What's the status?

Owner

brianmario commented Nov 18, 2012

I need to take another pass at the code, but afaik it was working last time I'd worked on it. Definitely want to make sure there are plenty of tests.

On Nov 18, 2012, at 1:48 PM, Aaron Stone notifications@github.com wrote:

If do a version 0.4.0, we should include this branch! What's the status?


Reply to this email directly or view it on GitHub.

Are there any updates on this being merged? If getting this merged means chipping i'm willing to help just point me in the direction of what needs working on.

Collaborator

sodabrew commented Jan 23, 2013

I think we're basically waiting to do a release with current material in master, which is somewhat blocked on #321, and then looking at this work for a next major version. One thing that would be helpful at your end is rebasing the commits up to current master (rather than merging changes from master into your branch, rebasing makes your commits sit at the top of the change history) to make it easier to review. Thanks!

Collaborator

sodabrew commented Jul 18, 2013

Just did a rebase of the stmt branch. If this is the best continuation of the work, I'll pull it into my rebase. See #405

Contributor

nyaxt commented Jul 24, 2013

Hello. I am happy to continue the work if merging became realistic. I would update the API interfaces if necessary.

As my current employer has a special policy for contributing to OSS, I'm contacting them for approval now.

Any progresss on that ?

@johncant johncant referenced this pull request Jan 7, 2014

Closed

Stmt rebase rebase rebase #477

Contributor

johncant commented Jan 8, 2014

#405 #477 #476 @nyaxt @brianmario @sodabrew @tenderlove

I've started rebasing @nyaxt 's branch onto @sodabrew 's most recent rebase. It is proving to be really hard, but basically I'm going to try and match the test successes/failures at every stage with @nyaxt 's branch, or the failures will get out of hand. They kind of already have, but hey this is rebasing and I can go back and sort it out....

Here is my WIP rebase: https://github.com/johncant/mysql2/commits/nyaxt_rebase1

Please let me know if I've missed anything major.

Contributor

justincase commented Jan 9, 2014

>> stmt = client.prepare("SELECT * FROM messages WHERE uuid = ?")
=> #<Mysql2::Statement:0x007fc082086728>
>> stmt.execute("7bfbc83b-0007-4572-90c7-0d7151699bc8")
ruby(27539,0x7fff7a1c8960) malloc: *** error for object 0x7faf0113cac0: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
Abort trap: 6

Occurs on all branches I've tried so far.

Contributor

johncant commented Jan 9, 2014

@justincase I think I know the cause of the above issue ^. I fixed it, and you can see the test and fix in my closed pull requests, but I didn't use the correct base branch. Looking at @nyaxt's code, it looks like he's fixed it, but I haven't made sure yet. I'm currently attempting to rebase his branch https://github.com/johncant/mysql2/tree/nyaxt_rebase1 - any help would be appreciated.

Next time I get a chance to look at it will most likely be Sunday, and I plan on rebasing my rebase again to replicate @nyaxt's TDD set of passing/failing specs at each commit so I only have to debug incrementally and in obvious places.

TL;DR: Please no-one use my branch as a base branch (although feel free to rebase it).

Sounds like we need a meta git to deal with source code history history!

Pretty stale topic, but could really use this feature.

Contributor

simi commented Jul 16, 2014

I'm not sure about status. Can anyone describe? I would like to finish this on my own. But I don't know what and which branch(es) to use.

Collaborator

sodabrew commented Jul 16, 2014

This is the most recent rebase PR: #405 but it is incomplete.

It's been on my radar to revive this branch and begin a push towards a 0.4.0 series.

Contributor

simi commented Jul 16, 2014

Ahh, I understand now. Thanks for info. Ping if you'll need help.

Contributor

justincase commented Jul 16, 2014

One of @johncant 's branches was working alright for me in testing. All the rebasing and different PRs are a tad disorienting though.

Collaborator

sodabrew commented Jul 16, 2014

Agreed, it's going to take a day of sorting out which commits are where and then getting them into a consistent rebase state to current master. Help appreciated!

Contributor

justincase commented Nov 1, 2014

Hi, @sodabrew. Have any new thoughts on this? @johncant's stmt-johncant branch (johncant@452799d) passes all specs with minimal issues but doesn't include @nyaxt commits yet.

Contributor

justincase commented Nov 2, 2014

I've created a new branch from scratch that contains everything up to 50a7054 + fixes and tests from @johncant.

https://github.com/justincase/mysql2/commits/prepared_mysql2

Collaborator

sodabrew commented Nov 2, 2014

@justincase This is awesome! I need some time to review the branches. I probably won't be able to look at this until next week. But, let me suggest the following: If you put together a PR that definitively supersedes all prior prepared statement PRs, I'll focus on review and merge of your PR.

Contributor

johncant commented Nov 2, 2014

Nice one! I was rebasing @nyaxt 's branch commit by commit, trying to make the same tests pass/fail as they did in @nyaxt 's own branch, but gave up a few commits in and lost interest due to a tricky error and changing to postgresql unrelatedly. @nyaxt got his prepared statement support working so long ago that the code has changed quite a bit.

Contributor

justincase commented Nov 3, 2014

Exactly. This "nogvl" stuff is pretty obscure. I manually "copy/paste'd" the code to fit the current master.

You can use prepared statements today with the PREPARE SQL syntax (Sequel does this iirc) though it would be nice to have proper support.

I'll create a new PR and see if I can get the rest of nyaxt's work in.

On 2 nov. 2014, at 23:43, John Cant notifications@github.com wrote:

Nice one! I was rebasing @nyaxt 's branch commit by commit, trying to make the same tests pass/fail as they did in @nyaxt 's own branch, but gave up a few commits in and lost interest due to a tricky error and changing to postgresql unrelatedly. @nyaxt got his prepared statement support working so long ago that the code has changed quite a bit.


Reply to this email directly or view it on GitHub.

Contributor

rdp commented May 11, 2015

Link to new PR possible please, just for us followers still wondering if mysql2 has preparedstatement support or not?

Contributor

justincase commented May 11, 2015

@rdp PR #591

@pmontrasio pmontrasio referenced this pull request May 21, 2015

Closed

Performance problems #623

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)
Contributor

justincase commented Jun 2, 2015

#591 has been merged into master.
@sodabrew I think we can close this PR?

Collaborator

sodabrew commented Jun 2, 2015

I was just about to ask you this - in a comment on 558, #558 (comment), you mentioned that you'd imported the work from this branch "... up to 50a7054. Most notably, his [nyaxt's] work on returning Mysql2::Result from Mysql2::Statement#execute is missing." - so I wanted to check on the work from 50a7054..f481190.

Based on a diff I just read through, it looks like you did get everything from this branch?

Contributor

justincase commented Jun 2, 2015

Correct, I continued working on nyaxt patches a long the way. Master diverged a lot since this PR so not all commits were applied 'as-is'. All functionality should be there however.

Collaborator

sodabrew commented Jun 2, 2015

This is just tremendous. Thank you again.

@sodabrew sodabrew closed this Jun 2, 2015

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