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.

Sign up for GitHub

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

Jump to bottom

Set default pool_max_conns to 20 for PostgreSQL #1852

Merged
merged 4 commits into from
Jan 23, 2023
Merged

Set default pool_max_conns to 20 for PostgreSQL #1852

merged 4 commits into from
Jan 23, 2023

Conversation

jkoenig134
Copy link
Contributor

Description

Closes #1844

Readiness checklist

  • I added/updated unit tests.
  • I added/updated integration/compatibility tests.
  • I added/updated comments and checked rendering.
  • I made spot refactorings.
  • I updated user documentation.
  • I ran task all, and it passed.
  • I ensured that PR title is good enough for the changelog.
  • (for maintainers only) I set Reviewers (@FerretDB/core), Assignee, Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@jkoenig134 jkoenig134 requested a review from a team as a code owner January 23, 2023 12:56
@CLAassistant
Copy link

CLAassistant commented Jan 23, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@AlekSi AlekSi added the code/enhancement Some user-visible feature could work better label Jan 23, 2023
@AlekSi AlekSi added this to the v0.9.0 Developer Preview milestone Jan 23, 2023
@AlekSi AlekSi enabled auto-merge (squash) January 23, 2023 13:04
@AlekSi AlekSi disabled auto-merge January 23, 2023 13:04
@AlekSi AlekSi changed the title set default pool_max_conns to 20 for PostgreSQL Set default pool_max_conns to 20 for PostgreSQL Jan 23, 2023
@AlekSi AlekSi enabled auto-merge (squash) January 23, 2023 13:04
@AlekSi
Copy link
Member

AlekSi commented Jan 23, 2023

Why not do that in

FerretDB/internal/handlers/pg/pgdb/pool.go

Lines 55 to 60 in d479051

// NewPool returns a new concurrency-safe connection pool.
//
// Passed context is used only by the first checking connection.
// Canceling it after that function returns does nothing.
func NewPool(ctx context.Context, uri string, logger *zap.Logger, p *state.Provider) (*Pool, error) {
config, err := pgxpool.ParseConfig(uri)

@jkoenig134
Copy link
Contributor Author

@AlekSi I assumed that like setting user + password pg.go should be responsible for the URI manipulation and in this method I do have a url.URL object instead of only a string like in pool.go.

@AlekSi
Copy link
Member

AlekSi commented Jan 23, 2023

Here we only use per-connection username and password. All other parameters, like host, port, and all query parameters are the same.

@jkoenig134
Copy link
Contributor Author

What about setting it in the NewOpts constructor?

@AlekSi
Copy link
Member

AlekSi commented Jan 23, 2023

Well, that's possible, but I really don't see a problem in using u, err := url.Parse(uri) and uri = u.String() in NewPool :) Seems to be the most natural place

@jkoenig134
set pool_max_conns in pool.go
09708b1
@jkoenig134
Merge branch 'Set-default-pool_max_conns-for-PostgreSQL-1844' of gith…
8d7817e 
…ub.com:jkoenig134/FerretDB into Set-default-pool_max_conns-for-PostgreSQL-1844
auto-merge was automatically disabled January 23, 2023 13:25

Head branch was pushed to by a user without write access

@AlekSi AlekSi enabled auto-merge (squash) January 23, 2023 13:26
@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #1852 (0a91e6b) into main (b77b7e8) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 0a91e6b differs from pull request most recent head 8d7817e. Consider uploading reports for the commit 8d7817e to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1852      +/-   ##
==========================================
+ Coverage   69.99%   70.02%   +0.03%     
==========================================
  Files         309      309              
  Lines       15197    15203       +6     
==========================================
+ Hits        10637    10646       +9     
  Misses       3590     3590              
+ Partials      970      967       -3
Impacted Files Coverage Δ
internal/handlers/pg/pg.go 79.03% <100.00%> (+2.24%) ⬆️
internal/handlers/pg/pgdb/collections.go 64.56% <0.00%> (-4.73%) ⬇️
internal/handlers/pg/pgdb/database_metadata.go 81.92% <0.00%> (-3.62%) ⬇️
internal/handlers/common/filter.go 84.22% <0.00%> (ø)
internal/clientconn/listener.go 77.04% <0.00%> (+1.53%) ⬆️
internal/clientconn/conn.go 47.53% <0.00%> (+2.60%) ⬆️
Flag Coverage Δ
integration 66.45% <100.00%> (+0.09%) ⬆️
mongodb 14.64% <100.00%> (+0.03%) ⬆️
pg 55.21% <100.00%> (+0.07%) ⬆️
tigris 45.26% <0.00%> (-0.02%) ⬇️
unit 28.11% <0.00%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@AlekSi AlekSi disabled auto-merge January 23, 2023 17:51
@AlekSi AlekSi merged commit c68831c into FerretDB:main Jan 23, 2023
@AlekSi
Copy link
Member

AlekSi commented Jan 23, 2023

Thank you!

@jkoenig134 jkoenig134 deleted the Set-default-pool_max_conns-for-PostgreSQL-1844 branch February 1, 2023 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekSi AlekSi AlekSi approved these changes

@w84thesun w84thesun Awaiting requested review from w84thesun w84thesun is a code owner automatically assigned from FerretDB/leads

@rumyantseva rumyantseva Awaiting requested review from rumyantseva rumyantseva is a code owner automatically assigned from FerretDB/leads

@chilagrow chilagrow Awaiting requested review from chilagrow chilagrow is a code owner automatically assigned from FerretDB/leads

Assignees
No one assigned
Labels
code/enhancement Some user-visible feature could work better
Projects
None yet
Milestone
v0.9.0 Developer Preview
Development

Successfully merging this pull request may close these issues.

Set default pool_max_conns=20 for PostgreSQL
3 participants
@jkoenig134 @CLAassistant @AlekSi