From 9247ef886df5bddb7e081b3af96d2de1d491b5a2 Mon Sep 17 00:00:00 2001 From: Arne Hormann Date: Mon, 27 Jan 2014 16:03:22 +0100 Subject: [PATCH 1/5] ad failing test --- driver_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/driver_test.go b/driver_test.go index 57bac668b..1dca680b9 100644 --- a/driver_test.go +++ b/driver_test.go @@ -1210,6 +1210,29 @@ func TestStmtMultiRows(t *testing.T) { }) } +func TestPreparedManyCols(t *testing.T) { + const repetitions = 1024 + runTests(t, dsn, func(dbt *DBTest) { + query := "SELECT ?" + strings.Repeat(",?", repetitions-1) + values := make([]sql.NullString, repetitions) + params := make([]interface{}, repetitions) + for i := range values { + params[i] = &values[i] + } + stmt, err := dbt.db.Prepare(query) + if err != nil { + dbt.Fatal(err) + } + defer stmt.Close() + rows, err := stmt.Query(params...) + if err != nil { + stmt.Close() + dbt.Fatal(err) + } + defer rows.Close() + }) +} + func TestConcurrent(t *testing.T) { if enabled, _ := readBool(os.Getenv("MYSQL_TEST_CONCURRENT")); !enabled { t.Skip("MYSQL_TEST_CONCURRENT env var not set") From a3fcad9b2bfe4905e35d840dc21c67e41cfdba2d Mon Sep 17 00:00:00 2001 From: Arne Hormann Date: Mon, 27 Jan 2014 18:55:05 +0100 Subject: [PATCH 2/5] made writeExecutePacket independent of number of arguments --- driver_test.go | 2 +- packets.go | 44 ++++++++++++++++++++++++++++---------------- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/driver_test.go b/driver_test.go index 1dca680b9..e0ab7a97a 100644 --- a/driver_test.go +++ b/driver_test.go @@ -1211,7 +1211,7 @@ func TestStmtMultiRows(t *testing.T) { } func TestPreparedManyCols(t *testing.T) { - const repetitions = 1024 + const repetitions = 32 // defaultBufSize runTests(t, dsn, func(dbt *DBTest) { query := "SELECT ?" + strings.Repeat(",?", repetitions-1) values := make([]sql.NullString, repetitions) diff --git a/packets.go b/packets.go index ff1a6eaba..f4ef14843 100644 --- a/packets.go +++ b/packets.go @@ -750,6 +750,7 @@ func (stmt *mysqlStmt) writeExecutePacket(args []driver.Value) error { ) } + const minPktLen = 4 + 1 + 4 + 1 + 4 mc := stmt.mc // Reset packet-sequence @@ -758,7 +759,7 @@ func (stmt *mysqlStmt) writeExecutePacket(args []driver.Value) error { var data []byte if len(args) == 0 { - data = mc.buf.takeBuffer(4 + 1 + 4 + 1 + 4) + data = mc.buf.takeBuffer(minPktLen) } else { data = mc.buf.takeCompleteBuffer() } @@ -787,10 +788,26 @@ func (stmt *mysqlStmt) writeExecutePacket(args []driver.Value) error { data[13] = 0x00 if len(args) > 0 { + pos := minPktLen // NULL-bitmap [(len(args)+7)/8 bytes] - nullMask := uint64(0) - - pos := 4 + 1 + 4 + 1 + 4 + ((len(args) + 7) >> 3) + var nullMask []byte + if maskLen, typesLen := (len(args)+7)/8, 1+2*len(args); pos+maskLen+typesLen >= len(data) { + // buffer has to be extended but we don't know by how much + // so we depend on append after nullMask fits. + // The default size didn't suffice and we have to deal with a lot of columns, + // so allocation size is hard to guess. + tmp := make([]byte, pos+maskLen+typesLen) + copy(tmp[:pos], data[:pos]) + data = tmp + nullMask = data[pos : pos+maskLen] + pos += maskLen + } else { + nullMask = data[pos : pos+maskLen] + for i := 0; i < maskLen; i++ { + nullMask[i] = 0 + } + pos += maskLen + } // newParameterBoundFlag 1 [1 byte] data[pos] = 0x01 @@ -798,23 +815,23 @@ func (stmt *mysqlStmt) writeExecutePacket(args []driver.Value) error { // type of each parameter [len(args)*2 bytes] paramTypes := data[pos:] - pos += (len(args) << 1) + pos += len(args) * 2 // value of each parameter [n bytes] paramValues := data[pos:pos] valuesCap := cap(paramValues) - for i := range args { + for i, arg := range args { // build NULL-bitmap - if args[i] == nil { - nullMask |= 1 << uint(i) + if arg == nil { + nullMask[i/8] |= 1 << (uint(i) & 7) // |= 1 << uint(i) paramTypes[i+i] = fieldTypeNULL paramTypes[i+i+1] = 0x00 continue } // cache types and values - switch v := args[i].(type) { + switch v := arg.(type) { case int64: paramTypes[i+i] = fieldTypeLongLong paramTypes[i+i+1] = 0x00 @@ -877,7 +894,7 @@ func (stmt *mysqlStmt) writeExecutePacket(args []driver.Value) error { } // Handle []byte(nil) as a NULL value - nullMask |= 1 << uint(i) + nullMask[i/8] |= 1 << (uint(i) & 7) // |= 1 << uint(i) paramTypes[i+i] = fieldTypeNULL paramTypes[i+i+1] = 0x00 @@ -913,7 +930,7 @@ func (stmt *mysqlStmt) writeExecutePacket(args []driver.Value) error { paramValues = append(paramValues, val...) default: - return fmt.Errorf("Can't convert type: %T", args[i]) + return fmt.Errorf("Can't convert type: %T", arg) } } @@ -926,11 +943,6 @@ func (stmt *mysqlStmt) writeExecutePacket(args []driver.Value) error { pos += len(paramValues) data = data[:pos] - - // Convert nullMask to bytes - for i, max := 0, (stmt.paramCount+7)>>3; i < max; i++ { - data[i+14] = byte(nullMask >> uint(i<<3)) - } } return mc.writePacket(data) From edded4d136a5d6da2d88d21d2cf0371868e73533 Mon Sep 17 00:00:00 2001 From: Arne Hormann Date: Mon, 27 Jan 2014 19:01:06 +0100 Subject: [PATCH 3/5] increase number of arguments in test (forgot to raise this again) --- driver_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/driver_test.go b/driver_test.go index e0ab7a97a..eb06207e3 100644 --- a/driver_test.go +++ b/driver_test.go @@ -1211,11 +1211,11 @@ func TestStmtMultiRows(t *testing.T) { } func TestPreparedManyCols(t *testing.T) { - const repetitions = 32 // defaultBufSize + const numParams = defaultBufSize runTests(t, dsn, func(dbt *DBTest) { - query := "SELECT ?" + strings.Repeat(",?", repetitions-1) - values := make([]sql.NullString, repetitions) - params := make([]interface{}, repetitions) + query := "SELECT ?" + strings.Repeat(",?", numParams-1) + values := make([]sql.NullString, numParams) + params := make([]interface{}, numParams) for i := range values { params[i] = &values[i] } From 30e57b5e29e3baf952f6385bcb3d816156f7f110 Mon Sep 17 00:00:00 2001 From: Arne Hormann Date: Mon, 27 Jan 2014 19:14:48 +0100 Subject: [PATCH 4/5] removed deprecated comments, improved my own --- packets.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packets.go b/packets.go index f4ef14843..49aaf1807 100644 --- a/packets.go +++ b/packets.go @@ -789,13 +789,13 @@ func (stmt *mysqlStmt) writeExecutePacket(args []driver.Value) error { if len(args) > 0 { pos := minPktLen - // NULL-bitmap [(len(args)+7)/8 bytes] + var nullMask []byte if maskLen, typesLen := (len(args)+7)/8, 1+2*len(args); pos+maskLen+typesLen >= len(data) { - // buffer has to be extended but we don't know by how much - // so we depend on append after nullMask fits. - // The default size didn't suffice and we have to deal with a lot of columns, - // so allocation size is hard to guess. + // buffer has to be extended but we don't know by how much so + // we depend on append after all data with known sizes fit. + // We stop at that because we deal with a lot of columns here + // which makes the required allocation size hard to guess. tmp := make([]byte, pos+maskLen+typesLen) copy(tmp[:pos], data[:pos]) data = tmp @@ -824,7 +824,7 @@ func (stmt *mysqlStmt) writeExecutePacket(args []driver.Value) error { for i, arg := range args { // build NULL-bitmap if arg == nil { - nullMask[i/8] |= 1 << (uint(i) & 7) // |= 1 << uint(i) + nullMask[i/8] |= 1 << (uint(i) & 7) paramTypes[i+i] = fieldTypeNULL paramTypes[i+i+1] = 0x00 continue @@ -894,7 +894,7 @@ func (stmt *mysqlStmt) writeExecutePacket(args []driver.Value) error { } // Handle []byte(nil) as a NULL value - nullMask[i/8] |= 1 << (uint(i) & 7) // |= 1 << uint(i) + nullMask[i/8] |= 1 << (uint(i) & 7) paramTypes[i+i] = fieldTypeNULL paramTypes[i+i+1] = 0x00 From 75c7231d4742b78cf303feb57c6e6e74f9e45488 Mon Sep 17 00:00:00 2001 From: Arne Hormann Date: Fri, 31 Jan 2014 19:34:17 +0100 Subject: [PATCH 5/5] incorporate Juliens suggestions and update the changelog --- CHANGELOG.md | 5 +++++ driver_test.go | 11 ++++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af0c10d19..97cf052c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ New Features: - Logging of critical errors is configurable with `SetLogger` +Bugfixes: + + - Allow more than 32 parameters in prepared statements + + ## Version 1.1 (2013-11-02) Changes: diff --git a/driver_test.go b/driver_test.go index eb06207e3..cad40ec61 100644 --- a/driver_test.go +++ b/driver_test.go @@ -1210,20 +1210,21 @@ func TestStmtMultiRows(t *testing.T) { }) } +// Regression test for +// * more than 32 NULL parameters (issue 209) +// * more parameters than fit into the buffer (issue 201) func TestPreparedManyCols(t *testing.T) { const numParams = defaultBufSize runTests(t, dsn, func(dbt *DBTest) { query := "SELECT ?" + strings.Repeat(",?", numParams-1) - values := make([]sql.NullString, numParams) - params := make([]interface{}, numParams) - for i := range values { - params[i] = &values[i] - } stmt, err := dbt.db.Prepare(query) if err != nil { dbt.Fatal(err) } defer stmt.Close() + // create more parameters than fit into the buffer + // which will take nil-values + params := make([]interface{}, numParams) rows, err := stmt.Query(params...) if err != nil { stmt.Close()