Add maxAllowedPacket DSN Parameter #489

xiezhenye · 2016-10-20T07:34:27Z

Description

Add a maxPacketAllowed DSN param and a config field to set it directly instead of fetch from server.

Some mysql proxy implements or other services using mysql protocol may not supports 'select @@max_allowed_packet'. maxPacketAllowed param can be used to walk around these problems.

It can also be used to limit the max row size returned from server.

Checklist

Code compiles correctly
Created tests which fail without the change (if possible)
All tests passing
Extended the README / documentation, if necessary
Added myself / the copyright holder to the AUTHORS file

methane · 2016-10-20T12:35:59Z

Some proxy implements not supports 'select @@max_allowed_packet' query.

I feel this sentence is worth enough to include in README.
(Many options doesn't have enough description about what is motivation to use the option.)

julienschmidt · 2016-10-20T18:06:33Z

What we currently do is indeed a hack we should maybe avoid in the future. It might be a good idea to just use MySQL's default value by default, allow modification via the maxPacketAllowed DSN parameter and use the current behavior only when explicitly set, e.g. via maxPacketAllowed=auto.

Anyway, @xiezhenye please use the PR template (edit you post) and make the necessary additions.

arnehormann · 2016-10-22T22:15:26Z

I prefer maxAllowedPacket to maxPacketAllowed so naming is consistent with the variable in MySQL. But if the name was intentional: for what the reason?

julienschmidt · 2016-10-25T16:08:06Z

AUTHORS

 Stan Putrya <root.vagner at gmail.com>
 Stanley Gunawan <gunawan.stanley at gmail.com>
 Xiaobing Jiang <s7v7nislands at gmail.com>
+Zhenye Xie <xiezhenye at gmail.com>


Please move your entry to the end (alphabetical order)

julienschmidt · 2016-10-25T16:08:14Z

README.md

+Default:       0
+```
+
+Max MySQL packet size allowed in bytes. Use `maxPacketAllowed` == 0 to fetch the `max_allowed_packet` variable from server.


Remove "MySQL" and replace "maxPacketAllowed == 0" with "Use maxPacketAllowed = 0" (single =).

Maybe also add "automatically" before "fetch"

julienschmidt · 2016-10-25T16:57:10Z

@arnehormann's comment is still valid. The parameter name should be consistent with the variable name (max_allowed_packet). Therefore it should be maxAllowedPacket.
Please also change that, other than that, the PR looks fine to me.

arnehormann · 2016-10-26T05:54:24Z

README.md

 I/O write timeout. The value must be a decimal number with an unit suffix ( *"ms"*, *"s"*, *"m"*, *"h"* ), such as *"30s"*, *"0.5m"* or *"1m30s"*.


+##### `maxPacketAllowed`


maxAllowedPacket also here and in DSN

julienschmidt · 2016-10-26T07:51:07Z

README.md

+Default:       0
+```
+
+Max packet size allowed in bytes. Use `maxAllowedPacket` = 0 to automatically fetch the `max_allowed_packet` variable from server.


Please write it as maxAllowedPacket=0 to make it consistent with other variables.

julienschmidt · 2016-10-26T07:51:11Z

driver.go

-		mc.Close()
-		return nil, err
+	if mc.cfg.MaxAllowedPacket > 0 {
+		mc.maxPacketAllowed = mc.cfg.MaxAllowedPacket


also change the name here

julienschmidt · 2016-10-26T07:51:14Z

driver.go

+			mc.Close()
+			return nil, err
+		}
+		mc.maxPacketAllowed = stringToInt(maxap) - 1


julienschmidt · 2016-10-26T07:51:16Z

driver.go

+		mc.maxPacketAllowed = stringToInt(maxap) - 1
 	}
