Skip to content
This repository has been archived by the owner on Jun 28, 2018. It is now read-only.

driver/mysql: use go-sql-driver's multistatements #91

Closed
wants to merge 1 commit into from
Closed

driver/mysql: use go-sql-driver's multistatements #91

wants to merge 1 commit into from

Conversation

fsouza
Copy link
Contributor

@fsouza fsouza commented Apr 26, 2016

I know that you've been looking into using mymysql instead of
go-sql-driver, but I don't think that this seems as a too big change. I
have some questions though:

  1. is it fine to look for the string "multiStatements=true" in the
    driver url or should I parse the URL and ensure that it's defined to
    be true in the query string?
  2. should we keep the fallback? I kept it because otherwise this change
    may break other people's flow/code
  3. I changed migrate/migrate_test to test this change (included mysql
    to driverUrls, setting multiStatements=true). Should I commit this
    change as well?
  4. Should I change or duplicate the test in mysql/mysql_test.go (I'd
    suggest testing both cases)

Any feedback is very appreciated.

The change is related to #1.

I know that you've been looking into using mymysql instead of
go-sql-driver, but I don't think that this seems as a too big change. I
have some questions though:

 1. is it fine to look for the string "multiStatements=true" in the
    driver url or should I parse the URL and ensure that it's defined to
    be true in the query string?
 2. should we keep the fallback? I kept it because otherwise this change
    may break other people's flow/code
 3. I changed migrate/migrate_test to test this change (included mysql
    to driverUrls, setting multiStatements=true). Should I commit this
    change as well?
 4. Should I change or duplicate the test in mysql/mysql_test.go (I'd
    suggest testing both cases)

Any feedback is very appreciated.

The change is related to #1.
@mattes mattes added the db/mysql label Feb 8, 2017
@mattes mattes added this to the v3 milestone Feb 8, 2017
@mattes
Copy link
Owner

mattes commented Feb 28, 2017

fixed in v3

purl.Query().Set("multiStatements", "true")

@mattes mattes closed this Feb 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants