Skip to content
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

database/sql: prepare command is always sent again when executing a statement on a DB pool with multiple open connections #32298

Open
nishaad78 opened this issue May 29, 2019 · 8 comments
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@nishaad78
Copy link

nishaad78 commented May 29, 2019

What did you do?

Executing a prepared sql query on a DB connection pool with at least 2 open connections always sends the Prepare command on two connections.

For reproducing the error, first enable general logging in your MySQL DB:

SET GLOBAL general_log = 'ON';
SET GLOBAL log_output = 'table';

Now, connect to your mysql DB:

package main

import (
	"sync"

	"database/sql"
	_ "github.com/go-sql-driver/mysql"
)

db, err := sql.Open("mysql", "***:***@/***")
if err != nil {
    panic(err.Error())
}
defer db.Close()

db.SetMaxIdleConns(10)
db.SetMaxOpenConns(10)

Then run some concurrent sql commands to make sure the pool has more than one open connection:

var wg = &sync.WaitGroup{}
for i := 0; i < 2; i++ {
	wg.Add(1)
	go func(i int) {
		var stmt, err = db.Prepare("SELECT * FROM mytable WHERE id = ?")
		if err != nil {
			panic(err.Error())
		}
		defer stmt.Close()
		_, err = stmt.Exec(1)
		if err != nil {
			panic(err.Error())
		}
		wg.Done()
	}(i)
}
wg.Wait()

Now that we have at least two open connections, run a prepare and execute command on the DB:

var stmt, err = db.Prepare("SELECT * FROM mytable WHERE id = ?")
if err != nil {
	panic(err.Error())
}
defer stmt.Close()
_, err = stmt.Exec(2)
if err != nil {
	panic(err.Error())
}

What did you expect to see?

mysql> SELECT thread_id,command_type,argument FROM mysql.general_log;

+-----------+--------------+------------------------------------------+
| thread_id | command_type | argument                                 |
+-----------+--------------+------------------------------------------+
|        78 | Prepare      | SELECT * FROM mytable WHERE id = ?       |
|        77 | Prepare      | SELECT * FROM mytable WHERE id = ?       |
|        78 | Execute      | SELECT * FROM mytable WHERE id = 1       |
|        77 | Execute      | SELECT * FROM mytable WHERE id = 1       |
|        78 | Close stmt   | 
|        77 | Close stmt   |                
|        78 | Prepare      | SELECT * FROM mytable WHERE id = ?       |
|        78 | Execute      | SELECT * FROM mytable WHERE id = 2       |
|        78 | Close stmt   |                       
|        78 | Quit         |                            
|        77 | Quit         |                        
+-----------+--------------+------------------------------------------+

What did you see instead?

mysql> SELECT thread_id,command_type,argument FROM mysql.general_log;

+-----------+--------------+------------------------------------------+
| thread_id | command_type | argument                                 |
+-----------+--------------+------------------------------------------+
|        38 | Prepare      | SELECT * FROM mytable WHERE id = ?       |
|        38 | Execute      | SELECT * FROM mytable WHERE id = 1       |
|        38 | Close stmt   |   
|        37 | Prepare      | SELECT * FROM mytable WHERE id = ?       |
|        38 | Prepare      | SELECT * FROM mytable WHERE id = ?       |
|        38 | Execute      | SELECT * FROM mytable WHERE id = 1       |
|        38 | Close stmt   |  
|        37 | Close stmt   | 
|        37 | Prepare      | SELECT * FROM mytable WHERE id = ?       |
|        38 | Prepare      | SELECT * FROM mytable WHERE id = ?       |
|        38 | Execute      | SELECT * FROM mytable WHERE id = 2       |
|        38 | Close stmt   |        
|        38 | Quit         |
|        37 | Close stmt   |    
|        37 | Quit         |
+-----------+--------------+------------------------------------------+

System details

mysql  Ver 8.0.16 for Linux on x86_64 (MySQL Community Server - GPL)
go version go1.12.5 darwin/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/najani/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/najani/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
GOROOT/bin/go version: go version go1.12.5 darwin/amd64
GOROOT/bin/go tool compile -V: compile version go1.12.5
uname -v: Darwin Kernel Version 18.2.0: Fri Oct  5 19:41:49 PDT 2018; root:xnu-4903.221.2~2/RELEASE_X86_64
ProductName:	Mac OS X
ProductVersion:	10.14.1
BuildVersion:	18B75
lldb --version: lldb-1000.11.38.2
  Swift-4.2
@nishaad78
Copy link
Author

After investigation, we found that the connStmt func never finds the connection the prepare command was sent on because db.conn func that it is calling is implemented as a LIFO, meaning that the connection just used for prepare will be the last one returned if nothing else happened in between prepare and execute.
ref: https://github.com/golang/go/blob/master/src/database/sql/sql.go#L2461-L2478

We tested this hypothesis by implementing a function that iterated over DB.freeConn and Stmt.css to find a match. This fixed this issue and we will open a PR.

@gopherbot
Copy link

Change https://golang.org/cl/179298 mentions this issue: database/sql: run prepare & execute commands on the same conn from DB pool

@kardianos
Copy link
Contributor

@nishaad78 I can review the CL for your for Go1.14 (we are still in a freeze for 1.13).

For your use case, have you considered using a dedicated Conn() and calling prepare and execute on a single Conn?

@kardianos kardianos self-assigned this May 29, 2019
@nishaad78
Copy link
Author

nishaad78 commented May 30, 2019 via email

@nishaad78
Copy link
Author

@kardianos could this qualify for a minor for 1.12?

@kardianos
Copy link
Contributor

@nishaad78 Assuming it is merged, no, it would (still) not qualify for a minor point release.

@agnivade agnivade added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 3, 2019
@agnivade agnivade added this to the Go1.14 milestone Jun 3, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@methane
Copy link
Contributor

methane commented Nov 1, 2019

Please test scalability on large (64+ cores) machine.

See #9484 and
https://go-review.googlesource.com/c/go/+/2207/

@kardianos
Copy link
Contributor

kardianos commented Jan 24, 2020

@nishaad78 I'm glad you choose to move to a dedicated connection.

Of the entire sql package code base, I find the statement sub-pools the most challenging. Your CL may be fine, but I highly doubt I will be able to merge or maintain it.

EDIT: To be clear, unless a simpler, very straightforward solution is presented, this will likely be declined.

@kardianos kardianos added Proposal and removed NeedsFix The path to resolution is known, but the work has not been done. labels Jan 24, 2020
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. help wanted and removed Proposal labels Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants