Issue 233 - Add :local_infile option and refactor mysql_options code #252

Merged
merged 6 commits into from Aug 1, 2012

Conversation

Projects
None yet
7 participants
Collaborator

sodabrew commented Apr 4, 2012

Tests pass, code cleaned up, cherry-picked to a new branch and tested, current with master.

Collaborator

sodabrew commented May 2, 2012

Ping. This is ready to merge please.

Collaborator

sodabrew commented May 16, 2012

Ping?

webandy commented May 19, 2012

I'm eager for this to go in and can test/verify the fix. I see the issue with homebrew installed mysql Ver 14.14 Distrib 5.5.20, for osx10.7 (i386) using readline 5.1. If I roll back to mysql 5.1 the issue (malformed packet error using LOAD DATA INFILE) is not present.

+1 to getting this committed as on a mysql2 fork until it is. @brianmario or @tenderlove , any chance of this happening?

cvincent commented Jun 5, 2012

+1, would like to see this merged in.

In the meantime, not sure how to specify sodabrew's branch in my Gemfile. I have this:

gem "mysql2", git: "https://github.com/sodabrew/mysql2.git", branch: "issue-233"

But updating the bundle doesn't seem to be taking enough time to have actually recompiled the native extensions (even after blowing away the old directory). It does say "Using mysql2 (0.3.11) from https://github.com/sodabrew/mysql2.git (at issue-233) with native extensions" but it never says "Installing". And I still get the Malformed packet error. Any clues?

+1, would like to see this merged. Worked like a charm for me.

menno commented Aug 1, 2012

I ran into this issue as well, and fixed it in my own fork before I saw the issues/pullrequests from @sodabrew. My fixes are at https://github.com/menno/mysql2/commit/cf72bc1fba86e48ec97b428efea14a7e854b97e7 (based on 0.2.x) and https://github.com/menno/mysql2/commit/30302b7c7b6083f771c7c955e65dce2793264147 (based on master). I did not refactor any code like sodabrew did.

After applying the patches I added "local_infile: true" to my database.yml and it all works as expected.

Hope to see this resolved officially soon!

Owner

brianmario commented Aug 1, 2012

@sodabrew would you mind getting this pull mergable again?
@menno I would have definitely preferred this be split into two pull requests. One that adds the option and another to refactor.

Merge branch 'master' into issue-233a
Conflicts:
	ext/mysql2/client.c
	ext/mysql2/client.h
Collaborator

sodabrew commented Aug 1, 2012

@brianmario Done, updated to current master in my issue-233 branch. Confirmed that specs pass. I could split this into two pull requests if you need; it is split as refactor first, then add :local_infile, as two separate commits in this pull request.

Oh this is such sweet awesome splendor. Thanks @sodabrew and @brianmario !

ext/mysql2/client.c
+ wrapper->reconnect_enabled = boolval;
+ }
+
+ return INT2NUM(result == 0);
@brianmario

brianmario Aug 1, 2012

Owner

Could we just return true/false here? Feels more ruby-ish to do that instead of return an int that represents the same. What do you think?

@sodabrew

sodabrew Aug 1, 2012

Collaborator

Sounds good to me!

brianmario added a commit that referenced this pull request Aug 1, 2012

Merge pull request #252 from sodabrew/issue-233
Issue 233 - Add :local_infile option and refactor mysql_options code

@brianmario brianmario merged commit 89ee259 into brianmario:master Aug 1, 2012

@sodabrew sodabrew referenced this pull request Aug 3, 2012

Closed

LOAD DATA LOCAL command #43

Owner

brianmario commented Aug 3, 2012

closes #43

Owner

brianmario commented Oct 16, 2012

Thinking about removing the Mysql2::Client#options ruby method. I've been trying to hide as much of the underlying libmysql API as I can in an attempt to wrap it in something more Ruby-like. We're not exposing any of the MYSQL_OPT_* options anyway so the caller would need to know the integer values of them from the libmysql headers in order to use this in Ruby-land anyway.

Thoughts?

Collaborator

sodabrew commented Oct 16, 2012

I have no objection to keeping this private and/or removing the public method -- you're right, there's not much you can do with the method yet. I was thinking we would expose more options as the need arose, but it's probably better to try and get all the options exposed in one shot when you're ready for it.

@sodabrew sodabrew referenced this pull request Oct 19, 2012

Merged

Remove options method. #318

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