From 6a7ad33524d7b28f58e6bfcacc44f384d5ac2673 Mon Sep 17 00:00:00 2001 From: Taimoor Zaeem Date: Fri, 31 Jul 2026 14:34:26 +0500 Subject: [PATCH] fix: db-channel-enabled not reloadable on config reload Fixes #4894. Signed-off-by: Taimoor Zaeem --- CHANGELOG.md | 1 + src/library/PostgREST/AppState.hs | 1 + src/library/PostgREST/AppState/Reload.hs | 47 ++++++++++++------- src/library/PostgREST/AppState/Types.hs | 15 ++++++ test/io/configs/sigusr2-settings.config | 3 ++ test/io/test_io.py | 58 ++++++++++++++++++++++++ 6 files changed, 108 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1bc91390..c98f27b47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ All notable changes to this project will be documented in this file. From versio - Stop reporting 503s errors unnecessarily while the schema cache is loading at startup by @mkleczek in #4880 - Fix responding with `Something went wrong` on Admin server when under EMFILE by @mkleczek in #5077 - Fix schema cache dump missing RPC transaction isolation level by @taimoorzaeem in #5079 +- Fix config `db-channel-enabled` not reloadable on config reload by @taimoorzaeem in #4894 ### Changed diff --git a/src/library/PostgREST/AppState.hs b/src/library/PostgREST/AppState.hs index 0a94ad3c2..c6e960b9f 100644 --- a/src/library/PostgREST/AppState.hs +++ b/src/library/PostgREST/AppState.hs @@ -75,6 +75,7 @@ initWithPool pool confRef loggerState metricsState observer appKiller = mdo <*> newIORef Nothing <*> newSchemaCacheStatus <*> newIORef False + <*> newIORef Nothing <*> makeDebouncer (retryingSchemaCacheLoad appState *> threadDelay 100000) -- 100ms cooldown <*> pure confRef <*> mkAutoUpdate defaultUpdateSettings { updateAction = getCurrentTime } diff --git a/src/library/PostgREST/AppState/Reload.hs b/src/library/PostgREST/AppState/Reload.hs index 3f42da798..1162ac900 100644 --- a/src/library/PostgREST/AppState/Reload.hs +++ b/src/library/PostgREST/AppState/Reload.hs @@ -184,42 +184,55 @@ readInDbConfig startingUp appState@AppState{stateObserver=observer} = do -- entries, because they were cached using the old secret update (getJwtCacheState appState) newConf + -- If db-channel-enabled is changed, then reload listener + when (((/=) `on` configDbChannelEnabled) newConf oldConf) $ do + -- 1. Kill the listener thread + getListenerThreadId appState >>= mapM_ (`throwTo` ListenerRestart) + putIsListenerOn appState False + -- 2. Restart listener + runListener appState + if startingUp then pass else observer ConfigSucceededObs + -- | Starts the Listener in a thread runListener :: AppState -> IO () runListener appState = do AppConfig{..} <- getConfig appState when configDbChannelEnabled $ do nextDelay <- newIORef 1 - void . forkIO . void $ retryingListen appState nextDelay False + listenerThreadId <- forkIO . void $ retryingListen appState nextDelay False + putListenerThreadId appState (Just listenerThreadId) -- | Starts a LISTEN connection and handles notifications. It recovers with exponential backoff with a cap of 32 seconds, if the LISTEN connection is lost. -- | This function never returns (but can throw) and return type enforces that. -retryingListen :: AppState -> IORef Int -> Bool -> IO Void +retryingListen :: AppState -> IORef Int -> Bool -> IO () retryingListen appState nextDelay hasDbListenerBug = do cfg@AppConfig{..} <- getConfig appState let dbChannel = toS configDbChannel - onError err = do - putIsListenerOn appState False - observer $ DBListenFail dbChannel (Right err) - when (isDbListenerBug err) $ - observer DBListenBugCallQueryFix - unless configDbPoolAutomaticRecovery $ - killApp appState - -- retry the listener - delay <- readIORef nextDelay - observer $ DBListenRetry delay - threadDelay (delay * oneSecondInMicro) - unless (delay == maxDelay) $ - writeIORef nextDelay (delay * 2) - -- loop running the listener - retryingListen appState nextDelay (isDbListenerBug err) + onError err = case fromException err of + Just ListenerRestart -> traverse_ killThread =<< getListenerThreadId appState + Nothing -> do -- for any other exception + putIsListenerOn appState False + observer $ DBListenFail dbChannel (Right err) + when (isDbListenerBug err) $ + observer DBListenBugCallQueryFix + unless configDbPoolAutomaticRecovery $ + killApp appState + + -- retry the listener + delay <- readIORef nextDelay + observer $ DBListenRetry delay + threadDelay (delay * oneSecondInMicro) + unless (delay == maxDelay) $ + writeIORef nextDelay (delay * 2) + -- loop running the listener + retryingListen appState nextDelay (isDbListenerBug err) -- Execute the listener with error handling handle onError $ do diff --git a/src/library/PostgREST/AppState/Types.hs b/src/library/PostgREST/AppState/Types.hs index 180c4461f..e310a6f9c 100644 --- a/src/library/PostgREST/AppState/Types.hs +++ b/src/library/PostgREST/AppState/Types.hs @@ -2,6 +2,7 @@ Module : PostgREST.AppState.Types Description : AppState data type and stateful functions -} +{-# LANGUAGE DeriveAnyClass #-} module PostgREST.AppState.Types where import qualified Hasql.Pool as SQL @@ -32,6 +33,8 @@ data AppState = AppState , stateSCacheStatus :: SchemaCacheStatus -- | State of the LISTEN channel , stateIsListenerOn :: IORef Bool + -- | Listener Thread ID + , stateListenerThreadId :: IORef (Maybe ThreadId) -- | starts the connection worker with a debounce , debouncedSCacheLoader :: IO () -- | Config that can change at runtime @@ -58,6 +61,12 @@ newtype SchemaCacheStatus = SchemaCacheStatus { getSCStatusTMVar :: TMVar Bool } +-- | +-- We define a custom exception and throw this on listener reload. The +-- KillThread exception can occur in an unexpected scenario, so we should +-- avoid using that. +data ListenerException = ListenerRestart deriving (Show, Exception) + getPgVersion :: AppState -> IO PgVersion getPgVersion = readIORef . statePgVersion @@ -94,5 +103,11 @@ killApp = stateKillApp putIsListenerOn :: AppState -> Bool -> IO () putIsListenerOn = atomicWriteIORef . stateIsListenerOn +getListenerThreadId :: AppState -> IO (Maybe ThreadId) +getListenerThreadId = readIORef . stateListenerThreadId + +putListenerThreadId :: AppState -> Maybe ThreadId -> IO () +putListenerThreadId = atomicWriteIORef . stateListenerThreadId + getObserver :: AppState -> ObservationHandler getObserver = stateObserver diff --git a/test/io/configs/sigusr2-settings.config b/test/io/configs/sigusr2-settings.config index 4bacedcdb..2eeda8f7c 100644 --- a/test/io/configs/sigusr2-settings.config +++ b/test/io/configs/sigusr2-settings.config @@ -6,3 +6,6 @@ jwt-secret = "invalidinvalidinvalidinvalidinvalid" # will be replaced in test log-level = "error" + +# will be replaced in test +db-channel-enabled = "false" diff --git a/test/io/test_io.py b/test/io/test_io.py index d51c2684d..a49e619dd 100644 --- a/test/io/test_io.py +++ b/test/io/test_io.py @@ -1968,3 +1968,61 @@ def test_config_log_level_is_reloadable(tmp_path, defaultenv): # log-level = debug now, so this log line must be logged assert any("Trying to borrow a connection from pool" in line for line in output) + + +def test_config_db_channel_enabled_is_reloadable(tmp_path, defaultenv): + "Config db-channel-enabled should be reloadable on SIGUSR2" + + config = (CONFIGSDIR / "sigusr2-settings.config").read_text() + configfile = tmp_path / "test.config" + configfile.write_text(config) + + with run(configfile, env=defaultenv, no_startup_stdout=False) as postgrest: + output = postgrest.read_stdout(nlines=7) + + # db-channel-enabled = false, so this shouldn't be logged + assert not any( + f'"{defaultenv["PGHOST"]}:5432" and listening for database notifications on the "pgrst" channel' + in line + for line in output + ) + + # change setting + configfile.write_text( + config.replace( + 'db-channel-enabled = "false"', 'db-channel-enabled = "true"' + ) + ) + + # reload + postgrest.process.send_signal(signal.SIGUSR2) + sleep_until_postgrest_config_reload() + + output = postgrest.read_stdout(nlines=7) + + # db-channel-enabled = true, so this logged + assert any( + f'"{defaultenv["PGHOST"]}:5432" and listening for database notifications on the "pgrst" channel' + in line + for line in output + ) + + # change setting back to false + configfile.write_text( + configfile.read_text().replace( + 'db-channel-enabled = "true"', 'db-channel-enabled = "false"' + ) + ) + + # reload + postgrest.process.send_signal(signal.SIGUSR2) + sleep_until_postgrest_config_reload() + + output = postgrest.read_stdout(nlines=7) + + # db-channel-enabled = false, so this shouldn't be logged + assert not any( + f'"{defaultenv["PGHOST"]}:5432" and listening for database notifications on the "pgrst" channel' + in line + for line in output + )