fix: Flush pool as late as possible during schema cache reloading by mkleczek · Pull Request #4645 · PostgREST/postgrest

mkleczek · 2026-02-11T13:11:26Z

DISCLAIMER:
This commit was authored entirely by a human without the assistance of LLMs.

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).

steve-chavez · 2026-02-12T21:36:05Z

The change seems reasonable. Is there a way to test the previous behavior and that this is an improvement?

I'm thinking we have the schemaCacheLoads metric but perhaps we should also have one metric for pool flushes? Then we might be able to test that this change reduces the flushes.

postgrest/src/PostgREST/Metrics.hs

Lines 22 to 33 in 99984d3

    
           data MetricsState = 
        
             MetricsState { 
        
               poolTimeouts         :: Counter, 
        
               poolAvailable        :: Gauge, 
        
               poolWaiting          :: Gauge, 
        
               poolMaxSize          :: Gauge, 
        
               schemaCacheLoads     :: Vector Label1 Counter, 
        
               schemaCacheQueryTime :: Gauge, 
        
               jwtCacheRequests     :: Counter, 
        
               jwtCacheHits         :: Counter, 
        
               jwtCacheEvictions    :: Counter 
        
             }

Related, just noted these two functions do the same perhaps we should deduplicate:

postgrest/src/PostgREST/AppState.hs

Lines 271 to 278 in 99984d3

    
           -- | Flush the connection pool so that any future use of the pool will 
        
           -- use connections freshly established after this call. 
        
           flushPool :: AppState -> IO () 
        
           flushPool AppState{..} = SQL.release statePool 
        
           -- | Destroy the pool on shutdown. 
        
           destroyPool :: AppState -> IO () 
        
           destroyPool AppState{..} = SQL.release statePool

mkleczek · 2026-02-15T19:30:26Z

The change seems reasonable. Is there a way to test the previous behavior and that this is an improvement?

I'm thinking we have the schemaCacheLoads metric but perhaps we should also have one metric for pool flushes? Then we might be able to test that this change reduces the flushes.

Done:

Added pgrst_db_pool_flushes_total metric.
Added HSpec module dedicated to metrics tests and a test to verify the new metric (I think it is a good idea to have one - we already have JWT cache spec tests that make use of metric state)
Added io tests to validate pool flushing behavior (both in normal conditions and with schema loading retries)

Related, just noted these two functions do the same perhaps we should deduplicate:

After latest changes they are no longer duplicate as destroyPool does not emit observations.

steve-chavez · 2026-02-17T02:13:55Z

Added pgrst_db_pool_flushes_total metric.
Added HSpec module dedicated to metrics tests and a test to verify the new metric (I think it is a good idea to have one - we already have JWT cache spec tests that make use of metric state)
Added io tests to validate pool flushing behavior (both in normal conditions and with schema loading retries)

@mkleczek Could you split those commits into another PR? 🙏 (without the fix here) That would make it easier to review.

mkleczek · 2026-02-17T13:16:27Z

Added pgrst_db_pool_flushes_total metric.
Added HSpec module dedicated to metrics tests and a test to verify the new metric (I think it is a good idea to have one - we already have JWT cache spec tests that make use of metric state)
Added io tests to validate pool flushing behavior (both in normal conditions and with schema loading retries)

@mkleczek Could you split those commits into another PR? 🙏 (without the fix here) That would make it easier to review.

@steve-chavez Done. Stacked this PR on top of #4658

mkleczek · 2026-02-25T05:54:03Z

@mkleczek Could you split those commits into another PR? 🙏 (without the fix here) That would make it easier to review.

@steve-chavez Done. Stacked this PR on top of #4658

@steve-chavez Rebased on #4668

steve-chavez · 2026-04-09T15:45:54Z

Needs rebasing now

taimoorzaeem · 2026-04-09T17:14:33Z

Is this a bug fix? If so, then it may also need a CHANGELOG.md entry.

steve-chavez · 2026-04-14T15:59:33Z

@mkleczek Could you please address the feedback here?

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).

mkleczek · 2026-04-14T20:02:57Z

@mkleczek Could you please address the feedback here?

Done

steve-chavez · 2026-04-14T21:55:36Z



-  it "Should flush pool multiple times when schema reloading retries" $ do
+  it "Should flush pool once when schema reloading retries" $ do


This is great, clearly proves the fix 👍

Co-authored-by: Steve Chavez <stevechavezast@gmail.com>

…stgREST#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).

taimoorzaeem · 2026-04-15T03:42:39Z

