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

Support replication connections through PgBouncer #876

Merged
merged 1 commit into from
May 6, 2024
Merged

Support replication connections through PgBouncer #876

merged 1 commit into from
May 6, 2024

Conversation

JelteF
Copy link
Member

@JelteF JelteF commented Jun 30, 2023 •
edited
Loading

In session pooling mode PgBouncer is pretty much a transparent proxy,
i.e. the client does normally not even need to know that PgBouncer is in
the middle. This allows things like load balancing and failovers without
the client needing to know about this at all. But as soon as replication
connections are needed, this was not possible anymore, because PgBouncer
would reject those instead of proxying them to the right server.

This PR fixes that by also proxying replication connections. They are
handled pretty differently from normal connections though. A client and
server replication connection will form a strong pair, as soon as one is
closed the other is closed too. So, there's no caching of the server
replication connections, like is done for regular connections. Reusing
replication connections comes with a ton of gotchas. Postgres will
throw errors in many cases when trying to do so. So simply not
doing it seems like a good tradeoff for ease of implementation.
Especially because replication connections are pretty much always
very long lived. So re-using them gains pretty much no performance
benefits.

Fixes #382

Depends on #945

@JelteF JelteF mentioned this pull request Jun 30, 2023
Closed
@JelteF JelteF force-pushed the replication-support branch 2 times, most recently from 186c1e1 to dff7db4 Compare June 30, 2023 13:00
@JelteF JelteF mentioned this pull request Jul 3, 2023
Merged
@JelteF JelteF force-pushed the replication-support branch 5 times, most recently from 42102ac to 1a3c5fb Compare July 3, 2023 11:15
@Melkij
Copy link

Melkij commented Jul 3, 2023

So re-using them gains pretty much no performance benefits.

Moreover, replication connections cannot be reliably reused at all. Attempting to do so will lead to various mysteries like

elog(ERROR, "no record found"); /* shouldn't happen */

in wal sender code. Wal sender implicitly assumes that it works with one wal receiver.
(Based on my experience with a single cloud service where admins tried to use connection pooling for replication too)

@JelteF
Copy link
Member Author

JelteF commented Jul 3, 2023

Moreover, replication connections cannot be reliably reused at all.

Thanks. I updated the wording in the PR to include that.

@JelteF JelteF force-pushed the replication-support branch 6 times, most recently from 7229637 to 422923a Compare July 3, 2023 14:12
JelteF
JelteF commented Jul 5, 2023
View reviewed changes
src/admin.c Outdated Show resolved Hide resolved
petere
petere reviewed Jul 10, 2023
View reviewed changes
doc/config.md Outdated Show resolved Hide resolved
include/bouncer.h Outdated Show resolved Hide resolved
include/bouncer.h Outdated Show resolved Hide resolved
@JelteF JelteF force-pushed the replication-support branch 2 times, most recently from c556d96 to 8a69366 Compare July 18, 2023 07:04
@JelteF JelteF force-pushed the replication-support branch 8 times, most recently from aee39a6 to c697684 Compare July 18, 2023 15:43
@eulerto
Copy link
Member

eulerto commented Sep 5, 2023

Isn't the cancel key change a requirement to this feature? I suggest you to create a separate commit for it.

@eulerto
Copy link
Member

eulerto commented Sep 5, 2023

Isn't the cancel key change a requirement to this feature? I suggest you to create a separate commit for it.

Ops... I realized that you did it. Another suggestion is to explain in the commit message that it is a requirement for this feature.

@JelteF JelteF force-pushed the replication-support branch 3 times, most recently from c936ee2 to d6b65d1 Compare September 6, 2023 07:16
@JelteF
Copy link
Member Author

JelteF commented Sep 6, 2023

Ops... I realized that you did it. Another suggestion is to explain in the commit message that it is a requirement for this feature.

I added a commit message and moved it before the actual replication change, but while doing that I realized I thought the change was special enough to warrant its own PR, so I made a new one: #945

petere
petere previously requested changes Oct 4, 2023
View reviewed changes
.editorconfig Outdated Show resolved Hide resolved
include/common/postgres_compat.h Outdated Show resolved Hide resolved
include/common/postgres_compat.h Outdated Show resolved Hide resolved
src/hba.c Outdated Show resolved Hide resolved
src/proto.c Show resolved Hide resolved
src/server.c Outdated Show resolved Hide resolved
include/bouncer.h Outdated Show resolved Hide resolved
src/server.c Show resolved Hide resolved
src/janitor.c Outdated Show resolved Hide resolved
src/janitor.c Outdated Show resolved Hide resolved
@JelteF JelteF force-pushed the replication-support branch 4 times, most recently from ddb1721 to 804ec1d Compare October 5, 2023 07:51
@JelteF JelteF force-pushed the replication-support branch 3 times, most recently from 0756a88 to 55f8cc4 Compare January 29, 2024 22:24
@JelteF JelteF mentioned this pull request Feb 7, 2024
Merged
@JelteF JelteF force-pushed the replication-support branch 2 times, most recently from d58be88 to f259bd1 Compare March 6, 2024 10:24
@JelteF JelteF force-pushed the replication-support branch 3 times, most recently from f3e82ef to c17ba04 Compare May 6, 2024 10:27
@JelteF JelteF enabled auto-merge (squash) May 6, 2024 10:28
@JelteF JelteF dismissed petere’s stale review May 6, 2024 10:29

All review items are resolved now

@JelteF JelteF disabled auto-merge May 6, 2024 10:33
@JelteF
Support replication connections through PgBouncer
d7e067d 
In session pooling mode PgBouncer is pretty much a transparent proxy,
i.e. the client does normally not even need to know that PgBouncer is in
the middle. This allows things like load balancing and failovers without
the client needing to know about this at all. But as soon as replication
connections are needed, this was not possible anymore, because PgBouncer
would reject those instead of proxying them to the right server.

This PR fixes that by also proxying replication connections. They are
handled pretty differently from normal connections though. A client and
server replication connection will form a strong pair, as soon as one is
closed the other is closed too. So, there's no caching of the server
replication connections, like is done for regular connections. Reusing
replication connections comes with a ton of gotchas. Postgres will throw
errors in many cases when trying to do so. So simply not doing it seems
like a good tradeoff for ease of implementation. Especially because
replication connections are pretty much always very long lived. So
re-using them gains pretty much no performance benefits.

Fixes pgbouncer#382
@JelteF JelteF merged commit e1c6c2f into pgbouncer:master May 6, 2024
7 checks passed
@JelteF JelteF deleted the replication-support branch May 6, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eulerto eulerto eulerto left review comments

@petere petere petere left review comments

@emelsimsek emelsimsek Awaiting requested review from emelsimsek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Getting error while running the pg_basebackup through PGBOUNCER
4 participants
@JelteF @Melkij @eulerto @petere