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.

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

feat(compute): Set default application_name for pgbouncer connections #9973

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

ololobus
Copy link
Member

@ololobus ololobus commented Dec 2, 2024

Problem

When client specifies application_name, pgbouncer propagates it to the Postgres. Yet, if client doesn't do it, we have hard time figuring out who opens a lot of Postgres connections (including the cloud_admin ones).

See this investigation as an example: https://neondb.slack.com/archives/C0836R0RZ0D

Summary of changes

I haven't found this documented, but it looks like pgbouncer accepts standard Postgres connstring parameters in the connstring in the [databases] section, so put the default application_name=pgbouncer there. That way, we will always see who opens Postgres connections. I did tests, and if client specifies a application_name, pgbouncer overrides this default, so it only works if it's not specified or set to blank &application_name= in the connection string.

This is the last place we could potentially open some Postgres connections without application_name. Everything else should be either of two:

  1. Direct client connections without application_name, but these should be strictly non-cloud_admin ones
  2. Some ad-hoc internal connections, so if we see spikes of unidentified cloud_admin connections, we will need to investigate it again.

Fixes neondatabase/cloud#20948

When client specifies `application_name`, pgbouncer propagates it to
the Postgres. Yet, if client doesn't do it, we have hard time figuring
out who opens a lot of Postgres connections (including the `cloud_admin`
ones).

I haven't found this documented, but it looks like pgbouncer accepts
standard Postgres connstring parameters in the connstring in the
`[databases]` section, so put the default `application_name=pgbouncer`
there. That way, we will always see who opens Postgres connections. I
did tests, and if client specifies a `application_name`, pgboucner overrides
this default, so it only works if it's not specified or set to blank
`&application_name=` in the connection string.

I think this is the last place we could potentially open some Postgres
connections without `application_name`. Everything else should be either
of two:
1. Direct client connections without `application_name`, but these
   should be strictly non`cloud_admin` ones
2. Some ad-hoc internal connections
so if we will see spikes of unidentified `cloud_admin` connections, we
will need to investigate it again.

Fixes neondatabase/cloud#20948
@ololobus ololobus requested a review from tristan957 December 2, 2024 20:56
Copy link

github-actions bot commented Dec 2, 2024

7018 tests run: 6710 passed, 0 failed, 308 skipped (full report)


Flaky tests (5)

Postgres 17

Postgres 16

Postgres 15

Postgres 14

Code coverage* (full report)

  • functions: 30.7% (8263 of 26881 functions)
  • lines: 47.8% (65195 of 136467 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
33ea9b1 at 2024-12-02T21:57:29.993Z :recycle:

@ololobus ololobus added this pull request to the merge queue Dec 4, 2024
Merged via the queue into main with commit 9a4157d Dec 4, 2024
82 checks passed
@ololobus ololobus deleted the alexk/pgbouncer-appname branch December 4, 2024 13:06
ololobus added a commit that referenced this pull request Dec 4, 2024
We didn't have a codeowner for `/compute`, so nobody was auto-assigned
for PRs like #9973

While on it:
1. Group codeowners into sections.
2. Remove control plane from the `/compute_tools` because it's
   primarily the internal `compute_ctl` code.
3. Add control plane (and compute) to `/libs/compute_api` because that's
   the shared public interface of the compute.
awarus pushed a commit that referenced this pull request Dec 5, 2024
…#9973)

## Problem

When client specifies `application_name`, pgbouncer propagates it to the
Postgres. Yet, if client doesn't do it, we have hard time figuring out
who opens a lot of Postgres connections (including the `cloud_admin`
ones).

See this investigation as an example:
https://neondb.slack.com/archives/C0836R0RZ0D

## Summary of changes

I haven't found this documented, but it looks like pgbouncer accepts
standard Postgres connstring parameters in the connstring in the
`[databases]` section, so put the default `application_name=pgbouncer`
there. That way, we will always see who opens Postgres connections. I
did tests, and if client specifies a `application_name`, pgbouncer
overrides this default, so it only works if it's not specified or set to
blank `&application_name=` in the connection string.

This is the last place we could potentially open some Postgres
connections without `application_name`. Everything else should be either
of two:
1. Direct client connections without `application_name`, but these
should be strictly non-`cloud_admin` ones
2. Some ad-hoc internal connections, so if we see spikes of unidentified
`cloud_admin` connections, we will need to investigate it again.

Fixes neondatabase/cloud#20948
ololobus added a commit that referenced this pull request Dec 5, 2024
We didn't have a codeowner for `/compute`, so nobody was auto-assigned
for PRs like #9973

While on it:
1. Group codeowners into sections.
2. Remove control plane from the `/compute_tools` because it's
   primarily the internal `compute_ctl` code.
3. Add control plane (and compute) to `/libs/compute_api` because that's
   the shared public interface of the compute.
github-merge-queue bot pushed a commit that referenced this pull request Dec 6, 2024
## Problem

We didn't have a codeowner for `/compute`, so nobody was auto-assigned
for PRs like #9973

## Summary of changes

While on it:
1. Group codeowners into sections.
2. Remove control plane from the `/compute_tools` because it's primarily
the internal `compute_ctl` code.
3. Add control plane (and compute) to `/libs/compute_api` because that's
the shared public interface of the compute.
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