Skip to content

Commit

Permalink
feat: add db-pool-automatic-recovery configuration to disable connect…
Browse files Browse the repository at this point in the history
…ion retrying
  • Loading branch information
taimoorzaeem authored Aug 24, 2023
1 parent b8b3145 commit 57fa271
Show file tree
Hide file tree
Showing 17 changed files with 67 additions and 6 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 9 additions & 5 deletions src/PostgREST/AppState.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
3 changes: 3 additions & 0 deletions src/PostgREST/CLI.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
|
Expand Down
3 changes: 3 additions & 0 deletions src/PostgREST/Config.hs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ data AppConfig = AppConfig
, configDbPoolAcquisitionTimeout :: Int
, configDbPoolMaxLifetime :: Int
, configDbPoolMaxIdletime :: Int
, configDbPoolAutomaticRecovery :: Bool
, configDbPreRequest :: Maybe QualifiedIdentifier
, configDbPreparedStatements :: Bool
, configDbRootSpec :: Maybe QualifiedIdentifier
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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")
Expand Down
1 change: 1 addition & 0 deletions test/io/configs/expected/aliases.config
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
1 change: 1 addition & 0 deletions test/io/configs/expected/boolean-numeric.config
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ""
Expand Down
1 change: 1 addition & 0 deletions test/io/configs/expected/boolean-string.config
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ""
Expand Down
1 change: 1 addition & 0 deletions test/io/configs/expected/defaults.config
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
1 change: 1 addition & 0 deletions test/io/configs/expected/no-defaults-with-db.config
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
1 change: 1 addition & 0 deletions test/io/configs/expected/no-defaults.config
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
1 change: 1 addition & 0 deletions test/io/configs/expected/types.config
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ""
Expand Down
1 change: 1 addition & 0 deletions test/io/configs/no-defaults-env.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions test/io/configs/no-defaults.config
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
6 changes: 5 additions & 1 deletion test/io/fixtures.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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;
32 changes: 32 additions & 0 deletions test/io/test_io.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions test/spec/SpecHelper.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 57fa271

Please sign in to comment.