feat: PgBouncer migration tooling — config parser, converter + proxysql-cli, and PgBouncer-compatible SHOW commands - #5566
Open
renecannao wants to merge 12 commits into
Open
feat: PgBouncer migration tooling — config parser, converter + proxysql-cli, and PgBouncer-compatible SHOW commands#5566renecannao wants to merge 12 commits into
renecannao wants to merge 12 commits into
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:
📝 WalkthroughWalkthroughAdds a PgBouncer compatibility subsystem: public Config data model and parser APIs, INI/auth/HBA parsers with include resolution, conversion to ProxySQL SQL (with dry‑run/strict modes), SHOW-command translation, a proxysql‑cli, Admin integration for import, build/test updates, and unit tests/fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant ConfigParser as ConfigParser (INI)
participant FileSystem as FileSystem
participant AuthParser as AuthFileParser
participant HBAParser as HBAParser
Caller->>ConfigParser: parse_config_file(path, config)
ConfigParser->>FileSystem: open pgbouncer.ini
FileSystem-->>ConfigParser: contents
ConfigParser->>ConfigParser: parse sections & connstrs
alt %include encountered
ConfigParser->>FileSystem: open included file
FileSystem-->>ConfigParser: included contents
ConfigParser->>ConfigParser: parse included contents (recursively)
end
alt resolve_referenced_files enabled
ConfigParser->>FileSystem: open auth_file (if set)
FileSystem-->>ConfigParser: auth contents
ConfigParser->>AuthParser: parse_auth_file(path)
AuthParser-->>ConfigParser: entries + errors
ConfigParser->>FileSystem: open auth_hba_file (if set)
FileSystem-->>ConfigParser: hba contents
ConfigParser->>HBAParser: parse_hba_file(path)
HBAParser-->>ConfigParser: rules + errors
end
ConfigParser-->>Caller: success/failure + config (errors/warnings populated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation All Stage 1 deliverables from issue Full details: Out of Scope Changes checkExplanation Admin_Handler.cpp and src/main.cpp additions are Stage 2 converter/CLI integration, appearing premature for a Stage 1 library PR but required for functional import support; all changes align with Stage 1 foundation and documented objectives. Full details: Title checkExplanation The title clearly summarizes the pull request's main changes: PgBouncer configuration migration tooling, the proxysql-cli entry point, and PgBouncer-compatible SHOW command support. It is specific and related to the changeset. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a PgBouncer compatibility module, providing parsers for pgbouncer.ini, userlist.txt, and pg_hba.conf files, along with a comprehensive unit testing suite. The review feedback highlights several areas for improvement in the parsing logic, specifically regarding robust integer range checking, handling quoted values in the global configuration section, refining comment-stripping to respect quotes, and supporting escaped double quotes in the HBA tokenizer.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an initial, standalone C++ parsing library under lib/pgbouncer_compat/ to read PgBouncer configuration files into structured data, forming Stage 1 of the PgBouncer→ProxySQL migration tooling effort.
Changes:
- Introduces
PgBouncer_Config.hplus parsers forpgbouncer.ini,userlist.txt, andpg_hba.conf. - Adds TAP unit tests and fixture files covering expected and malformed inputs.
- Integrates the new objects into
lib/Makefileand registers a standalone unit-test target intest/tap/tests/unit/Makefile.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
include/PgBouncer_Config.h |
Defines the public data model + free-function parsing API. |
lib/pgbouncer_compat/PgBouncer_ConfigParser.{h,cpp} |
Implements INI + connstr parsing, includes, and referenced-file resolution. |
lib/pgbouncer_compat/PgBouncer_AuthFileParser.{h,cpp} |
Implements userlist.txt parsing with password type detection. |
lib/pgbouncer_compat/PgBouncer_HBAParser.{h,cpp} |
Implements PgBouncer-flavored pg_hba.conf parsing. |
test/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp |
Adds unit coverage for config/auth/HBA parsing and include behavior. |
test/tap/tests/unit/Makefile |
Registers new unit test and adds a standalone build rule. |
test/tap/tests/unit/fixtures/pgbouncer_compat/* |
Adds test inputs for minimal/full/include/malformed configs, HBA, and userlist. |
lib/Makefile |
Adds new .oo objects and explicit build rules for the new module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
lib/pgbouncer_compat/PgBouncer_ConfigParser.cpp (2)
536-542: Inconsistent path separator handling for relative%includepaths.Line 520 correctly handles both
/and\\when extractingbase_dir, but line 540 only checks for/when determining if a path is absolute. On Windows, paths likeC:\path\file.iniwould be incorrectly treated as relative.♻️ Consistent cross-platform path check
std::string inc_path = trim(trimmed.substr(9)); // Resolve relative paths against the base directory - if (!inc_path.empty() && inc_path[0] != '/') { + if (!inc_path.empty() && inc_path[0] != '/' && + !(inc_path.size() >= 2 && inc_path[1] == ':')) { // Windows drive letter inc_path = base_dir + inc_path; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pgbouncer_compat/PgBouncer_ConfigParser.cpp` around lines 536 - 542, The code treats an include path as absolute only if it starts with '/', which breaks on Windows; update the logic in PgBouncer_ConfigParser.cpp around the %include handling (variables: trimmed, resolve_includes, inc_path, base_dir) to detect absolute paths cross-platform — accept paths starting with '/' or '\\', and also treat Windows drive-letter prefixes (e.g., 'C:') as absolute — otherwise prepend base_dir when making inc_path relative.
54-65: Potential integer overflow onlongtointcast.
std::stolreturnslongwhich is 64-bit on many platforms. The cast tointcan truncate large values. For PgBouncer config values this is unlikely to cause issues in practice, but consider adding range validation.♻️ Optional: Add range check before cast
bool ConfigParser::parse_int(const std::string& value, int& result) { if (value.empty()) return false; try { size_t pos = 0; long v = std::stol(value, &pos); if (pos != value.size()) return false; + if (v < std::numeric_limits<int>::min() || v > std::numeric_limits<int>::max()) return false; result = static_cast<int>(v); return true; } catch (...) { return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pgbouncer_compat/PgBouncer_ConfigParser.cpp` around lines 54 - 65, The parse_int function currently parses into a long via std::stol and blindly casts to int (in ConfigParser::parse_int), which can truncate/overflow; modify parse_int to validate that the parsed long (v) is within the int range (INT_MIN <= v <= INT_MAX) before assigning to result and returning true, and return false if out-of-range or on parse errors; use the existing pos check from std::stol and keep the same error handling behavior.lib/pgbouncer_compat/PgBouncer_HBAParser.cpp (1)
44-78: Tokenizer doesn't handle escaped quotes inside double-quoted strings.Unlike
AuthFileParser::parse_quoted_string, this tokenizer doesn't handle""escape sequences within quoted tokens. If an HBA file contains a value with embedded quotes (e.g.,"user with ""quotes"""), it would be incorrectly parsed. This is unlikely in practice for pg_hba.conf, but worth noting.♻️ Optional: Add escaped quote handling for consistency
if (in_quotes) { if (c == '"') { + // Check for escaped quote "" + if (i + 1 < line.size() && line[i + 1] == '"') { + token += '"'; + ++i; + } else { in_quotes = false; + } } else { token += c; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pgbouncer_compat/PgBouncer_HBAParser.cpp` around lines 44 - 78, The tokenizer HBAParser::tokenize must handle escaped double quotes inside quoted tokens like AuthFileParser::parse_quoted_string does; update the in_quotes branch so when you see a '"' check if the next character is also '"' (i+1 < line.size() && line[i+1] == '"'), and if so append a single '"' to token and advance the index to skip the escaped quote, otherwise treat it as the closing quote; preserve existing behavior for comments, whitespace splitting, and final token push.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/PgBouncer_Config.h`:
- Around line 1-2: The include guard macro PGBOUNCER_CONFIG_H does not follow
the required pattern for headers in include/, so replace the existing guard
macro (PGBOUNCER_CONFIG_H) with the formal pattern __CLASS_PGBOUNCER_CONFIG_H
throughout the file (both the `#ifndef/`#define and the trailing `#endif` comment)
so the header uses __CLASS_PGBOUNCER_CONFIG_H consistently; update any
references to PGBOUNCER_CONFIG_H in this header to the new macro name to avoid
mismatches.
- Around line 10-220: Rename all public types and function signatures to include
the protocol prefix (e.g., PgSQL_) and update all references; specifically
rename structs and enum: ParseMessage -> PgSQL_ParseMessage, GlobalSettings ->
PgSQL_GlobalSettings, Database -> PgSQL_Database, User -> PgSQL_User, Peer ->
PgSQL_Peer, AuthFileEntry -> PgSQL_AuthFileEntry, HBARule -> PgSQL_HBARule,
Config -> PgSQL_Config, and enum AuthType -> PgSQL_AuthType (adjust enum values
if needed); also update parser function signatures parse_config_file,
parse_auth_file, parse_hba_file to accept/return PgSQL_Config / PgSQL_* types
accordingly and update any code that constructs or references these types and
their members to use the new names.
In `@lib/pgbouncer_compat/PgBouncer_HBAParser.h`:
- Line 10: Rename the class HBAParser to PgSQL_HBAParser and update all
corresponding declarations and usages: change the class declaration name in the
header, rename any constructors/destructors (e.g., HBAParser::HBAParser ->
PgSQL_HBAParser::PgSQL_HBAParser), update forward declarations, type references,
factory functions, method calls, casts, and any places the symbol HBAParser is
referenced in .cpp/.hpp/.h files and tests; ensure header guards, exports, and
ABI-visible symbols (if any) are updated to match the new class name so builds
and linkage continue to succeed.
In `@test/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp`:
- Around line 351-363: The TAP plan in main() is wrong: replace the call
plan(127) with plan(135) so the declared test count matches the actual 135
assertions run by test_minimal_config(), test_full_config(), test_auth_file(),
test_hba_file(), test_malformed_config(), test_include_directive(),
test_nonexistent_file(), and test_defaults(); ensure the documented per-test
comments (counts next to each test call) are updated to reflect the correct
distribution or remove stale counts, then return exit_status() as before.
---
Nitpick comments:
In `@lib/pgbouncer_compat/PgBouncer_ConfigParser.cpp`:
- Around line 536-542: The code treats an include path as absolute only if it
starts with '/', which breaks on Windows; update the logic in
PgBouncer_ConfigParser.cpp around the %include handling (variables: trimmed,
resolve_includes, inc_path, base_dir) to detect absolute paths cross-platform —
accept paths starting with '/' or '\\', and also treat Windows drive-letter
prefixes (e.g., 'C:') as absolute — otherwise prepend base_dir when making
inc_path relative.
- Around line 54-65: The parse_int function currently parses into a long via
std::stol and blindly casts to int (in ConfigParser::parse_int), which can
truncate/overflow; modify parse_int to validate that the parsed long (v) is
within the int range (INT_MIN <= v <= INT_MAX) before assigning to result and
returning true, and return false if out-of-range or on parse errors; use the
existing pos check from std::stol and keep the same error handling behavior.
In `@lib/pgbouncer_compat/PgBouncer_HBAParser.cpp`:
- Around line 44-78: The tokenizer HBAParser::tokenize must handle escaped
double quotes inside quoted tokens like AuthFileParser::parse_quoted_string
does; update the in_quotes branch so when you see a '"' check if the next
character is also '"' (i+1 < line.size() && line[i+1] == '"'), and if so append
a single '"' to token and advance the index to skip the escaped quote, otherwise
treat it as the closing quote; preserve existing behavior for comments,
whitespace splitting, and final token push.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e48e2aa3-5ba7-40c0-9a6a-009621a4590c
📒 Files selected for processing (17)
include/PgBouncer_Config.hlib/Makefilelib/pgbouncer_compat/PgBouncer_AuthFileParser.cpplib/pgbouncer_compat/PgBouncer_AuthFileParser.hlib/pgbouncer_compat/PgBouncer_ConfigParser.cpplib/pgbouncer_compat/PgBouncer_ConfigParser.hlib/pgbouncer_compat/PgBouncer_HBAParser.cpplib/pgbouncer_compat/PgBouncer_HBAParser.htest/tap/tests/unit/Makefiletest/tap/tests/unit/fixtures/pgbouncer_compat/full.initest/tap/tests/unit/fixtures/pgbouncer_compat/include_databases.initest/tap/tests/unit/fixtures/pgbouncer_compat/include_main.initest/tap/tests/unit/fixtures/pgbouncer_compat/malformed.initest/tap/tests/unit/fixtures/pgbouncer_compat/minimal.initest/tap/tests/unit/fixtures/pgbouncer_compat/pg_hba.conftest/tap/tests/unit/fixtures/pgbouncer_compat/userlist.txttest/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: Agent
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
lib/pgbouncer_compat/PgBouncer_AuthFileParser.hlib/pgbouncer_compat/PgBouncer_HBAParser.hlib/pgbouncer_compat/PgBouncer_ConfigParser.hlib/pgbouncer_compat/PgBouncer_AuthFileParser.cpptest/tap/tests/unit/pgbouncer_config_parser_unit-t.cpplib/pgbouncer_compat/PgBouncer_ConfigParser.cpplib/pgbouncer_compat/PgBouncer_HBAParser.cppinclude/PgBouncer_Config.h
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
lib/pgbouncer_compat/PgBouncer_AuthFileParser.cpptest/tap/tests/unit/pgbouncer_config_parser_unit-t.cpplib/pgbouncer_compat/PgBouncer_ConfigParser.cpplib/pgbouncer_compat/PgBouncer_HBAParser.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention
Files:
lib/pgbouncer_compat/PgBouncer_AuthFileParser.cpplib/pgbouncer_compat/PgBouncer_ConfigParser.cpplib/pgbouncer_compat/PgBouncer_HBAParser.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Files:
test/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards must follow the pattern
#ifndef_CLASS*_H
Files:
include/PgBouncer_Config.h
🧠 Learnings (9)
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
lib/Makefiletest/tap/tests/unit/Makefile
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
lib/Makefiletest/tap/tests/unit/Makefile
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Feature tiers are controlled by build flags: PROXYSQL31=1 for v3.1.x, PROXYSQLGENAI=1 for v4.0.x; PROXYSQLGENAI=1 implies PROXYSQL31=1
Applied to files:
lib/Makefile
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Admin interface uses SQLite3 backend for SQL-based configuration and schema versions are tracked in ProxySQL_Admin_Tables_Definitions.h
Applied to files:
lib/Makefile
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Applied to files:
lib/Makefile
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Applied to files:
test/tap/tests/unit/Makefiletest/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Applied to files:
test/tap/tests/unit/Makefiletest/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp
🪛 Clang (14.0.6)
test/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp
[error] 9-9: 'tap.h' file not found
(clang-diagnostic-error)
include/PgBouncer_Config.h
[error] 4-4: 'string' file not found
(clang-diagnostic-error)
🔇 Additional comments (23)
test/tap/tests/unit/fixtures/pgbouncer_compat/include_databases.ini (1)
1-2: Fixture looks correct for include-database coverage.This is a clean minimal fixture for validating
%includedatabase merge behavior.test/tap/tests/unit/fixtures/pgbouncer_compat/minimal.ini (1)
1-6: Minimal fixture is well-scoped.Good baseline input for parser default/value-path unit tests.
test/tap/tests/unit/fixtures/pgbouncer_compat/userlist.txt (1)
1-4: Auth fixture coverage is strong.This includes the key credential formats and quoting edge case needed for parser validation.
test/tap/tests/unit/fixtures/pgbouncer_compat/include_main.ini (1)
1-5: Include fixture is correctly structured.Good focused input for validating
%includeresolution from a primary config.test/tap/tests/unit/fixtures/pgbouncer_compat/malformed.ini (1)
1-9: Malformed fixture is effective for negative-path testing.This should exercise unknown-key and unknown-section error reporting as intended.
lib/Makefile (1)
119-123: Build integration changes look good.The new parser modules are consistently added to object aggregation and have explicit compilation rules aligned with their subdirectory layout.
Also applies to: 155-163
test/tap/tests/unit/fixtures/pgbouncer_compat/pg_hba.conf (1)
1-6: HBA fixture has good rule coverage.It exercises multiple connection types, auth methods, and option parsing in a compact form.
test/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp (7)
9-9: Static analysis false positive —tap.his resolved via Makefile include path.The Clang error about
tap.hnot found is a false positive. The Makefile correctly specifies-I$(TAP_IDIR)which resolves the include path at build time.
22-35: LGTM! Minimal config test with appropriate assertions.The test validates core parsing functionality and default values for unspecified fields.
40-187: LGTM! Comprehensive full config test with thorough coverage.The test validates all major parsing scenarios: global settings, database entries (including multi-host, wildcard, connect_query), user overrides, peer definitions, and auth file resolution. The iteration-based lookups correctly avoid assumptions about ordering.
192-217: LGTM! Auth file standalone parsing test.Good coverage of password type detection (PLAIN, MD5, SCRAM) and escaped quote handling.
222-260: LGTM! HBA file parsing test with good rule coverage.Tests local, host (IPv4/IPv6), hostssl with options, and hostnossl reject rules.
265-289: LGTM! Malformed config error detection test.Appropriately tests that invalid configs are rejected and errors are populated.
294-312: LGTM! Include directive test with fallback handling.The conditional check for empty databases vector with explicit skip markers is a good pattern for handling test dependencies.
test/tap/tests/unit/Makefile (1)
316-325: LGTM! Standalone test build rule is correctly structured.The build rule appropriately compiles the PgBouncer parser sources directly with the test file, links only
tap.oandtap_noise_stubs.o(nolibproxysql.a), and adds the correct include path. This aligns with the PR objective of a standalone library with no ProxySQL dependencies. Based on learnings: "pure data-structure/utility tests only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional."test/tap/tests/unit/fixtures/pgbouncer_compat/full.ini (1)
1-52: LGTM! Comprehensive test fixture covering key parsing scenarios.The fixture appropriately covers multiple parsing scenarios: global settings, multi-host databases with comma-separated hosts, wildcard database entry (
*), single-quotedconnect_query, per-user overrides, and peer definitions. This provides good coverage for the unit tests.lib/pgbouncer_compat/PgBouncer_AuthFileParser.h (1)
1-35: LGTM! Clean header design with appropriate encapsulation.The public API is minimal and well-documented. The class correctly encapsulates parsing logic with private helpers. The namespace
PgBouncer::provides appropriate scoping for this compatibility library.lib/pgbouncer_compat/PgBouncer_ConfigParser.h (1)
1-57: LGTM! Well-designed public API with appropriate configuration options.The
parse()method's optional parameters (resolve_includes,resolve_referenced_files) provide flexibility for different use cases (e.g., testing without file I/O). TheMAX_INCLUDE_DEPTHconstant properly guards against infinite recursion.lib/pgbouncer_compat/PgBouncer_AuthFileParser.cpp (1)
1-158: LGTM! Robust auth file parser implementation.The implementation correctly handles:
- Escaped quotes (
""→") in quoted strings- Password type detection matching PgBouncer's expected formats (SCRAM-SHA-256$, md5+32hex, plain)
- Graceful error recovery (continues parsing after malformed lines)
- Comment lines starting with
#or;The behavior of silently ignoring extra fields after the second quoted string appropriately matches PgBouncer's documented behavior.
lib/pgbouncer_compat/PgBouncer_ConfigParser.cpp (2)
591-603: Inline comment stripping logic could incorrectly truncate values containing#or;.The current logic strips inline comments for
[pgbouncer]section values that don't start with a single quote. However, unquoted values containing#or;(e.g., a parameter that legitimately contains these characters) would be truncated. This matches PgBouncer's behavior, but the comment could clarify this is intentional.
650-671: LGTM! Referenced file resolution at correct depth.The
include_depth_ == 0check ensures auth/HBA files are resolved only once after the main parse completes, avoiding redundant parsing during nested%includeprocessing.lib/pgbouncer_compat/PgBouncer_HBAParser.cpp (2)
84-166: LGTM! Robust HBA record parsing with appropriate validation.The parser correctly handles:
- All valid connection types (local, host, hostssl, hostnossl)
- Address/mask detection heuristics for non-CIDR addresses
- All PgBouncer-supported authentication methods
- Strict key=value option parsing
The mask detection logic at lines 125-136 appropriately handles the ambiguous case where a separate netmask follows an address without CIDR notation.
172-202: LGTM! File parsing with proper error accumulation.The parser continues processing after individual record errors, accumulating all diagnostics while returning overall success/failure status.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp`:
- Around line 138-150: The current generator builds dest_db but still inserts
routing for db.name only, silently dropping db.dbname aliases; update the logic
in PgBouncer_ConfigConverter.cpp around dest_db, sql, comment and rule_id so
that if db.dbname != db.name you do not emit a misleading pgsql_query_rules
INSERT for db.name but instead surface the mapping as unmappable (e.g., add a
warning/error entry or record it in the converter's diagnostics) indicating the
alias (db.name) points to a different backend (db.dbname) until you implement
backend DB rewrite; ensure the diagnostic references the rule_id and both
db.name and db.dbname so callers can find and address these entries.
- Around line 319-345: The UPDATE statements that unconditionally set use_ssl=1
(pushed into result.entries) must be scoped to only the rows inserted by this
import; change the SQL to target only imported servers/users (e.g. add a WHERE
clause that limits to the IDs/hosts inserted or to a temporary/import marker set
during INSERT). Concretely, when creating server/user rows in
PgBouncer_ConfigConverter (the code that pushes INSERTs into result.entries),
record the identifiers (or add a transient column/value like
source='pgbouncer_import') and then replace "UPDATE pgsql_servers SET
use_ssl=1;" and the analogous users UPDATE with "UPDATE pgsql_servers SET
use_ssl=1 WHERE <identifier IN (list) OR source='pgbouncer_import'>". Apply the
same scoping change for the other block mentioned (lines ~401-410) and ensure
emit_set calls remain unchanged.
- Around line 372-463: convert_hba_rules currently appends INSERTs into
pgsql_firewall_whitelist_rules and treats HBA "reject" rules as comments which
can leave stale allow entries and lose ordered-deny semantics; change the logic
so that when processing rules in ConfigConverter::convert_hba_rules you first
emit a statement to rebuild the whitelist table (e.g. TRUNCATE or DELETE FROM
pgsql_firewall_whitelist_rules) before emitting any INSERTs, and treat any
rule.method == "reject" as a hard conversion failure (call add_issue with
strict=true or otherwise fail the conversion for that rule) instead of emitting
a comment; update the code paths that set result.entries and
result.variable_count to reflect the new delete-then-insert flow and ensure the
unique symbols mentioned (convert_hba_rules, pgsql_firewall_whitelist_rules,
rule.method == "reject") are used to locate and modify the logic.
In `@lib/pgbouncer_compat/PgBouncer_ShowCommands.cpp`:
- Around line 71-100: The query_pools function currently cross-joins
runtime_pgsql_users (su) with stats_pgsql_connection_pool (cp) and only GROUPs
BY su.username, making cp.* columns (cp.ConnUsed, cp.ConnFree, cp.hostgroup,
etc.) nondeterministic when multiple cp rows exist; fix by either (A) adding the
pool dimension(s) to the GROUP BY (e.g., include cp.hostgroup or the pool's
unique identifier alongside su.username and ensure selected cp columns are also
grouped) or (B) replacing raw cp columns with explicit aggregates (e.g.,
SUM(cp.ConnUsed) AS sv_active, SUM(cp.ConnFree) AS sv_idle, AVG(cp.Latency_us)
AS latency_us, SUM(cp.Queries) AS Queries, SUM(cp.Bytes_data_sent) AS
Bytes_data_sent, SUM(cp.Bytes_data_recv) AS Bytes_data_recv) and keep GROUP BY
su.username; edit query_pools to implement one of these two deterministic
approaches referencing cp.ConnUsed, cp.ConnFree, cp.Latency_us, cp.Queries,
cp.Bytes_data_sent, cp.Bytes_data_recv and the GROUP BY clause.
- Around line 269-300: The parser currently accepts extra trailing tokens (e.g.,
"SHOW POOLS foo") because it only checks the command token; update the dispatch
in the SHOW handling (use symbols tokens, cmd_idx, cmd, out_query and the
query_* functions like query_pools/query_servers) to reject any extra tokens
after the command by verifying that cmd_idx + 1 == tokens.size() (or
tokens.size() == cmd_idx + 1) before calling the query_* functions and return
false if there are trailing tokens so malformed statements produce a syntax
error.
In `@lib/pgbouncer_compat/ProxySQL_CLI.cpp`:
- Around line 97-101: The loop in ProxySQL_CLI.cpp is appending an extra
semicolon to each SQL statement even though ConversionResult.entries already
contain terminated SQL; update the loop that iterates over result.entries (and
uses entry.sql) to output entry.sql as-is without adding an extra ";" (or
conditionally trim/avoid double-terminating) so the printed statements do not
become ";;".
- Around line 64-73: In the failure branch that checks result.success in
ProxySQL_CLI.cpp, stop writing the generated SQL to stdout; either omit the call
to PgBouncer::ConfigConverter::format_dry_run(...) or send its output to stderr
instead of std::cout so downstream piped commands won't receive partial SQL;
update the block that currently calls std::cout <<
PgBouncer::ConfigConverter::format_dry_run(result, config_path, strict) to not
write to stdout (use std::cerr or remove) and ensure the function still returns
1.
In `@test/tap/tests/unit/pgbouncer_converter_unit-t.cpp`:
- Around line 277-292: The TAP plan in main() is off by one: update the call to
plan(39) to reflect the actual 40 assertions run by the helper tests (change
plan(39) to plan(40)) so the plan count matches the sum of assertions executed
by test_minimal_conversion(), test_multi_host_conversion(),
test_wildcard_database(), test_user_pool_mode_mapping(), test_global_settings(),
test_strict_mode(), test_relaxed_mode(), test_query_rules(),
test_dry_run_format(), and test_tls_conversion().
In `@test/tap/tests/unit/pgbouncer_show_commands_unit-t.cpp`:
- Around line 190-203: The TAP plan in main() is wrong—plan(39) but there are 38
checks from the calls to test_show_command_recognition(),
test_case_insensitive(), test_trailing_semicolon(), test_extended_variant(),
test_sql_output_columns(), test_non_matching_queries(), and
test_unsupported_commands(); update the plan to the correct count by replacing
plan(39) with plan(38) (or adjust the individual test functions if you intend a
different total) so TAP passes when all CHECKs succeed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4785b6e6-c70b-4922-b080-368d0e961158
📒 Files selected for processing (10)
lib/Makefilelib/pgbouncer_compat/PgBouncer_ConfigConverter.cpplib/pgbouncer_compat/PgBouncer_ConfigConverter.hlib/pgbouncer_compat/PgBouncer_ShowCommands.cpplib/pgbouncer_compat/PgBouncer_ShowCommands.hlib/pgbouncer_compat/ProxySQL_CLI.cpplib/pgbouncer_compat/ProxySQL_CLI.htest/tap/tests/unit/Makefiletest/tap/tests/unit/pgbouncer_converter_unit-t.cpptest/tap/tests/unit/pgbouncer_show_commands_unit-t.cpp
✅ Files skipped from review due to trivial changes (2)
- lib/pgbouncer_compat/ProxySQL_CLI.h
- lib/Makefile
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
test/tap/tests/unit/pgbouncer_show_commands_unit-t.cpplib/pgbouncer_compat/PgBouncer_ShowCommands.htest/tap/tests/unit/pgbouncer_converter_unit-t.cpplib/pgbouncer_compat/PgBouncer_ShowCommands.cpplib/pgbouncer_compat/PgBouncer_ConfigConverter.hlib/pgbouncer_compat/ProxySQL_CLI.cpplib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
test/tap/tests/unit/pgbouncer_show_commands_unit-t.cpptest/tap/tests/unit/pgbouncer_converter_unit-t.cpplib/pgbouncer_compat/PgBouncer_ShowCommands.cpplib/pgbouncer_compat/ProxySQL_CLI.cpplib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Files:
test/tap/tests/unit/pgbouncer_show_commands_unit-t.cpptest/tap/tests/unit/pgbouncer_converter_unit-t.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention
Files:
lib/pgbouncer_compat/PgBouncer_ShowCommands.cpplib/pgbouncer_compat/ProxySQL_CLI.cpplib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Admin interface uses SQLite3 backend for SQL-based configuration and schema versions are tracked in ProxySQL_Admin_Tables_Definitions.h
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Applied to files:
test/tap/tests/unit/Makefiletest/tap/tests/unit/pgbouncer_show_commands_unit-t.cpptest/tap/tests/unit/pgbouncer_converter_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
test/tap/tests/unit/Makefile
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
test/tap/tests/unit/Makefilelib/pgbouncer_compat/PgBouncer_ShowCommands.htest/tap/tests/unit/pgbouncer_converter_unit-t.cpplib/pgbouncer_compat/PgBouncer_ConfigConverter.hlib/pgbouncer_compat/ProxySQL_CLI.cpplib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Applied to files:
test/tap/tests/unit/Makefiletest/tap/tests/unit/pgbouncer_show_commands_unit-t.cpptest/tap/tests/unit/pgbouncer_converter_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/pgbouncer_show_commands_unit-t.cpptest/tap/tests/unit/pgbouncer_converter_unit-t.cpp
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/pgbouncer_show_commands_unit-t.cpptest/tap/tests/unit/pgbouncer_converter_unit-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Applied to files:
lib/pgbouncer_compat/PgBouncer_ConfigConverter.h
🪛 Clang (14.0.6)
test/tap/tests/unit/pgbouncer_show_commands_unit-t.cpp
[error] 6-6: 'tap.h' file not found
(clang-diagnostic-error)
lib/pgbouncer_compat/PgBouncer_ShowCommands.h
[error] 4-4: 'string' file not found
(clang-diagnostic-error)
test/tap/tests/unit/pgbouncer_converter_unit-t.cpp
[error] 6-6: 'tap.h' file not found
(clang-diagnostic-error)
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
include/PgBouncer_HBAParser.h (1)
1-2: Use the repository-standard public header guard.This guard does not match the required
__CLASS_*_Hpattern for headers underinclude/.As per coding guidelines,
include/**/*.{h,hpp}: Include guards must follow the pattern#ifndef__CLASS_*_H.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/PgBouncer_HBAParser.h` around lines 1 - 2, The header guard macro PGBOUNCER_HBA_PARSER_H does not follow the project standard; replace it with the repository-standard pattern for public headers (use the __CLASS_*_H form) so the include guard matches the guideline. Update the `#ifndef/`#define/#endif trio in include/PgBouncer_HBAParser.h to use a guard like __CLASS_PGBOUNCER_HBA_PARSER_H (or the exact class-based name required by the repo), ensuring the symbol is used consistently in the `#ifndef`, `#define` and trailing `#endif`.include/PgBouncer_ConfigParser.h (1)
1-2: Use the repository-standard public header guard.This guard does not match the required
__CLASS_*_Hpattern for headers underinclude/.As per coding guidelines,
include/**/*.{h,hpp}: Include guards must follow the pattern#ifndef__CLASS_*_H.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/PgBouncer_ConfigParser.h` around lines 1 - 2, The header guard macro in PgBouncer_ConfigParser.h (currently PGBOUNCER_CONFIG_PARSER_H) doesn't follow the repository standard __CLASS_*_H pattern; rename the macro to the standard form (e.g., __CLASS_PGBOUNCER_CONFIG_PARSER_H) and update both the `#ifndef` and `#define` (and the matching `#endif` comment if present) so the include guard around the contents of the header and the symbol referenced by class/function declarations (e.g., PgBouncer_ConfigParser) use the new macro consistently.include/PgBouncer_ConfigConverter.h (1)
1-2: Use the repository-standard public header guard.This guard does not match the required
__CLASS_*_Hpattern for headers underinclude/.As per coding guidelines,
include/**/*.{h,hpp}: Include guards must follow the pattern#ifndef__CLASS_*_H.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/PgBouncer_ConfigConverter.h` around lines 1 - 2, The header guard in PgBouncer_ConfigConverter.h uses PGBOUNCER_CONFIG_CONVERTER_H but must follow the repo standard `#ifndef` __CLASS_*_H pattern; change the guard macro to __CLASS_PgBouncer_ConfigConverter_H (update both the `#ifndef` and `#define`) and update the trailing `#endif` comment if present so the file consistently uses __CLASS_PgBouncer_ConfigConverter_H as the include guard.include/PgBouncer_ShowCommands.h (1)
1-2: Use the repository-standard public header guard.This guard does not match the required
__CLASS_*_Hpattern for headers underinclude/.As per coding guidelines,
include/**/*.{h,hpp}: Include guards must follow the pattern#ifndef__CLASS_*_H.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/PgBouncer_ShowCommands.h` around lines 1 - 2, Replace the non-conforming header guard PGBOUNCER_SHOW_COMMANDS_H with the repository-standard pattern by renaming the macro to __CLASS_PGBOUNCER_SHOWCOMMANDS_H (or __CLASS_PGBOUNCER_SHOW_COMMANDS_H if underscores desired consistently) in the `#ifndef` and `#define` lines and update the matching `#endif` comment; ensure the new guard uses the __CLASS_*_H format required by the include/* headers (update both occurrences of the old macro to the new one).include/PgBouncer_AuthFileParser.h (1)
1-2: Use the repository-standard public header guard.This guard does not match the required
__CLASS_*_Hpattern for headers underinclude/.As per coding guidelines,
include/**/*.{h,hpp}: Include guards must follow the pattern#ifndef__CLASS_*_H.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/PgBouncer_AuthFileParser.h` around lines 1 - 2, The header guard macro PGBOUNCER_AUTH_FILE_PARSER_H does not follow the repo standard; replace both occurrences of PGBOUNCER_AUTH_FILE_PARSER_H in include/PgBouncer_AuthFileParser.h (the `#ifndef` and `#define`) with a guard following the __CLASS_*_H pattern, e.g. __CLASS_PGBOUNCER_AUTHFILEPARSER_H, and update the matching `#endif` comment if present to reference the new macro.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/PgBouncer_AuthFileParser.h`:
- Around line 29-30: detect_password_type incorrectly returns AuthType::MD5 from
inside the hex-validation loop, accepting strings where only the first hex
character is valid; fix detect_password_type in PgBouncer_AuthFileParser.cpp to
(1) first check the overall format (must start with "md5" and have exact length
35), (2) then validate that every character in the 32-character suffix
(positions after "md5") is a hex digit without returning early inside the loop,
and (3) only return AuthType::MD5 after the full suffix has been validated;
otherwise return the appropriate non-MD5 AuthType or an error.
In `@include/PgBouncer_ConfigConverter.h`:
- Around line 45-46: convert_users() currently strips AuthFileEntry::type when
mapping parsed user entries to the ConversionResult, losing SCRAM/MD5 metadata;
update the conversion so it preserves the original AuthFileEntry::type (copy the
type field from the parsed user entry) into the corresponding user record placed
into ConversionResult (ensure the ConversionResult user struct has/uses a type
field), modifying convert_users() in
lib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp and any related mapping code
so downstream consumers can distinguish plaintext vs MD5 vs SCRAM entries.
In `@lib/Admin_Handler.cpp`:
- Around line 5372-5377: The loop applying conversion statements (iterating
result.entries and calling SPA->admindb->execute(entry.sql.c_str())) must be
made atomic and abort on first SQL error: begin a transaction (e.g., execute
"BEGIN"), execute each entry.sql in order, check the return/throw from
SPA->admindb->execute and if any statement fails immediately ROLLBACK the
transaction and surface/log the failing SQL (include entry.sql and the error),
otherwise COMMIT after all succeed; update the code around the loop to perform
BEGIN before it, per-statement error handling that triggers ROLLBACK on failure
and returns/propagates the error, and a final COMMIT on success.
- Around line 5393-5415: The PgBouncer-specific SHOW handling
(PgBouncer::get_unsupported_show_message and PgBouncer::translate_show_command)
must run before the generic MySQL-style SHOW rewrites so SHOW DATABASES and
similar PgBouncer commands are translated for PgSQL_Session; move the entire
PgBouncer block (the if constexpr branch that calls get_unsupported_show_message
and translate_show_command and performs l_free/l_strdup on query) to execute
earlier, before the generic SHOW rewrite code paths that currently rewrite SHOW
DATABASES (so that query_no_space/query_no_space_length is checked/translated by
PgBouncer::translate_show_command first), preserving the existing error send
(SPA->send_error_msg_to_client), run_query flag setting, and the goto
__run_query behavior.
- Around line 5302-5317: The code currently strips "DRY RUN" and "IGNORE
WARNINGS" by substring which mangles quoted paths; instead detect these flags
only if they appear as trailing tokens in cmd_rest. Update the logic around
cmd_rest/dry_run/ignore_warnings to: trim trailing whitespace, perform a
case-insensitive check that the end of cmd_rest matches the whole flag sequence
(e.g. ends-with "DRY RUN" or "IGNORE WARNINGS") or tokenize cmd_rest respecting
quotes and check the last one or two tokens for the flags, set
dry_run/ignore_warnings and remove only those trailing tokens from cmd_rest;
ensure you use the same identifiers (cmd_rest, dry_run, ignore_warnings) and
preserve quoted paths when modifying cmd_rest.
- Around line 5294-5390: This IMPORT PGBOUNCER CONFIG handling path mutates
SPA->admindb but is not restricted to admin sessions; add a privilege check at
the start of this block to reject non-admin/stats sessions. Specifically, before
parsing or using SPA->admindb, call the same session-admin check used in the
nearby stats-session rejection (reuse the existing helper or condition that
rejects stats sessions), and if the check fails, call
SPA->send_error_msg_to_client(sess, ...) to return a permission error, free
path_buf if allocated, set run_query = false and goto __run_query; reference
variables/functions: sess, SPA->admindb, SPA->send_error_msg_to_client, and the
existing stats-session rejection logic to copy the exact check and message.
---
Nitpick comments:
In `@include/PgBouncer_AuthFileParser.h`:
- Around line 1-2: The header guard macro PGBOUNCER_AUTH_FILE_PARSER_H does not
follow the repo standard; replace both occurrences of
PGBOUNCER_AUTH_FILE_PARSER_H in include/PgBouncer_AuthFileParser.h (the `#ifndef`
and `#define`) with a guard following the __CLASS_*_H pattern, e.g.
__CLASS_PGBOUNCER_AUTHFILEPARSER_H, and update the matching `#endif` comment if
present to reference the new macro.
In `@include/PgBouncer_ConfigConverter.h`:
- Around line 1-2: The header guard in PgBouncer_ConfigConverter.h uses
PGBOUNCER_CONFIG_CONVERTER_H but must follow the repo standard `#ifndef`
__CLASS_*_H pattern; change the guard macro to
__CLASS_PgBouncer_ConfigConverter_H (update both the `#ifndef` and `#define`) and
update the trailing `#endif` comment if present so the file consistently uses
__CLASS_PgBouncer_ConfigConverter_H as the include guard.
In `@include/PgBouncer_ConfigParser.h`:
- Around line 1-2: The header guard macro in PgBouncer_ConfigParser.h (currently
PGBOUNCER_CONFIG_PARSER_H) doesn't follow the repository standard __CLASS_*_H
pattern; rename the macro to the standard form (e.g.,
__CLASS_PGBOUNCER_CONFIG_PARSER_H) and update both the `#ifndef` and `#define` (and
the matching `#endif` comment if present) so the include guard around the contents
of the header and the symbol referenced by class/function declarations (e.g.,
PgBouncer_ConfigParser) use the new macro consistently.
In `@include/PgBouncer_HBAParser.h`:
- Around line 1-2: The header guard macro PGBOUNCER_HBA_PARSER_H does not follow
the project standard; replace it with the repository-standard pattern for public
headers (use the __CLASS_*_H form) so the include guard matches the guideline.
Update the `#ifndef/`#define/#endif trio in include/PgBouncer_HBAParser.h to use a
guard like __CLASS_PGBOUNCER_HBA_PARSER_H (or the exact class-based name
required by the repo), ensuring the symbol is used consistently in the `#ifndef`,
`#define` and trailing `#endif`.
In `@include/PgBouncer_ShowCommands.h`:
- Around line 1-2: Replace the non-conforming header guard
PGBOUNCER_SHOW_COMMANDS_H with the repository-standard pattern by renaming the
macro to __CLASS_PGBOUNCER_SHOWCOMMANDS_H (or __CLASS_PGBOUNCER_SHOW_COMMANDS_H
if underscores desired consistently) in the `#ifndef` and `#define` lines and update
the matching `#endif` comment; ensure the new guard uses the __CLASS_*_H format
required by the include/* headers (update both occurrences of the old macro to
the new one).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3770a0f-0fd6-4502-82d8-fc1cb1022493
📒 Files selected for processing (8)
include/PgBouncer_AuthFileParser.hinclude/PgBouncer_ConfigConverter.hinclude/PgBouncer_ConfigParser.hinclude/PgBouncer_HBAParser.hinclude/PgBouncer_ShowCommands.hinclude/ProxySQL_CLI.hlib/Admin_Handler.cppsrc/main.cpp
✅ Files skipped from review due to trivial changes (1)
- include/ProxySQL_CLI.h
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
src/main.cppinclude/PgBouncer_ShowCommands.hinclude/PgBouncer_HBAParser.hinclude/PgBouncer_ConfigParser.hinclude/PgBouncer_AuthFileParser.hlib/Admin_Handler.cppinclude/PgBouncer_ConfigConverter.h
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
src/main.cpplib/Admin_Handler.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards must follow the pattern
#ifndef_CLASS*_H
Files:
include/PgBouncer_ShowCommands.hinclude/PgBouncer_HBAParser.hinclude/PgBouncer_ConfigParser.hinclude/PgBouncer_AuthFileParser.hinclude/PgBouncer_ConfigConverter.h
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention
Files:
lib/Admin_Handler.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Admin interface uses SQLite3 backend for SQL-based configuration and schema versions are tracked in ProxySQL_Admin_Tables_Definitions.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
src/main.cppinclude/PgBouncer_ShowCommands.hlib/Admin_Handler.cpp
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
src/main.cpp
🪛 Clang (14.0.6)
include/PgBouncer_ShowCommands.h
[error] 4-4: 'string' file not found
(clang-diagnostic-error)
🔇 Additional comments (2)
src/main.cpp (2)
2682-2683: LGTM!Forward declaration for the CLI entrypoint is appropriately placed before
main()to enable the early dispatch.
2687-2700: No action needed — pointer comparison is well-defined.The comparison
last_bsep > last_sepis not undefined behavior. Both pointers originate fromstrrchr()on the same string (binary_name) or are NULL. Per the C++ standard, comparing pointers derived from the same object (or with nullptr) is well-defined. The short-circuit evaluation (if (last_bsep && ...)ensureslast_bsepis non-NULL before the comparison, making the operation safe regardless oflast_sep's value.> Likely an incorrect or invalid review comment.
renecannao
added a commit
that referenced
this pull request
Apr 3, 2026
Critical fixes: - Admin_Handler: Gate IMPORT PGBOUNCER CONFIG to admin sessions only (stats connections can no longer mutate config) - Admin_Handler: Wrap import execution in BEGIN/ROLLBACK for atomicity (partial failures no longer leave inconsistent state) - Admin_Handler: Parse DRY RUN/IGNORE WARNINGS as trailing tokens after quoted path (paths containing those strings no longer break parsing) - ProxySQL_CLI: Never write SQL to stdout on failed conversion (prevents partial SQL from being piped to mysql in error cases) Important fixes: - ConfigParser: Add INT_MIN/INT_MAX range checking in parse_int() and UINT_MAX checking in parse_uint() to prevent overflow - ConfigParser: Detect unterminated single quotes in connection string pairs and report parse error instead of silently accepting - ConfigParser: Include <climits> for range check constants - ConfigParser: Set file field in "cannot open" error message - ConfigConverter: DELETE FROM pgsql_firewall_whitelist_rules before importing HBA rules (prevents stale entries) - ShowCommands: Fix SHOW POOLS query to avoid cross-join that produced nondeterministic results with multiple pool rows - ProxySQL_CLI: Remove duplicate semicolons in non-dry-run output - Admin_Handler: Fix admindb->execute() return type (bool not int)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/pgbouncer_compat/ProxySQL_CLI.cpp (2)
119-120: Quoteconfig_pathin the suggested shell command.The printed example can break for paths with spaces/shell metacharacters when users copy-paste it.
One simple option
- << " proxysql-cli import-pgbouncer " << config_path + << " proxysql-cli import-pgbouncer '" << config_path << "'" << " | mysql -h 127.0.0.1 -P 6032 -u admin -p\n";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pgbouncer_compat/ProxySQL_CLI.cpp` around lines 119 - 120, The help/example line prints a shell command using the variable config_path without quoting, which breaks for paths with spaces or special chars; update the output in ProxySQL_CLI.cpp where the stream appends config_path to emit a quoted/escaped path (e.g., wrap it in double quotes by writing '"' << config_path << '"' or use std::quoted(config_path) from <iomanip>) so the printed command is safe to copy-paste in a shell.
25-33: Treat option-likeargv[2]as missing required path.
import-pgbouncer --dry-runis currently interpreted asconfig_path="--dry-run", which produces a parse error instead of immediate CLI usage feedback.Suggested hardening
- if (argc < 3) { + if (argc < 3 || (argv[2] != nullptr && std::strncmp(argv[2], "--", 2) == 0)) { std::cerr << "Error: import-pgbouncer requires a config file path.\n\n"; print_usage(); return 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pgbouncer_compat/ProxySQL_CLI.cpp` around lines 25 - 33, The code currently treats argv[2] as the required config_path unconditionally, so invoking import-pgbouncer --dry-run sets config_path to "--dry-run" instead of flagging the missing path; modify the argument validation in main (the argc/argv handling around config_path, dry_run, ignore_warnings) to detect option-like values (e.g., argv[2] starts with '-' or is equal to "--dry-run"/"-d"/other flags) and treat them as a missing config path: if argv[2] begins with '-' then print the usage/error and return 1 (or alternatively parse flags first using getopt/arg parsing before assigning config_path) so that config_path is only set when a non-option argument is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/pgbouncer_compat/ProxySQL_CLI.cpp`:
- Around line 119-120: The help/example line prints a shell command using the
variable config_path without quoting, which breaks for paths with spaces or
special chars; update the output in ProxySQL_CLI.cpp where the stream appends
config_path to emit a quoted/escaped path (e.g., wrap it in double quotes by
writing '"' << config_path << '"' or use std::quoted(config_path) from
<iomanip>) so the printed command is safe to copy-paste in a shell.
- Around line 25-33: The code currently treats argv[2] as the required
config_path unconditionally, so invoking import-pgbouncer --dry-run sets
config_path to "--dry-run" instead of flagging the missing path; modify the
argument validation in main (the argc/argv handling around config_path, dry_run,
ignore_warnings) to detect option-like values (e.g., argv[2] starts with '-' or
is equal to "--dry-run"/"-d"/other flags) and treat them as a missing config
path: if argv[2] begins with '-' then print the usage/error and return 1 (or
alternatively parse flags first using getopt/arg parsing before assigning
config_path) so that config_path is only set when a non-option argument is
provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68da265e-e413-4f40-a1ba-033ee22035e1
📒 Files selected for processing (5)
lib/Admin_Handler.cpplib/pgbouncer_compat/PgBouncer_ConfigConverter.cpplib/pgbouncer_compat/PgBouncer_ConfigParser.cpplib/pgbouncer_compat/PgBouncer_ShowCommands.cpplib/pgbouncer_compat/ProxySQL_CLI.cpp
✅ Files skipped from review due to trivial changes (2)
- lib/Admin_Handler.cpp
- lib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/pgbouncer_compat/PgBouncer_ShowCommands.cpp
- lib/pgbouncer_compat/PgBouncer_ConfigParser.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
lib/pgbouncer_compat/ProxySQL_CLI.cpp
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
lib/pgbouncer_compat/ProxySQL_CLI.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention
Files:
lib/pgbouncer_compat/ProxySQL_CLI.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Admin interface uses SQLite3 backend for SQL-based configuration and schema versions are tracked in ProxySQL_Admin_Tables_Definitions.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
lib/pgbouncer_compat/ProxySQL_CLI.cpp
📚 Learning: 2026-03-26T16:39:02.446Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.
Applied to files:
lib/pgbouncer_compat/ProxySQL_CLI.cpp
🔇 Additional comments (2)
lib/pgbouncer_compat/ProxySQL_CLI.cpp (2)
64-75: Good safety fix in failure path output routing.Failed conversion output stays on
stderr(and only in--dry-run), which avoids leaking SQL into pipedstdoutworkflows.
99-104: Good fix: SQL entries are emitted without extra statement terminators.Printing
entry.sqldirectly avoids producing;;in non-dry-run output.
|
Add a standalone C++ library that parses PgBouncer configuration files into structured data, as the foundation for PgBouncer-to-ProxySQL migration tooling. Components: - PgBouncer_Config.h: Data structures for all PgBouncer config entities - PgBouncer_ConfigParser: INI parser for pgbouncer.ini with all 4 sections ([pgbouncer], [databases], [users], [peers]), %include directive support, and connection string parsing - PgBouncer_AuthFileParser: Parser for userlist.txt with MD5/SCRAM/plain password type detection and double-quote escaping - PgBouncer_HBAParser: Parser for pg_hba.conf with support for all connection types, auth methods, and key=value options The parser library has zero ProxySQL dependencies - it uses only the C++ standard library. This makes it independently testable and reusable. Unit test covers: minimal config, full config with all sections, auth file formats, HBA rules, malformed configs (strict error detection), %include directives, nonexistent files, and default value verification (127 tests).
…ages 2+3) (#5564, #5565) Stage 2 - Config Converter + CLI: - PgBouncer_ConfigConverter: Converts parsed PgBouncer config into ProxySQL SQL statements with full parameter mapping - Databases → pgsql_servers + pgsql_query_rules (auto hostgroup assignment) - Multi-host entries → multiple server rows with equal weight - Users → pgsql_users with pool mode mapping (session→fast_forward, transaction→transaction_persistent, statement→defaults) - Globals → pgsql-* variables with unit conversion (s→ms) - HBA rules → pgsql_firewall_whitelist entries - Strict mode (default): fails on unmappable parameters - Relaxed mode (--ignore-warnings): warns and continues - Dry-run output: commented SQL with summary - ProxySQL_CLI: Entry point for proxysql-cli binary (argv[0] detection) - Subcommand: import-pgbouncer <path> [--dry-run] [--ignore-warnings] - Never starts daemon, always exits after command - 39 unit tests covering: minimal conversion, multi-host, wildcard database, pool mode mapping, global settings, strict/relaxed modes, query rules, dry-run format, TLS settings Stage 3 - PgBouncer-Compatible SHOW Commands: - PgBouncer_ShowCommands: Translates PgBouncer SHOW commands to equivalent ProxySQL SQL queries with exact column output format - SHOW POOLS, STATS, SERVERS, CLIENTS, DATABASES, USERS, CONFIG, VERSION, STATE, LISTS - SHOW EXTENDED variant adds ProxySQL-specific columns - Unsupported commands (DNS_HOSTS, PEERS, etc.) return clear errors - Case-insensitive, handles trailing semicolons and extra whitespace - 39 unit tests covering: command recognition, case insensitivity, trailing semicolons, EXTENDED variant, SQL output columns, non-matching queries, unsupported commands
Integration of PgBouncer migration tooling into ProxySQL core:
1. Admin command (Admin_Handler.cpp):
- IMPORT PGBOUNCER CONFIG FROM '/path' [DRY RUN] [IGNORE WARNINGS]
- Parses PgBouncer config, converts to SQL, executes against admin DB
- DRY RUN returns converted SQL as a result set
- Strict mode (default) fails on unmappable parameters
- IGNORE WARNINGS allows partial conversion
2. PgBouncer SHOW commands (Admin_Handler.cpp):
- Intercepts SHOW POOLS/STATS/SERVERS/CLIENTS/DATABASES/USERS/CONFIG/
VERSION/STATE/LISTS on PgSQL admin port
- Translates to equivalent ProxySQL SQL with PgBouncer-exact columns
- SHOW EXTENDED variant adds ProxySQL-specific columns
- Unsupported commands return descriptive error messages
- Only active on PgSQL sessions (if constexpr guard)
3. proxysql-cli mode (main.cpp):
- Detects argv[0] == "proxysql-cli" at start of main()
- Routes to proxysql_cli_main() which never starts the daemon
- Symlink: ln -s proxysql proxysql-cli
4. Headers copied to include/ following ProxySQL convention.
Lightweight CI that builds and runs the three standalone PgBouncer compatibility unit tests (205 tests total) on every push to the feature branch and on PRs touching pgbouncer_compat files. No Docker or backend infrastructure needed — tests are standalone with zero ProxySQL runtime dependencies.
Critical fixes: - Admin_Handler: Gate IMPORT PGBOUNCER CONFIG to admin sessions only (stats connections can no longer mutate config) - Admin_Handler: Wrap import execution in BEGIN/ROLLBACK for atomicity (partial failures no longer leave inconsistent state) - Admin_Handler: Parse DRY RUN/IGNORE WARNINGS as trailing tokens after quoted path (paths containing those strings no longer break parsing) - ProxySQL_CLI: Never write SQL to stdout on failed conversion (prevents partial SQL from being piped to mysql in error cases) Important fixes: - ConfigParser: Add INT_MIN/INT_MAX range checking in parse_int() and UINT_MAX checking in parse_uint() to prevent overflow - ConfigParser: Detect unterminated single quotes in connection string pairs and report parse error instead of silently accepting - ConfigParser: Include <climits> for range check constants - ConfigParser: Set file field in "cannot open" error message - ConfigConverter: DELETE FROM pgsql_firewall_whitelist_rules before importing HBA rules (prevents stale entries) - ShowCommands: Fix SHOW POOLS query to avoid cross-join that produced nondeterministic results with multiple pool rows - ProxySQL_CLI: Remove duplicate semicolons in non-dry-run output - Admin_Handler: Fix admindb->execute() return type (bool not int)
The generated SQL and several SHOW translations referenced columns that do not exist, so they failed the moment they were executed. The unit tests only compared strings, which is why all 205 of them passed against broken SQL. Generated SQL: - pgsql_query_rules has no `schemaname` column (that is the MySQL table); the PgSQL one calls it `database`. Every routing rule INSERT was invalid. - pgsql_firewall_whitelist_rules likewise uses `database`, and declares `digest` and `comment` NOT NULL with no default, so the INSERT was invalid. SHOW translations: - SHOW CLIENTS selected `db` from stats_pgsql_processlist; the column is `database`. - SHOW DATABASES and SHOW EXTENDED SERVERS read weight, max_connections and max_replication_lag from stats_pgsql_connection_pool, which has none of them. They now read (or join) runtime_pgsql_servers. - SHOW DATABASES was unreachable: Admin_Handler's generic SHOW block claims that command word first. The PgBouncer block now runs before it, and translate_show_command() rejects trailing tokens so everything else still falls through. - SHOW EXTENDED was a no-op for eight of the ten commands. Semantics that were silently wrong: - convert_users() dropped AuthFileEntry::type and wrote MD5/SCRAM verifiers into pgsql_users.password. ProxySQL derives both the MD5 response and the SCRAM verifier from the cleartext password, so those credentials could never authenticate. Now reported per user (fatal in strict mode). - A `dbname=` alias was dropped, emitting a rule that routes to the hostgroup while passing the client's database name through unchanged. - HBA `reject` rules were emitted as an SQL comment and still enabled the whitelist, implying a denial that the allow-only whitelist cannot express. Parser: - Inline-comment stripping is quote-aware, and quoted [pgbouncer] values are unquoted. Previously any value starting with a quote skipped stripping entirely and kept its quotes. - The HBA tokenizer handles doubled "" escapes, preserves empty quoted tokens instead of shifting later fields, and reports an unterminated quote instead of returning truncated tokens. - parse() is a full load: re-parsing into the same Config no longer duplicates every database, user and rule. - parse_uint() rejects a leading '-' rather than relying on stoul wraparound. Also removes the six duplicated headers under lib/pgbouncer_compat/ (identical copies of the include/ ones) and switches the include guards to the project's __CLASS_*_H convention. Unit tests: 205 -> 256, with a regression test for each defect above.
…ocument it Tests were never registered in test/tap/groups/groups.json, so none of them ran in any TAP group. The three unit tests now sit in unit-tests-g1 alongside the other unit tests, and a new integration test joins legacy-g4 next to the other pgsql-* admin tests. The new integration test, pgsql-pgbouncer_compat-t, executes every PgBouncer SHOW command (plain and EXTENDED) and every statement the converter emits against a live PgSQL admin port. That is the coverage that was missing: the unit tests compare generated strings, which is why three statements that could not execute at all passed the whole suite. It also checks that a non-PgBouncer SHOW still reaches ProxySQL's own handling, and that a failed IMPORT leaves the connection usable. CI-pgbouncer-compat.yml is removed rather than fixed. It was pinned to `branches: [feature/pgbouncer-compat]`, so it would have stopped running the moment it merged, and it was a self-contained workflow in a repo where every CI-*.yml on v3.0 is a thin caller into ci-*.yml@GH-Actions (see doc/GH-Actions/README.md). Registering the tests in groups.json is what actually gets them running: legacy-g4 covers the integration test today, and the unit tests are compiled by CI-builds and will run under CI-unittests when that workflow is re-enabled (it is disabled repo-wide, see #5603). Packaging never shipped proxysql-cli, so the argv[0] dispatch in main() was unreachable from an installed package: - rpm (rhel + suse): a real symlink created in %install, picked up by the existing %{_bindir}/* glob so rpm owns and removes it. - deb: created in postinst and removed in postrm, since equivs `Files:` handles regular files only. - tarball: bin/proxysql-cli wrapper execing libexec/proxysql-cli. The extra indirection is needed because the existing bin/proxysql wrapper execs proxysql.bin, which would make argv[0] "proxysql.bin", and `exec -a` is not available in POSIX sh. - make install / uninstall. verify-package-install.bash now checks proxysql-cli is on PATH and dispatches to CLI mode, so a packaging regression fails the release verification instead of shipping. Its self-test gains a matching stub. doc/PGBOUNCER_COMPAT.md replaces the design spec the PR and all three issues pointed at, which lived in a working directory and was never committed. It documents the implemented behaviour: the full parameter mapping, the SHOW support matrix, and — deliberately explicit — everything that is not mapped and why, including the pre-hashed-password limitation.
…instance
Running pgsql-pgbouncer_compat-t destroyed the test instance's PostgreSQL
configuration, and every test that ran afterwards failed with
PgSQL_Session.cpp:4184 [ERROR] ProxySQL Error:
Access denied for user 'postgres'@'...' (using password: NO)
because pgsql_users and pgsql_servers had been emptied.
Cause: the converter's output ends with SAVE PGSQL {SERVERS,USERS,QUERY RULES,
VARIABLES} TO DISK, and the test executed every statement it produced. Those
SAVEs overwrote the instance's on-disk configuration, so the test's own
"LOAD ... FROM DISK" restore then faithfully restored the clobbered copy. The
data directory is a host bind-mount, so recreating the ProxySQL container did
not recover it either — the damage outlived the container.
The test now skips the SAVE statements (they are fixed SQL with no generated
identifiers, so no mapping coverage is lost) and asserts they were emitted.
Everything else it runs touches memory and runtime only, which leaves the
on-disk copy a valid source to restore from. The restore is widened to cover
variables and the firewall whitelist, and reports a failed restore via diag()
so a broken teardown is visible rather than silent.
Also adds the cheap "SHOW " prefix guard around the PgBouncer block in
Admin_Handler. Now that the block sits ahead of the generic SHOW dispatch, it
would otherwise normalize and tokenize every admin query, not just SHOW ones.
Verified against a live legacy-g4 instance:
- pgsql-proxysql_cmd_test-t passes before and after pgsql-pgbouncer_compat-t
in the same session (it failed on the second run before this fix)
- pgsql_users/pgsql_servers still hold the infra's own rows afterwards
- 6/6 of the admin + pgsql regression set pass
- pgsql-pgbouncer_compat-t: 49/49
auth_type was parsed into the Config and then ignored: the converter never read it and check_unmappable() never mentioned it. The frontend authentication method of the pooler being replaced simply vanished from the import, with no error even in strict mode -- which contradicts the converter's own strict-by-default contract, and does so for the most security-relevant setting in pgbouncer.ini. It maps cleanly onto the existing pgsql-authentication_method variable (PgSQL_Thread.cpp, range 1-3): plain / password -> 1 (cleartext) md5 -> 2 scram-sha-256 -> 3 The values ProxySQL cannot express are now reported (fatal in strict mode): trust and any (ProxySQL always verifies the user against pgsql_users), hba (pgsql-authentication_method is global, so per-rule pg_hba.conf methods cannot select the frontend method), and cert / pam. Also records in doc/PGBOUNCER_COMPAT.md that the pre-hashed-password limitation is expected to lift with #5865 / #5863, which teaches pgsql_users.password to hold a SCRAM verifier or md5 hash directly -- the same formats userlist.txt already stores -- together with the constraints that come with it (md5 secret needs an md5 backend, verifier needs a scram-sha-256 backend and must be byte-identical to the backend's rolpassword). Converter unit tests: 52 -> 68.
renecannao
force-pushed
the
feature/pgbouncer-compat
branch
from
August 27, 2026 08:50
3f1027b to
5f61301
Compare
renecannao
changed the title
|
|
SonarCloud's quality gate failed on the PR with an E security rating from seven cpp:S2068 "hard-coded password" findings, all of them in the new test files. Each is a synthetic fixture value assigned to a PgBouncer::AuthFileEntry built in memory -- the md5 one is the hash of the empty string, and the SCRAM one is a structurally-valid but meaningless verifier whose salt and keys are base64 "salt"/"str". None is a real credential and none reaches a credential store; two of them exist precisely to assert that a hashed credential is reported as unusable. Annotated with // NOSONAR(cpp:S2068) plus a per-line reason, following the convention already used for the same rule in test/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpp:168. Tests rebuilt and re-run: pgbouncer_converter_unit-t 68/68, pgsql-pgbouncer_compat-t links clean.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
37 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="include/PgBouncer_ConfigParser.h">
<violation number="1" location="include/PgBouncer_ConfigParser.h:4">
P3: The header declares parse_connstr_pairs(...) with std::pair and std::vector return arguments but only directly includes <string> and <functional> (the latter is unused). It relies on PgBouncer_Config.h transitively pulling in <vector>/<utility>; if that include tree shifts, this header silently breaks. Include <vector> and <utility> directly and drop the unused <functional>.</violation>
</file>
<file name="lib/pgbouncer_compat/PgBouncer_AuthFileParser.cpp">
<violation number="1" location="lib/pgbouncer_compat/PgBouncer_AuthFileParser.cpp:146">
P2: When reading `userlist.txt` fails after some records, `parse` returns success and the migration can import only a partial credential set. Check the stream error state before returning success and add a parse error.</violation>
</file>
<file name="lib/pgbouncer_compat/PgBouncer_HBAParser.cpp">
<violation number="1" location="lib/pgbouncer_compat/PgBouncer_HBAParser.cpp:226">
P2: When a file read fails after some records, `parse()` returns success with only the records read so far. Check `in.bad()` after the loop, add a parse error, and return failure to prevent importing a truncated HBA file.</violation>
</file>
<file name="lib/pgbouncer_compat/ProxySQL_CLI.cpp">
<violation number="1" location="lib/pgbouncer_compat/ProxySQL_CLI.cpp:125">
P1: When any generated statement fails during the pipe-based import, earlier deletes and inserts remain committed because this stream is not transactional. Wrap the emitted SQL in a transaction and use client error-stop/rollback semantics to prevent a partially wiped configuration.</violation>
<violation number="2" location="lib/pgbouncer_compat/ProxySQL_CLI.cpp:142">
P1: The copy-paste command sends generated `pgsql_*` configuration to MySQL admin port 6032, but the PgSQL admin workflow uses port 6132. Print a `psql -p 6132` pipeline instead.</violation>
</file>
<file name="doc/PGBOUNCER_COMPAT.md">
<violation number="1" location="doc/PGBOUNCER_COMPAT.md:95">
P2: The mapping table lists `log_min_duration` as mapping to `pgsql-long_query_time`, but the converter maps `query_timeout` to that variable (PgBouncer_ConfigConverter.cpp:361-364); `log_min_duration` is not mapped at all. Rename the parameter so the documented mapping matches the implementation.</violation>
</file>
<file name="lib/Admin_Handler.cpp">
<violation number="1" location="lib/Admin_Handler.cpp:5144">
P3: This PgBouncer SHOW translation/untranslation block is gated only on `PgSQL_Session` and runs before any `session_type` check, so it also fires for stats (non-admin) PgSQL sessions. The translated SQL for SERVERS/DATABASES/USERS/CONFIG/VERSION/STATE queries `global_variables`, `runtime_pgsql_servers`, and `runtime_pgsql_users`, which live in the admin DB (`main`), not the stats DB. On a stats session `__run_query` executes against `SPA->statsdb`, so those SHOW commands will fail with missing-table errors while others (POOLS/LISTS, which only touch `stats_pgsql_connection_pool`) succeed — inconsistent behavior. The adjacent IMPORT block correctly guards with `sess->session_type != PROXYSQL_SESSION_ADMIN`. Restrict this block to admin sessions too.</violation>
<violation number="2" location="lib/Admin_Handler.cpp:5551">
P3: `::toupper` is applied to each `char` of `upper_rem` directly. When a byte is outside the unsigned char range (values >= 0x80), passing a potentially negative `int` to `::toupper` is undefined behavior. The same PR's `PgBouncer_ShowCommands.cpp` correctly casts via `static_cast<unsigned char>` before calling it, so this spot is inconsistent with that fix. Cast the value before the call.</violation>
<violation number="3" location="lib/Admin_Handler.cpp:5557">
P2: Trailing flag text is matched by substring, so malformed commands such as `IGNORE WARNINGSX` silently disable strict conversion. Parse exact flag tokens and reject unknown trailing tokens.</violation>
<violation number="4" location="lib/Admin_Handler.cpp:5613">
P1: Every non-dry-run import eventually executes ProxySQL admin commands as raw SQLite statements. Dispatch `SET`/`LOAD`/`SAVE` entries through the admin command handling path, or execute only SQLite statements here and handle the admin commands separately.</violation>
</file>
<file name="lib/pgbouncer_compat/PgBouncer_ShowCommands.cpp">
<violation number="1" location="lib/pgbouncer_compat/PgBouncer_ShowCommands.cpp:83">
P1: `SHOW POOLS` reports each backend hostname as the database name, so monitoring displays incorrect pool identities. Join the routing metadata to recover database names, or return an explicitly unavailable value instead of fabricating one.</violation>
<violation number="2" location="lib/pgbouncer_compat/PgBouncer_ShowCommands.cpp:95">
P2: `SHOW POOLS` always reports `statement`, even when the imported pool is transaction or session mode. Derive the mode from the applicable runtime user/database configuration, or mark it unavailable instead of returning a false value.</violation>
<violation number="3" location="lib/pgbouncer_compat/PgBouncer_ShowCommands.cpp:112">
P2: `SHOW STATS` collapses all databases into one row labeled `default`, misattributing per-database counters. Select the digest `database` and group every aggregate by it.</violation>
<violation number="4" location="lib/pgbouncer_compat/PgBouncer_ShowCommands.cpp:113">
P2: `count_star` counts queries, not transactions, yet this populates `total_xact_count` from it. Return zero for the unavailable transaction metric rather than reporting a false count.</violation>
<violation number="5" location="lib/pgbouncer_compat/PgBouncer_ShowCommands.cpp:224">
P1: `SHOW DATABASES` returns backend hostnames as `name` and one row per server, not configured database names. Join `runtime_pgsql_query_rules.database` by hostgroup and aggregate the server rows before translating this command.</violation>
<violation number="6" location="lib/pgbouncer_compat/PgBouncer_ShowCommands.cpp:319">
P2: `SHOW LISTS` emits only five list names and omits `free_clients`, `used_clients`, `login_clients`, `dns_names`, and `dns_zones`. Add those rows with zero or available ProxySQL-derived counts so consumers receive the complete list schema.</violation>
</file>
<file name="lib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp">
<violation number="1" location="lib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp:80">
P1: When an otherwise valid PgBouncer config has no database entries, the import leaves stale PostgreSQL servers and query rules active instead of replacing them. Queue the cleanup statements before returning for an empty database section.</violation>
<violation number="2" location="lib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp:106">
P2: When PgBouncer sets `default_pool_size` to a value other than 20, databases without an explicit `pool_size` receive the wrong `max_connections`. Use `config.global.default_pool_size` when `db.pool_size` is `-1`.</violation>
<violation number="3" location="lib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp:337">
P1: When a PgBouncer config leaves `max_client_conn` at 100, this guard emits no `SET`, so ProxySQL keeps 2048 and permits far more clients than the source configuration. Apply the parsed value or compare against ProxySQL's default rather than PgBouncer's default; the same source-default mistake affects other mappings in this function.</violation>
<violation number="4" location="lib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp:349">
P1: This maps PgBouncer's idle timeout to ProxySQL's maximum connection age and ignores `server_lifetime`, producing the wrong connection-retirement behavior. Map `g.server_lifetime` to `pgsql-connection_max_age_ms` instead.</violation>
<violation number="5" location="lib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp:397">
P2: When the source config has no [databases] section, the unscoped `UPDATE pgsql_servers SET use_ssl=1` in convert_globals() still runs, but convert_databases() returned early without emitting `DELETE FROM pgsql_servers`, so the UPDATE hits pre-existing rows that were not part of this import and silently enables SSL on unrelated backends. Guard the UPDATE on an emitted DELETE (e.g., skip it unless `config.databases` is non-empty, or scope it via the imported hostgroups/hostnames).</violation>
<violation number="6" location="lib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp:470">
P1: An HBA rule using the valid `ldap` method is silently dropped, and relaxed imports can delete existing whitelist rules without replacing it. Classify `ldap` as unmappable so strict mode fails and relaxed mode warns.</violation>
<violation number="7" location="lib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp:484">
P1: A `hostssl` rule scoped to one address or database makes SSL mandatory for every connection by the matching user, or every user for `all`. Preserve the HBA match scope or report this hostssl rule as unmappable instead of broadening it silently.</violation>
<violation number="8" location="lib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp:491">
P2: When pg_hba.conf uses the separate address/netmask form, the generated whitelist matches only the bare address instead of the intended subnet. Convert the address and mask to ProxySQL's CIDR representation before inserting the rule.</violation>
</file>
<file name="test/tap/tests/unit/pgbouncer_converter_unit-t.cpp">
<violation number="1" location="test/tap/tests/unit/pgbouncer_converter_unit-t.cpp:139">
P3: These two assertions cannot detect a regression in the pool-mode mapping, which is the behavior they claim to verify. The converter always lists `fast_forward` and `transaction_persistent` in the INSERT column set for every user, so the substring appears even when the mapped value is 0. Check the inserted row's value for the specific user (e.g. that the `session_user` INSERT contains fast_forward=1) instead of the bare column name.</violation>
<violation number="2" location="test/tap/tests/unit/pgbouncer_converter_unit-t.cpp:514">
P3: The `// 3` count comment on test_wildcard_database is inaccurate: the function issues only two assertions. Fix the comment to `// 2` so the per-test counts stay consistent with plan(68).</violation>
</file>
<file name="test/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp">
<violation number="1" location="test/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp:255">
P3: `rules[3].options.at("map")` throws `std::out_of_range` if rule 3 has no `map` option. Because the preceding `CHECK(rules[3].options.count("map") > 0)` only records a failure but does not stop execution, a parser regression that drops the option would still reach `.at()` and abort the whole test binary via an uncaught exception, hiding the TAP diagnostic instead of producing a clean failure. Guard the lookup.</violation>
<violation number="2" location="test/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp:405">
P3: The `|| dbs_first == 1` escape hatch lets this assertion pass even if re-parsing actually failed to replace the content (whenever the first file happened to contain one database), so it no longer proves the "different file replaces previous content" claim. It is also dead here since `dbs_first` comes from full.ini. Drop the disjunct, and note the comment says "a failed parse" while the code runs a successful parse of a different file.</violation>
<violation number="3" location="test/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp:466">
P3: The per-function test-count comments in main() and the section headers are wrong: test_minimal_config runs 8, test_full_config 64, test_hba_file 22, and test_defaults 10, not the 7/42/19/9 the comments claim. plan() must exactly match the executed count, so these stale counts mislead anyone adjusting the plan. Update the comments (or drop the counts) to match each function's actual CHECK calls.</violation>
</file>
<file name="test/infra/control/verify-package-install.bash">
<violation number="1" location="test/infra/control/verify-package-install.bash:158">
P3: Under `set -euo pipefail`, this standalone pipeline in the dispatch-fail else branch aborts the whole test script the moment `proxysql-cli help` exits nonzero (pipefail propagates the status), before `ALL_OK=1` and the failure summary can run. Guard the diagnostic pipeline so the script always reaches the final PASS/FAIL decision.</violation>
</file>
<file name="docker/images/proxysql/deb-compliant/ctl/proxysql.ctl">
<violation number="1" location="docker/images/proxysql/deb-compliant/ctl/proxysql.ctl:31">
P3: postinst runs `ln -sf proxysql /usr/bin/proxysql-cli` on every install *and* every upgrade, and `-f` unconditionally unlinks whatever currently sits at `/usr/bin/proxysql-cli`. A pre-existing file or symlink placed there by an operator or another package is silently destroyed the first time ProxySQL is installed or upgraded. Guard the create so an existing path is not clobbered.</violation>
</file>
<file name="lib/pgbouncer_compat/PgBouncer_ConfigParser.cpp">
<violation number="1" location="lib/pgbouncer_compat/PgBouncer_ConfigParser.cpp:337">
P2: Any PgBouncer config containing the documented `log_min_duration` setting fails parsing before conversion. Add the setting to the parsed model and map it instead of treating it as unknown.</violation>
<violation number="2" location="lib/pgbouncer_compat/PgBouncer_ConfigParser.cpp:569">
P2: A `%include` directive separated by a tab is rejected as a malformed key-value line. Recognize any whitespace after `%include` so valid tab-formatted configs load.</violation>
<violation number="3" location="lib/pgbouncer_compat/PgBouncer_ConfigParser.cpp:631">
P2: An unterminated quoted global value such as `logfile = 'path` is silently accepted with its quote intact. Report an unterminated quote instead of importing a malformed value.</violation>
</file>
<file name="test/tap/tests/unit/Makefile">
<violation number="1" location="test/tap/tests/unit/Makefile:868">
P2: In ASAN/TSAN builds (WITHASAN=1/TEST_WITHASAN=1) these three pgbouncer link rules omit `$(WASAN)`. `tap.o` is compiled with `$(OPT)` (which includes `$(WASAN)`), so the sanitizer-instrumented object is linked here without `-fsanitize=address`, yielding undefined `__asan_*` references (link failure), or leaving the test uninstrumented when it does link. Every sibling standalone rule (ezoption_parser_unit-t, mcp_client_unit-t, mysql_resolution_unit-t) appends `$(WASAN)`; add it to the three new rules too.</violation>
</file>
<file name="test/infra/control/test-verify-package-install.bash">
<violation number="1" location="test/infra/control/test-verify-package-install.bash:67">
P3: The verifier adds two failure branches for proxysql-cli (not on PATH; does not dispatch to CLI mode), but this test only exercises the happy path — the fake proxysql-cli always prints 'import-pgbouncer'. Add a case where proxysql-cli is absent from PATH, or where a fake returns daemon-style output without 'import-pgbouncer', to verify the new FAIL detection actually fails the package verification (the exact packaging regression the verifier comment warns about).</violation>
</file>
<file name="src/main.cpp">
<violation number="1" location="src/main.cpp:2794">
P3: On Windows the dispatcher can never match: the executable is `proxysql-cli.exe`, so `strcmp(binary_name, "proxysql-cli")` always fails and invoking proxysql-cli silently falls through and starts the full ProxySQL daemon. The block also compares `last_bsep > last_sep` where `last_sep` is NULL for any path without `/` (pointer-to-NULL comparison is undefined behavior), and `/` stripping runs before the `\\` search so a mixed-separator path is mishandled. Strip any `.exe` suffix and resolve the final separator consistently.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| << result.variable_count << " variables.\n" | ||
| << "Pipe the output to ProxySQL admin interface to apply:\n" | ||
| << " proxysql-cli import-pgbouncer " << shell_quote(config_path) | ||
| << " | mysql -h 127.0.0.1 -P 6032 -u admin -p\n"; |
There was a problem hiding this comment.
P1: The copy-paste command sends generated pgsql_* configuration to MySQL admin port 6032, but the PgSQL admin workflow uses port 6132. Print a psql -p 6132 pipeline instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/pgbouncer_compat/ProxySQL_CLI.cpp, line 142:
<comment>The copy-paste command sends generated `pgsql_*` configuration to MySQL admin port 6032, but the PgSQL admin workflow uses port 6132. Print a `psql -p 6132` pipeline instead.</comment>
<file context>
@@ -0,0 +1,164 @@
+ << result.variable_count << " variables.\n"
+ << "Pipe the output to ProxySQL admin interface to apply:\n"
+ << " proxysql-cli import-pgbouncer " << shell_quote(config_path)
+ << " | mysql -h 127.0.0.1 -P 6032 -u admin -p\n";
+ return 0;
+}
</file context>
| if (!entry.comment.empty()) { | ||
| std::cout << "-- " << entry.comment << "\n"; | ||
| } | ||
| std::cout << entry.sql << "\n"; |
There was a problem hiding this comment.
P1: When any generated statement fails during the pipe-based import, earlier deletes and inserts remain committed because this stream is not transactional. Wrap the emitted SQL in a transaction and use client error-stop/rollback semantics to prevent a partially wiped configuration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/pgbouncer_compat/ProxySQL_CLI.cpp, line 125:
<comment>When any generated statement fails during the pipe-based import, earlier deletes and inserts remain committed because this stream is not transactional. Wrap the emitted SQL in a transaction and use client error-stop/rollback semantics to prevent a partially wiped configuration.</comment>
<file context>
@@ -0,0 +1,164 @@
+ if (!entry.comment.empty()) {
+ std::cout << "-- " << entry.comment << "\n";
+ }
+ std::cout << entry.sql << "\n";
+ }
+
</file context>
| bool exec_ok = true; | ||
| for (const auto& entry : result.entries) { | ||
| if (!entry.sql.empty()) { | ||
| bool rc = SPA->admindb->execute(entry.sql.c_str()); |
There was a problem hiding this comment.
P1: Every non-dry-run import eventually executes ProxySQL admin commands as raw SQLite statements. Dispatch SET/LOAD/SAVE entries through the admin command handling path, or execute only SQLite statements here and handle the admin commands separately.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/Admin_Handler.cpp, line 5613:
<comment>Every non-dry-run import eventually executes ProxySQL admin commands as raw SQLite statements. Dispatch `SET`/`LOAD`/`SAVE` entries through the admin command handling path, or execute only SQLite statements here and handle the admin commands separately.</comment>
<file context>
@@ -5456,6 +5494,147 @@ void admin_session_handler(S* sess, void *_pa, PtrSize_t *pkt) {
+ bool exec_ok = true;
+ for (const auto& entry : result.entries) {
+ if (!entry.sql.empty()) {
+ bool rc = SPA->admindb->execute(entry.sql.c_str());
+ if (!rc) {
+ exec_ok = false;
</file context>
| // server table is the right source -- not the runtime counter table. | ||
| std::string q = | ||
| "SELECT " | ||
| "hostname AS name, " |
There was a problem hiding this comment.
P1: SHOW DATABASES returns backend hostnames as name and one row per server, not configured database names. Join runtime_pgsql_query_rules.database by hostgroup and aggregate the server rows before translating this command.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/pgbouncer_compat/PgBouncer_ShowCommands.cpp, line 224:
<comment>`SHOW DATABASES` returns backend hostnames as `name` and one row per server, not configured database names. Join `runtime_pgsql_query_rules.database` by hostgroup and aggregate the server rows before translating this command.</comment>
<file context>
@@ -0,0 +1,433 @@
+ // server table is the right source -- not the runtime counter table.
+ std::string q =
+ "SELECT "
+ "hostname AS name, "
+ "hostname AS host, "
+ "port AS port, "
</file context>
| static std::string query_pools(bool extended) { | ||
| std::string q = | ||
| "SELECT " | ||
| "cp.srv_host AS database, " |
There was a problem hiding this comment.
P1: SHOW POOLS reports each backend hostname as the database name, so monitoring displays incorrect pool identities. Join the routing metadata to recover database names, or return an explicitly unavailable value instead of fabricating one.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/pgbouncer_compat/PgBouncer_ShowCommands.cpp, line 83:
<comment>`SHOW POOLS` reports each backend hostname as the database name, so monitoring displays incorrect pool identities. Join the routing metadata to recover database names, or return an explicitly unavailable value instead of fabricating one.</comment>
<file context>
@@ -0,0 +1,433 @@
+static std::string query_pools(bool extended) {
+ std::string q =
+ "SELECT "
+ "cp.srv_host AS database, "
+ "'-' AS user, "
+ "0 AS cl_active, "
</file context>
| // A failed parse into a previously-populated Config must not leave the | ||
| // old content behind. | ||
| PgBouncer::parse_config_file("fixtures/pgbouncer_compat/minimal.ini", config); | ||
| CHECK(config.databases.size() != dbs_first || dbs_first == 1, |
There was a problem hiding this comment.
P3: The || dbs_first == 1 escape hatch lets this assertion pass even if re-parsing actually failed to replace the content (whenever the first file happened to contain one database), so it no longer proves the "different file replaces previous content" claim. It is also dead here since dbs_first comes from full.ini. Drop the disjunct, and note the comment says "a failed parse" while the code runs a successful parse of a different file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp, line 405:
<comment>The `|| dbs_first == 1` escape hatch lets this assertion pass even if re-parsing actually failed to replace the content (whenever the first file happened to contain one database), so it no longer proves the "different file replaces previous content" claim. It is also dead here since `dbs_first` comes from full.ini. Drop the disjunct, and note the comment says "a failed parse" while the code runs a successful parse of a different file.</comment>
<file context>
@@ -0,0 +1,480 @@
+ // A failed parse into a previously-populated Config must not leave the
+ // old content behind.
+ PgBouncer::parse_config_file("fixtures/pgbouncer_compat/minimal.ini", config);
+ CHECK(config.databases.size() != dbs_first || dbs_first == 1,
+ "parsing a different file replaces the previous content");
+}
</file context>
Suggested change
| CHECK(config.databases.size() != dbs_first || dbs_first == 1, | |
| CHECK(config.databases.size() != dbs_first, | |
| "parsing a different file replaces the previous content"); |
| CHECK_STR(rules[3].address, "10.0.0.0/8", "rule 3 address"); | ||
| CHECK_STR(rules[3].method, "cert", "rule 3 method"); | ||
| CHECK(rules[3].options.count("map") > 0, "rule 3 has map option"); | ||
| CHECK_STR(rules[3].options.at("map"), "mymap", "rule 3 map=mymap"); |
There was a problem hiding this comment.
P3: rules[3].options.at("map") throws std::out_of_range if rule 3 has no map option. Because the preceding CHECK(rules[3].options.count("map") > 0) only records a failure but does not stop execution, a parser regression that drops the option would still reach .at() and abort the whole test binary via an uncaught exception, hiding the TAP diagnostic instead of producing a clean failure. Guard the lookup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/unit/pgbouncer_config_parser_unit-t.cpp, line 255:
<comment>`rules[3].options.at("map")` throws `std::out_of_range` if rule 3 has no `map` option. Because the preceding `CHECK(rules[3].options.count("map") > 0)` only records a failure but does not stop execution, a parser regression that drops the option would still reach `.at()` and abort the whole test binary via an uncaught exception, hiding the TAP diagnostic instead of producing a clean failure. Guard the lookup.</comment>
<file context>
@@ -0,0 +1,480 @@
+ CHECK_STR(rules[3].address, "10.0.0.0/8", "rule 3 address");
+ CHECK_STR(rules[3].method, "cert", "rule 3 method");
+ CHECK(rules[3].options.count("map") > 0, "rule 3 has map option");
+ CHECK_STR(rules[3].options.at("map"), "mymap", "rule 3 map=mymap");
+
+ // hostnossl reject
</file context>
Suggested change
| CHECK_STR(rules[3].options.at("map"), "mymap", "rule 3 map=mymap"); | |
| if (auto it = rules[3].options.find("map"); it != rules[3].options.end()) { | |
| CHECK_STR(it->second, "mymap", "rule 3 map=mymap"); | |
| } else { | |
| CHECK(false, "rule 3 map=mymap - SKIPPED (no map option)"); | |
| } |
| // Parse remaining tokens for flags | ||
| { | ||
| std::string upper_rem = remaining; | ||
| std::transform(upper_rem.begin(), upper_rem.end(), upper_rem.begin(), ::toupper); |
There was a problem hiding this comment.
P3: ::toupper is applied to each char of upper_rem directly. When a byte is outside the unsigned char range (values >= 0x80), passing a potentially negative int to ::toupper is undefined behavior. The same PR's PgBouncer_ShowCommands.cpp correctly casts via static_cast<unsigned char> before calling it, so this spot is inconsistent with that fix. Cast the value before the call.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/Admin_Handler.cpp, line 5551:
<comment>`::toupper` is applied to each `char` of `upper_rem` directly. When a byte is outside the unsigned char range (values >= 0x80), passing a potentially negative `int` to `::toupper` is undefined behavior. The same PR's `PgBouncer_ShowCommands.cpp` correctly casts via `static_cast<unsigned char>` before calling it, so this spot is inconsistent with that fix. Cast the value before the call.</comment>
<file context>
@@ -5456,6 +5494,147 @@ void admin_session_handler(S* sess, void *_pa, PtrSize_t *pkt) {
+ // Parse remaining tokens for flags
+ {
+ std::string upper_rem = remaining;
+ std::transform(upper_rem.begin(), upper_rem.end(), upper_rem.begin(), ::toupper);
+ // Remove extra whitespace for matching
+ // Check for "DRY RUN" and "IGNORE WARNINGS" as whole tokens
</file context>
Suggested change
| std::transform(upper_rem.begin(), upper_rem.end(), upper_rem.begin(), ::toupper); | |
| std::transform(upper_rem.begin(), upper_rem.end(), upper_rem.begin(), | |
| [](unsigned char c) { return std::toupper(c); }); |
| if constexpr (std::is_same_v<S, PgSQL_Session>) { | ||
| // Cheap gate: this block now sits ahead of the generic SHOW dispatch, so | ||
| // without it every admin query would be normalized and tokenized twice. | ||
| if (query_no_space_length > 5 && !strncasecmp("SHOW ", query_no_space, 5)) { |
There was a problem hiding this comment.
P3: This PgBouncer SHOW translation/untranslation block is gated only on PgSQL_Session and runs before any session_type check, so it also fires for stats (non-admin) PgSQL sessions. The translated SQL for SERVERS/DATABASES/USERS/CONFIG/VERSION/STATE queries global_variables, runtime_pgsql_servers, and runtime_pgsql_users, which live in the admin DB (main), not the stats DB. On a stats session __run_query executes against SPA->statsdb, so those SHOW commands will fail with missing-table errors while others (POOLS/LISTS, which only touch stats_pgsql_connection_pool) succeed — inconsistent behavior. The adjacent IMPORT block correctly guards with sess->session_type != PROXYSQL_SESSION_ADMIN. Restrict this block to admin sessions too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/Admin_Handler.cpp, line 5144:
<comment>This PgBouncer SHOW translation/untranslation block is gated only on `PgSQL_Session` and runs before any `session_type` check, so it also fires for stats (non-admin) PgSQL sessions. The translated SQL for SERVERS/DATABASES/USERS/CONFIG/VERSION/STATE queries `global_variables`, `runtime_pgsql_servers`, and `runtime_pgsql_users`, which live in the admin DB (`main`), not the stats DB. On a stats session `__run_query` executes against `SPA->statsdb`, so those SHOW commands will fail with missing-table errors while others (POOLS/LISTS, which only touch `stats_pgsql_connection_pool`) succeed — inconsistent behavior. The adjacent IMPORT block correctly guards with `sess->session_type != PROXYSQL_SESSION_ADMIN`. Restrict this block to admin sessions too.</comment>
<file context>
@@ -5125,6 +5128,41 @@ void admin_session_handler(S* sess, void *_pa, PtrSize_t *pkt) {
+ if constexpr (std::is_same_v<S, PgSQL_Session>) {
+ // Cheap gate: this block now sits ahead of the generic SHOW dispatch, so
+ // without it every admin query would be normalized and tokenized twice.
+ if (query_no_space_length > 5 && !strncasecmp("SHOW ", query_no_space, 5)) {
+ // Check for unsupported PgBouncer SHOW commands first
+ std::string unsupported_msg = PgBouncer::get_unsupported_show_message(query_no_space, query_no_space_length);
</file context>
| // Find the last path separator | ||
| const char* last_sep = strrchr(binary_name, '/'); | ||
| if (last_sep) binary_name = last_sep + 1; | ||
| #ifdef _WIN32 |
There was a problem hiding this comment.
P3: On Windows the dispatcher can never match: the executable is proxysql-cli.exe, so strcmp(binary_name, "proxysql-cli") always fails and invoking proxysql-cli silently falls through and starts the full ProxySQL daemon. The block also compares last_bsep > last_sep where last_sep is NULL for any path without / (pointer-to-NULL comparison is undefined behavior), and / stripping runs before the \\ search so a mixed-separator path is mishandled. Strip any .exe suffix and resolve the final separator consistently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main.cpp, line 2794:
<comment>On Windows the dispatcher can never match: the executable is `proxysql-cli.exe`, so `strcmp(binary_name, "proxysql-cli")` always fails and invoking proxysql-cli silently falls through and starts the full ProxySQL daemon. The block also compares `last_bsep > last_sep` where `last_sep` is NULL for any path without `/` (pointer-to-NULL comparison is undefined behavior), and `/` stripping runs before the `\\` search so a mixed-separator path is mishandled. Strip any `.exe` suffix and resolve the final separator consistently.</comment>
<file context>
@@ -2780,8 +2780,26 @@ void watchdog_main_loop() {
+ // Find the last path separator
+ const char* last_sep = strrchr(binary_name, '/');
+ if (last_sep) binary_name = last_sep + 1;
+#ifdef _WIN32
+ const char* last_bsep = strrchr(binary_name, '\\');
+ if (last_bsep && last_bsep > last_sep) binary_name = last_bsep + 1;
</file context>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/tap/tests/unit/pgbouncer_converter_unit-t.cpp">
<violation number="1" location="test/tap/tests/unit/pgbouncer_converter_unit-t.cpp:390">
P3: The comment claims `md5d41d8cd98f00b204e9800998ecf8427` is "md5 of the empty string", but the actual MD5 of the empty string is `d41d8cd98f00b204e9800998ecf8427e` (32 hex chars). The fixture value is 31 hex chars and omits the trailing `e`, so the annotation is inaccurate. Update the comment or the value so the claim matches (and the verifier is a well-formed `md5` + 32 hex hash).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| PgBouncer::AuthFileEntry md5e; | ||
| md5e.username = "alice"; | ||
| md5e.password = "md5d41d8cd98f00b204e9800998ecf8427"; // NOSONAR(cpp:S2068): md5 of the empty string, used to assert the MD5 verifier is reported as unusable. |
There was a problem hiding this comment.
P3: The comment claims md5d41d8cd98f00b204e9800998ecf8427 is "md5 of the empty string", but the actual MD5 of the empty string is d41d8cd98f00b204e9800998ecf8427e (32 hex chars). The fixture value is 31 hex chars and omits the trailing e, so the annotation is inaccurate. Update the comment or the value so the claim matches (and the verifier is a well-formed md5 + 32 hex hash).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/unit/pgbouncer_converter_unit-t.cpp, line 390:
<comment>The comment claims `md5d41d8cd98f00b204e9800998ecf8427` is "md5 of the empty string", but the actual MD5 of the empty string is `d41d8cd98f00b204e9800998ecf8427e` (32 hex chars). The fixture value is 31 hex chars and omits the trailing `e`, so the annotation is inaccurate. Update the comment or the value so the claim matches (and the verifier is a well-formed `md5` + 32 hex hash).</comment>
<file context>
@@ -387,19 +387,19 @@ void test_hashed_password_is_flagged() {
PgBouncer::AuthFileEntry md5e;
md5e.username = "alice";
- md5e.password = "md5d41d8cd98f00b204e9800998ecf8427";
+ md5e.password = "md5d41d8cd98f00b204e9800998ecf8427"; // NOSONAR(cpp:S2068): md5 of the empty string, used to assert the MD5 verifier is reported as unusable.
md5e.type = PgBouncer::AuthType::MD5;
config.auth_entries.push_back(md5e);
</file context>
Suggested change
| md5e.password = "md5d41d8cd98f00b204e9800998ecf8427"; // NOSONAR(cpp:S2068): md5 of the empty string, used to assert the MD5 verifier is reported as unusable. | |
| md5e.password = "md5d41d8cd98f00b204e9800998ecf8427e"; // NOSONAR(cpp:S2068): md5 of the empty string, used to assert the MD5 verifier is reported as unusable. |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.0 #5566 +/- ##
===========================================
+ Coverage 27.10% 59.76% +32.66%
===========================================
Files 159 621 +462
Lines 82482 177424 +94942
Branches 22458 45190 +22732
===========================================
+ Hits 22359 106043 +83684
+ Misses 53815 47639 -6176
- Partials 6308 23742 +17434
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PgBouncer migration tooling: read an existing PgBouncer configuration into ProxySQL, and answer PgBouncer's
SHOWcommands on the PostgreSQL admin port so existing monitoring keeps working after the switch.This PR delivers all three stages of the initiative, not just Stage 1 as the original title said.
What's included
Stage 1 — parser library (
lib/pgbouncer_compat/, #5563)PgBouncer_ConfigParser—pgbouncer.ini: all four sections,%includeto 10 levels, connection-stringkey=valueparsingPgBouncer_AuthFileParser—userlist.txt, with plain/MD5/SCRAM detectionPgBouncer_HBAParser—pg_hba.conf(local/host/hostssl/hostnossl, all auth methods,key=valueoptions)Stage 2 — converter, CLI, admin command (#5564)
PgBouncer_ConfigConverter— emits ProxySQL SQL forpgsql_servers/pgsql_users/pgsql_query_rules/ firewall whitelist /pgsql-*variables; strict by default,IGNORE WARNINGSto downgradeproxysql-cli import-pgbouncer <path> [--dry-run] [--ignore-warnings]— theproxysqlbinary under a second name, dispatched onargv[0], never starts the daemonIMPORT PGBOUNCER CONFIG FROM '<path>' [DRY RUN] [IGNORE WARNINGS]on the admin interfaceStage 3 — PgBouncer-compatible
SHOW(#5565)POOLS,STATS,SERVERS,CLIENTS,DATABASES,USERS,CONFIG,VERSION,STATE,LISTS), each with aSHOW EXTENDEDform appending ProxySQL-specific columnsDocumentation —
doc/PGBOUNCER_COMPAT.md: the full parameter mapping, theSHOWsupport matrix, and an explicit list of what is not mapped and why.Correctness work in this PR
The generated SQL and several
SHOWtranslations referenced columns that do not exist, so they failed the moment they were executed. The original unit tests compared generated strings, which is why all 205 of them passed against SQL that could not run:pgsql_query_ruleshas noschemanamecolumn (that is the MySQL table; the PgSQL one calls itdatabase) — every routing-rule INSERT was invalidpgsql_firewall_whitelist_ruleslikewise usesdatabase, and declaresdigest/commentNOT NULLwith no defaultSHOW CLIENTSselecteddbfromstats_pgsql_processlist; the column isdatabaseSHOW DATABASES/SHOW EXTENDED SERVERSreadweight,max_connectionsandmax_replication_lagfromstats_pgsql_connection_pool, which has none of themSHOW DATABASESwas unreachable — ProxySQL's genericSHOWblock claims that command word firstSHOW EXTENDEDwas a no-op for eight of the ten commandsSemantics that were silently wrong:
convert_users()droppedAuthFileEntry::typeand wrote MD5/SCRAM verifiers intopgsql_users.password. ProxySQL derives both the MD5 response and the SCRAM verifier from the cleartext password, so those credentials could never authenticate. Now reported per user, fatal in strict mode. See the note on SCRAM verifier & md5 credential storage with SCRAM/md5 backend pass-through - PostgreSQL #5865 below.auth_typewas parsed and then ignored entirely — neither mapped nor reported. It now maps topgsql-authentication_method(plain→1,md5→2,scram-sha-256→3), withtrust/any/hba/cert/pamreported.dbname=alias was dropped, emitting a rule that routes to the hostgroup while passing the client's database name through unchangedrejectrules were emitted as an SQL comment and still enabled the whitelist, implying a denial the allow-only whitelist cannot expressParser: quote-aware inline-comment stripping and unquoting of
[pgbouncer]values; HBA""escapes, empty quoted tokens and unterminated-quote detection;parse()is a full load rather than an append.Also removes six duplicated headers under
lib/pgbouncer_compat/(byte-identical copies of theinclude/ones) and switches the guards to the project's__CLASS_*_Hconvention.Tests
Unit tests 205 → 272, with a regression test for each defect above.
New integration test
test/tap/tests/pgsql-pgbouncer_compat-t(49 assertions) executes everySHOWcommand, plain andEXTENDED, and every statement the converter emits, against a live PgSQL admin port. That is the coverage that was missing — string comparison cannot catch a wrong column name.All four tests are now registered in
test/tap/groups/groups.json(they previously ran in no TAP group): the three unit tests inunit-tests-g1, the integration test inlegacy-g4.CI-pgbouncer-compat.ymlis removed rather than fixed — it was pinned tobranches: [feature/pgbouncer-compat]so it would have stopped running the moment it merged, and it was a self-contained workflow in a repo where everyCI-*.ymlonv3.0is a thin caller intoci-*.yml@GH-Actions. Registering the tests ingroups.jsonis what actually gets them running.Packaging
proxysql-cliwas never packaged, so theargv[0]dispatch was unreachable from an installed package. Now shipped by rpm (real symlink in%install, owned by rpm), deb (postinst/postrm, since equivsFiles:handles regular files only), the tarball (a wrapper execinglibexec/proxysql-cli, because the existing wrapper would makeargv[0]beproxysql.binandexec -ais not POSIX sh), andmake install.verify-package-install.bashnow checks it, so a packaging regression fails release verification.Verification
PROXYSQL31=1)pgsql-pgbouncer_compat-t: 49/49 against a livelegacy-g4instancepgsql-proxysql_cmd_test-trunning both before and after the new test in the same session (verifying test isolation)v3.0(was 2356 commits behind) and merges cleanlyRelationship to #5865 / #5863
PR #5865 lets
pgsql_users.passwordhold a SCRAM verifier or anmd5…hash directly — exactly the formatsuserlist.txtalready stores. When it merges, the pre-hashed-password limitation above should lift and auserlist.txtwill import as-is, which matters because a migrating operator frequently does not have the cleartext passwords.The two PRs are code-independent (they overlap only in
groups.jsonandtest/tap/tests/unit/Makefile, both additive), so they can land in either order. The follow-up is tracked in #6134 rather than blocking this PR.Issues