-	mc.maxPacketAllowed = stringToInt(maxap) - 1
 	if mc.maxPacketAllowed < maxPacketSize {


julienschmidt · 2016-10-26T07:51:24Z

dsn.go


+	if cfg.MaxAllowedPacket > 0 {
+		if hasParam {
+			buf.WriteString("&maxPacketAllowed=")


julienschmidt · 2016-10-26T07:51:26Z

dsn.go

+			buf.WriteString("&maxPacketAllowed=")
+		} else {
+			hasParam = true
+			buf.WriteString("?maxPacketAllowed=")


julienschmidt · 2016-10-26T07:51:29Z

dsn.go

 				return
 			}
-
+		case "maxPacketAllowed":


julienschmidt · 2016-10-26T07:51:32Z

dsn_test.go

 }, {
-	"user:password@/dbname?loc=UTC&timeout=30s&readTimeout=1s&writeTimeout=1s&allowAllFiles=1&clientFoundRows=true&allowOldPasswords=TRUE&collation=utf8mb4_unicode_ci",
-	&Config{User: "user", Passwd: "password", Net: "tcp", Addr: "127.0.0.1:3306", DBName: "dbname", Collation: "utf8mb4_unicode_ci", Loc: time.UTC, Timeout: 30 * time.Second, ReadTimeout: time.Second, WriteTimeout: time.Second, AllowAllFiles: true, AllowOldPasswords: true, ClientFoundRows: true},
+	"user:password@/dbname?loc=UTC&timeout=30s&readTimeout=1s&writeTimeout=1s&allowAllFiles=1&clientFoundRows=true&allowOldPasswords=TRUE&collation=utf8mb4_unicode_ci&maxPacketAllowed=16777216",


julienschmidt · 2016-10-26T07:54:23Z

dsn.go

 	MultiStatements         bool // Allow multiple statements in one query
 	ParseTime               bool // Parse time values to time.Time
 	Strict                  bool // Return warnings as errors
+	MaxAllowedPacket	int  // Max packet size allowed


Please move it after Loc *time.Location // Location for time.Time values.
We just put all the bool vars at the end for more efficient struct packing.

julienschmidt · 2016-10-26T08:21:45Z

README.md

+Default:       0
+```
+
+Max packet size allowed in bytes. Use `maxAllowedPacket`=0 to automatically fetch the `max_allowed_packet` variable from server.


`maxAllowedPacket=0`

julienschmidt · 2016-10-26T08:54:39Z

Thanks a lot!

xiezhenye and others added 3 commits October 20, 2016 15:12

add maxPacketAllowed dsn param

50b3c01

add docs

e78ecc8

Merge branch 'master' into master

1793086

julienschmidt added this to the v1.3 milestone Oct 20, 2016

julienschmidt added enhancement documentation labels Oct 20, 2016

xiezhenye added 2 commits October 21, 2016 02:31

add author, add test

873fcb6

Merge branch 'master' into master

b3f09b7

julienschmidt requested changes Oct 25, 2016

View reviewed changes

fix AUTHOR file and README

9e928e1

julienschmidt changed the title ~~add a maxPacketAllowed dsn param~~ Add maxAllowedPacket DSN Parameter Oct 25, 2016

julienschmidt removed the documentation label Oct 25, 2016

julienschmidt mentioned this pull request Oct 25, 2016

Do not automatically query max_allowed_packet #499

Closed

change the param name from MaxPacketAllowed to MaxAllowedPacket

84d597b

arnehormann reviewed Oct 26, 2016

View reviewed changes

arnehormann mentioned this pull request Oct 26, 2016

README: Clarify strict mode #500

Merged

5 tasks

fix maxAllowedPacket param name in README

9ab50a7

julienschmidt requested changes Oct 26, 2016

View reviewed changes

julienschmidt reviewed Oct 26, 2016

View reviewed changes

xiezhenye added 2 commits October 26, 2016 16:01

rename all maxPacketAllowed to maxAllowedPacket

4c1b46f

fix doc format

b77594f

julienschmidt reviewed Oct 26, 2016

View reviewed changes

fix doc

c1f7d07

julienschmidt approved these changes Oct 26, 2016

View reviewed changes

Merge branch 'master' into master

a049a66

julienschmidt merged commit b81e73c into go-sql-driver:master Oct 26, 2016

julienschmidt mentioned this pull request Oct 26, 2016

README: Sort variables alphabetically #503

Merged

		I/O write timeout. The value must be a decimal number with an unit suffix ( "ms", "s", "m", "h" ), such as "30s", "0.5m" or "1m30s".


		##### `maxPacketAllowed`

Add maxAllowedPacket DSN Parameter #489

Add maxAllowedPacket DSN Parameter #489

Uh oh!

Conversation

xiezhenye commented Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

methane commented Oct 20, 2016

Uh oh!

julienschmidt commented Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arnehormann commented Oct 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienschmidt commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienschmidt commented Oct 26, 2016

Uh oh!

Uh oh!

xiezhenye commented Oct 20, 2016 •

edited

Loading

julienschmidt commented Oct 20, 2016 •

edited

Loading