feat: make database connection pool size configurable#21403
feat: make database connection pool size configurable#21403dannykopping merged 11 commits intomainfrom
Conversation
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Many other settings have 0 = unlimited Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
|
I've checked in with @spikecurtis. We're deferring review on this until Danny is back from PTO. |
|
|
||
| - **Capacity**: `go_sql_max_open_connections - go_sql_in_use_connections` shows | ||
| how many connections are available for new requests. If this is 0, Coder | ||
| Server performance will start to degrade. |
There was a problem hiding this comment.
I think rate(go_sql_wait_duration_seconds_total[1m]) is a better metric for detecting when we don't have enough connections. The in_use_connections metric is instantaneous when the metrics are collected and thus depends on the exact timing of collection.
There was a problem hiding this comment.
Good call-out. go_sql_wait_duration_seconds_total is more tricky to reason about (how much is too much?), but it's worth including. rate is a per-second measure which also hinders interpretation; I've gone for increase instead and aggregated by pod.
cli/server.go
Outdated
| if err != nil { | ||
| return 0, xerrors.Errorf("invalid max idle connections %q: must be %q or a positive integer", configuredIdle, codersdk.PostgresConnMaxIdleAuto) | ||
| } | ||
| if idle < 1 { |
There was a problem hiding this comment.
Why is the minimum 1? Zero is a an allowed value.
There was a problem hiding this comment.
I have hesitations about allowing 0 for a few reasons:
0means "unlimited" in other settings, and this is the opposite- Having no idle conns would mean additional latency on each query
0actually means2🙃
The reasons are fairly weak though, and it's molly-coddling the operator a bit.
I'm happy to allow 0 and document its semantics, and set a negative value for idle conns when the given value is 0 to actually mean 0.
WDYT?
There was a problem hiding this comment.
"0 actually means 2" is only an internal implementation detail. If you call db.SetMaxIdleConns(0) like we would, you get zero idle conns. https://cs.opensource.google/go/go/+/refs/tags/go1.25.5:src/database/sql/sql.go;drc=6961c3775f9358d2fe0b72f2626c4485228203d0;l=1001
There was a problem hiding this comment.
Ah, should've looked at that. Thanks for keeping me honest 👍
WDYT about points 1 & 2?
There was a problem hiding this comment.
re: 1, zero meaning unlimited in other contexts doesn't feel especially relevant in whether to allow it or not in this context. Is the concern that an operator wants "unlimited" and would put in 0? We don't have any text explaining why 0 is not allowed.
re: 2, the whole point of allowing customers to adjust this value is to allow them to trade off latency & churn against idle resource utilization. Zero doesn't seem especially worse than 1, for example. At high scale they are indistinguishable.
There was a problem hiding this comment.
Zero doesn't seem especially worse than 1, for example. At high scale they are indistinguishable.
Good point
Addressed in f1cd598
There was a problem hiding this comment.
Is the concern that an operator wants "unlimited" and would put in 0?
Indeed; I'ved added some verbiage to specify what 0 means.
Addressed in b10370a
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Closes #21360
A few considerations/notes:
"auto"by default so it's not another knob one has to twiddle, and sets max idle = max conns / 3