What version of Go are you using (go version)?

$ go version
go version go1.16.6 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="/Users/alandaws/bin"
GOCACHE="/Users/alandaws/Library/Caches/go-build"
GOENV="/Users/alandaws/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/alandaws/pkg/mod"
GONOPROXY="github.ibm.com"
GONOSUMDB="github.ibm.com"
GOOS="darwin"
GOPATH="/Users/alandaws"
GOPRIVATE="github.ibm.com"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.6"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/06/jjq25lws0cbcxv37ns0hb_mm0000gn/T/go-build3808160546=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

In the following handler, where *Server has db *sql.DB:

func (s *Server) health(w http.ResponseWriter, r *http.Request) {
	ctx := r.Context()
	conn, err := s.db.Conn(ctx)
	if err != nil {
		http.Error(w, "Cannot connect to database: "+err.Error(), http.StatusBadGateway)
		return
	}
	defer conn.Close()
	if err := conn.Raw(func(driverConn interface{}) error {
		dc, ok := driverConn.(pinger)
		if !ok {
			return errors.New("database is not pingable")
		}
		if err := dc.Ping(ctx); err != nil {
			return fmt.Errorf("failed to ping databse: %w", err)
		}
		return nil
	}); err != nil {
		http.Error(w, "Error pinging database: "+err.Error(), http.StatusBadGateway)
	}
	fmt.Fprintln(w, "imok")
}

(yes I am aware PingContext() exists)

What did you expect to see?

When f() used in the call to Raw() fails, I expected the connection to be closed, as happens with other sql.Conn methods.

What did you see instead?

When the ping fails, the bad connection is not closed. It is then ultimately returned to the connection pool after sql.Conn.Close() is called.

In my use case this leads to the handler repeatedly failing and then things like kubernetes liveness probe failures and service restarts.

Analysis

The Raw() function will only close the connection if the error returned by f() is (==) driver.ErrBadConn https://cs.opensource.google/go/go/+/refs/tags/go1.16.6:src/database/sql/sql.go;drc=422dc83baa2816ca1d9a0aa3f1aaf4c47c8098ad;l=1979.

I think this behaviour should be documented. Currently "Once f returns and err is nil, the Conn will continue to be usable until Conn.Close is called." is inaccurate. Perhaps "Once f returns and err is not equal to driver.ErrBadConn, the Conn will continue to be usable until Conn.Close is called." would be more precise?

It might also be worth changing the behaviour so that either any non-nil error, or any error which wraps driver.ErrBadConn will close the connection, the current behaviour surprised me.