Deprecate CheckConn in favor of Ping

Author: jackc

conn.go

@@ -382,11 +382,9 @@ func quoteIdentifier(s string) string {
 	return `"` + strings.ReplaceAll(s, `"`, `""`) + `"`
 }
 
-// Ping executes an empty sql statement against the *Conn
-// If the sql returns without error, the database Ping is considered successful, otherwise, the error is returned.
+// Ping delegates to the underlying *pgconn.PgConn.Ping.
 func (c *Conn) Ping(ctx context.Context) error {
-	_, err := c.Exec(ctx, ";")
-	return err
+	return c.pgConn.Ping(ctx)
 }
 
 // PgConn returns the underlying *pgconn.PgConn. This is an escape hatch method that allows lower level access to the

pgconn/pgconn.go

@@ -1669,27 +1669,35 @@ func (pgConn *PgConn) EscapeString(s string) (string, error) {
 	return strings.Replace(s, "'", "''", -1), nil
 }
 
-// CheckConn checks the underlying connection without writing any bytes. This is currently implemented by reading and
-// buffering until the read would block or an error occurs. This can be used to check if the server has closed the
-// connection. If this is done immediately before sending a query it reduces the chances a query will be sent that fails
+// CheckConn checks the underlying connection without writing any bytes. This is currently implemented by doing a read
+// with a very short deadline. This can be useful because a TCP connection can be broken such that a write will appear
+// to succeed even though it will never actually reach the server. Reading immediately before a write will detect this
+// condition. If this is done immediately before sending a query it reduces the chances a query will be sent that fails
 // without the client knowing whether the server received it or not.
+//
+// Deprecated: CheckConn is deprecated in favor of Ping. CheckConn cannot detect all types of broken connections where
+// the write would still appear to succeed. Prefer Ping unless on a high latency connection.
 func (pgConn *PgConn) CheckConn() error {
-	// if err := pgConn.lock(); err != nil {
-	// 	return err
-	// }
-	// defer pgConn.unlock()
+	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Millisecond)
+	defer cancel()
 
-	rr := pgConn.ExecParams(context.Background(), "select 1", nil, nil, nil, nil)
-	_, err := rr.Close()
-	return err
+	_, err := pgConn.ReceiveMessage(ctx)
+	if err != nil {
+		if !Timeout(err) {
+			return err
+		}
+	}
 
-	// err := pgConn.conn.BufferReadUntilBlock()
-	// if err != nil && !errors.Is(err, nbconn.ErrWouldBlock) {
-	// 	return err
-	// }
 	return nil
 }
 
+// Ping pings the server. This can be useful because a TCP connection can be broken such that a write will appear to
+// succeed even though it will never actually reach the server. Pinging immediately before sending a query reduces the
+// chances a query will be sent that fails without the client knowing whether the server received it or not.
+func (pgConn *PgConn) Ping(ctx context.Context) error {
+	return pgConn.Exec(ctx, "-- ping").Close()
+}
+
 // makeCommandTag makes a CommandTag. It does not retain a reference to buf or buf's underlying memory.
 func (pgConn *PgConn) makeCommandTag(buf []byte) CommandTag {
 	return CommandTag{s: string(buf)}

pgconn/pgconn_test.go

@@ -2469,7 +2469,7 @@ func TestConnCheckConn(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 	defer cancel()
 
-	// Intentionally using TCP connection for more predictable close behavior. (Not sure if Unix domain sockets would behave subtlely different.)
+	// Intentionally using TCP connection for more predictable close behavior. (Not sure if Unix domain sockets would behave subtly different.)
 
 	connString := os.Getenv("PGX_TEST_TCP_CONN_STRING")
 	if connString == "" {
@@ -2501,6 +2501,47 @@ func TestConnCheckConn(t *testing.T) {
 	require.Error(t, err)
 }
 
+func TestConnPing(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	// Intentionally using TCP connection for more predictable close behavior. (Not sure if Unix domain sockets would behave subtly different.)
+
+	connString := os.Getenv("PGX_TEST_TCP_CONN_STRING")
+	if connString == "" {
+		t.Skipf("Skipping due to missing environment variable %v", "PGX_TEST_TCP_CONN_STRING")
+	}
+
+	c1, err := pgconn.Connect(ctx, connString)
+	require.NoError(t, err)
+	defer c1.Close(ctx)
+
+	if c1.ParameterStatus("crdb_version") != "" {
+		t.Skip("Server does not support pg_terminate_backend() (https://github.com/cockroachdb/cockroach/issues/35897)")
+	}
+
+	err = c1.Exec(ctx, "set log_statement = 'all'").Close()
+	require.NoError(t, err)
+
+	err = c1.Ping(ctx)
+	require.NoError(t, err)
+
+	c2, err := pgconn.Connect(ctx, connString)
+	require.NoError(t, err)
+	defer c2.Close(ctx)
+
+	_, err = c2.Exec(ctx, fmt.Sprintf("select pg_terminate_backend(%d)", c1.PID())).ReadAll()
+	require.NoError(t, err)
+
+	// Give a little time for the signal to actually kill the backend.
+	time.Sleep(500 * time.Millisecond)
+
+	err = c1.Ping(ctx)
+	require.Error(t, err)
+}
+
 func TestPipelinePrepare(t *testing.T) {
 	t.Parallel()
 

pgxpool/pool.go

@@ -504,7 +504,7 @@ func (p *Pool) Acquire(ctx context.Context) (*Conn, error) {
 		cr := res.Value()
 
 		if res.IdleDuration() > time.Second {
-			err := cr.conn.PgConn().CheckConn()
+			err := cr.conn.Ping(ctx)
 			if err != nil {
 				res.Destroy()
 				continue