feat: support pgbouncer for multitenant database#865
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a production-ready pgBouncer container (Dockerfile, entrypoint script, ini template, README) that auto-tunes pool settings from AWS RDS instance type, supports TLS and healthchecks, and generates runtime config and credentials. Adds a GitHub Actions job to build and publish the pgBouncer image. Extends application config with storageS3DisableChecksum, multitenantDatabasePoolUrl, and multitenantMaxConnections; updates multitenant DB pooling and queue startup to use an optional pool URL and adjustable pool sizing. S3 client checksum behavior can be toggled via config. Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant PgBouncer as PgBouncer (container)
participant Postgres as Postgres (RDS)
Client->>PgBouncer: Connect using DATABASE_URL (may include TLS)
PgBouncer->>Postgres: Open or reuse pooled backend connection (auth via userlist, server TLS if configured)
Postgres-->>PgBouncer: Connection accepted / query result
PgBouncer-->>Client: Return query result
Note over PgBouncer,Postgres: Healthcheck: nc to Postgres:port -> validate connectivity/version
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.docker/pgbouncer/entrypoint.sh:
- Around line 204-249: The TLS_CONFIG variable is being built with embedded
newlines and then injected via sed -e "s|{{TLS_CONFIG}}|${TLS_CONFIG}|g", which
breaks when TLS_CONFIG is multiline; instead write TLS_CONFIG to a temporary
file (e.g., /tmp/pgbouncer_tls.conf) when DATABASE_SSL_ROOT_CERT is set (the
same logic around TLS_CONFIG and map_sslmode), then generate
/etc/pgbouncer/pgbouncer.ini by using sed to replace the {{TLS_CONFIG}}
placeholder by reading that file (use sed '/{{TLS_CONFIG}}/ { r
/tmp/pgbouncer_tls.conf' -e 'd' }' or equivalent) so the multiline content is
inserted safely from the temp file; ensure the rest of the sed substitutions for
single-line fields remain the same and remove the temp file after creation.
- Around line 14-24: The parsing logic using variables like userinfo, rest,
DB_USER and DB_PASS must validate that userinfo contains a ":" and both username
and password are non-empty (reject formats like "admin@host" with a clear error
and exit), and must URL-decode percent-encoded credentials before assigning
DB_USER and DB_PASS (or before writing to userlist.txt) so e.g.
"admin%40company" becomes "admin@company"; implement a check after extracting
userinfo to verify the presence of ":" and non-empty parts, perform
percent-decoding on the credential string (then split into DB_USER and DB_PASS),
and on failure print an error and exit to prevent using an incorrect DB_PASS.
In @.github/workflows/release.yml:
- Around line 119-171: The publish_pgbouncer job uses outdated action majors
(docker/metadata-action@v4, docker/setup-buildx-action@v2,
docker/login-action@v2, docker/build-push-action@v3,
aws-actions/configure-aws-credentials@v1); update those action references inside
the publish_pgbouncer job (and all other workflow jobs) to the supported majors:
docker/metadata-action@v5, docker/setup-buildx-action@v3,
docker/login-action@v3, docker/build-push-action@v6, and
aws-actions/configure-aws-credentials@v5, keeping the same inputs and step ids
(e.g., step id meta, the two configure aws credentials steps, Login to ECR
account steps, and the build-push step) to preserve behavior and outputs.
I guess meant pgbouncer but checking if you didn't forget anything you intended |
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/release.yml:
- Around line 119-171: The publish_pgbouncer job uses outdated action versions;
update the uses entries within the publish_pgbouncer steps (e.g., the
actions/checkout, docker/metadata-action (step id meta),
docker/setup-buildx-action, aws-actions/configure-aws-credentials,
docker/login-action, and docker/build-push-action steps) to match the required
versions used elsewhere: actions/checkout@v6, docker/metadata-action@v5,
docker/setup-buildx-action@v3, aws-actions/configure-aws-credentials@v5,
docker/login-action@v3, and docker/build-push-action@v6 so static analysis and
GitHub Actions accept the job.
b2554a2 to
b19cc9b
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/release.yml:
- Around line 130-163: Update the deprecated action versions used in the
publish_pgbouncer job: replace docker/metadata-action@v4 with
docker/metadata-action@v5, docker/setup-buildx-action@v2 with
docker/setup-buildx-action@v3, docker/login-action@v2 with
docker/login-action@v3, docker/build-push-action@v3 with
docker/build-push-action@v6, and aws-actions/configure-aws-credentials@v1 with
aws-actions/configure-aws-credentials@v5; edit the steps that reference those
exact identifiers (docker/metadata-action, docker/setup-buildx-action,
docker/login-action, docker/build-push-action,
aws-actions/configure-aws-credentials) so the workflow uses the supported majors
to avoid actionlint failures.
| USER postgres | ||
|
|
||
| HEALTHCHECK --interval=10s --timeout=3s --start-period=5s --retries=3 \ | ||
| CMD printf "SHOW VERSION;\n" | nc 127.0.0.1 6432 >/dev/null 2>&1 || exit 1 |
There was a problem hiding this comment.
I would drop printf since it doesn't validate protocol anyway
|
There is a code path where pgbouncer has an issue:
|
| query_timeout = 0 | ||
| query_wait_timeout = 120 | ||
| client_idle_timeout = 0 | ||
| ignore_startup_parameters = extra_float_digits,statement_timeout,lock_timeout,idle_in_transaction_session_timeout,options |
There was a problem hiding this comment.
since statement_timeout ignored, don't we need to pass query_timeout to enforce PG_QUEUE_READ_WRITE_TIMEOUT config?
…enant-db-pool-support
6578169 to
aa831cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/release.yml (1)
130-163:⚠️ Potential issue | 🟠 Major
publish_pgbounceruses action majors that actionlint says won’t run.These versions are flagged as too old for current runners, so the release workflow will fail until they’re bumped.
Proposed fix
- - id: meta - uses: docker/metadata-action@v4 + - id: meta + uses: docker/metadata-action@v5 @@ - - uses: docker/setup-buildx-action@v2 + - uses: docker/setup-buildx-action@v3 @@ - - name: configure aws credentials - staging - uses: aws-actions/configure-aws-credentials@v1 + - name: configure aws credentials - staging + uses: aws-actions/configure-aws-credentials@v5 @@ - - name: Login to ECR account - staging - uses: docker/login-action@v2 + - name: Login to ECR account - staging + uses: docker/login-action@v3 @@ - - name: configure aws credentials - prod - uses: aws-actions/configure-aws-credentials@v1 + - name: configure aws credentials - prod + uses: aws-actions/configure-aws-credentials@v5 @@ - - name: Login to ECR account - prod - uses: docker/login-action@v2 + - name: Login to ECR account - prod + uses: docker/login-action@v3 @@ - - uses: docker/build-push-action@v3 + - uses: docker/build-push-action@v6🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 130 - 163, Update the GitHub Actions usages to supported major versions so actionlint stops flagging them: replace docker/metadata-action@v4 with docker/metadata-action@v5, docker/setup-buildx-action@v2 with docker/setup-buildx-action@v3, aws-actions/configure-aws-credentials@v1 with aws-actions/configure-aws-credentials@v2, and docker/build-push-action@v3 with docker/build-push-action@v4 (leave docker/login-action@v2 if already current); update each use: lines referencing docker/metadata-action, docker/setup-buildx-action, aws-actions/configure-aws-credentials, and docker/build-push-action to the new majors and run the workflow to verify no breaking input changes are required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/internal/queue/queue.ts`:
- Around line 33-46: The current multitenant branch throws if
multitenantDatabaseUrl is absent even when multitenantDatabasePoolUrl is
provided, causing createPgBoss to crash for pooler-only configs; change the
logic in the isMultitenant block to only throw when neither
multitenantDatabasePoolUrl nor multitenantDatabaseUrl is set, set url =
multitenantDatabasePoolUrl if present (fall back to multitenantDatabaseUrl
otherwise), and set migrate = false when using multitenantDatabasePoolUrl so
createPgBoss is invoked with the pool URL and correct migrate flag; update any
use of the url and migrate variables (e.g., the createPgBoss invocation)
accordingly.
---
Duplicate comments:
In @.github/workflows/release.yml:
- Around line 130-163: Update the GitHub Actions usages to supported major
versions so actionlint stops flagging them: replace docker/metadata-action@v4
with docker/metadata-action@v5, docker/setup-buildx-action@v2 with
docker/setup-buildx-action@v3, aws-actions/configure-aws-credentials@v1 with
aws-actions/configure-aws-credentials@v2, and docker/build-push-action@v3 with
docker/build-push-action@v4 (leave docker/login-action@v2 if already current);
update each use: lines referencing docker/metadata-action,
docker/setup-buildx-action, aws-actions/configure-aws-credentials, and
docker/build-push-action to the new majors and run the workflow to verify no
breaking input changes are required.
ℹ️ Review info
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
.docker/pgbouncer/Dockerfile.docker/pgbouncer/README.md.docker/pgbouncer/entrypoint.sh.docker/pgbouncer/pgbouncer.ini.template.github/workflows/release.ymlsrc/config.tssrc/internal/database/multitenant-db.tssrc/internal/queue/queue.tssrc/storage/backend/s3/adapter.ts
✅ Files skipped from review due to trivial changes (1)
- .docker/pgbouncer/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/internal/database/multitenant-db.ts
- src/storage/backend/s3/adapter.ts
| let url = pgQueueConnectionURL ?? databaseURL | ||
| let migrate = true | ||
|
|
||
| if (isMultitenant && !pgQueueConnectionURL) { | ||
| if (!multitenantDatabaseUrl) { | ||
| throw new Error( | ||
| 'running storage in multi-tenant but DB_MULTITENANT_DATABASE_URL is not set' | ||
| ) | ||
| } | ||
| url = multitenantDatabaseUrl | ||
| url = multitenantDatabasePoolUrl || multitenantDatabaseUrl | ||
|
|
||
| if (multitenantDatabasePoolUrl) { | ||
| migrate = false | ||
| } |
There was a problem hiding this comment.
Pooler-only multitenant config still fails in createPgBoss.
createPgBoss throws when multitenantDatabaseUrl is missing even if multitenantDatabasePoolUrl is set, so pooler-only configs crash on startup. start() already allows the pool URL—this should be consistent.
Proposed fix
- if (isMultitenant && !pgQueueConnectionURL) {
- if (!multitenantDatabaseUrl) {
+ if (isMultitenant && !pgQueueConnectionURL) {
+ if (!multitenantDatabaseUrl && !multitenantDatabasePoolUrl) {
throw new Error(
- 'running storage in multi-tenant but DB_MULTITENANT_DATABASE_URL is not set'
+ 'running storage in multi-tenant but DB_MULTITENANT_DATABASE_URL or DB_MULTITENANT_DATABASE_POOL_URL is not set'
)
}
url = multitenantDatabasePoolUrl || multitenantDatabaseUrl
if (multitenantDatabasePoolUrl) {
migrate = false
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/internal/queue/queue.ts` around lines 33 - 46, The current multitenant
branch throws if multitenantDatabaseUrl is absent even when
multitenantDatabasePoolUrl is provided, causing createPgBoss to crash for
pooler-only configs; change the logic in the isMultitenant block to only throw
when neither multitenantDatabasePoolUrl nor multitenantDatabaseUrl is set, set
url = multitenantDatabasePoolUrl if present (fall back to multitenantDatabaseUrl
otherwise), and set migrate = false when using multitenantDatabasePoolUrl so
createPgBoss is invoked with the pool URL and correct migrate flag; update any
use of the url and migrate variables (e.g., the createPgBoss invocation)
accordingly.
What kind of change does this PR introduce?
Feat
What is the current behavior?
No support for pooler on multitenant database
What is the new behavior?