Make the behaviour of the database_config more predictable
Current situation:
By default, omnibus sets the database connection pool size to 1: https://gitlab.com/gitlab-org/omnibus-gitlab/-/blob/7f30b17c2111afc72307e5956f20b286826340f6/files/gitlab-cookbooks/gitlab/attributes/default.rb#L346
When running in a multi-threaded environment (puma, sidekiq), the database_config
initializer will increase the number of connections if it was lower than what we think we need: https://gitlab.com/gitlab-org/gitlab/-/blob/1f4606a40d7b919e1d6deb3561aa69785442d7b0/config/initializers/database_config.rb#L28-39. If the configuration was set to a value that was higher than what we think we need, we'll respect it.
Suggestion:
Set the db_pool
to nil
by default. If nothing is set, the initializer will adjust the database config to the number we think we need, for both multi-threaded and single-threaded environments.
If db_pool
is configured, we'll respect it, possibly emitting a warning when we think it's too low.
The following discussion from !34371 (merged) should be addressed:
-
@mkaeppler started a discussion: (+4 comments) 👍 Just a reminder (also to others who might look at this) that this will not help in the case of Unicorn, since none of this will execute. But we can drop themulti_threaded?
check in a separate MR, better to make baby steps here. It should be safe though becausemax_threads
would just be 1 in that case.Also, and this honestly really applies to any of these changes we have made or plan to make: if I was an administrator of GitLab on premises, and I would set
pool
to whatever I think it should be, isn't it quite confusing that we override these user supplied values? I'm beginning to think as well that maybe we should make a clear distinction between the cases of "it was user provided" (then leave it alone) or "it was not set at all" so we set it to the best possible value we can think of, including headroom and all that jazz.I know this has been the case before, but previously we would only override a user provided setting if it was obviously too low and would likely lead to an unstable system, so it was more of an "emergency override", whereas now we always change the user provided value. I would prefer an approach where we do not set any default for this in Omnibus at all, so the default becomes whatever this expression computes, but if it's set, use that value verbatim.
Or, do not provide a
pool
setting at all...?nit: I would consider just moving this onto one line. I think our column limit is 100 or more. It will make it easier to read.