Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

db settings: reload on connection recovery, only get them from global/current-database scope and change dash to underscore #1760

Merged
merged 4 commits into from
Mar 6, 2021
Merged

db settings: reload on connection recovery, only get them from global/current-database scope and change dash to underscore #1760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6b901fe
Correct db settings to use "_" instead of "-"
steve-chavez Feb 17, 2021
9a73faa
Restrict db settings to current db and global
steve-chavez Feb 18, 2021
ba54ec1
Reread in-db config when recoverying connection
steve-chavez Feb 21, 2021
980c4cd
Change db-load-guc-config to db-config
steve-chavez Feb 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 46 additions & 27 deletions main/Main.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,15 @@ main = do
-- read command/path from commad line
CLI{cliCommand, cliPath} <- readCLIShowHelp env

-- build the 'AppConfig' from the config file path
conf <- either panic identity <$> readConfig mempty env cliPath
-- build the 'AppConfig' from the config file path and env vars
pathEnvConf <- either panic identity <$> readAppConfig mempty env cliPath Nothing Nothing

-- read external files
dbUriFile <- readDbUriFile $ configDbUri pathEnvConf
secretFile <- readSecretFile $ configJwtSecret pathEnvConf

-- add the external files to AppConfig
conf <- either panic identity <$> readAppConfig mempty env cliPath dbUriFile secretFile

-- These are config values that can't be reloaded at runtime. Reloading some of them would imply restarting the web server.
let
Expand All @@ -87,7 +94,7 @@ main = do
poolSize = configDbPoolSize conf
poolTimeout = configDbPoolTimeout' conf
logLevel = configLogLevel conf
gucConfigEnabled = configDbLoadGucConfig conf
dbConfigEnabled = configDbConfig conf

-- create connection pool with the provided settings, returns either a 'Connection' or a 'ConnectionError'. Does not throw.
pool <- P.acquire (poolSize, poolTimeout, dbUri)
Expand All @@ -104,10 +111,19 @@ main = do
-- Config that can change at runtime
refConf <- newIORef conf

let configRereader startingUp = reReadConfig startingUp pool gucConfigEnabled env cliPath refConf

-- re-read and override the config if db-load-guc-config is true
when gucConfigEnabled $ configRereader True
let
-- re-reads config file + db config
dbConfigReReader startingUp = when dbConfigEnabled $
reReadConfig startingUp pool dbConfigEnabled env cliPath refConf dbUriFile secretFile
-- re-reads jwt-secret external file + config file + db config
fullConfigReReader =
reReadConfig False pool dbConfigEnabled env cliPath refConf
dbUriFile =<< -- db-uri external file could be re-read, but it doesn't make sense as db-uri is not reloadable
readSecretFile (configJwtSecret pathEnvConf)

-- Override the config with config options from the db
-- TODO: the same operation is repeated on connectionWorker, ideally this would be done only once, but dump CmdDumpConfig needs it for tests.
dbConfigReReader True

case cliCommand of
CmdDumpConfig ->
Expand All @@ -126,7 +142,8 @@ main = do
-- This is passed to the connectionWorker method so it can kill the main thread if the PostgreSQL's version is not supported.
mainTid <- myThreadId

let connWorker = connectionWorker mainTid pool refConf refDbStructure refIsWorkerOn (dbChannelEnabled, mvarConnectionStatus)
let connWorker = connectionWorker mainTid pool refConf refDbStructure refIsWorkerOn (dbChannelEnabled, mvarConnectionStatus) $
dbConfigReReader False

-- Sets the initial refDbStructure
connWorker
Expand All @@ -149,13 +166,13 @@ main = do

-- Re-read the config on SIGUSR2
void $ installHandler sigUSR2 (
Catch $ configRereader False
Catch fullConfigReReader
) Nothing
#endif

-- reload schema cache + config on NOTIFY
when dbChannelEnabled $
listener dbUri dbChannel pool refConf refDbStructure mvarConnectionStatus connWorker $ configRereader False
listener dbUri dbChannel pool refConf refDbStructure mvarConnectionStatus connWorker fullConfigReReader

