From 09447c5c4e4997714fd4cd93b528bc3f3b639d47 Mon Sep 17 00:00:00 2001 From: Robert Vollmert Date: Tue, 14 Feb 2023 15:39:10 +0100 Subject: [PATCH] feat: use hasql-pool-0.9, add db-pool-max-lifetime (fixes #2638) - db-pool-acquisition-timeout is no longer optional, defaults to 10s - new option db-pool-max-lifetime limits the maximal lifetime of a postgresql connection, defaults to 30m --- CHANGELOG.md | 4 ++++ nix/overlays/haskell-packages.nix | 18 ++++++++++++++++++ postgrest.cabal | 6 +++--- src/PostgREST/AppState.hs | 9 +++++---- src/PostgREST/CLI.hs | 3 +++ src/PostgREST/Config.hs | 12 ++++++++---- stack.yaml | 2 ++ stack.yaml.lock | 14 ++++++++++++++ test/io/configs/expected/aliases.config | 3 ++- .../io/configs/expected/boolean-numeric.config | 3 ++- test/io/configs/expected/boolean-string.config | 3 ++- test/io/configs/expected/defaults.config | 3 ++- ...defaults-with-db-other-authenticator.config | 3 ++- .../expected/no-defaults-with-db.config | 3 ++- test/io/configs/expected/no-defaults.config | 3 ++- test/io/configs/expected/types.config | 3 ++- test/io/configs/no-defaults-env.yaml | 3 ++- test/io/configs/no-defaults.config | 3 ++- test/spec/Main.hs | 2 +- test/spec/SpecHelper.hs | 3 ++- 20 files changed, 80 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 697898c5e2..68575db497 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,10 @@ This project adheres to [Semantic Versioning](http://semver.org/). + This can be used to override the OpenAPI spec with a custom database function - #1567, On bulk inserts, missing values can get the column DEFAULT by using the `Prefer: missing=default` header - @steve-chavez - #2501, Allow filtering by`IS DISTINCT FROM` using the `isdistinct` operator, e.g. `/people?alias=isdistinct.foo` + - #2663, Limit maximal postgresql connection lifetime - @robx + + New option `db-pool-max-lifetime` (default 30m) + + `db-pool-acquisition-timeout` is no longer optional and defaults to 10s + + Fixes postgresql resource leak with long-lived connections (#2638) ### Fixed diff --git a/nix/overlays/haskell-packages.nix b/nix/overlays/haskell-packages.nix index c6ce85ed58..f650278e9d 100644 --- a/nix/overlays/haskell-packages.nix +++ b/nix/overlays/haskell-packages.nix @@ -38,6 +38,24 @@ let sha256 = "1wmyhldk0k14y8whp1p4akrkqxf5snh8qsbm7fv5f7kz95nyffd0"; }) { }); + + hasql-notifications = lib.dontCheck + (prev.callHackageDirect + { + pkg = "hasql-notifications"; + ver = "0.2.0.4"; + sha256 = "sha256-fm1xiDyvDkb5WLOJ73/s8wrWEW23XFS7luAv2brfr8I="; + } + { }); + + hasql-pool = lib.dontCheck + (prev.callHackageDirect + { + pkg = "hasql-pool"; + ver = "0.9"; + sha256 = "sha256-5UshbbaBVY8eJ/9VagNVVxonRwMcd7UmGqDc35pJNFY="; + } + { }); } // extraOverrides final prev; in { diff --git a/postgrest.cabal b/postgrest.cabal index cc35428cee..c72bf04234 100644 --- a/postgrest.cabal +++ b/postgrest.cabal @@ -91,7 +91,7 @@ library , hasql >= 1.6.1.1 && < 1.7 , hasql-dynamic-statements >= 0.3.1 && < 0.4 , hasql-notifications >= 0.1 && < 0.3 - , hasql-pool >= 0.8.0.6 && < 0.9 + , hasql-pool >= 0.9 && < 0.10 , hasql-transaction >= 1.0.1 && < 1.1 , heredoc >= 0.2 && < 0.3 , http-types >= 0.12.2 && < 0.13 @@ -231,7 +231,7 @@ test-suite spec , bytestring >= 0.10.8 && < 0.12 , case-insensitive >= 1.2 && < 1.3 , containers >= 0.5.7 && < 0.7 - , hasql-pool >= 0.8.0.2 && < 0.9 + , hasql-pool >= 0.9 && < 0.10 , hasql-transaction >= 1.0.1 && < 1.1 , heredoc >= 0.2 && < 0.3 , hspec >= 2.3 && < 2.10 @@ -250,7 +250,7 @@ test-suite spec , transformers-base >= 0.4.4 && < 0.5 , wai >= 3.2.1 && < 3.3 , wai-extra >= 3.0.19 && < 3.2 - ghc-options: -O0 -Werror -Wall -fwarn-identities + ghc-options: -threaded -O0 -Werror -Wall -fwarn-identities -fno-spec-constr -optP-Wno-nonportable-include-path -fno-warn-missing-signatures -fwrite-ide-info diff --git a/src/PostgREST/AppState.hs b/src/PostgREST/AppState.hs index 1a36a1864d..93372a9431 100644 --- a/src/PostgREST/AppState.hs +++ b/src/PostgREST/AppState.hs @@ -111,10 +111,11 @@ destroy = destroyPool initPool :: AppConfig -> IO SQL.Pool initPool AppConfig{..} = - SQL.acquire configDbPoolSize timeoutMicroseconds $ toUtf8 configDbUri - where - timeoutMicroseconds = (* oneSecond) <$> configDbPoolAcquisitionTimeout - oneSecond = 1000000 + SQL.acquire + configDbPoolSize + (fromIntegral configDbPoolAcquisitionTimeout) + (fromIntegral configDbPoolMaxLifetime) + (toUtf8 configDbUri) -- | Run an action with a database connection. usePool :: AppState -> SQL.Session a -> IO (Either SQL.UsageError a) diff --git a/src/PostgREST/CLI.hs b/src/PostgREST/CLI.hs index b447fb6fbe..30f59ea393 100644 --- a/src/PostgREST/CLI.hs +++ b/src/PostgREST/CLI.hs @@ -151,6 +151,9 @@ exampleConfigFile = |## Time in seconds to wait to acquire a slot from the connection pool |# db-pool-acquisition-timeout = 10 | + |## Time in seconds after which to recycle pool connections + |# db-pool-max-lifetime = 1800 + | |## 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 74c0032ad4..773745d9d5 100644 --- a/src/PostgREST/Config.hs +++ b/src/PostgREST/Config.hs @@ -71,7 +71,8 @@ data AppConfig = AppConfig , configDbMaxRows :: Maybe Integer , configDbPlanEnabled :: Bool , configDbPoolSize :: Int - , configDbPoolAcquisitionTimeout :: Maybe Int + , configDbPoolAcquisitionTimeout :: Int + , configDbPoolMaxLifetime :: Int , configDbPreRequest :: Maybe QualifiedIdentifier , configDbPreparedStatements :: Bool , configDbRootSpec :: Maybe QualifiedIdentifier @@ -132,7 +133,8 @@ toText conf = ,("db-max-rows", maybe "\"\"" show . configDbMaxRows) ,("db-plan-enabled", T.toLower . show . configDbPlanEnabled) ,("db-pool", show . configDbPoolSize) - ,("db-pool-acquisition-timeout", maybe "\"\"" show . configDbPoolAcquisitionTimeout) + ,("db-pool-acquisition-timeout", show . configDbPoolAcquisitionTimeout) + ,("db-pool-max-lifetime", show . configDbPoolMaxLifetime) ,("db-pre-request", q . maybe mempty dumpQi . configDbPreRequest) ,("db-prepared-statements", T.toLower . show . configDbPreparedStatements) ,("db-root-spec", q . maybe mempty dumpQi . configDbRootSpec) @@ -222,7 +224,8 @@ parser optPath env dbSettings = (optInt "max-rows") <*> (fromMaybe False <$> optBool "db-plan-enabled") <*> (fromMaybe 10 <$> optInt "db-pool") - <*> optInt "db-pool-acquisition-timeout" + <*> (fromMaybe 10 <$> optInt "db-pool-acquisition-timeout") + <*> (fromMaybe 1800 <$> optInt "db-pool-max-lifetime") <*> (fmap toQi <$> optWithAlias (optString "db-pre-request") (optString "pre-request")) <*> (fromMaybe True <$> optBool "db-prepared-statements") @@ -359,7 +362,8 @@ parser optPath env dbSettings = let dbSettingName = T.pack $ dashToUnderscore <$> toS key in if dbSettingName `notElem` [ "server_host", "server_port", "server_unix_socket", "server_unix_socket_mode", "admin_server_port", "log_level", - "db_uri", "db_channel_enabled", "db_channel", "db_pool", "db_pool_acquisition_timeout", "db_config"] + "db_uri", "db_channel_enabled", "db_channel", "db_pool", "db_pool_acquisition_timeout", + "db_pool_max_lifetime", "db_config"] then lookup dbSettingName dbSettings else Nothing diff --git a/stack.yaml b/stack.yaml index bf9ba9ef18..f54257a84d 100644 --- a/stack.yaml +++ b/stack.yaml @@ -12,3 +12,5 @@ nix: extra-deps: - git: https://github.com/PostgREST/postgresql-libpq.git commit: 890a0a16cf57dd401420fdc6c7d576fb696003bc + - hasql-notifications-0.2.0.4 + - hasql-pool-0.9 diff --git a/stack.yaml.lock b/stack.yaml.lock index dabb9ea08c..ef9eadf45b 100644 --- a/stack.yaml.lock +++ b/stack.yaml.lock @@ -15,6 +15,20 @@ packages: original: commit: 890a0a16cf57dd401420fdc6c7d576fb696003bc git: https://github.com/PostgREST/postgresql-libpq.git +- completed: + hackage: hasql-notifications-0.2.0.4@sha256:9a09fa9b97feadd9492c8bd8bc6b9cffe0513510102f08374b0c45ecd479ed67,2028 + pantry-tree: + sha256: 56f9e240728e7a65711dde45fa2e2075b914e32cd370424aaa4572392378a60e + size: 452 + original: + hackage: hasql-notifications-0.2.0.4 +- completed: + hackage: hasql-pool-0.9@sha256:db7a37f6b3a922c37adc3c7ced47a7c10786d1f171e47a735a6e812a587ba44c,2111 + pantry-tree: + sha256: 49b1181d28c6f5317e794671c2dae155754b834bdcfa30f7e5dbad28e4cf0249 + size: 346 + original: + hackage: hasql-pool-0.9 snapshots: - completed: sha256: 4905c93319aa94aa53da8f41d614d7bacdbfe6c63a8c6132d32e6e62f24a9af4 diff --git a/test/io/configs/expected/aliases.config b/test/io/configs/expected/aliases.config index 09fb052705..47b3389f0f 100644 --- a/test/io/configs/expected/aliases.config +++ b/test/io/configs/expected/aliases.config @@ -5,7 +5,8 @@ db-extra-search-path = "public" db-max-rows = 1000 db-plan-enabled = false db-pool = 10 -db-pool-acquisition-timeout = "" +db-pool-acquisition-timeout = 10 +db-pool-max-lifetime = 1800 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 08644dcff1..e4bdf9aa59 100644 --- a/test/io/configs/expected/boolean-numeric.config +++ b/test/io/configs/expected/boolean-numeric.config @@ -5,7 +5,8 @@ db-extra-search-path = "public" db-max-rows = "" db-plan-enabled = false db-pool = 10 -db-pool-acquisition-timeout = "" +db-pool-acquisition-timeout = 10 +db-pool-max-lifetime = 1800 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 08644dcff1..e4bdf9aa59 100644 --- a/test/io/configs/expected/boolean-string.config +++ b/test/io/configs/expected/boolean-string.config @@ -5,7 +5,8 @@ db-extra-search-path = "public" db-max-rows = "" db-plan-enabled = false db-pool = 10 -db-pool-acquisition-timeout = "" +db-pool-acquisition-timeout = 10 +db-pool-max-lifetime = 1800 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 1fdc513bee..6cb2efb52f 100644 --- a/test/io/configs/expected/defaults.config +++ b/test/io/configs/expected/defaults.config @@ -5,7 +5,8 @@ db-extra-search-path = "public" db-max-rows = "" db-plan-enabled = false db-pool = 10 -db-pool-acquisition-timeout = "" +db-pool-acquisition-timeout = 10 +db-pool-max-lifetime = 1800 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 56345137c4..520c99e425 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 @@ -5,7 +5,8 @@ db-extra-search-path = "public,extensions,other" db-max-rows = 100 db-plan-enabled = true db-pool = 1 -db-pool-acquisition-timeout = 10 +db-pool-acquisition-timeout = 30 +db-pool-max-lifetime = 3600 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 4684baebbf..a8d2aa3e90 100644 --- a/test/io/configs/expected/no-defaults-with-db.config +++ b/test/io/configs/expected/no-defaults-with-db.config @@ -5,7 +5,8 @@ db-extra-search-path = "public,extensions,private" db-max-rows = 1000 db-plan-enabled = true db-pool = 1 -db-pool-acquisition-timeout = 10 +db-pool-acquisition-timeout = 30 +db-pool-max-lifetime = 3600 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 d739a9aa14..a57f0737a6 100644 --- a/test/io/configs/expected/no-defaults.config +++ b/test/io/configs/expected/no-defaults.config @@ -5,7 +5,8 @@ db-extra-search-path = "public,test" db-max-rows = 1000 db-plan-enabled = true db-pool = 1 -db-pool-acquisition-timeout = 10 +db-pool-acquisition-timeout = 30 +db-pool-max-lifetime = 3600 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 9ab6d60347..8623f8735a 100644 --- a/test/io/configs/expected/types.config +++ b/test/io/configs/expected/types.config @@ -5,7 +5,8 @@ db-extra-search-path = "public" db-max-rows = "" db-plan-enabled = false db-pool = 10 -db-pool-acquisition-timeout = "" +db-pool-acquisition-timeout = 10 +db-pool-max-lifetime = 1800 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 af152722b9..763910df30 100644 --- a/test/io/configs/no-defaults-env.yaml +++ b/test/io/configs/no-defaults-env.yaml @@ -7,7 +7,8 @@ PGRST_DB_EXTRA_SEARCH_PATH: public, test PGRST_DB_MAX_ROWS: 1000 PGRST_DB_PLAN_ENABLED: true PGRST_DB_POOL: 1 -PGRST_DB_POOL_ACQUISITION_TIMEOUT: 10 +PGRST_DB_POOL_ACQUISITION_TIMEOUT: 30 +PGRST_DB_POOL_MAX_LIFETIME: 3600 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 3da3f7b72f..5db2c68355 100644 --- a/test/io/configs/no-defaults.config +++ b/test/io/configs/no-defaults.config @@ -5,7 +5,8 @@ db-extra-search-path = "public, test" db-max-rows = 1000 db-plan-enabled = true db-pool = 1 -db-pool-acquisition-timeout = 10 +db-pool-acquisition-timeout = 30 +db-pool-max-lifetime = 3600 db-pre-request = "please_run_fast" db-prepared-statements = false db-root-spec = "openapi_v3" diff --git a/test/spec/Main.hs b/test/spec/Main.hs index 65ca199d36..38fb744c9c 100644 --- a/test/spec/Main.hs +++ b/test/spec/Main.hs @@ -65,7 +65,7 @@ import qualified Feature.RpcPreRequestGucsSpec main :: IO () main = do - pool <- P.acquire 3 Nothing $ toUtf8 $ configDbUri testCfg + pool <- P.acquire 3 10 60 $ toUtf8 $ configDbUri testCfg actualPgVersion <- either (panic . show) id <$> P.use pool queryPgVersion diff --git a/test/spec/SpecHelper.hs b/test/spec/SpecHelper.hs index 3ea2a083eb..7fe9934127 100644 --- a/test/spec/SpecHelper.hs +++ b/test/spec/SpecHelper.hs @@ -82,7 +82,8 @@ baseCfg = let secret = Just $ encodeUtf8 "reallyreallyreallyreallyverysafe" in , configDbMaxRows = Nothing , configDbPlanEnabled = False , configDbPoolSize = 10 - , configDbPoolAcquisitionTimeout = Nothing + , configDbPoolAcquisitionTimeout = 10 + , configDbPoolMaxLifetime = 1800 , configDbPreRequest = Just $ QualifiedIdentifier "test" "switch_role" , configDbPreparedStatements = True , configDbRootSpec = Nothing