Skip to content

Handle auth_query correctly for replication connections#1166

Merged
JelteF merged 3 commits into
pgbouncer:masterfrom
JelteF:fix-1164
Dec 2, 2024
Merged

Handle auth_query correctly for replication connections#1166
JelteF merged 3 commits into
pgbouncer:masterfrom
JelteF:fix-1164

Conversation

@JelteF
Copy link
Copy Markdown
Member

@JelteF JelteF commented Sep 15, 2024

The way we send an auth_query is a bit strange. We temporarily set the
pool of the client to the pool for the auth_user, and then continue
connection startup like normal. This broke for replication connections
because we would send the replication parameter for that connection too.
This fixes that by only doing our special connection setup logic for
replication connections if we're not currently trying to send an
auth_query.

Fixes #1164

@JelteF JelteF force-pushed the fix-1164 branch 4 times, most recently from 51180fe to 8cacb6a Compare September 19, 2024 06:58
@dbarrosop
Copy link
Copy Markdown

Hi, how is it going? just wanted to check if there was any progress here as this is still not working as expected. Thanks!

The way we send an auth_query is a bit strange. We temporarily set the
pool of the client to the pool for the auth_user, and then continue
connection startup like normal. This broke for replication connections
because we would send the replication parameter for that connection too.
This fixes that by only doing special connection setup thing for
replication connections if we're not currently trying to send an
auth_query.

Fixes pgbouncer#1164
Copy link
Copy Markdown
Member

@eulerto eulerto left a comment

Choose a reason for hiding this comment

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

For some reason, the test_physical_rep_pg_basebackup is failing for me. There is no explicit errors in the log (Postgres and PgBouncer).

==================================================== short test summary info =====================================================
FAILED test/test_replication.py::test_physical_rep_pg_basebackup - Failed: Timeout >30.0s
=============================== 1 failed, 210 passed, 7 skipped, 13 warnings in 138.12s (0:02:18) ================================
make: *** [Makefile:162: check] Error 1

Comment thread src/client.c Outdated
Comment thread src/client.c
@JelteF
Copy link
Copy Markdown
Member Author

JelteF commented Nov 4, 2024

For some reason, the test_physical_rep_pg_basebackup is failing for me. There is no explicit errors in the log (Postgres and PgBouncer).

Is this only with this PR, or also on master? I cannot reproduce it, and CI is running fine too. So I'm leaning towards merging this unless this is only happening on this PR for you.

Regarding debugging, did you uncomment the following line during the test? That should give much more info into what's happening.
https://github.com/pgbouncer/pgbouncer/blob/master/test/utils.py#L919

@benchub
Copy link
Copy Markdown
Contributor

benchub commented Nov 14, 2024

FWIW, this issue was blocking us (because it turns out HUP'ing pgBouncer also resets connections which were made with credentials found in auth_file, and resetting logical replication connections on a regular basis is not great), but applying this patch seems to work well in our testing.

With this patch, we can now successfully establish and maintain a logical replication session authenticated with auth_query, even while we HUP pgBouncer every few minutes.

@JelteF JelteF merged commit 49623c6 into pgbouncer:master Dec 2, 2024
@d3qtian
Copy link
Copy Markdown

d3qtian commented Dec 19, 2024

Hi @JelteF
Thank you for addressing this issue and providing a fix! We are currently in urgent need of this functionality for our project. Could you kindly let us know when this fix will be released in the next version of pgbouncer? Your update will help us plan accordingly.

Thanks again for the great work and support!

rajaryanece pushed a commit to rajaryanece/pgbouncer that referenced this pull request Jan 16, 2025
The way we send an auth_query is a bit strange. We temporarily set the
pool of the client to the pool for the auth_user, and then continue
connection startup like normal. This broke for replication connections
because we would send the replication parameter for that connection too.
This fixes that by only doing our special connection setup logic for
replication connections if we're not currently trying to send an
auth_query.

Fixes pgbouncer#1164
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.

logical replication and auth_query issues

5 participants