From 7c74f6cf0a0ed5dd9c41d8accd0627c30621926d Mon Sep 17 00:00:00 2001 From: Steve Chavez Date: Thu, 1 Aug 2024 10:37:48 -0500 Subject: [PATCH] fix: schema cache loading before the in-db config (#3670) Fixes #3660. Load the config after getting the pg version but before loading the schema. The regression happened on f09655b. Also remove schema cache load wrapper and separate db queries in different functions. Co-authored-by: Laurence Isla --- src/PostgREST/AppState.hs | 22 ++++++------ ...est_schema_cache_snapshot[dbRoutines].yaml | 34 +++++++++++++++++++ test/io/fixtures.sql | 17 ++++++++++ test/io/test_io.py | 17 ++++++++++ 4 files changed, 79 insertions(+), 11 deletions(-) diff --git a/src/PostgREST/AppState.hs b/src/PostgREST/AppState.hs index fe8d8b7ee..45dd2669a 100644 --- a/src/PostgREST/AppState.hs +++ b/src/PostgREST/AppState.hs @@ -161,7 +161,7 @@ initWithPool (sock, adminSock) pool conf loggerState metricsState observer = do deb <- let decisecond = 100000 in mkDebounce defaultDebounceSettings - { debounceAction = internalSchemaCacheLoad appState + { debounceAction = retryingSchemaCacheLoad appState , debounceFreq = decisecond , debounceEdge = leadingEdge -- runs the worker at the start and the end } @@ -355,13 +355,6 @@ putSCacheStatus = atomicWriteIORef . stateSCacheStatus getObserver :: AppState -> ObservationHandler getObserver = stateObserver -internalSchemaCacheLoad :: AppState -> IO () -internalSchemaCacheLoad appState = do - AppConfig{..} <- getConfig appState - void $ retryingSchemaCacheLoad appState - -- We cannot retry reading the in-db config after it fails immediately, because it could have user errors. We just report the error and continue. - when configDbConfig $ readInDbConfig False appState - -- | Try to load the schema cache and retry if it fails. -- -- This is done by repeatedly: 1) flushing the pool, 2) querying the version and validating that the postgres version is supported by us, and 3) loading the schema cache. @@ -369,16 +362,17 @@ internalSchemaCacheLoad appState = do -- -- + Because connections cache the pg catalog(see #2620) -- + For rapid recovery. Otherwise, the pool idle or lifetime timeout would have to be reached for new healthy connections to be acquired. -retryingSchemaCacheLoad :: AppState -> IO (Maybe PgVersion, Maybe SchemaCache) +retryingSchemaCacheLoad :: AppState -> IO () retryingSchemaCacheLoad appState@AppState{stateObserver=observer, stateMainThreadId=mainThreadId} = - retrying retryPolicy shouldRetry (\RetryStatus{rsIterNumber, rsPreviousDelay} -> do + void $ retrying retryPolicy shouldRetry (\RetryStatus{rsIterNumber, rsPreviousDelay} -> do when (rsIterNumber > 0) $ do let delay = fromMaybe 0 rsPreviousDelay `div` oneSecondInUs observer $ ConnectionRetryObs delay putNextListenerDelay appState delay flushPool appState - (,) <$> qPgVersion <*> qSchemaCache + + (,) <$> qPgVersion <*> (qInDbConfig *> qSchemaCache) ) where qPgVersion :: IO (Maybe PgVersion) @@ -400,6 +394,11 @@ retryingSchemaCacheLoad appState@AppState{stateObserver=observer, stateMainThrea putPgVersion appState actualPgVersion return $ Just actualPgVersion + qInDbConfig :: IO () + qInDbConfig = do + AppConfig{..} <- getConfig appState + when configDbConfig $ readInDbConfig False appState + qSchemaCache :: IO (Maybe SchemaCache) qSchemaCache = do conf@AppConfig{..} <- getConfig appState @@ -438,6 +437,7 @@ retryingSchemaCacheLoad appState@AppState{stateObserver=observer, stateMainThrea oneSecondInUs = 1000000 -- one second in microseconds -- | Reads the in-db config and reads the config file again +-- | We don't retry reading the in-db config after it fails immediately, because it could have user errors. We just report the error and continue. readInDbConfig :: Bool -> AppState -> IO () readInDbConfig startingUp appState@AppState{stateObserver=observer} = do AppConfig{..} <- getConfig appState diff --git a/test/io/__snapshots__/test_cli/test_schema_cache_snapshot[dbRoutines].yaml b/test/io/__snapshots__/test_cli/test_schema_cache_snapshot[dbRoutines].yaml index 8c46a3393..89262c2e8 100644 --- a/test/io/__snapshots__/test_cli/test_schema_cache_snapshot[dbRoutines].yaml +++ b/test/io/__snapshots__/test_cli/test_schema_cache_snapshot[dbRoutines].yaml @@ -218,6 +218,40 @@ pdSchema: public pdVolatility: Volatile +- - qiName: reset_db_schemas_config + qiSchema: public + - - pdDescription: null + pdFuncSettings: [] + pdHasVariadic: false + pdName: reset_db_schemas_config + pdParams: [] + pdReturnType: + contents: + contents: + qiName: void + qiSchema: pg_catalog + tag: Scalar + tag: Single + pdSchema: public + pdVolatility: Volatile + +- - qiName: change_db_schemas_config + qiSchema: public + - - pdDescription: null + pdFuncSettings: [] + pdHasVariadic: false + pdName: change_db_schemas_config + pdParams: [] + pdReturnType: + contents: + contents: + qiName: void + qiSchema: pg_catalog + tag: Scalar + tag: Single + pdSchema: public + pdVolatility: Volatile + - - qiName: change_db_schema_and_full_reload qiSchema: public - - pdDescription: null diff --git a/test/io/fixtures.sql b/test/io/fixtures.sql index 53446adee..7c8658cd3 100644 --- a/test/io/fixtures.sql +++ b/test/io/fixtures.sql @@ -33,6 +33,9 @@ GRANT CREATE SCHEMA v1; GRANT USAGE ON SCHEMA v1 TO postgrest_test_anonymous; +CREATE SCHEMA test; +GRANT USAGE ON SCHEMA test TO postgrest_test_anonymous; + CREATE TABLE authors_only (); GRANT SELECT ON authors_only TO postgrest_test_author; @@ -226,3 +229,17 @@ $$ language sql; create function get_statement_timeout(items) returns text as $$ select current_setting('statement_timeout', true) as statement_timeout $$ language sql; + +create function change_db_schemas_config() returns void as $_$ +begin + alter role postgrest_test_authenticator set pgrst.db_schemas = 'test'; +end $_$ volatile security definer language plpgsql; + +create function reset_db_schemas_config() returns void as $_$ +begin + alter role postgrest_test_authenticator reset pgrst.db_schemas; +end $_$ volatile security definer language plpgsql ; + +create function test.get_current_schema() returns text as $$ + select current_schema()::text; +$$ language sql; diff --git a/test/io/test_io.py b/test/io/test_io.py index b9130472e..610fba78c 100644 --- a/test/io/test_io.py +++ b/test/io/test_io.py @@ -1618,3 +1618,20 @@ def test_admin_metrics(defaultenv): assert "pgrst_db_pool_waiting" in response.text assert "pgrst_db_pool_available" in response.text assert "pgrst_db_pool_timeouts_total" in response.text + + +def test_schema_cache_startup_load_with_in_db_config(defaultenv, metapostgrest): + "verify that the Schema Cache loads correctly at startup, using the in-db `pgrst.db_schemas` config" + + response = metapostgrest.session.post("/rpc/change_db_schemas_config") + assert response.text == "" + assert response.status_code == 204 + + with run(env=defaultenv) as postgrest: + response = postgrest.session.get("/rpc/get_current_schema") + assert response.text == '"test"' + assert response.status_code == 200 + + response = metapostgrest.session.post("/rpc/reset_db_schemas_config") + assert response.text == "" + assert response.status_code == 204