Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support/db: Allow passing context.Context with deadline to Session methods #1950

Closed
bartekn opened this issue Nov 19, 2019 · 5 comments
Closed

Comments

@bartekn
Copy link
Contributor

bartekn commented Nov 19, 2019

What problem does your feature solve?

It should be possible to pass context.Context to set timeout for queries sent to a database. This is to prevent long running queries to occupy DB connection for a long period of time.

What would you like to see?

The easiest way to solve this is to use sqlx.*Context methods that allow passing context.Context and update db.Session methods to accept context.Context param.

What alternatives are there?

Users can set statement_timeout in Postgres however it requires changing DB config and also return an error that is unexpected for an application.

@bartekn bartekn added this to the Horizon 0.24.0 milestone Nov 19, 2019
@bartekn
Copy link
Contributor Author

bartekn commented Nov 19, 2019

Adding as a stretch goal to Horizon 0.24.0.

@leighmcculloch
Copy link
Member

Users can set statement_timeout in Postgres however it ... return an error that is unexpected for an application.

When using the github.com/lib/pq driver and passing a context with a timeout, the pq package will run a go routine in the background of a query that waits for the timer to complete, and if it times out it opens a new connection to the Postgres server telling it to cancel the query. This causes the original connection to receive an error from postgres in the same way that it would be if the statement_timeout is set on the server. The way the error surfaces back to the app is almost exactly the same in both cases, so using a context won't make it easier for the application to handle the situation.

It's also worth highlighting that the timeout requires that second connection to postgres to manually cancel the query.

Example using context timeout:

&pq.Error{Severity:"ERROR", Code:"57014", Message:"canceling statement due to user request", Detail:"", Hint:"", Position:"", InternalPosition:"", InternalQuery:"", Where:"", Schema:"", Table:"", Column:"", DataTypeName:"", Constraint:"", File:"postgres.c", Line:"3124", Routine:"ProcessInterrupts"}

Example using statement timeout:

&pq.Error{Severity:"ERROR", Code:"57014", Message:"canceling statement due to statement timeout", Detail:"", Hint:"", Position:"", InternalPosition:"", InternalQuery:"", Where:"", Schema:"", Table:"", Column:"", DataTypeName:"", Constraint:"", File:"postgres.c", Line:"3094", Routine:"ProcessInterrupts"}

However, using a context does give us the ability to control the timeout independently for each query. If we need that then using the context seems like the right way to go but we should be aware of the tradeoff above.

We could also investigate setting the statement_timeout locally inside a postgres transaction, but I don't think it is easy to do that with pq.

I think we should evaluate all three approaches:

  • Setting a timeout on the context.
  • Setting a local statement_timeout inside the database transaction that applies just to that database transaction.
  • Setting a global statement_timeout on the database.

@bartekn
Copy link
Contributor Author

bartekn commented Nov 19, 2019

return an error that is unexpected for an application.

What I was trying to say is that when a developer passes ctx with deadline he/she expects that the returned error can be canceling statement... so can ex. log it with WARN level instead of ERROR level. It's especially useful for open-source apps where app has no control over DB configuration (other than setting it per session).

The other advantage is that all future apps that use db.Session will be able to kill long running queries without having to change default DB configuration.

However, using a context does give us the ability to control the timeout independently for each query.

It's also possible that this depends on the config value. Ex. Horizon admin can define timeout period for requests (ex. to make sure the user receives the response before LB idle timeout): --connection-timeout. In this case we can set DB query timeout to connection timeout minus 2 sec.

@bartekn
Copy link
Contributor Author

bartekn commented Nov 21, 2019

Realized that Session.Ctx is there but it's only used in BeginTx.

@bartekn
Copy link
Contributor Author

bartekn commented Dec 2, 2019

Closed in #1977.

@bartekn bartekn closed this as completed Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants