Fix connection leaks #190
Fix connection leaks #190
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| results = append(results, ts...) | ||
| } | ||
| results, err := buildTimeSeries(rows, q) |
There was a problem hiding this comment.
[golangci-lint-github-pr-review] reported by reviewdog 🐶
ineffectual assignment to err (ineffassign)
| @@ -287,19 +279,20 @@ func (q *pgxQuerier) getResultRows(startTimestamp int64, endTimestamp int64, hin | |||
|
|
|||
| sqlQuery := buildMetricNameSeriesIDQuery(cases) | |||
| rows, err := q.conn.Query(context.Background(), sqlQuery, values...) | |||
| defer rows.Close() | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
I believe this is wrong for this interface
| @@ -342,6 +336,7 @@ func (q *pgxQuerier) querySingleMetric(metric string, filter metricTimeRangeFilt | |||
| return nil, nil, err | |||
| } | |||
| rows, err := q.conn.Query(context.Background(), sqlQuery, values...) | |||
| defer rows.Close() | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
I believe this is wrong for this interface
| metrics, series, err := getSeriesPerMetric(rows) | ||
|
|
||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| results := make([]pgx.Rows, 0, len(metrics)) | ||
| // TODO this assume on average on row per-metric. Is this right? |
|
@antekresic I added a commit with more batching, would you re-review? |
|
|
||
| batchResults, err := q.conn.SendBatch(context.Background(), batch) |
There was a problem hiding this comment.
[golangci-lint-github-pr-review] reported by reviewdog 🐶
ineffectual assignment to err (ineffassign)
|
|
||
| batchResults, err := q.conn.SendBatch(context.Background(), batch) | ||
| defer batchResults.Close() |
There was a problem hiding this comment.
[golangci-lint-github-pr-review] reported by reviewdog 🐶
SA5001: should check returned error before deferring batchResults.Close() (staticcheck)
The old test was difficult to read and update since the state of each query was spread across multiple slices. This commit consolidates them all into a single sqlQuery struct allowing us to determine the inputs and outputs for each query at a glance. We also switch to a new mock that works better with this format, and is able to report errors as they occur.
Keeping around a pgx.Rows is dangerous, as doing so keeps a lock on the underlying connection. This means that the connecter can self-starve if it gets partway through a Rows and decides it needs to fetch more data from the database. To fix this, this commit materializes all pgx.Rows into a []timescaleRow as soon as possible.
This is a drive-by performance optimization: there's no need to send each of these queries individually, we can batch them together and save on multiple roundtrip times.
JLockerman
force-pushed
the
fix-connection-leak
branch
from
6fcd7de
to
266e2b6
Compare
| numQueries += 1 | ||
| } | ||
|
|
||
| batchResults, err := q.conn.SendBatch(context.Background(), batch) |
There was a problem hiding this comment.
This error return should be checked.
|
|
||
| results = append(results, ts...) | ||
| } | ||
| results, err := buildTimeSeries(rows, q) |
There was a problem hiding this comment.
this err should be checked or returned.
| log.Error("err", row.err) | ||
| return out | ||
| } | ||
| } |
There was a problem hiding this comment.
you need to check in.Err() here too.
Keeping around a
pgx.Rowsis dangerous, as doing so keeps a lock on the underlying connection. This means that the connecter can self-starve if it gets partway through a Rows and decides it needs to fetch more data from the database. To fix this, this commit materializes allpgx.Rowsinto a[]timescaleRowas soon as possible.(Further, It looks like there's no guarantee that a
SeriesSetwill be read until the end anyway)This commit also refactors the
TestPGXQuerierQuerytest, as updating the old version to the new code was too onerous, and batches multi-metrics queries together, as I was rewriting that code anyway. It seems to give a nice performance increase.