-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add pgbouncer #3884
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
Add pgbouncer #3884
Conversation
|
@aldy505 shall we make this part of the default setup? |
|
I think it would be great if it was, tracing UI is leaving a lot of idle connections, breaking sentry due to maximum connections reached, pgcat should help with that. |
|
@BYK If so, are there any preferences for a pgcat config file? Just a new folder+file |
1be9416 to
c08cf43
Compare
|
I'm really torn between making this default vs not. |
|
Additional insight: Our setup also now regularly reaches the connection limit and that freezes Sentry entirely. |
|
The reason we added this to our setup, was the same. We saw a large amount of idle connections and errors - even with low traffic in ingress and a single user doing Internal demo of trace views, and issues views. |
|
Alright. Lets make it as default. |
/pgcat.toml is fine, no need for a directory. Just like the redis.conf and nginx.conf. |
|
Got it. Will push update later |
|
I am out of RAM on my laptop for a test-run, which is unfortunate. @aldy505 Does the integration tests boot a new installed environment, or should I just boot a VM at work, to test out from scratch-installation? |
Yep, they do |
|
@aldy505 Can you approve workflow runs, so I can do a quick look using those, before spinning up a VM? (I'm gonna do both, either way) |
|
@frederikspang looks like it's broken: |
aldy505
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great if you can add healthcheck to pgcat container. See postgresml/pgcat#607
|
Anyway I'll just... |
|
|
@aldy505 Wanna go for the healthcheck command in the linked PR? Or just for pg_isready like the postgres container? |
Up to you. What we need is to move other containers to depends on pgcat rather than postgres. |
|
they depend on both right now, in the x-sentry-defaults; We can remove postgres, no problem. |
Yep yep, let's make sure pgcat is healthy first before any other container starts. |
|
@BYK Can you approve workflow run? :) |
|
done |
aldy505
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
|
On a side note, I really didn't like #2740 then :) Let's revert that after merging this and changing |
Co-authored-by: Reinaldy Rafli <[email protected]>
b7241cc to
dd775d1
Compare
|
I want to merge this first, then (in concurrent manner):
|
This PR may only be merged after #3884 has been merged
|
Install script is here: #3898 @aminvakil can you create the revert PR, so I can quickly approve & merge? |
Done in #3899 . |
|
Cool. Let me merge this one. |
This reverts commit 2862432.
Add an optional-modification for pgbouncer. This has been extracted from our internal setup after discussion on #self-hosted in Discord.
Regarding pgbouncer, see also #2740
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.