Make database heartbeat more robust#21734
Conversation
mvdbeek
changed the title
Only check session.connection().invalidated when there's an existing transaction. This avoids checking out a new connection from the pool that would never be returned, causing "too many clients" errors during integration tests. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| session = session() | ||
| trans = session.get_transaction() | ||
| if (trans and not trans.is_active) or session.connection().invalidated: | ||
| if trans and (not trans.is_active or session.connection().invalidated): |
There was a problem hiding this comment.
@jdavcs can you sanity check this ? the integration tests were causing postgres to run out of connections prior to this change, supposedly because session.connection() checking out a connection and then never close it, so now we only check if there's an existing transaction.
| session = session() | ||
| trans = session.get_transaction() | ||
| if (trans and not trans.is_active) or session.connection().invalidated: | ||
| if trans and (not trans.is_active or session.connection().invalidated): |
There was a problem hiding this comment.
I don't think this affects anything: if condition satisifes, we only do session.rollback(), and accoriding to the docs, if no transaction is in progress, this method is a pass-through.(ref + ref). So in the current version (before this change), it would either rollback an existing transaction or do nothing, after the change it will do exactly the same thing.
I didn't find any mention of a connection-related side effect in case of a noop rollback, but I didn't dig too deep. That said, the edit doesn't do any harm, so if you think it's worth a try, that's fine.
There was a problem hiding this comment.
Isn't this what happens:
if session.connection().invalidated and invalidated is False we don't enter the branch but we have created a new connection that never gets cleaned up ?
Why else would this fix the integration tests ?
There was a problem hiding this comment.
Also see the commit message for a different phrasing in case that's not clear ?
There was a problem hiding this comment.
Ooooh! You are right! We create a connection in the if condition test - in order to test whether it's invalidated! Because, as per docs, "Either the Connection corresponding to the current transaction is returned, or if no transaction is in progress, a new one is begun and the Connection returned". Yikes. This was subtle. And pretty terrible. Thanks!
jdavcs
left a comment
jdavcs
left a comment
There was a problem hiding this comment.
looks fine (see inline comment)
|
This PR was merged without a "kind/" label, please correct. |
nsoranzo
deleted the
make_database_heartbeat_survive_lost_database_connection
branch
mvdbeek
changed the title
4 participants
Should fix #21713
How to test the changes?
(Select all options that apply)
License