search

Kount / pq-timeouts

Postgres driver for Go that wraps lib/pq to provide read and write timeouts.
MIT License
38 stars 17 forks source link

This logic should be in the original driver #1

Closed georgysavva closed 6 years ago

georgysavva commented 7 years ago

Hi! I agree with you, that the absence of the read/write timeouts in the lib/pq is a serious drawback in HA environment. And it's really cool that you wrote a wrapper with such functional. But consider following situation: we have read timeout=2s, write timeout=2s, we have already opened connection and network failure occur, we send our query. We begin to wait for the response, imagine that the response size is 1024 bytes. because of the network failure db sends response with very high latency 1 byte per second, we call conn.read(1024) and we fit into read timeout, but we receive only 1 byte, therefore we need to call read 1024 times to receive the whole response.

In order to timeout worked as expected we shouldn't call conn.SetReadDeadline in every conn.Read call. We should call conn.SetReadDeadline(2s) before any read operations. Therefore timeouts logic should be in the original driver. As in other libraries https://github.com/go-pg/pg for example.

pneisen commented 7 years ago

I agree and I tried to pull request this functionality into lib/pq, but the maintainers of lib/pq were not interested. This wrapper was a workaround.

That said, I think this issue is solved and this wrapper is not needed with the addition of context support to database/sql in Go 1.8.

georgysavva commented 7 years ago

As far as I know context doesn't work if driver doesn't implement cancellation logic.

pneisen commented 7 years ago

True, but I believe lib/pq implemented this logic in this pull request: https://github.com/lib/pq/pull/535