-
Notifications
You must be signed in to change notification settings - Fork 64
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
Improve query runner error handling #642
Conversation
Co-authored-by: Keiko Oda <[email protected]>
@@ -94,7 +95,7 @@ func run(ctx context.Context, server *state.Server, collectionOpts state.Collect | |||
db.ExecContext(ctx, postgres.QueryMarkerSQL+"BEGIN READ ONLY") | |||
|
|||
err = db.QueryRowContext(ctx, comment+prefix+query.QueryText).Scan(&result) | |||
firstErr := err | |||
firstErr = err | |||
|
|||
// Run EXPLAIN ANALYZE a second time to get a warm cache result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #641 (comment), you mentioned that this is executed even if the firstErr is not nil for the case of first one failed with a timeout. However, even though the cause was a timeout, the second one will fail regardless:
pgaweb=# set statement_timeout = 1000;
SET
pgaweb=# begin read only;
BEGIN
pgaweb=*# select pg_sleep(2);
ERROR: canceling statement due to statement timeout
pgaweb=!# select pg_sleep(0.5);
ERROR: current transaction is aborted, commands ignored until end of transaction block
Meaning this code doesn't quite do what you want it to do. When the firstErr is not nil (for whatever the reason), this second query execution will always be ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Maybe the query text should contain BEGIN READ ONLY
and COMMIT
so each query is in its own transaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, confirmed that it's reporting the correct error message 👍
This PR updates the query runner to submit the first error it sees, to avoid reporting an unhelpful
current transaction is aborted
error back to the server.BEGIN READ ONLY
was a late addition to #628, so this wasn't caught in manual testing.This also fixes an error I saw while testing this, where
server.QueryRuns
is nil: