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.

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

Preview max db conns #1064

Merged
merged 1 commit into from
Oct 18, 2024
Merged

Preview max db conns #1064

merged 1 commit into from
Oct 18, 2024

Conversation

brennanjl
Copy link
Collaborator

From @outerlook via Slack:

Frontend team from time to time still crashes the server batching requests. Is that option to reduce requests concurrency available?

db, err := d.dbOpener(d.ctx, d.cfg.AppCfg.DBName, 24)

how much effort would it take to make this 24 configurable? Also it's expected that with this lower value, a queue system will kick in to manage requests, or would them fail?

This PR adds a app.db-max-connections flag which specifies the maximum number of connections the engine will hold to Postgres. The default is still 24, but it is now configurable. The minimum is 2, and kwild will fail quickly if the user configures 1.

I did some local testing to ensure that this applies backpressure properly, and can confirm it does. A the number of long-running concurrent reads repeatedly execute against a database, the average time per query increases. I tested it with both reads from the same table (which actually get backpressure from Postgres's lock contention before the max connections are an issue), as well as with separate tables (which obviously did not have issues with lock contention).

@brennanjl brennanjl changed the base branch from main to preview October 9, 2024 17:07
@brennanjl
Copy link
Collaborator Author

brennanjl commented Oct 9, 2024

Once I get some feedback on this I will open the same PR to main and v0.9. I put it here so that Raffael can test it (they still use preview)

@jchappelow
Copy link
Member

"Crashes the server" refers to the OOM issues with the postgres process, or kwild is crashing?

@outerlook
Copy link

@jchappelow postgres was initially halting the instance for many minutes with too much memory usage, then I lowered the docker container limit to just be killed and restarted instead

@jchappelow
Copy link
Member

@jchappelow postgres was initially halting the instance for many minutes with too much memory usage, then I lowered the docker container limit to just be killed and restarted instead

Gotcha, thanks for clarifying.

Copy link
Member

@jchappelow jchappelow left a comment

Choose a reason for hiding this comment

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

LGTM, except that we should probably enforce a minimum, and I have a feeling 3 or 4 is more practical. We'd have to go through and count, but I think we make some read connections in a number of places, and i would hate for there to be a deadlock somehow if the circumstances are just right.

@brennanjl
Copy link
Collaborator Author

LGTM, except that we should probably enforce a minimum, and I have a feeling 3 or 4 is more practical. We'd have to go through and count, but I think we make some read connections in a number of places, and i would hate for there to be a deadlock somehow if the circumstances are just right.

A minimum of 2 is enforced, but we can raise it

@brennanjl brennanjl merged commit fbf5c0c into preview Oct 18, 2024
@brennanjl brennanjl deleted the preview-max-db-conns branch October 18, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants