-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Clear db connection after database error #3344
Clear db connection after database error #3344
Conversation
The build is failed because of this. I'm not seeing any clippy errors in local but will make changes as per the build log. |
b973a6e
to
9f701e1
Compare
@weiznich There is a table definition in the connection.rs file which could be moved to the relevant test method. I don't see a reason to keep it outside. The test I added encloses table definition within it which is inspired from other test cases, example. @seeekr has made a suggestion that we can move that table definition to the relevant test method (and other similar pattern in the codebase, if any) using following changeset to keep the changes minimal, fn managing_updated_at_for_table() {
table! {
#[sql_name = "auto_time"]
auto_time_table {
id -> Integer,
n -> Integer,
updated_at -> Timestamp,
}
}
use auto_time_table::dsl::{auto_time_table as auto_time, n, updated_at};
... Initially, I made changes to enclose that table definition, different from this, but it was cluttering the actual changes. I have reverted it to push only changes relevant to the ticket. Let me know your thoughts about it, whether we want to include such changes in current PR or in a separate one. |
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.
Thanks for filling this PR 👍
This looks already good, but I feel that we should act a bit more careful here in terms of error propagation.
diesel/src/pg/connection/result.rs
Outdated
// this will indicate that the previous command is complete and | ||
// the same connection is ready to process next command. | ||
// https://www.postgresql.org/docs/current/libpq-async.html | ||
while conn.get_next_result()?.is_some() {} |
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.
We want to return the orginal error here, so we want to avoid using ?
at all. Something like
while conn.get_next_result()?.is_some() {} | |
while conn.get_next_result().map(Option::is_some).unwrap_or(true) {} |
(although this will recursively call the same function (PgResult::new
) again and potentially run this loop there again. I believe that should be fine as this should stop at some layer wen get_next_result()
returns None
)
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.
@weiznich Is the default value true
intended? Wouldn't it keep the loop running even after some error? Am I missing something?
This is fine if even after error only if we know for sure that subsequent get_next_result
calls will definitely return None
at some point. I think you are also saying the same thing in the brackets, right?
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.
Yes true
is the correct default value as far as I can tell. The libpq documentation states that PQgetResult
(which is called internally by this function) must be called till it returns null
. It does not talk about any errors there, so I assume that means we literally need to call it again till it returns null.
That by itself seems reasonable as get_next_result()
calls PQgetResult
(which cannot return an error) and then PgResult::new()
(which can return an error). So the only way get_next_result()
would ever return an error is by hitting basically the current code block. We would like to ignore that as the first error message is relevant.
I would be fine with having these changes in this PR, as long as they are in a different commit |
9f701e1
to
3ee3619
Compare
Added separate commit to move table definition to its relevant test method. I haven't looked into all the tests file because of the time crunch but updated the same file where I added the test for the ticket. Also, updated the polling code as per your suggestion. Thanks, @weiznich. |
Thanks again for providing the example + working on the fix 👍 I will try to fix #3330 as well and then put all the recent fixes into a 2.0.1 patch release. I cannot give an ETA on the exact release date. |
Closes #3342