Repository navigation
Conversation
edjubert
reviewed
Oct 28, 2026
| // Callers for the same connection wait for one load instead of starting their own. | ||
| let mut snapshot = slot.snapshot.lock().unwrap(); | ||
| if let Some(cache) = snapshot.as_ref() { | ||
| let mut state = slot.state.lock().unwrap(); |
Contributor
There was a problem hiding this comment.
The cached failure survives connection-option changes that preserve host/port/username/database. I reproduced this by loading under a restricted startup role, then replacing the pool with working options: the replacement pool can read pg_proc, but load still returns the previous permission error. Explicit cache invalidation immediately restores loading.
ConnectionManager recreates pools when settings_fingerprint changes, whereas this failure remains attached only to ConnectionKey, potentially delaying recovery by up to 60 seconds.
Maybe we could invalidate the cached failure when the effective connection options change, and add a regression test for this case.
This branch has not been deployed
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
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.
Stacked on #945.
Since #903, only connection failures put a database into the reconnect backoff, so that a statement that fails on a working connection doesn't take the database offline. The flip side, noted in #903's risks: when the schema snapshot query itself fails on a working connection, for example because the role may not read part of the catalog, nothing throttles it. Every request that needs the schema (diagnostics, hover, completions, inlay hints, semantic tokens) ran the snapshot queries again, only for them to fail the same way.
What changed
Backofftype (server/backoff.rs) holds the schedule thatConnectionManageralready used: 5 seconds after the first failure, doubling with every failure in a row, up to a minute.ConnectionManageruses it instead of its ownattempts/next_retry_atfields; its behaviour doesn't change.SnapshotManager::loadreturns that error right away instead of querying again; after it, the next request retries, and a success clears it. Connection failures are still left toConnectionManager, as before, so they aren't backed off twice.pgls.invalidateSchemaCache(which clears all snapshots) or connecting as another role or to another database retries right away, so a user who fixed the permission doesn't have to wait.Requests during the backoff still get the error, not an empty result, so hover keeps saying why the schema is missing.
Tests
snapshot_manager::tests::failing_snapshot_queries_are_backed_off: a connection as a role that may not readpg_catalog.pg_procloads the snapshot and getspermission denied. The test then grants the permission; the next load still returns the error without querying, and after the backoff expires the load succeeds and clears the failure. Without the backoff, the second load succeeds and the test fails.backoff::tests::doubles_up_to_a_minutecovers the schedule. The existingconnection_managertests pass unchanged apart from building aBackoff.Not covered
Update: The test no longer creates a login role, which outlived the test's database because roles belong to the cluster and showed up in other tests' role completions. Its connections run as
test_loginfrom the test migrations instead (-c role=test_loginas a startup option); the revoke only affects the test's own database.Update: the remembered failure belongs to the connection options it happened with. The connection manager creates a new pool when settings that
ConnectionKeydoesn't cover change (password, connection string parameters such as a startuprole), and a load with that pool retries right away instead of returning the old error until the backoff ends. Test:failing_snapshot_queries_are_retried_with_other_options(a pool withrole=test_loginfails, a pool for the same host, user and database without it loads right away; fails before).