Skip to content

Do not clear pool while establishing connection if we'll retry it#6431

Merged
vonzshik merged 2 commits intomainfrom
6427-establish-connection-retry-clear-pool-fix
Feb 2, 2026
Merged

Do not clear pool while establishing connection if we'll retry it#6431
vonzshik merged 2 commits intomainfrom
6427-establish-connection-retry-clear-pool-fix

Conversation

@vonzshik
Copy link
Contributor

Fixes #6427

@vonzshik vonzshik requested a review from roji as a code owner January 29, 2026 10:56
/// <param name="markHostAsOffline">Whether we treat host as down, even if we're still connecting to PostgreSQL instance.</param>
/// <returns>The exception given in <paramref name="reason"/> for chaining calls.</returns>
internal Exception Break(Exception reason)
internal Exception Break(Exception reason, bool markHostAsOffline = false)
Copy link
Member

@roji roji Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am wondering if we want to not mark as offline by default... I haven't audited all the places where we call Break, but given the current behavior is to always mark as offline, it may be safer to go the other way and say that we mark as offline unless otherwise specified?

But I don't have the full picture and am fine if you think this is the better way.

Copy link
Contributor Author

@vonzshik vonzshik Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it may be safer to go the other way and say that we mark as offline unless otherwise specified?

If I understood you correctly, that's pretty much exactly how it's working right now. The idea is that we do not mark as offline if we're still connecting (connector's state is Connecting) to the host, with the exception if we break with markHostAsOffline = true (which only happens in NpgsqlConnector.Open, after all retries are exhausted).

That means, after we fully established connection, any break will mark as offline (there are other conditions, like being a transient error, but nothing changed in that regard).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, this parameter is only meaningful for the Connecting state (in others we always mark as default in any case).

I'd suggest just tweaking the parameter name to make that clear (I got confused), maybe markHostOfflineOnConnecting or similar...

@vonzshik vonzshik enabled auto-merge (squash) February 2, 2026 10:19
@vonzshik vonzshik merged commit 2328a2c into main Feb 2, 2026
17 checks passed
@vonzshik vonzshik deleted the 6427-establish-connection-retry-clear-pool-fix branch February 2, 2026 10:25
vonzshik added a commit that referenced this pull request Feb 2, 2026
@vonzshik
Copy link
Contributor Author

vonzshik commented Feb 2, 2026

Backported to 10.0.2 via 39eaacf

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.

Rethink pool clear on connection break while establishing connection

2 participants