pg bouncer upgrade #1572
pg bouncer upgrade #1572
Conversation
|
Next steps are to work on the tests for this upgrade + default settings. |
encima
force-pushed
the
sam/pg-bouncer-upgrade
branch
from
f6b6c7a to
bbd1190
Compare
|
No config to add/change here as defaults are good; customisable config can be added to API later. Errors occur when testing in |
|
@encima re-ran the failed test. |
|
Tests for pgbouncer still to be written:
|
encima
force-pushed
the
sam/pg-bouncer-upgrade
branch
from
65c9561 to
074c41a
Compare
encima
force-pushed
the
sam/pg-bouncer-upgrade
branch
from
074c41a to
78fd845
Compare
encima
force-pushed
the
sam/pg-bouncer-upgrade
branch
from
939694e to
d147824
Compare
|
Noticed the make step seems to make source and docs now; fixing |
|
Benefit of 1.240 is that we do not need to change the default settings as max_prepared_statements is enabled by default and set to 200; we can expose configs later and optimise by instance Only clients connecting with prepared statements enabled (i.e. |
WalkthroughThis pull request upgrades PgBouncer from version 1.19.0 to 1.25.1, introduces a new Nix derivation for PgBouncer, extends Ansible installation conditions to include stage2_nix builds, adds dependency packages, and implements version validation in the test infrastructure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ansible/tasks/setup-pgbouncer.yml (1)
51-51: Typo:nolignshould benologin.The shell path
/usr/sbin/nolignappears to be a typo. This will cause user creation to fail or set an invalid shell.🐛 Proposed fix
- shell: '/usr/sbin/nolign' + shell: '/usr/sbin/nologin'
🤖 Fix all issues with AI agents
In `@ansible/tasks/setup-pgbouncer.yml`:
- Line 80: Change the task conditional that currently reads "when: nixpkg_mode"
so the placeholder files are created for all relevant build modes; update the
when expression to "debpkg_mode or nixpkg_mode or stage2_nix" (so downstream
lineinfile tasks that omit create and the subsequent file permission tasks will
find the files). Edit the task containing the current when: nixpkg_mode
condition (the placeholder file creation task) to use the expanded boolean
expression to match the parent import and ensure compatibility with the
lineinfile (lines ~103-106) and permission tasks (lines ~108-116).
In `@nix/pgbouncer.nix`:
- Around line 1-52: The pgbouncer derivation currently lists c-ares in
buildInputs but never enables it in configureFlags, so either remove c-ares from
the buildInputs list or enable it by adding the "--with-cares" configure flag;
locate the stdenv.mkDerivation block for pgbouncer and update the buildInputs
array (remove c-ares) if you don't want c-ares support, or append "--with-cares"
to the configureFlags expression (alongside the existing "--with-systemd"
option) if you do want c-ares DNS resolver enabled.
In `@testinfra/test_ami_nix.py`:
- Around line 174-175: The module declares EXPECTED_PGBOUNCER_VERSION
(initialized by load_expected_pgbouncer_version()) and PGBOUNCER_BINARY but
never uses them; either remove these unused module-level constants or add tests
that exercise them. To fix: if PgBouncer behavior is not needed, delete
EXPECTED_PGBOUNCER_VERSION, PGBOUNCER_BINARY and the call to
load_expected_pgbouncer_version() to avoid import-time work; otherwise implement
tests (e.g., test_pgbouncer_binary_exists and test_pgbouncer_version_matches)
that reference PGBOUNCER_BINARY and EXPECTED_PGBOUNCER_VERSION and validate the
binary exists and its version matches load_expected_pgbouncer_version() (using
subprocess or fixture helpers). Ensure references use the exact symbol names
EXPECTED_PGBOUNCER_VERSION, PGBOUNCER_BINARY, and
load_expected_pgbouncer_version() so future reviewers can locate the change.
🧹 Nitpick comments (1)
testinfra/test_ami_nix.py (1)
150-171: Consider more robust YAML/Nix parsing.The current string-based parsing is fragile—it doesn't handle quoted values consistently, comments on the same line, or whitespace variations. For YAML, consider using
yaml.safe_load()(already have PyYAML via boto3 dependencies). For Nix, regex would be more reliable.♻️ Optional: Use YAML parser for vars.yml
def load_expected_pgbouncer_version() -> str: + import yaml repo_root = Path(__file__).resolve().parent.parent ansible_vars = repo_root / "ansible" / "vars.yml" if ansible_vars.exists(): with ansible_vars.open() as f: - for raw_line in f: - line = raw_line.strip() - if line.startswith("pgbouncer_release:"): - return line.split(":", 1)[1].strip().strip('"') + data = yaml.safe_load(f) + if data and "pgbouncer_release" in data: + return str(data["pgbouncer_release"]) nix_file = repo_root / "nix" / "pgbouncer.nix" if nix_file.exists(): with nix_file.open() as f: - for raw_line in f: - line = raw_line.strip() - if line.startswith("version ="): - value = line.split("=", 1)[1].strip() - return value.strip(";").strip('"') + import re + content = f.read() + match = re.search(r'version\s*=\s*"([^"]+)"', content) + if match: + return match.group(1) raise RuntimeError( "Could not determine expected PgBouncer version from configuration files" )
| - 'custom-overrides.ini' | ||
| - 'generated-optimizations.ini' | ||
| - 'ssl-config.ini' | ||
| when: nixpkg_mode |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the Ansible file and examine its context
find . -name "setup-pgbouncer.yml" -type fRepository: supabase/postgres
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Check if other tasks or configurations depend on these placeholder files
rg -n "custom-overrides.ini|generated-optimizations.ini|ssl-config.ini" --type yaml -C2Repository: supabase/postgres
Length of output: 3416
🏁 Script executed:
#!/bin/bash
# Search for where nixpkg_mode, stage2_nix, and debpkg_mode are used in Ansible tasks
rg -n "nixpkg_mode|stage2_nix|debpkg_mode" --type yaml | head -50Repository: supabase/postgres
Length of output: 3347
🏁 Script executed:
#!/bin/bash
# Read the setup-pgbouncer.yml file to see context around line 80
wc -l ansible/tasks/setup-pgbouncer.ymlRepository: supabase/postgres
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Get lines 60-90 to understand the full task context
sed -n '60,90p' ansible/tasks/setup-pgbouncer.ymlRepository: supabase/postgres
Length of output: 929
🏁 Script executed:
#!/bin/bash
# Search for other task definitions that might create these placeholder files
rg -n "name:|touch|copy|file:" ansible/tasks/setup-pgbouncer.yml | head -30Repository: supabase/postgres
Length of output: 1118
🏁 Script executed:
#!/bin/bash
# Check if debpkg_mode tasks anywhere create these placeholder files
rg -B5 -A5 "custom-overrides.ini|generated-optimizations.ini" ansible/tasks/Repository: supabase/postgres
Length of output: 791
🏁 Script executed:
#!/bin/bash
# Read the entire setup-pgbouncer.yml file
cat -n ansible/tasks/setup-pgbouncer.ymlRepository: supabase/postgres
Length of output: 5005
🏁 Script executed:
#!/bin/bash
# Check if these tasks that reference the placeholder files have any conditions
sed -n '95,120p' ansible/tasks/setup-pgbouncer.ymlRepository: supabase/postgres
Length of output: 822
🏁 Script executed:
#!/bin/bash
# Search for where setup-pgbouncer.yml is included in playbook.yml to see the context
rg -B2 -A2 "setup-pgbouncer" ansible/playbook.ymlRepository: supabase/postgres
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Search for any alternative mechanisms that might create these files in debpkg_mode
rg -n "ssl-config.ini|custom-overrides.ini|generated-optimizations.ini" ansible/Repository: supabase/postgres
Length of output: 922
🏁 Script executed:
#!/bin/bash
# Check what the tmpfiles.d template does - it might regenerate these files
cat -n ansible/files/pgbouncer_config/tmpfiles.d-pgbouncer.conf.j2Repository: supabase/postgres
Length of output: 185
🏁 Script executed:
#!/bin/bash
# Check if lineinfile has a create option or if it requires the file to exist
sed -n '102,107p' ansible/tasks/setup-pgbouncer.ymlRepository: supabase/postgres
Length of output: 248
🏁 Script executed:
#!/bin/bash
# Verify the database_optimizations.service to confirm it depends on generated-optimizations.ini
cat -n ansible/files/database_optimizations.serviceRepository: supabase/postgres
Length of output: 626
🏁 Script executed:
#!/bin/bash
# Check if the entire setup-pgbouncer.yml is conditionally imported or unconditionally
grep -B5 -A5 "import_tasks.*setup-pgbouncer" ansible/playbook.ymlRepository: supabase/postgres
Length of output: 354
Add build mode conditions to placeholder file creation.
The when: nixpkg_mode condition is too restrictive. Downstream tasks (line 103-106: lineinfile without create option, line 108-116: file permissions) will fail for debpkg_mode and stage2_nix builds because these placeholder files won't exist. Update the condition to when: debpkg_mode or nixpkg_mode or stage2_nix to match the parent task import and ensure files exist for all build modes that run setup-pgbouncer.yml.
🤖 Prompt for AI Agents
In `@ansible/tasks/setup-pgbouncer.yml` at line 80, Change the task conditional
that currently reads "when: nixpkg_mode" so the placeholder files are created
for all relevant build modes; update the when expression to "debpkg_mode or
nixpkg_mode or stage2_nix" (so downstream lineinfile tasks that omit create and
the subsequent file permission tasks will find the files). Edit the task
containing the current when: nixpkg_mode condition (the placeholder file
creation task) to use the expanded boolean expression to match the parent import
and ensure compatibility with the lineinfile (lines ~103-106) and permission
tasks (lines ~108-116).
What kind of change does this PR introduce?
pgbouncer needs updating for for prepared statement support, addressing incorporation of upgrades
This PR updates the version to 1.24.1
This PR performs the upgrade, repackages the build of pgbouncer from source in a nix package, and will cover automated testing in testinfra as well.
Summary by CodeRabbit
New Features
Updates
✏️ Tip: You can customize this high-level summary in your review settings.