fix: Flush pool as late as possible during schema cache reloading

[mkleczek]

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]

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]

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]

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]

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]

@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