Merged
Conversation
Remove the 'driver' submodule, organize all core connection logic into connections.core. Introduce a ConnectionManager that is responsible for managing connection pools, what owner_uris are associated with them, connections in active transactions, and more.
This creates an "orphaned" connection that is disconnected from a PooledConnection context. Orphaned connections can still be returned to their pool, though this will not be tracked by the ConnectionManager. OE still holding a long-lived connection was due to the complexity of changes to refactor OE to use PooledConnection, specifically in pgsmo and smo. This means for every ConnectionDetail connected to, there will be 2 connections min - one from the pool, and one for OE. This is still an improvement. Will leave a refactor of OE to PooledConnection for future work, if necessary.
This is due to considerations on the query editor being able to set connection-level properties like GUC or temporary tables. These shouldn't be shared amongst query editors, and so the connection for a query editor window is really tied to that editor only.
Not quite working yet
Also remove unused switch database request
This fixes an implementation that held long-lived connections seperate for owner_uris based on the incoming connection_name or type. This was breaking the transfer logic. There should be a one-to-one between a long-lived connection and an owner_uri.
This was correct beforehand, but didn't use the static method that was extracted so that testing would use same logic as normal code flow. Renamed to make more clear it may handle the azure token, but not if there's no token
This change extracts Configuration into it's own file, and makes a note of how to add additional configuration that gets synced to vscode. Adds max_connections to configuration.
mmcfarland
approved these changes
Apr 3, 2025
Member
mmcfarland
left a comment
mmcfarland
left a comment
There was a problem hiding this comment.
Looks good! We paired on a review and I ran through a bunch of manual testing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR modifies the way pgtoolsservices manages connections.
Connection Manager
It introduces a centralized ConnectionManager that handles all connection and disconnection to databases for URIs (files or object explorer nodes).
Pooled Connections
Previously, connections were created and tracked by a URI and a connectiontype. Now, a connection pool is created per connection details. Connection details are considered equivalent based on the hash of their connection parameters. A
PooledConnectionis introduced, which is a context manager to get and return a connection from the connection pools.There are two cases where the connection is pulled out of the pool without a context manager with "get_long_lived_connection" - the ObjectExplorer and query editors.
The ObjectExplorer code is tied into using a specific connection and refactoring was deemed too large a change for this PR. Adding type information to pgsmo and smo, and refactoring to use pooled connections, is left as future work.
A query editors can use pooled connections; however, this is not ideal as it can result in unintended behavior. If the user sets session-specific state, like GUC values or temporary tables, then the connection should persist with that editor and not be used by other usages that may be unintentionally impacted by that session state. Because the number of ways session state can change is basically unbounded, the connections are "stuck" to the query editor for the lifetime of that editor.
Prior to the decision to have all query editor connections be "sticky", there was logic put into the connection manager that stated if a connection was being returned to the connection manager pool that was still in an active transaction, then that connection would be held out of the pool and "stuck" to the owner_uri that requested it. This process in the connection manager was kept in, as it may be useful in the future if there are instances where there might be a transaction opened by a connection associated with an owner_uri and should not be ruturned to the pool until that transaction is completed.
Azure token fetching
This PR also includes a change that enables PGTS to fetch an azure token that it detects is expired. This can happen in the case where a connection pool was created with a valid token at the time, but the token expires and a new connection has to be made from the pool. This required implementing the
connection/fetchAzureTokenhandler in the client, which is done in a separate change for VSCode.