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

Add Row.Err method #950

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add Row.Err method #950

wants to merge 1 commit into from

Conversation

eandre
Copy link

@eandre eandre commented Mar 1, 2021

Expose Rows.Err as a method on Row, to allow for easily checking
whether the query returned an error without calling Scan.

@eandre
Copy link
Author

eandre commented Mar 1, 2021

I realize this is a somewhat breaking change since Row is an interface (though the doc comment explicitly allows for adding methods). If this is deemed too breaking, it would be great to add the method without adding it to the interface so consumers can define their own interface and convert connRow to it.

Expose Rows.Err as a method on Row, to allow for easily checking
whether the query returned an error without calling Scan.
@jackc
Copy link
Owner

jackc commented Mar 5, 2021

Expose Rows.Err as a method on Row, to allow for easily checking whether the query returned an error without calling Scan.

What is the reason for this? Part of the advantage of QueryRow(...).Scan(...) is that it only needs one error check.

@eandre
Copy link
Author

eandre commented Mar 5, 2021

My use case is for a library that provides instrumentation. It wraps the pgx.Row object and collects information on what query was run, how long it took, and whether or not it succeeded.

I'd like to collect this information as soon as the query completes (to accurately compute duration) and even if the client doesn't call.Scan immediately. That's best achieved by checking the error as soon as the query completes,before the client calls .Scan.

I previously used database/sql (which provides an .Err method) for this but recently migrated to pgx and this was one of the few snags. I worked around it by using Query directly and creating my own wrapper equivalent to pgxRow. But it would be nice to not have to do that.

@jackc
Copy link
Owner

jackc commented Mar 6, 2021

I'd like to collect this information as soon as the query completes (to accurately compute duration) and even if the client doesn't call.Scan immediately. That's best achieved by checking the error as soon as the query completes,before the client calls .Scan.

There's no guarantee that will do what you want (though it usually will). Query (which QueryRow is built on) does not return when the query is complete. It returns when the PG server sends back a RowDescription message. The query may still be running on the server. The query may even still fail part way through returning result rows.

It so happens that what you are trying to do will usually work because PG tries to bundle messages together for syscall and network efficiency and pgx reads up to the RowDescription message before returning from Query. But that is relying on implementation details of PostgreSQL and pgx.

The only way to know for sure that a query is complete and did not have an error is close Rows explicitly or via Scan until Scan returns false. Then check Rows.Err.

previously used database/sql (which provides an .Err method) for this

This same limitation would apply when using pgx as a database/sql driver. It will probably do what you want, but no guarantee.

@eandre
Copy link
Author

eandre commented Mar 6, 2021

That's good feedback, thanks. In practice it's more intended for measuring for how long the application goroutine is blocked on reading from the database, and not the actual query time, so it's probably good enough for now, but it's worth considering the approach and doing something more sophisticated.

It still seems useful to me for a library to check for an error without calling .Scan, even if it's best-effort. But you obviously have much more experience with the nuances of Postgres and pgx, so do as you see best fit :)

@eandre
Copy link
Author

eandre commented Aug 21, 2021

Hey @jackc, thoughts on this? Would be great to get it in if possible.

@jackc
Copy link
Owner

jackc commented Aug 21, 2021

As is, the only error this could detect would would be sending the query and that is all the time it would measure as well. I think both Err() and timing the QueryRow call would be very misleading.

For these to be accurate, QueryRow would need to block until it read the first row.

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

Successfully merging this pull request may close these issues.

2 participants