-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix connect to MySQL 4.1 #114
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
Conversation
Are you sure this works? I guess it won't if you use a password: http://dev.mysql.com/doc/internals/en/authentication-method.html#secure-password-authentication
when using password, it is the same.
fiorix
commented
Aug 1, 2013
Try running the test cases and see what happens: https://github.com/go-sql-driver/mysql/wiki/Testing
successful using mysql-5.1.70. but failed using mysql-4.1.22.
fiorix
commented
Aug 1, 2013
Then it'll most likely be rejected. :)
but it seems the auth have not test using mysql-4.1. I try to find out why failed.
BTW: why I can't commnet in #113 ? because it is closed?
this is message, if using mysql4.1. unpatch code will be panic.
panic: runtime error: index out of range
goroutine 1 [running]:
github.com/go-sql-driver/mysql.readLengthEncodedInteger(0xc2000ab037, 0x1, 0xfc9, 0x0, 0x420b00, ...)
/Users/bing/source/project/go/src/github.com/go-sql-driver/mysql/utils.go:406 +0x324
github.com/go-sql-driver/mysql.skipLengthEnodedString(0xc2000ab037, 0x1, 0xfc9, 0xfc9, 0x0, ...)
/Users/bing/source/project/go/src/github.com/go-sql-driver/mysql/utils.go:366 +0x39
github.com/go-sql-driver/mysql.(_mysqlConn).readColumns(0xc20008e380, 0x1, 0xc20008a420, 0x1, 0x1, ...)
/Users/bing/source/project/go/src/github.com/go-sql-driver/mysql/packets.go:480 +0x241
github.com/go-sql-driver/mysql.(_mysqlConn).getSystemVar(0xc20008e380, 0x1804f0, 0x12, 0x0, 0x0, ...)
/Users/bing/source/project/go/src/github.com/go-sql-driver/mysql/connection.go:228 +0x174
github.com/go-sql-driver/mysql.(_mysqlDriver).Open(0x243cd0, 0x1989f0, 0x3e, 0x104b20, 0x0, ...)
/Users/bing/source/project/go/src/github.com/go-sql-driver/mysql/driver.go:70 +0x3af
database/sql.(_DB).conn(0xc20008d060, 0xc20007b270, 0x243cd0, 0xc20008d060)
/Users/bing/source/hg/go/src/pkg/database/sql/sql.go:484 +0x1e1
database/sql.(*DB).Ping(0xc20008d060, 0x5, 0x1989f0)
/Users/bing/source/hg/go/src/pkg/database/sql/sql.go:407 +0x25
main.main()
/tmp/t.go:16 +0x15a
goroutine 2 [syscall]:
@s7v7nislands I see 3 of your comments in #133, but it's still better to contain the discussion to one issue (this one). I'm a little out of my depth with this and don't have much time right now, so I'm leaving this to @julienschmidt - who probably doesn't have much time either but doesn't have to read as much MySQL protocol spec docs as I'd have to because he intimately knows this stuff already... 😉
@fiorix with the new commit, it can pass the testing, except these:
--- FAIL: TestDateTime (0.00 seconds)
driver_test.go:83: Error on Exec CREATE TABLE test (value DATE): Error 1231: Variable 'sql_mode' can't be set to the value of 'ALLOW_INVALID_DATES'
--- FAIL: TestStrict (0.00 seconds)
driver_test.go:83: Error on Exec CREATE TABLE test (a TINYINT NOT NULL, b CHAR(4)): Error 1231: Variable 'sql_mode' can't be set to the value of 'ALLOW_INVALID_DATES'
this is because 'ALLOW_INVALID_DATES' is added in mysql5.0.
https://dev.mysql.com/doc/refman/5.0/en/server-sql-mode.html#sqlmode_allow_invalid_dates
mysql-5 1 70-eof-packet
mysql-5 1 70-version
mysql-4 1 22-eof-packet
mysql-4 1 22-vrersion
you can see the mysql-4.1.22 eof-packet (EOF marker in the photo) do't have the 2 fields like mysql-5.1.70
copy from #113
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked it up: http://dev.mysql.com/doc/internals/en/generic-response-packets.html#packet-EOF_Packet - a change between 4.0 and 4.1. This looks ok. But what about readRow and readBinaryRow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already check at the initial handshake that the server supports protocol 41+. We are quite certain not starting to support a protocol obsolete since 9 years. So this change is gratuitous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specification seems to be incorrect for MySQL 4.1 here. Other users report the same issue with other drivers, e.g. http://code.google.com/p/assql/issues/detail?id=81
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still concerned about the missing length check changes in readRow and readBinaryRow (see above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is no bug in this way. in mysql4.1 it only have 2 functions misuse the send_eof.
see: http://bugs.mysql.com/bug.php?id=70211
so only when eof_packet after column defines packet, the eof_packet is wrong formated.
I hate that we can't test this with Travis. It looks ok with some nits I'd like you to address. And I still want Juliens ok. But this is the first part of the review to get this going.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the following code here:
// Reserved [8 bit] pos++
Please ping us (write a comment) when the changes are made.
LGTM then.
I have updated the source.
My english is not very well, do I misunderstand your meaning?
packets.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer only one if, len(data) >= 12 && binary.LittleEndian.Uint16(data[pos:pos+2]) > 0.
That's a style comment only, I'm unable to understand the consequences of this code right now.
I'll probably take a deeper look into this later, but LGTM if @julienschmidt says it's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http://dev.mysql.com/doc/internals/en/com-stmt-prepare-response.html
because there are also have a EOF_Packet. so the length may be different in mysql4.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is saw the latest change, I thought the same as Arne, one if would be better code style.
If you want to change this, you could also add a short comment. Something like this:
// Check for warnings count > 0, only available in MySQL > 4.1http://bugs.mysql.com/bug.php?id=70211
I filed a bug about this.
Thanks for the contribution!
And let's see if the response to the bug report provides new insights.
Added a validation when, loc is not defined OR is UTC to set database session to use the UTC time zone, as proposed in go-sql-driver#114
for #113