Skip to content

feat: configure database connection pool settings #4633

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

Closed
wants to merge 4 commits into from

Conversation

Alwayswithme
Copy link
Contributor

@Alwayswithme Alwayswithme commented Apr 16, 2025

Description

Changed default connection pool configuration for all database drivers.

Implementation Details

Based on benchmark testing and research, I found setting max connections to twice the number of CPU cores provides an optimal balance between performance and resource utilization, as shown in the benchmark results. However, I'm open to discussion on these specific values:

cores := runtime.NumCPU()
driver.GetDB().SetMaxOpenConns(cores * 2)      // **Should this be cores * 2?**
driver.GetDB().SetMaxIdleConns(cores)  // **Is half of max connections appropriate?**
driver.GetDB().SetConnMaxLifetime(time.Minute * 5)

Review Questions

  1. Are these default values appropriate for most use cases?
  2. Should we make these values configurable via config file?

References

Benchmark

Tested with SQLite driver on Linux (Intel i5-3230M @ 2.60GHz, 2 cores/4 threads):

goos: linux
goarch: amd64
pkg: github.com/usememos/memos/store/test
cpu: Intel(R) Core(TM) i5-3230M CPU @ 2.60GHz
BenchmarkDBConnPool/default_unlimited-4         	     100	  49079319 ns/op	         0 wait_count	         0 wait_duration_ms	   12509 B/op	      94 allocs/op
BenchmarkDBConnPool/max_conns_equals_cores-4    	     127	   9230205 ns/op	       123.0 wait_count	     54868 wait_duration_ms	    1963 B/op	      53 allocs/op
BenchmarkDBConnPool/max_conns_double_cores-4    	     100	  11410104 ns/op	        92.00 wait_count	     29387 wait_duration_ms	    3155 B/op	      55 allocs/op
BenchmarkDBConnPool/max_conns_25-4              	     100	  20643346 ns/op	        74.00 wait_count	     19161 wait_duration_ms	    3640 B/op	      60 allocs/op
BenchmarkDBConnPool/max_conns_50-4              	     100	  41783602 ns/op	        50.00 wait_count	      9294 wait_duration_ms	    4428 B/op	      68 allocs/op
BenchmarkDBConnPool/max_conns_100-4             	     100	  36894592 ns/op	         0 wait_count	         0 wait_duration_ms	    5111 B/op	      83 allocs/op
PASS
ok  	github.com/usememos/memos/store/test	20.815s

@Alwayswithme Alwayswithme requested a review from boojack as a code owner April 16, 2025 19:36
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the default database connection pool configurations and adds benchmark tests to compare various connection pool settings based on CPU cores.

  • Sets default values for max open connections, max idle connections, and connection max lifetime in the driver initialization.
  • Introduces benchmark tests to evaluate different connection pool settings and report connection pool metrics.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
store/test/store_bench_test.go Adds benchmark tests for multiple connection pool configurations.
store/db/db.go Updates default DB connection pool settings derived from the number of CPU cores.
Comments suppressed due to low confidence (2)

store/test/store_bench_test.go:28

  • [nitpick] Consider renaming 'lifeTime' to 'connMaxLifetime' for clarity and consistency with the parameter it configures.
lifeTime := time.Hour

store/test/store_bench_test.go:126

  • [nitpick] Consider uncommenting and reporting additional metrics (max_open_conns, conns_in_use, idle_conns) to provide a more comprehensive view of the connection pool performance in benchmarks.
// b.ReportMetric(float64(endStats.MaxOpenConnections), "max_open_conns")

@johnnyjoygh
Copy link
Collaborator

  • Are these default values appropriate for most use cases?
  • Should we make these values configurable via config file?

We currently haven't got any performance issues related to the database based on user feedback, so I think we can first add a flag to control it and avoid a breaking change (even though it's a good change).

- Introduced flags for configuring database connection pool:
  - `--max-open-conns`: Maximum number of open database connections
  - `--max-idle-conns`: Maximum number of idle database connections
  - `--conn-max-lifetime`: Maximum lifetime of a database connection
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.

2 participants