rows.Close() modifying underlying data buffer

[datbth]

Reproduce

func TestMysqlDriverBuffer(t *testing.T) {
	cntor := makeMysqlConnector(nil)
	connUrl, urlErr := cntor.ConnectionUrl()
	require.NoError(t, urlErr)

	fmt.Printf("conn url: %s\n", connUrl)

	conn, connErr := mysql.MySQLDriver{}.Open(connUrl)
	require.NoError(t, connErr)
	defer conn.Close()

	sql := `
		WITH RECURSIVE t(x) AS (
			SELECT 0 UNION ALL
			SELECT x + 1 FROM t WHERE x <= 349
		)
		SELECT
			x,
			'abc' y
		FROM t
	`

	rows, queryErr := conn.(driver.QueryerContext).QueryContext(context.Background(), sql, nil)
	require.NoError(t, queryErr)

	result := make([][]driver.Value, 2)
	for i := range result {
		row := make([]driver.Value, 2)
		nextErr := rows.Next(row)
		if nextErr == io.EOF {
			break
		}
		require.NoError(t, nextErr)
		fmt.Printf("row %d: %v, %v\n", i, row[0], row[1])
		result[i] = row
	}

	rows.Close()
	for i := range result {
		fmt.Printf("result row %d: %v, %v\n", i, result[i][0], result[i][1])
	}
}

Expected Output

=== RUN   TestMysqlDriverBuffer
conn url: root:testtest@tcp(localhost:3308)/htest?parseTime=true&timeout=10000ms&tls=preferred
row 0: 0, [97 98 99]
row 1: 1, [97 98 99]
result row 0: 0, [97 98 99]
result row 1: 1, [97 98 99]

Actual Output

=== RUN   TestMysqlDriverBuffer
conn url: root:testtest@tcp(localhost:3308)/htest?parseTime=true&timeout=10000ms&tls=preferred
row 0: 0, [97 98 99]
row 1: 1, [97 98 99]
result row 0: 0, [97 98 99]
result row 1: 1, [49 3 97]

Description

It appears that row.Close() is modifying the underlying data buffer, making it not possible to use the scanned data properly after closing (at least without having to iterate and copy the buffers).

I believe this does not conform with database/sql/driver (ref):

Notes

• Tested with Mysql 8.0.42, 9.3.0
• Same issue happens with other data types that are scanned into []byte, such as Decimal (e.g. replace 'abc' y with 0.01 y)
• The minimal x <= value at which I can reproduce this issue is 349, but this number varies with the length of the data values. Can try to increase this number to increase the chance of reproducing the issue.
• Does not happen when not calling rows.Close()
• Without calling rows.Close()
  • It is not possible to re-use the connection for subsequent queries
  • It can lead to unexpected issues

[datbth]

I managed to reproduce the issue when sending the scanned rows via a channel, without using rows.Close(). I'm not really sure whether this case is the same or not.

func TestMysqlDriverDecimal(t *testing.T) {
	cntor := makeMysqlConnector(nil)
	connUrl, urlErr := cntor.ConnectionUrl()
	require.NoError(t, urlErr)

	fmt.Printf("conn url: %s\n", connUrl)

	conn, connErr := mysql.MySQLDriver{}.Open(connUrl)
	require.NoError(t, connErr)
	defer conn.Close()

	sql := `
		WITH RECURSIVE t(x) AS (
			SELECT 0 UNION ALL
			SELECT x + 1 FROM t WHERE x <= 500
		)
		SELECT
			x,
			'abc' y
		FROM t
	`

	rows, queryErr := conn.(driver.QueryerContext).QueryContext(context.Background(), sql, nil)
	require.NoError(t, queryErr)

	ch := make(chan []driver.Value, 2048)
	var wg sync.WaitGroup
	wg.Add(1)
	go func() {
		i := 0
		for row := range ch {
			time.Sleep(50 * time.Millisecond)
			fmt.Printf("result row %d: %v, %v\n", i, row[0], row[1])
			i += 1
		}
		wg.Done()
	}()

	i := 0
	for {
		row := make([]driver.Value, 2)
		nextErr := rows.Next(row)
		if nextErr == io.EOF {
			break
		}
		require.NoError(t, nextErr)
		fmt.Printf("row %d: %v, %v\n", i, row[0], row[1])
		ch <- row
		i += 1
	}
	close(ch)

	wg.Wait()
}

Actual Output

(snippet)

row 492: 492, [97 98 99]
row 493: 493, [97 98 99]
row 494: 494, [97 98 99]
row 495: 495, [97 98 99]
row 496: 496, [97 98 99]
row 497: 497, [97 98 99]
row 498: 498, [97 98 99]
row 499: 499, [97 98 99]
row 500: 500, [97 98 99]
row 501: 501, [97 98 99]
result row 0: 0, [97 98 99]
result row 1: 1, [49 3 97]
result row 2: 2, [51 53 50]
result row 3: 3, [102 3 51]
result row 4: 4, [0 0 103]
result row 5: 5, [99 8 0]
result row 6: 6, [97 98 99]
result row 7: 7, [54 3 97]
result row 8: 8, [51 53 55]
result row 9: 9, [107 3 51]
result row 10: 10, [0 108 3]

...

result row 99: 99, [97 98 99]
result row 100: 100, [97 98 99]
result row 101: 101, [97 98 99]
result row 102: 102, [97 98 99]
result row 103: 103, [97 98 99]
result row 104: 104, [97 98 99]
result row 105: 105, [97 98 99]
result row 106: 106, [97 98 99]
result row 107: 107, [97 98 99]
result row 108: 108, [97 98 99]
result row 109: 109, [97 98 99]
result row 110: 110, [97 98 99]
result row 111: 111, [97 98 99]
result row 112: 112, [97 98 99]
result row 113: 113, [97 98 99]
result row 114: 114, [97 98 99]
result row 115: 115, [97 98 99

[methane]

database/sql copies data. So scanned data can be used after Next() or Close(), except sql.RawBytes.

I believe this does not conform with database/sql/driver (ref):

The doc is wrong.
This comment was added this commit.

After several years, this restriction is removed.

• issue
• commit1
• commit2

They just missed to remove the comment in the driver.Rows.Next()

[methane]

golang/go#73632

[datbth]

oh okay. Thank you for your reply.

It seems that different drivers are doing this differently. For example, microsoft/go-mssqldb copies the buffer: microsoft/go-mssqldb@90c2c7e.

I'm also working with other DB drivers such as pgx, duckdb, bigquery, athena, etc, but I have only encountered this with mysql. (I'll probably need to revise the others).

Though within go-sql-sever/mysql itself, other types like LongLong, Float, Int are parsed, so they are sort of copied. Hence, there is a slight inconsistency.

Would it make sense to copy byte arrays by default, except when using sql.RawBytes?

[methane]

It seems that different drivers are doing this differently. For example, microsoft/go-mssqldb copies the buffer: microsoft/go-mssqldb@90c2c7e.

The commit is in 2015. We had other workaround but removed it in last December.
#1643

[methane]

Would it make sense to copy byte arrays by default, except when using sql.RawBytes?

Its job of database/sql. database/sql copies buffer. If we copy, two copies happens.
It doesn't make sense at all.

[methane]

See https://github.com/golang/go/blob/d000963d045bb279d347dbd3551e9468422c17af/src/database/sql/convert.go#L254-L273

[datbth]

okay noted. For my case, I want to use the Driver directly to maintain my own connection pooling with custom logics. But it's fair that you are optimizing for database/sql usages.

Perhaps there could be an inline/README doc for this.

Thank you for your time.