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

Context support #786

Open
GeertJohan opened this issue Sep 7, 2018 · 13 comments
Open

Context support #786

GeertJohan opened this issue Sep 7, 2018 · 13 comments

Comments

@GeertJohan
Copy link

GeertJohan commented Sep 7, 2018

I'd like for pq to support the *Context calls.

In one of my projects I started using github.com/basvanbeek/ocsql. It wraps the driver and adds OpenCensus tracing. This works great, even when wrapping the sql.DB with sqlx for extra functionality. However, it requires the driver to support Context calls as defined in the stdlib (BeginTx, ExecContext, etc.). Therefore, I (temporarily?) switched to [github.com/jackc/pgx/stdlib](https://godoc.org/github.com/jackc/pgx/stdlib

I think that pg should support these functions. https://golang.org/pkg/database/sql/driver

@GeertJohan
Copy link
Author

Slightly related to #667

@Gunni
Copy link

Gunni commented Sep 10, 2018

I'd like to push for support aswell, i had a heck of a time debugging why the database connection has such an insanely long timeout when the database is unresponsive!

The database was also ignoring the timeout context it was being given, and with a default timeout of 42 seconds!!! (in my case a timeout of at MOST 5 second is plenty!!)

@basvanbeek
Copy link

ocsql will work regardless of lib/pq supporting *Context or not. ocsql needs you as consumer to call the *Context methods so it can extract the current SpanContext from context.Context. After that the driver can either support context.Context (as it could provide cancellation/deadline logic) or not at all.

Long story short. Support for *Context would be highly appreciated for timeout but also expensive query cancellation in case upstream no longer cares for the query result.

@maddyblue
Copy link
Collaborator

I'm not aware of a reason to use lib/pq over pgx. It's better supported, more correct, and is faster.

@GeertJohan
Copy link
Author

@basvanbeek For me, ocsql was complaining that it was missing the Context* methods in the sql driver, and could not continue. Perhaps this was a bug or missing feature that has been implemented since I last tried it? I'm now using pgx so it's all fine now.

@mjibson lib/pq recovers when a connection is broken. If I restart my database, the first few calls using pgx fail with connection errors. Not nice. Also: I'm not sure if pgx is faster when used as stdlib driver for database/sql. Afaik the benchmark was done using pgx directly.

@cgilling
Copy link

cgilling commented Dec 2, 2018

I might be missing something, but isn't it implemented here?

func (cn *conn) QueryContext(ctx context.Context, query string, args []driver.NamedValue) (driver.Rows, error) {

@KillerX
Copy link

KillerX commented Dec 3, 2018

@cgilling I was wondering the same thing (we are using *Context pretty much exclusively), but for some reason ocsql looses the information about the parent span. The results appear in the traces (Stackdriver for us) but they are stand alone instead of connected to the relevant parent span.

This happens in the simplest cases, and even when a span is manually created just before the query, like in the example below.

func (db DatabaseHandler) GetStuff(ctx context.Context, someParam string) (*db.Row) {
	ctx, span := trace.StartSpan(ctx, "SomeQuery")
	defer span.End()

	row := db.PreparedQuery.QueryRowContext(ctx, someParam)

	return row
}

In this case the new span will be correctly correlated to the parent span but the query will be its own separate trace.

I have tested pgx as @mjibson suggested, and without changing anything else than the driver, the queries are correctly correlated.

This leads me to believe that there is some issue with how the context is handled, but I was at this point unable to discover what exactly is the issue.

@basvanbeek
Copy link

It seems lib/pq does not support driver.StmtQueryContext and driver.StmtExecContext. It only implements the Context methods on a driver.Conn.

ocsql tries to be transparent to sql/database on which enhancement interfaces are implemented. It hides interfaces not implemented by the underlying driver. In this case this means a userland call to stmt.QueryRowContext in database/sql is sent to driver's stmt.Query thus dropping the context.

Best would be for lib/pq to implement these functions.

@KillerX
Copy link

KillerX commented Dec 3, 2018

@basvanbeek thanks for the explanation. Turns out there is actually a pr pending for exactly this: #768

@montanaflynn
Copy link

Any update on this? The PR is still open.

@Gunni
Copy link

Gunni commented Apr 2, 2019

Does #839 fix this?

@KillerX
Copy link

KillerX commented Apr 3, 2019

@Gunni I don't think so since functions like QueryContext are still missing (at least as far as I can see).

@Gunni
Copy link

Gunni commented Dec 30, 2023

This bug is now 5 years old, starting primary school soon, I wonder if it will ever be fixed...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants