Skip to content

Commit abd76ca

Browse files
mkleczektaimoorzaeem
authored andcommitted
fix: Flush pool as late as possible during schema cache reloading (#4645)
retryingSchemaCacheLoad flushes the pool upon every retry before it starts reloading the schema. This is too early as schema reloading might take some time during which new connections might be acquired. The consequence is that: * upon successful schema cache reload we might have some connections created with the old schema cache * we close connections upon each retry and under load we will keep closing and re-opening connections until schema cache load succeeds This change is to make sure we flush the pool only after successful schema cache querying but before loading (so that connections acquired during loading wait for it and do not interfere with timing the loading process).
1 parent df87ce4 commit abd76ca

4 files changed

Lines changed: 14 additions & 10 deletions

File tree

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,10 @@ All notable changes to this project will be documented in this file. From versio
88

99
- Log when the pool is released during schema cache reload on `log-level=debug` by @mkleczek in #4668
1010

11+
### Fixed
12+
13+
- Fix unnecessary connection pool flushes during schema cache reloading by @mkleczek in #4645
14+
1115
## [14.9] - 2026-04-10
1216

1317
### Added

src/PostgREST/AppState.hs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -313,8 +313,6 @@ retryingSchemaCacheLoad appState@AppState{stateObserver=observer, stateMainThrea
313313
observer $ ConnectionRetryObs delay
314314
putNextListenerDelay appState delay
315315

316-
flushPool appState
317-
318316
(,) <$> qPgVersion <*> (qInDbConfig *> qSchemaCache)
319317
)
320318
where
@@ -363,6 +361,10 @@ retryingSchemaCacheLoad appState@AppState{stateObserver=observer, stateMainThrea
363361
-- IORef on putSchemaCache. This is why SCacheStatus is put at SCPending here to signal the Admin server (using isPending) that we're on a recovery state.
364362
putSCacheStatus appState SCPending
365363
putSchemaCache appState $ Just sCache
364+
-- Flush the pool after loading the schema cache to reset any stale session cache entries
365+
-- We do it after successfully querying the schema cache (because this can fail and during retries we would flush the pool repeatedly unnecessarily)
366+
-- and after marking sCacheStatus as pending,
367+
flushPool appState
366368
observer $ SchemaCacheQueriedObs resultTime
367369
(t, _) <- timeItT $ observer $ SchemaCacheSummaryObs $ showSummary sCache
368370
observer $ SchemaCacheLoadedObs t

test/io/test_io.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -692,7 +692,7 @@ def test_log_level(level, defaultenv):
692692
response = postgrest.session.get("/")
693693
assert response.status_code == 200
694694

695-
output = postgrest.read_stdout(nlines=7)
695+
output = postgrest.read_stdout(nlines=9)
696696

697697
if level == "crit":
698698
assert len(output) == 0
@@ -730,7 +730,7 @@ def test_log_level(level, defaultenv):
730730
r'- - postgrest_test_anonymous \[.+\] "GET / HTTP/1.1" 200 \d+ "" "python-requests/.+"',
731731
],
732732
)
733-
assert len(output) == 7
733+
assert len(output) == 9
734734
assert any("Connection" and "is available" in line for line in output)
735735
assert any("Connection" and "is used" in line for line in output)
736736

@@ -1367,7 +1367,7 @@ def test_db_error_logging_to_stderr(level, defaultenv, metapostgrest):
13671367
assert response.status_code == 500
13681368

13691369
# ensure the message appears on the logs
1370-
output = postgrest.read_stdout(nlines=6)
1370+
output = postgrest.read_stdout(nlines=8)
13711371

13721372
if level == "crit":
13731373
assert len(output) == 0
@@ -1584,7 +1584,7 @@ def test_log_pool_req_observation(level, defaultenv):
15841584

15851585
if level == "debug":
15861586
output = postgrest.read_stdout(nlines=7)
1587-
assert len(output) == 6
1587+
assert len(output) == 7
15881588
match_log(output, [pool_req, pool_req_fullfill])
15891589
elif level == "info":
15901590
output = postgrest.read_stdout(nlines=4)

test/observability/Observation/SchemaCacheSpec.hs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ spec = describe "Server started with metrics enabled" $ do
2727
waitFor (1 * sec) "SchemaCacheLoadedObs" $ \x -> [ o | o@SchemaCacheLoadedObs{} <- pure x ]
2828

2929

30-
it "Should flush pool multiple times when schema reloading retries" $ do
30+
it "Should flush pool once when schema reloading retries" $ do
3131
SpecState{specAppState = appState, specObsChan} <- getState
3232
let waitFor = waitForObs specObsChan
3333

@@ -36,16 +36,14 @@ spec = describe "Server started with metrics enabled" $ do
3636
AppState.putConfig appState $ cfg { configDbSchemas = pure "bad_schema" }
3737
AppState.schemaCacheLoader appState
3838

39-
waitFor (1 * sec) "PoolFlushed 1" $ \x -> [ o | o@PoolFlushed <- pure x ]
4039
waitFor (1 * sec) "SchemaCacheErrorObs" $ \x -> [ o | o@SchemaCacheErrorObs{} <- pure x ]
4140

4241
-- Restore configuration
4342
AppState.putConfig appState cfg
4443

4544
-- Wait for 2 seconds so that retry can happen
46-
waitFor (2 * sec) "PoolFlushed 2" $ \x -> [ o | o@PoolFlushed <- pure x ]
45+
waitFor (2 * sec) "PoolFlushed" $ \x -> [ o | o@PoolFlushed <- pure x ]
4746
waitFor (1 * sec) "SchemaCacheQueriedObs" $ \x -> [ o | o@SchemaCacheQueriedObs{} <- pure x ]
4847
waitFor (1 * sec) "SchemaCacheLoadedObs" $ \x -> [ o | o@SchemaCacheLoadedObs{} <- pure x ]
49-
5048
where
5149
sec = 1000000

0 commit comments

Comments
 (0)