From 5b345e80e12e5a8ae0f15d9a5a94b3473d431c90 Mon Sep 17 00:00:00 2001 From: Jack Christensen Date: Sat, 5 Jul 2014 09:25:13 -0500 Subject: [PATCH] Remove SelectValueTo Benchmarks revealed that it is no longer performant enough to pull its own wait. Using go_db_bench to copy JSON results to HTTP responses it was ~20% *slower* for ~4BK responses and less than 10% faster for +1MB responses. The the performance problem was in io.CopyN / io.Copy. io.Copy allocates a 32KB buffer if it doesn't have io.WriterTo or io.ReaderFrom available. This extra alloc on every request was more expensive than just reading the result into a string and writing it out to the response body. Tests indicated that if MsgReader implemented a custom Copy that used a shared buffer it might have a few percent performance advantage. But the additional complexity is not worth the performance gain. --- README.md | 8 ----- bench_test.go | 18 ----------- conn.go | 58 ----------------------------------- conn_pool.go | 12 -------- conn_test.go | 83 -------------------------------------------------- helper_test.go | 7 ----- msg_reader.go | 17 ----------- 7 files changed, 203 deletions(-) diff --git a/README.md b/README.md index 0487c4ec..7e61d2c8 100644 --- a/README.md +++ b/README.md @@ -114,14 +114,6 @@ data type is to set that OID's transcoder. See example_value_transcoder_test.go for an example of a custom transcoder for the PostgreSQL point type. -### SelectValueTo - -There are some cases where Go is used as an HTTP server that is directly -relaying single values from PostgreSQL (such as JSON or binary blobs). -SelectValueTo copies the single returned value directly from PostgreSQL to a -io.Writer. This can be faster than SelectValue then write especially when the -values are at least many KB in size. - ### Null Mapping As pgx uses interface{} for all values SQL nulls are mapped to nil. This diff --git a/bench_test.go b/bench_test.go index c3863880..bf1e4627 100644 --- a/bench_test.go +++ b/bench_test.go @@ -2,7 +2,6 @@ package pgx_test import ( "github.com/jackc/pgx" - "io/ioutil" "math/rand" "testing" ) @@ -48,23 +47,6 @@ func BenchmarkSelectValuePreparedNarrow(b *testing.B) { } } -func BenchmarkSelectValueToPreparedNarrow(b *testing.B) { - conn := mustConnect(b, *defaultConnConfig) - defer closeConn(b, conn) - createNarrowTestData(b, conn) - - // Get random ids outside of timing - ids := make([]int32, b.N) - for i := 0; i < b.N; i++ { - ids[i] = 1 + rand.Int31n(9999) - } - - b.ResetTimer() - for i := 0; i < b.N; i++ { - mustSelectValueTo(b, conn, ioutil.Discard, "getMultipleNarrowByIdAsJSON", ids[i], ids[i]+10) - } -} - func BenchmarkConnPool(b *testing.B) { config := pgx.ConnPoolConfig{ConnConfig: *defaultConnConfig, MaxConnections: 5} pool, err := pgx.NewConnPool(config) diff --git a/conn.go b/conn.go index 129cbef1..ffd011f7 100644 --- a/conn.go +++ b/conn.go @@ -311,50 +311,6 @@ func (c *Conn) SelectValue(sql string, arguments ...interface{}) (interface{}, e return v, nil } -// SelectValueTo executes sql that returns a single value and writes that value to w. -// No type conversions will be performed. The raw bytes will be written directly to w. -// This is ideal for returning JSON, files, or large text values directly over HTTP. - -// sql can be either a prepared statement name or an SQL string. arguments will be -// sanitized before being interpolated into sql strings. arguments should be -// referenced positionally from the sql string as $1, $2, etc. -// -// Returns a UnexpectedColumnCountError if exactly one column is not found -// Returns a NotSingleRowError if exactly one row is not found -func (c *Conn) SelectValueTo(w io.Writer, sql string, arguments ...interface{}) (err error) { - startTime := time.Now() - - defer func() { - if err == nil { - endTime := time.Now() - c.logger.Info("SelectValueTo", "sql", sql, "args", arguments, "time", endTime.Sub(startTime)) - } else { - c.logger.Error(fmt.Sprintf("SelectValueTo `%s` with %v failed: %v", sql, arguments, err)) - } - }() - - var numRowsFound int64 - - qr, _ := c.Query(sql, arguments...) - - for qr.NextRow() { - if len(qr.fields) != 1 { - qr.Close() - return UnexpectedColumnCountError{ExpectedCount: 1, ActualCount: int16(len(qr.fields))} - } - - numRowsFound++ - if numRowsFound != 1 { - qr.Close() - return NotSingleRowError{RowCount: numRowsFound} - } - - var rr RowReader - rr.CopyBytes(qr, w) - } - return qr.Err() -} - // Prepare creates a prepared statement with name and sql. sql can contain placeholders // for bound parameters. These placeholders are referenced positional as $1, $2, etc. func (c *Conn) Prepare(name, sql string) (ps *PreparedStatement, err error) { @@ -598,20 +554,6 @@ func (rr *RowReader) ReadValue(qr *QueryResult) interface{} { } } -func (rr *RowReader) CopyBytes(qr *QueryResult, w io.Writer) { - _, size, ok := qr.NextColumn() - if !ok { - return - } - - if size == -1 { - qr.Fatal(errors.New("Unexpected null")) - return - } - - qr.MsgReader().CopyN(w, size) -} - type QueryResult struct { pool *ConnPool conn *Conn diff --git a/conn_pool.go b/conn_pool.go index db65a842..3bac3de9 100644 --- a/conn_pool.go +++ b/conn_pool.go @@ -3,7 +3,6 @@ package pgx import ( "errors" log "gopkg.in/inconshreveable/log15.v2" - "io" "sync" ) @@ -177,17 +176,6 @@ func (p *ConnPool) SelectValue(sql string, arguments ...interface{}) (v interfac return c.SelectValue(sql, arguments...) } -// SelectValueTo acquires a connection, delegates the call to that connection, and releases the connection -func (p *ConnPool) SelectValueTo(w io.Writer, sql string, arguments ...interface{}) (err error) { - var c *Conn - if c, err = p.Acquire(); err != nil { - return - } - defer p.Release(c) - - return c.SelectValueTo(w, sql, arguments...) -} - // Exec acquires a connection, delegates the call to that connection, and releases the connection func (p *ConnPool) Exec(sql string, arguments ...interface{}) (commandTag CommandTag, err error) { var c *Conn diff --git a/conn_test.go b/conn_test.go index 60896848..731f5b35 100644 --- a/conn_test.go +++ b/conn_test.go @@ -462,42 +462,6 @@ func TestConnQueryReadTooManyValues(t *testing.T) { ensureConnValid(t, conn) } -func TestQueryResultCopyBytes(t *testing.T) { - t.Parallel() - - conn := mustConnect(t, *defaultConnConfig) - defer closeConn(t, conn) - - var mimeType string - var buf bytes.Buffer - - qr, err := conn.Query("select 'application/json', '[1,2,3,4,5]'::json") - if err != nil { - t.Fatalf("conn.Query failed: ", err) - } - - for qr.NextRow() { - var rr pgx.RowReader - mimeType = rr.ReadString(qr) - rr.CopyBytes(qr, &buf) - } - qr.Close() - - if qr.Err() != nil { - t.Fatalf("conn.Query failed: ", err) - } - - if mimeType != "application/json" { - t.Errorf(`Expected mimeType to be "application/json", but it was "%v"`, mimeType) - } - - if bytes.Compare(buf.Bytes(), []byte("[1,2,3,4,5]")) != 0 { - t.Fatalf("CopyBytes did not write expected data: %v", string(buf.Bytes())) - } - - ensureConnValid(t, conn) -} - func TestConnectionSelectValue(t *testing.T) { t.Parallel() @@ -542,53 +506,6 @@ func TestConnectionSelectValue(t *testing.T) { } } -func TestConnectionSelectValueTo(t *testing.T) { - t.Parallel() - - conn := mustConnect(t, *defaultConnConfig) - defer closeConn(t, conn) - - var err error - - var buf bytes.Buffer - if err := conn.SelectValueTo(&buf, "select '[1,2,3,4,5]'::json"); err != nil { - t.Fatalf("SelectValueTo failed: %v", err) - } - if bytes.Compare(buf.Bytes(), []byte("[1,2,3,4,5]")) != 0 { - t.Fatalf("SelectValueTo did not write expected data: %v", string(buf.Bytes())) - } - - // NotSingleRowError - err = conn.SelectValueTo(&buf, "select * from (values ('Matthew'), ('Mark'), ('Luke')) t") - if _, ok := err.(pgx.NotSingleRowError); !ok { - t.Fatalf("Multiple matching rows should have returned NotSingleRowError: %#v", err) - } - if conn.IsAlive() { - mustSelectValue(t, conn, "select 1") // ensure it really is alive and usable - } else { - t.Fatal("SelectValueTo NotSingleRowError should not have killed connection") - } - - // UnexpectedColumnCountError - err = conn.SelectValueTo(&buf, "select * from (values ('Matthew', 'Mark', 'Luke')) t") - if _, ok := err.(pgx.UnexpectedColumnCountError); !ok { - t.Fatalf("Multiple matching rows should have returned UnexpectedColumnCountError: %#v", err) - } - if conn.IsAlive() { - mustSelectValue(t, conn, "select 1") // ensure it really is alive and usable - } else { - t.Fatal("SelectValueTo UnexpectedColumnCountError should not have killed connection") - } - - // Null - err = conn.SelectValueTo(&buf, "select null") - if err == nil || err.Error() != "Unexpected null" { - t.Fatalf("Expected null error: %#v", err) - } - - ensureConnValid(t, conn) -} - func TestPrepare(t *testing.T) { t.Parallel() diff --git a/helper_test.go b/helper_test.go index ff5dd0a0..f51e1240 100644 --- a/helper_test.go +++ b/helper_test.go @@ -2,7 +2,6 @@ package pgx_test import ( "github.com/jackc/pgx" - "io" "testing" ) @@ -42,9 +41,3 @@ func mustSelectValue(t testing.TB, conn *pgx.Conn, sql string, arguments ...inte } return } - -func mustSelectValueTo(t testing.TB, conn *pgx.Conn, w io.Writer, sql string, arguments ...interface{}) { - if err := conn.SelectValueTo(w, sql, arguments...); err != nil { - t.Fatalf("SelectValueTo unexpectedly failed with %v: %v", sql, err) - } -} diff --git a/msg_reader.go b/msg_reader.go index 55bd6a11..5bc1170d 100644 --- a/msg_reader.go +++ b/msg_reader.go @@ -178,20 +178,3 @@ func (r *MsgReader) ReadString(count int32) string { return string(b) } - -func (r *MsgReader) CopyN(w io.Writer, count int32) { - if r.err != nil { - return - } - - r.msgBytesRemaining -= count - if r.msgBytesRemaining < 0 { - r.Fatal(errors.New("read past end of message")) - return - } - - _, err := io.CopyN(w, r.reader, int64(count)) - if err != nil { - r.Fatal(err) - } -}