From 57fa2719ddade4c25aa12acea355ca6ee93a466e Mon Sep 17 00:00:00 2001 From: Taimoor Zaeem Date: Thu, 24 Aug 2023 07:08:04 +0500 Subject: [PATCH] feat: add db-pool-automatic-recovery configuration to disable connection retrying --- CHANGELOG.md | 4 +++ src/PostgREST/AppState.hs | 14 +++++--- src/PostgREST/CLI.hs | 3 ++ src/PostgREST/Config.hs | 3 ++ test/io/configs/expected/aliases.config | 1 + .../configs/expected/boolean-numeric.config | 1 + .../io/configs/expected/boolean-string.config | 1 + test/io/configs/expected/defaults.config | 1 + ...efaults-with-db-other-authenticator.config | 1 + .../expected/no-defaults-with-db.config | 1 + test/io/configs/expected/no-defaults.config | 1 + test/io/configs/expected/types.config | 1 + test/io/configs/no-defaults-env.yaml | 1 + test/io/configs/no-defaults.config | 1 + test/io/fixtures.sql | 6 +++- test/io/test_io.py | 32 +++++++++++++++++++ test/spec/SpecHelper.hs | 1 + 17 files changed, 67 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1790da7e5..b1f5643e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,10 @@ This project adheres to [Semantic Versioning](http://semver.org/). ## Unreleased +### Added + + - #1614, Add `db-pool-automatic-recovery` configuration to disable connection retrying - @taimoorzaeem + ### Fixed - #2899, Fix `application/vnd.pgrst.array` not accepted as a valid mediatype - @taimoorzaeem diff --git a/src/PostgREST/AppState.hs b/src/PostgREST/AppState.hs index 7f4ac565c..89e3cb69d 100644 --- a/src/PostgREST/AppState.hs +++ b/src/PostgREST/AppState.hs @@ -281,8 +281,9 @@ internalConnectionWorker appState = work -- Fatal error when connecting logWithZTime appState reason >> killThread (getMainThreadId appState) NotConnected -> - -- Unreachable because establishConnection will keep trying to connect - return () + -- Unreachable because establishConnection will keep trying to connect, unless disable-recovery is turned on + unless configDbPoolAutomaticRecovery + $ logWithZTime appState "Automatic recovery disabled, exiting." >> killThread (getMainThreadId appState) Connected actualPgVersion -> do -- Procede with initialization putPgVersion appState actualPgVersion @@ -344,9 +345,10 @@ establishConnection appState = shouldRetry :: RetryStatus -> ConnectionStatus -> IO Bool shouldRetry rs isConnSucc = do + AppConfig{..} <- getConfig appState let delay = fromMaybe 0 (rsPreviousDelay rs) `div` backoffMicroseconds - itShould = NotConnected == isConnSucc + itShould = NotConnected == isConnSucc && configDbPoolAutomaticRecovery when itShould . logWithZTime appState $ "Attempting to reconnect to the database in " <> (show delay::Text) @@ -420,7 +422,7 @@ listener appState = do waitListener appState -- forkFinally allows to detect if the thread dies - void . flip forkFinally (handleFinally dbChannel) $ do + void . flip forkFinally (handleFinally dbChannel configDbPoolAutomaticRecovery) $ do dbOrError <- acquire $ toUtf8 (addFallbackAppName prettyVersion configDbUri) case dbOrError of Right db -> do @@ -431,7 +433,9 @@ listener appState = do _ -> die $ "Could not listen for notifications on the " <> dbChannel <> " channel" where - handleFinally dbChannel _ = do + handleFinally _ False _ = + logWithZTime appState "Automatic recovery disabled, exiting." >> killThread (getMainThreadId appState) + handleFinally dbChannel True _ = do -- if the thread dies, we try to recover logWithZTime appState $ "Retrying listening for notifications on the " <> dbChannel <> " channel.." putIsListenerOn appState False diff --git a/src/PostgREST/CLI.hs b/src/PostgREST/CLI.hs index 8f4626ad4..7f39d212b 100644 --- a/src/PostgREST/CLI.hs +++ b/src/PostgREST/CLI.hs @@ -162,6 +162,9 @@ exampleConfigFile = |## Time in seconds after which to recycle unused pool connections |# db-pool-max-idletime = 30 | + |## Allow autmatic database connection retrying + |# db-pool-automatic-recovery = true + | |## Stored proc to exec immediately after auth |# db-pre-request = "stored_proc_name" | diff --git a/src/PostgREST/Config.hs b/src/PostgREST/Config.hs index de0ed82e8..4221ff59d 100644 --- a/src/PostgREST/Config.hs +++ b/src/PostgREST/Config.hs @@ -80,6 +80,7 @@ data AppConfig = AppConfig , configDbPoolAcquisitionTimeout :: Int , configDbPoolMaxLifetime :: Int , configDbPoolMaxIdletime :: Int + , configDbPoolAutomaticRecovery :: Bool , configDbPreRequest :: Maybe QualifiedIdentifier , configDbPreparedStatements :: Bool , configDbRootSpec :: Maybe QualifiedIdentifier @@ -147,6 +148,7 @@ toText conf = ,("db-pool-acquisition-timeout", show . configDbPoolAcquisitionTimeout) ,("db-pool-max-lifetime", show . configDbPoolMaxLifetime) ,("db-pool-max-idletime", show . configDbPoolMaxIdletime) + ,("db-pool-automatic-recovery", T.toLower . show . configDbPoolAutomaticRecovery) ,("db-pre-request", q . maybe mempty dumpQi . configDbPreRequest) ,("db-prepared-statements", T.toLower . show . configDbPreparedStatements) ,("db-root-spec", q . maybe mempty dumpQi . configDbRootSpec) @@ -241,6 +243,7 @@ parser optPath env dbSettings roleSettings roleIsolationLvl = <*> (fromMaybe 1800 <$> optInt "db-pool-max-lifetime") <*> (fromMaybe 30 <$> optWithAlias (optInt "db-pool-timeout") (optInt "db-pool-max-idletime")) + <*> (fromMaybe True <$> optBool "db-pool-automatic-recovery") <*> (fmap toQi <$> optWithAlias (optString "db-pre-request") (optString "pre-request")) <*> (fromMaybe True <$> optBool "db-prepared-statements") diff --git a/test/io/configs/expected/aliases.config b/test/io/configs/expected/aliases.config index fedbac8b0..96e74e2ad 100644 --- a/test/io/configs/expected/aliases.config +++ b/test/io/configs/expected/aliases.config @@ -8,6 +8,7 @@ db-pool = 10 db-pool-acquisition-timeout = 10 db-pool-max-lifetime = 1800 db-pool-max-idletime = 5 +db-pool-automatic-recovery = true db-pre-request = "check_alias" db-prepared-statements = true db-root-spec = "open_alias" diff --git a/test/io/configs/expected/boolean-numeric.config b/test/io/configs/expected/boolean-numeric.config index 37abeb035..438cd3589 100644 --- a/test/io/configs/expected/boolean-numeric.config +++ b/test/io/configs/expected/boolean-numeric.config @@ -8,6 +8,7 @@ db-pool = 10 db-pool-acquisition-timeout = 10 db-pool-max-lifetime = 1800 db-pool-max-idletime = 30 +db-pool-automatic-recovery = true db-pre-request = "" db-prepared-statements = false db-root-spec = "" diff --git a/test/io/configs/expected/boolean-string.config b/test/io/configs/expected/boolean-string.config index 37abeb035..438cd3589 100644 --- a/test/io/configs/expected/boolean-string.config +++ b/test/io/configs/expected/boolean-string.config @@ -8,6 +8,7 @@ db-pool = 10 db-pool-acquisition-timeout = 10 db-pool-max-lifetime = 1800 db-pool-max-idletime = 30 +db-pool-automatic-recovery = true db-pre-request = "" db-prepared-statements = false db-root-spec = "" diff --git a/test/io/configs/expected/defaults.config b/test/io/configs/expected/defaults.config index ab914aeac..3561d7428 100644 --- a/test/io/configs/expected/defaults.config +++ b/test/io/configs/expected/defaults.config @@ -8,6 +8,7 @@ db-pool = 10 db-pool-acquisition-timeout = 10 db-pool-max-lifetime = 1800 db-pool-max-idletime = 30 +db-pool-automatic-recovery = true db-pre-request = "" db-prepared-statements = true db-root-spec = "" diff --git a/test/io/configs/expected/no-defaults-with-db-other-authenticator.config b/test/io/configs/expected/no-defaults-with-db-other-authenticator.config index dceefd8ae..226b1a734 100644 --- a/test/io/configs/expected/no-defaults-with-db-other-authenticator.config +++ b/test/io/configs/expected/no-defaults-with-db-other-authenticator.config @@ -8,6 +8,7 @@ db-pool = 1 db-pool-acquisition-timeout = 30 db-pool-max-lifetime = 3600 db-pool-max-idletime = 60 +db-pool-automatic-recovery = false db-pre-request = "test.other_custom_headers" db-prepared-statements = false db-root-spec = "other_root" diff --git a/test/io/configs/expected/no-defaults-with-db.config b/test/io/configs/expected/no-defaults-with-db.config index 0676308c4..d434cf778 100644 --- a/test/io/configs/expected/no-defaults-with-db.config +++ b/test/io/configs/expected/no-defaults-with-db.config @@ -8,6 +8,7 @@ db-pool = 1 db-pool-acquisition-timeout = 30 db-pool-max-lifetime = 3600 db-pool-max-idletime = 60 +db-pool-automatic-recovery = false db-pre-request = "test.custom_headers" db-prepared-statements = false db-root-spec = "root" diff --git a/test/io/configs/expected/no-defaults.config b/test/io/configs/expected/no-defaults.config index 2be86049f..a657ef851 100644 --- a/test/io/configs/expected/no-defaults.config +++ b/test/io/configs/expected/no-defaults.config @@ -8,6 +8,7 @@ db-pool = 1 db-pool-acquisition-timeout = 30 db-pool-max-lifetime = 3600 db-pool-max-idletime = 60 +db-pool-automatic-recovery = false db-pre-request = "please_run_fast" db-prepared-statements = false db-root-spec = "openapi_v3" diff --git a/test/io/configs/expected/types.config b/test/io/configs/expected/types.config index 7259e4a43..bd0d91f4e 100644 --- a/test/io/configs/expected/types.config +++ b/test/io/configs/expected/types.config @@ -8,6 +8,7 @@ db-pool = 10 db-pool-acquisition-timeout = 10 db-pool-max-lifetime = 1800 db-pool-max-idletime = 30 +db-pool-automatic-recovery = true db-pre-request = "" db-prepared-statements = true db-root-spec = "" diff --git a/test/io/configs/no-defaults-env.yaml b/test/io/configs/no-defaults-env.yaml index 126075e4d..158cd0e87 100644 --- a/test/io/configs/no-defaults-env.yaml +++ b/test/io/configs/no-defaults-env.yaml @@ -10,6 +10,7 @@ PGRST_DB_POOL: 1 PGRST_DB_POOL_ACQUISITION_TIMEOUT: 30 PGRST_DB_POOL_MAX_LIFETIME: 3600 PGRST_DB_POOL_MAX_IDLETIME: 60 +PGRST_DB_POOL_AUTOMATIC_RECOVERY: false PGRST_DB_PREPARED_STATEMENTS: false PGRST_DB_PRE_REQUEST: please_run_fast PGRST_DB_ROOT_SPEC: openapi_v3 diff --git a/test/io/configs/no-defaults.config b/test/io/configs/no-defaults.config index 2a9f73614..9c7419b16 100644 --- a/test/io/configs/no-defaults.config +++ b/test/io/configs/no-defaults.config @@ -8,6 +8,7 @@ db-pool = 1 db-pool-acquisition-timeout = 30 db-pool-max-lifetime = 3600 db-pool-max-idletime = 60 +db-pool-automatic-recovery = false db-pre-request = "please_run_fast" db-prepared-statements = false db-root-spec = "openapi_v3" diff --git a/test/io/fixtures.sql b/test/io/fixtures.sql index 7315ee5ee..bc1dd9670 100644 --- a/test/io/fixtures.sql +++ b/test/io/fixtures.sql @@ -173,4 +173,8 @@ select application_name from pg_stat_activity where application_name ilike 'postgrest%' limit 1; -$$ +$$; + +create function terminate_pgrst() returns setof record as $$ +select pg_terminate_backend(pid) from pg_stat_activity where application_name iLIKE '%postgrest%'; +$$ language sql security definer; diff --git a/test/io/test_io.py b/test/io/test_io.py index 036e074f2..55a0712a2 100644 --- a/test/io/test_io.py +++ b/test/io/test_io.py @@ -1063,3 +1063,35 @@ def test_succeed_w_role_having_superuser_settings(defaultenv): response = postgrest.session.get("/projects", headers=headers) print(response.text) assert response.status_code == 200 + + +def test_fail_with_invalid_dbname_and_automatic_recovery_disabled(defaultenv): + "Should fail without retries when automatic recovery is disabled and dbname is invalid" + dbname = "INVALID" + uri = f'postgresql://?dbname={dbname}&host={defaultenv["PGHOST"]}&user={defaultenv["PGUSER"]}' + env = { + **defaultenv, + "PGRST_DB_URI": uri, + "PGRST_DB_POOL_AUTOMATIC_RECOVERY": "false", + } + + with run(env=env, wait_for_readiness=False) as postgrest: + exitCode = wait_until_exit(postgrest) + assert exitCode == 1 + + +def test_fail_with_automatic_recovery_disabled_and_terminated_using_query(defaultenv): + "Should fail without retries when automatic recovery is disabled and pg_terminate_backend(pid) is called" + + env = { + **defaultenv, + "PGRST_DB_POOL_AUTOMATIC_RECOVERY": "false", + } + + with run(env=env) as postgrest: + os.system( + f'psql -d {defaultenv["PGDATABASE"]} -U {defaultenv["PGUSER"]} -h {defaultenv["PGHOST"]} --set ON_ERROR_STOP=1 -a -c "SELECT terminate_pgrst()"' + ) + + exitCode = wait_until_exit(postgrest) + assert exitCode == 1 diff --git a/test/spec/SpecHelper.hs b/test/spec/SpecHelper.hs index cc96ad5ab..5ef0cd6d4 100644 --- a/test/spec/SpecHelper.hs +++ b/test/spec/SpecHelper.hs @@ -91,6 +91,7 @@ baseCfg = let secret = Just $ encodeUtf8 "reallyreallyreallyreallyverysafe" in , configDbPoolAcquisitionTimeout = 10 , configDbPoolMaxLifetime = 1800 , configDbPoolMaxIdletime = 600 + , configDbPoolAutomaticRecovery = True , configDbPreRequest = Just $ QualifiedIdentifier "test" "switch_role" , configDbPreparedStatements = True , configDbRootSpec = Nothing