If we decide to backport this, we'd need to first backport 5d4f82d from #4668 (already applied ad90721).

steve-chavez · 2026-04-15T17:49:40Z

@taimoorzaeem Yes, those should be fine to backport. This one too.

…stgREST#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).

) 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).

mkleczek force-pushed the flush-pool-after-schema-cache-load branch 3 times, most recently from d5a6f71 to 7221241 Compare February 11, 2026 19:50

mkleczek force-pushed the flush-pool-after-schema-cache-load branch 2 times, most recently from a5f9ece to f9ea218 Compare February 15, 2026 21:13

mkleczek force-pushed the flush-pool-after-schema-cache-load branch from f9ea218 to 24e71b5 Compare February 17, 2026 13:14

mkleczek force-pushed the flush-pool-after-schema-cache-load branch 3 times, most recently from b428649 to 3c033ab Compare February 17, 2026 20:28

steve-chavez mentioned this pull request Feb 17, 2026

add(metrics): expose db pool flushes counter #4658

Closed

mkleczek force-pushed the flush-pool-after-schema-cache-load branch 2 times, most recently from e601d59 to 16aedf7 Compare February 25, 2026 05:51

mkleczek force-pushed the flush-pool-after-schema-cache-load branch from 16aedf7 to 0a348f7 Compare February 25, 2026 18:54

mkleczek mentioned this pull request Feb 25, 2026

refactor(test): provide means to validate metrics and observations #4671

Merged

mkleczek force-pushed the flush-pool-after-schema-cache-load branch 5 times, most recently from 2e23441 to 3e7308f Compare March 6, 2026 10:30

mkleczek force-pushed the flush-pool-after-schema-cache-load branch 4 times, most recently from e82492b to ba16275 Compare March 13, 2026 06:49

mkleczek force-pushed the flush-pool-after-schema-cache-load branch 2 times, most recently from 39e391c to ad7ecd4 Compare March 31, 2026 06:55

taimoorzaeem mentioned this pull request Apr 2, 2026

v14: refactor(test): provide means to validate metrics and observations #4773

Merged

mkleczek force-pushed the flush-pool-after-schema-cache-load branch 8 times, most recently from dc113cf to b24d652 Compare April 9, 2026 12:47

mkleczek force-pushed the flush-pool-after-schema-cache-load branch from b24d652 to 9130429 Compare April 9, 2026 16:36

mkleczek force-pushed the flush-pool-after-schema-cache-load branch 2 times, most recently from ec7d1b6 to 0367a9c Compare April 14, 2026 07:02

mkleczek force-pushed the flush-pool-after-schema-cache-load branch from 0367a9c to a836402 Compare April 14, 2026 20:02

mkleczek force-pushed the flush-pool-after-schema-cache-load branch from a836402 to 9271907 Compare April 14, 2026 20:02

mkleczek requested a review from taimoorzaeem April 14, 2026 20:03

steve-chavez approved these changes Apr 14, 2026

View reviewed changes

Comment thread src/PostgREST/AppState.hs

Comment thread CHANGELOG.md Outdated

steve-chavez reviewed Apr 14, 2026

View reviewed changes

chore: add comment and clear changelog

817eb8f

Co-authored-by: Steve Chavez <stevechavezast@gmail.com>

steve-chavez merged commit aca58c8 into PostgREST:main Apr 14, 2026
32 checks passed

taimoorzaeem mentioned this pull request Apr 16, 2026

v14: fix: Flush pool as late as possible during schema cache reloading #4820

Merged



		it "Should flush pool multiple times when schema reloading retries" $ do
		it "Should flush pool once when schema reloading retries" $ do

Uh oh!

Conversation

mkleczek commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steve-chavez commented Feb 12, 2026

Uh oh!

mkleczek commented Feb 15, 2026

Uh oh!

steve-chavez commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkleczek commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkleczek commented Feb 25, 2026

Uh oh!

steve-chavez commented Apr 9, 2026

Uh oh!

taimoorzaeem commented Apr 9, 2026

Uh oh!

steve-chavez commented Apr 14, 2026

Uh oh!

mkleczek commented Apr 14, 2026

Uh oh!

Uh oh!

Uh oh!

steve-chavez Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

taimoorzaeem commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steve-chavez commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

mkleczek commented Feb 11, 2026 •

edited

Loading

steve-chavez commented Feb 17, 2026 •

edited

Loading

mkleczek commented Feb 17, 2026 •

edited

Loading

taimoorzaeem commented Apr 15, 2026 •

edited

Loading