From 3ea2f57d8bd3233d751c26134711812259a64d41 Mon Sep 17 00:00:00 2001 From: Jack Christensen Date: Sat, 3 Jun 2023 18:54:58 -0500 Subject: [PATCH] Deprecate CheckConn in favor of Ping --- conn.go | 6 ++---- pgconn/pgconn.go | 36 ++++++++++++++++++++++-------------- pgconn/pgconn_test.go | 43 ++++++++++++++++++++++++++++++++++++++++++- pgxpool/pool.go | 2 +- 4 files changed, 67 insertions(+), 20 deletions(-) diff --git a/conn.go b/conn.go index 5cb88de3..5315b1b1 100644 --- a/conn.go +++ b/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 diff --git a/pgconn/pgconn.go b/pgconn/pgconn.go index 02226e0a..5143244d 100644 --- a/pgconn/pgconn.go +++ b/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)} diff --git a/pgconn/pgconn_test.go b/pgconn/pgconn_test.go index 5bd26246..2be99500 100644 --- a/pgconn/pgconn_test.go +++ b/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() diff --git a/pgxpool/pool.go b/pgxpool/pool.go index cc837017..f4c3a30c 100644 --- a/pgxpool/pool.go +++ b/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