go-sql-driver / mysql

Go MySQL Driver is a MySQL driver for Go's (golang) database/sql package
https://pkg.go.dev/github.com/go-sql-driver/mysql
Mozilla Public License 2.0
14.54k stars 2.32k forks source link

Exec: should ErrPktTooLarge be returned for interpolated params instead of db.ErrSkip? #358

Open twmb opened 9 years ago

twmb commented 9 years ago

When using a very large interpolated statement, interpolateParams returns db.ErrSkip when the interpolated statement exceeds maxPacketAllowed. Inside database/sql, if ErrSkip is returned, Exec falls back to the prepare/execute cycle and returns any resultant error. Prepared statements have a limit of 65535 statements. Any error other than ErrSkip is returned immediately and no further prepare/execute is attempted.

I use interpolateParams to get statements larger than 65535 statements and to avoid the prepare/execute cycle. Returning ErrPktTooLarge allows users to either trim their packet size (drop the number of params to be interpolated) or to increase the max_allowed_packet inside mysql.

The current behavior is reasonable and acceptable because of the prepare/execute fallback. ErrPktTooLarge is also, in my opinion, reasonable and allows programs to self adjust or fix what they are doing.

Can it be an option to return ErrPktTooLarge?

twmb commented 9 years ago

Alternatively, expose a function that we can pass our statement and arguments to to get what interpolateParams returns. If Exec fails, we can manually check.

methane commented 9 years ago

I think it isn't suitable for connection level option specified by dsl. Since It's rare case that programmer modify query manually.

There are two ideas.

  1. Add API like mysql.InterpolateParams(db *sql.DB, query string, args... interface{}) (string, error). It fetches options and variables from db.
  2. Add struct like mysql.ExecuteOption{FallbackOnInterpolatedQueryIsTooLarge bool, ...} and it can be passed as the first argument of db.Exec() and db.Query(). e.g. db.Query(query, &option, largeArgs...).
julienschmidt commented 9 years ago

I don't really see the benefit in the struct and it will probably be impossible to implement it in a sensible way thanks to the restrictiveness of database/sql: https://github.com/golang/go/blob/master/src/database/sql/convert.go#L73 (and L53). That means only bool, []byte, float64, int64, string and time.Time can be passed as arguments.

I think we should go with option 1.