search

kwilteam / kwil-db

Kwil DB, the database for web3
https://www.kwil.com/
Other
36 stars 12 forks source link

Preview max db conns #1064

Closed brennanjl closed 1 month ago

brennanjl commented 1 month ago

From @outerlook via Slack:

Frontend team from time to time still crashes the server batching requests. Is that option to reduce requests concurrency available? https://github.com/kwilteam/kwil-db/blob/cb0f6f6c19644f9ec405078b54c615eb4c6e8d35/cmd/kwild/server/build.go#L429 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 commented 1 month ago

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 commented 1 month ago

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

outerlook commented 1 month ago

@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 commented 1 month ago

@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.

brennanjl commented 1 month ago

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