-- ask for the OS time at most once per second
getTime <- mkAutoUpdate defaultUpdateSettings {updateAction = getCurrentTime}
Expand Down Expand Up @@ -211,7 +228,8 @@ connectionWorker
-> IORef Bool -- ^ Used as a binary Semaphore
-> (Bool, MVar ConnectionStatus) -- ^ For interacting with the LISTEN channel
-> IO ()
connectionWorker mainTid pool refConf refDbStructure refIsWorkerOn (dbChannelEnabled, mvarConnectionStatus) = do
-> IO ()
connectionWorker mainTid pool refConf refDbStructure refIsWorkerOn (dbChannelEnabled, mvarConnectionStatus) dbCfReader = do
isWorkerOn <- readIORef refIsWorkerOn
unless isWorkerOn $ do -- Prevents multiple workers to be running at the same time. Could happen on too many SIGUSR1s.
atomicWriteIORef refIsWorkerOn True
Expand All @@ -227,6 +245,7 @@ connectionWorker mainTid pool refConf refDbStructure refIsWorkerOn (dbChannelEna
NotConnected -> return () -- Unreachable because connectionStatus will keep trying to connect
Connected actualPgVersion -> do -- Procede with initialization
putStrLn ("Connection successful" :: Text)
dbCfReader -- this could be fail because the connection drops, but the loadSchemaCache will pick the error and retry again
scStatus <- loadSchemaCache pool actualPgVersion refConf refDbStructure
case scStatus of
SCLoaded -> pure () -- do nothing and proceed if the load was successful
Expand Down Expand Up @@ -272,15 +291,6 @@ connectionStatus pool =
putStrLn $ "Attempting to reconnect to the database in " <> (show delay::Text) <> " seconds..."
return itShould

loadDbSettings :: P.Pool -> IO [(Text, Text)]
loadDbSettings pool = do
result <- P.use pool $ HT.transaction HT.ReadCommitted HT.Read $ HT.statement mempty dbSettingsStatement
case result of
Left e -> do
hPutStrLn stderr ("An error ocurred when trying to query database settings for the config parameters:\n" <> show e :: Text)
pure []
Right x -> pure x

-- | Load the DbStructure by using a connection from the pool.
loadSchemaCache :: P.Pool -> PgVersion -> IORef AppConfig -> IORef (Maybe DbStructure) -> IO SCacheStatus
loadSchemaCache pool actualPgVersion refConf refDbStructure = do
Expand Down Expand Up @@ -340,20 +350,29 @@ listener dbUri dbChannel pool refConf refDbStructure mvarConnectionStatus connWo
errorMessage = "Could not listen for notifications on the " <> dbChannel <> " channel" :: Text
retryMessage = "Retrying listening for notifications on the " <> dbChannel <> " channel.." :: Text

-- | Re-reads the config at runtime.
reReadConfig :: Bool -> P.Pool -> Bool -> Environment -> Maybe FilePath -> IORef AppConfig -> IO ()
reReadConfig startingUp pool gucConfigEnabled env path refConf = do
dbSettings <- if gucConfigEnabled then loadDbSettings pool else pure []
readConfig dbSettings env path >>= \case
-- | Re-reads the config plus config options from the db
reReadConfig :: Bool -> P.Pool -> Bool -> Environment -> Maybe FilePath -> IORef AppConfig -> Maybe Text -> Maybe BS.ByteString -> IO ()
reReadConfig startingUp pool dbConfigEnabled env path refConf dbUriFile secretFile = do
dbSettings <- if dbConfigEnabled then loadDbSettings else pure []
readAppConfig dbSettings env path dbUriFile secretFile >>= \case
Left err ->
if startingUp
then panic err -- die on invalid config if the program is starting up
else hPutStrLn stderr $ "Failed config reload. " <> err
else hPutStrLn stderr $ "Failed loading in-database config. " <> err
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The term in-database config is much clearer.
Perhaps the db-load-guc-config option should be changed to config-in-db?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I see your point about db-load-guc-config - it's clumsy. I'd like to keep the db- prefix, though.

What about db-has-config = true|false ? Or just db-config.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The has looks a bit weird inside a config option. I'll go with db-config, it's already much better than db-load-guc-config.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wolfgangwalther What do you think of having db-config to be false by default

The main argument for it being true was to have the least amount of config options: #1729 (comment). But that isn't possible atm, because db-anon-role and db-schemas are required.

However I'm thinking that only defining db-uri could be done and still maintain the required fields restriction. This by having an DbUriAppConfig that is used previous to AppConfig. With that even non-reloadable config options(like db-pool) could be set with the db-config.

But anyway the above is still not done, so db-config is not an advantage now regarding few config options.

It's just that I feel db-config=false is right because most users will not use it(or won't be able, as mentioned on #1729 (comment)). It's a special use case. Having it by default gives potential for errors(because of the query done, twice) on all instances.

WDYT?

Copy link
Member Author

@steve-chavez steve-chavez Mar 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

db-anon-role and db-schemas are required.
This by having an DbUriAppConfig that is used previous to AppConfig

Actually, a better solution for this would be to:

  • make db-anon-role optional. Make db-anon-role optional #1689
  • make db-schemas default to public. This seems like a nice default that will always work. We can put a message on stdout to make it clear when the default is used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

db-anon-role and db-schemas are required.
This by having an DbUriAppConfig that is used previous to AppConfig

Actually, a better solution for this would be to:

* make `db-anon-role` optional. #1689

* make `db-schemas` default to `public`. This seems like a nice default that will always work. We can put a message on stdout to make it clear when the default is used.

Much, much better, yes!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just that I feel db-config=false is right because most users will not use it(or won't be able, as mentioned on #1729 (comment)). It's a special use case. Having it by default gives potential for errors(because of the query done, twice) on all instances.

WDYT?

I think the risk involved is very small. There is no overhead either - even though it's a separate statement, it's basically just part of reloading the schema cache. And in the big picture, loading config options from the database is just a tiny piece of reading the whole schema.

tbh: I actually wonder whether we really need the config option at all. It could just be enabled all the time. It's probably most useful for writing tests - but that's about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in the big picture, loading config options from the database is just a tiny piece of reading the whole schema.

Ah, you're totally right here! I remember trying to do this at first, but I had some issues with the code.

I think it could be done now. We'd have to enable reloading only a part of the schema cache(the guc settings) for SIGUSR2/NOTIFY. Also make the --dump-config wait for the connectionWoker to finish(maybe an MVar) and only print the config contents to stdout(no Attempt to connect to db.. or tests fail).

tbh: I actually wonder whether we really need the config option at all. It could just be enabled all the time.

I think the same way now. If the guc settings are part of the schema cache, we can remove the config option.

Right conf -> do
atomicWriteIORef refConf conf
if startingUp
then pass
else putStrLn ("Config reloaded" :: Text)
else putStrLn ("In-database config loaded" :: Text)
where
loadDbSettings :: IO [(Text, Text)]
loadDbSettings = do
result <- P.use pool $ HT.transaction HT.ReadCommitted HT.Read $ HT.statement mempty dbSettingsStatement
case result of
Left e -> do
hPutStrLn stderr ("An error ocurred when trying to query database settings for the config parameters:\n" <> show e :: Text)
pure []
Right x -> pure x

-- | Dump DbStructure schema to JSON
dumpSchema :: P.Pool -> AppConfig -> IO LBS.ByteString
Expand Down
Loading