Rewrite NpgsqlDatabaseCreator.Exists() to do SELECT 1 #3648
+79
−73
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#3646 reported a memory leak: when NpgsqlDatabaseCreateor.Exists() is called (from DatabaseFacadeExtensions.CanConnect()), it was using NpgsqlConnection.CloneWith() to create a new, unpooled connection, which it then tried to connect to the database. The problem is, the cloned connection doesn't use an explicit data source, meaning that a data source is internally created inside Npgsql, referenced by the internal static dictionary. If the password continuously changes (rotating auth tokens), that led to increasing memory use which never gets freed.
This is ultimately npgsql/npgsql#3387: we should be able to call ClearPool() on the cloned connection to have it unreference the internal data source, allowing it to get garbage collected - though in theory, a real user in the application might be using the same connection string, which EF would then cause to get cleared; while quite contrived, it shows how bad the legacy connection mechanism is.
This PR simply changes to sending
SELECT 1to the database, which is what the SQL Server provider does as well. This does have the drawback of returning "exists" (and also "can connect") even when e.g. the password has changed (because there are pooled connections which allow executingSELECT 1), but that's seems like an unimportant edge case (the same pooled connections will be there for actual usage etc.). And the CloneWith() mechanism here has been the source of lots of trouble over the years, without adding much real value.This also adds the execution strategy to Exists(), which allows retrying on transient errors.
Fixes #3646
Thanks @vonzshik for the initial investigation.