fix(tests): resolve TAP test failures in multi-group runs#5619
Merged
renecannao merged 57 commits intov3.0from Apr 13, 2026
Merged
fix(tests): resolve TAP test failures in multi-group runs#5619renecannao merged 57 commits intov3.0from
renecannao merged 57 commits intov3.0from
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:
📝 WalkthroughWalkthroughAdded many infra-dbdeployer test infrastructures and scripts, updated infra .env sourcing and TAP group mappings, hardened multiple tests (NPE guards, retries, diagnostics), adjusted ProxySQL test behaviors and GTID-related rules, and changed PgSQL monitor scheduler to clamp next-run timestamps after variable refresh. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as PgSQL Scheduler
participant Monitor as PgSQL Monitor Thread
participant Config as Task Config Store
participant Timestamps as NextIntervalState
Scheduler->>Monitor: call refresh_variables()
activate Monitor
Monitor-->>Scheduler: returns updated params (intervals)
deactivate Monitor
Scheduler->>Config: fetch updated task params
activate Config
Config-->>Scheduler: new_interval values
deactivate Config
loop each interval type (ping/connect/readonly/repl_lag)
Scheduler->>Timestamps: read current next_*_at
Scheduler->>Scheduler: compute clamp = cur_intv_start + new_interval
alt new_interval > 0 and next_*_at > clamp
Scheduler->>Timestamps: set next_*_at = clamp
Scheduler->>Scheduler: emit debug log (clamp applied)
end
end
Scheduler->>Scheduler: compute_next_intvs()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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 updates infrastructure configurations for PostgreSQL and MySQL, enhances test reliability with retry logic for log reading, and introduces safety checks for JSON and database result parsing. Key feedback includes addressing a potential crash due to an unchecked iterator in set_testing-240-t.cpp, ensuring proper stream state management during log retries in pgsql-copy_from_stdin_session_parameter-t.cpp, and removing hardcoded rule IDs in test_binlog_reader-t.cpp to avoid primary key conflicts.
| } | ||
|
|
||
| if (k.value() != el.value()) { // different in mysql | ||
| if (k != mysql_vars.end() && k.value().is_string() && k.value() != el.value()) { // different in mysql |
There was a problem hiding this comment.
The iterator s is accessed at lines 381 and 382 without verifying if it reached the end of the container. If sql_mode is not found in proxysql_vars["conn"], this will result in a crash or undefined behavior. Please add a check for s != proxysql_vars["conn"].end() to the condition.
if (k != mysql_vars.end() && k.value().is_string() && s != proxysql_vars["conn"].end() && k.value() != el.value()) { // different in mysql
Comment on lines
+113
to
+117
| // First try the stream-based check (fast path, no filesystem hit) | ||
| const auto& [_, cmd_lines] { get_matching_lines(f_proxysql_log, command_regex) }; | ||
| if (!cmd_lines.empty()) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The retry loop uses get_matching_lines on the f_proxysql_log stream. If the first attempt reaches the end of the file, the EOF bit is set on the stream, and subsequent calls will fail immediately without reading any new data that might have been flushed to the file in the meantime. You should clear the stream state and refresh the position inside the loop to ensure retries actually read new content.
Suggested change
| // First try the stream-based check (fast path, no filesystem hit) | |
| const auto& [_, cmd_lines] { get_matching_lines(f_proxysql_log, command_regex) }; | |
| if (!cmd_lines.empty()) { | |
| return true; | |
| } | |
| // First try the stream-based check (fast path, no filesystem hit) | |
| f_proxysql_log.clear(f_proxysql_log.rdstate() & ~std::ios_base::failbit); | |
| f_proxysql_log.seekg(f_proxysql_log.tellg()); | |
| const auto& [_, cmd_lines] { get_matching_lines(f_proxysql_log, command_regex) }; | |
| if (!cmd_lines.empty()) { | |
| return true; | |
| } |
Comment on lines
+431
to
+444
| query = "INSERT INTO mysql_query_rules (rule_id,active,username,match_digest,destination_hostgroup,apply,gtid_from_hostgroup,comment) " | ||
| "VALUES (1,1,'sbtest8','^SELECT.*FOR UPDATE'," + std::to_string(WHG) + ",1,null,'test_binlog_reader-t')"; | ||
| MYSQL_QUERY(proxysql_admin, query.c_str()); | ||
| mysql_free_result(mysql_store_result(proxysql_admin)); | ||
|
|
||
| query = "INSERT INTO mysql_query_rules (rule_id,active,username,match_digest,destination_hostgroup,apply,gtid_from_hostgroup,comment) " | ||
| "VALUES (2,1,'sbtest8','^SELECT'," + std::to_string(RHG) + ",1," + std::to_string(WHG) + ",'test_binlog_reader-t')"; | ||
| MYSQL_QUERY(proxysql_admin, query.c_str()); | ||
| mysql_free_result(mysql_store_result(proxysql_admin)); | ||
|
|
||
| query = "INSERT INTO mysql_query_rules (rule_id,active,username,match_digest,destination_hostgroup,apply,gtid_from_hostgroup,comment) " | ||
| "VALUES (3,1,'sbtest8','.*'," + std::to_string(WHG) + ",1,null,'test_binlog_reader-t')"; | ||
| MYSQL_QUERY(proxysql_admin, query.c_str()); | ||
| mysql_free_result(mysql_store_result(proxysql_admin)); |
There was a problem hiding this comment.
Hardcoding rule_id (1, 2, 3) in the INSERT statements can cause primary key conflicts in multi-group test runs if multiple tests share the same ProxySQL instance. To better support multi-group runs as intended by this PR, it is recommended to omit the rule_id column and let ProxySQL's auto-increment mechanism handle the ID assignment.
Suggested change
| query = "INSERT INTO mysql_query_rules (rule_id,active,username,match_digest,destination_hostgroup,apply,gtid_from_hostgroup,comment) " | |
| "VALUES (1,1,'sbtest8','^SELECT.*FOR UPDATE'," + std::to_string(WHG) + ",1,null,'test_binlog_reader-t')"; | |
| MYSQL_QUERY(proxysql_admin, query.c_str()); | |
| mysql_free_result(mysql_store_result(proxysql_admin)); | |
| query = "INSERT INTO mysql_query_rules (rule_id,active,username,match_digest,destination_hostgroup,apply,gtid_from_hostgroup,comment) " | |
| "VALUES (2,1,'sbtest8','^SELECT'," + std::to_string(RHG) + ",1," + std::to_string(WHG) + ",'test_binlog_reader-t')"; | |
| MYSQL_QUERY(proxysql_admin, query.c_str()); | |
| mysql_free_result(mysql_store_result(proxysql_admin)); | |
| query = "INSERT INTO mysql_query_rules (rule_id,active,username,match_digest,destination_hostgroup,apply,gtid_from_hostgroup,comment) " | |
| "VALUES (3,1,'sbtest8','.*'," + std::to_string(WHG) + ",1,null,'test_binlog_reader-t')"; | |
| MYSQL_QUERY(proxysql_admin, query.c_str()); | |
| mysql_free_result(mysql_store_result(proxysql_admin)); | |
| query = "INSERT INTO mysql_query_rules (active,username,match_digest,destination_hostgroup,apply,gtid_from_hostgroup,comment) " | |
| "VALUES (1,'sbtest8','^SELECT.*FOR UPDATE'," + std::to_string(WHG) + ",1,null,'test_binlog_reader-t')"; | |
| MYSQL_QUERY(proxysql_admin, query.c_str()); | |
| mysql_free_result(mysql_store_result(proxysql_admin)); | |
| query = "INSERT INTO mysql_query_rules (active,username,match_digest,destination_hostgroup,apply,gtid_from_hostgroup,comment) " | |
| "VALUES (1,'sbtest8','^SELECT'," + std::to_string(RHG) + ",1," + std::to_string(WHG) + ",'test_binlog_reader-t')"; | |
| MYSQL_QUERY(proxysql_admin, query.c_str()); | |
| mysql_free_result(mysql_store_result(proxysql_admin)); | |
| query = "INSERT INTO mysql_query_rules (active,username,match_digest,destination_hostgroup,apply,gtid_from_hostgroup,comment) " | |
| "VALUES (1,'sbtest8','.*'," + std::to_string(WHG) + ",1,null,'test_binlog_reader-t')"; | |
| MYSQL_QUERY(proxysql_admin, query.c_str()); | |
| mysql_free_result(mysql_store_result(proxysql_admin)); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/tap/tests/pgsql-copy_from_stdin_session_parameter-t.cpp`:
- Around line 121-123: The fallback call to
get_matching_lines_from_filename(g_proxysql_log_path, command_regex, false, 0)
passes max_lines=0 which causes every line to be dropped; change the final
argument to a non-zero "unlimited" value (e.g.
std::numeric_limits<size_t>::max() or SIZE_MAX) so the fallback reads all lines
instead of none, keeping the rest of the call (g_proxysql_log_path,
command_regex, false) and the return check (!cmd_lines.empty()) unchanged.
In `@test/tap/tests/set_testing-240-t.cpp`:
- Around line 378-382: The sql_mode comparison dereferences the proxysql
iterator s without a null-check; update the sql_mode block to mirror the
optimizer_switch fix by verifying s != proxysql_vars["conn"].end() before
calling s.value(), and only compare el.value() == s.value() (and construct
s_val) when s is valid; reference the iterators k, s and containers mysql_vars
and proxysql_vars and the elements el to locate and apply the guard.
In `@test/tap/tests/test_binlog_reader-t.cpp`:
- Around line 424-450: Before you overwrite mysql_query_rules for user
'sbtest8', capture the existing rules and ensure they are restored in the test
teardown so other tests aren't affected; specifically, call the same
proxysql_admin/MYSQL_QUERY sequence to SELECT and store the current rows for
username='sbtest8' (use the same proxysql_admin handle and MYSQL_QUERY helper)
before the block that DELETEs and INSERTs, and in the test's teardown/cleanup
routine use those stored rows to DELETE any test-inserted rules and re-INSERT
the original rows (or LOAD MYSQL QUERY RULES TO RUNTIME after restoring) so the
original routing state is reinstated.
🪄 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: 503d56ab-7926-4985-acae-2b500915430f
📒 Files selected for processing (16)
test/infra/docker-pgsql16-single/conf/pgsql/pgsql1/postgresql.conftest/infra/infra-mysql57-binlog/.envtest/infra/infra-mysql57-binlog/conf/mysql/mysql1/my.cnftest/infra/infra-mysql57-binlog/conf/mysql/mysql2/my.cnftest/infra/infra-mysql57-binlog/conf/mysql/mysql3/my.cnftest/infra/infra-mysql57/conf/mysql/mysql1/my.cnftest/infra/infra-mysql57/conf/mysql/mysql2/my.cnftest/infra/infra-mysql57/conf/mysql/mysql3/my.cnftest/infra/infra-pgsql17-repl/conf/pgsql/pgsql1/postgresql.conftest/infra/infra-pgsql17-repl/conf/pgsql/pgsql2/postgresql.conftest/tap/groups/groups.jsontest/tap/tests/pgsql-copy_from_stdin_session_parameter-t.cpptest/tap/tests/set_testing-240-t.cpptest/tap/tests/set_testing-240.htest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_dns_cache-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers controlled by flags:PROXYSQL31=1for v3.1.x (FFTO, TSDB),PROXYSQLGENAI=1for v4.0.x (GenAI, MCP, Anomaly Detection).PROXYSQLGENAI=1impliesPROXYSQL31=1which impliesPROXYSQLFFTO=1andPROXYSQLTSDB=1
Conditional compilation uses#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSEto gate feature-specific code
Class names use PascalCase with protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session,ProxySQL_Admin)
Member variables use snake_case naming convention
Constants and macros use UPPER_SNAKE_CASE naming convention
Require C++17 standard for all source code
Use RAII (Resource Acquisition Is Initialization) pattern for resource management in C++ code
Use jemalloc for memory allocation in C++ code
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
test/tap/tests/set_testing-240.htest/tap/tests/test_dns_cache-t.cpptest/tap/tests/pgsql-copy_from_stdin_session_parameter-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/set_testing-240-t.cpp
test/tap/tests/**/{test_*.cpp,*-t.cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow naming pattern
test_*.cppor*-t.cppin test/tap/tests/
Files:
test/tap/tests/test_dns_cache-t.cpptest/tap/tests/pgsql-copy_from_stdin_session_parameter-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/set_testing-240-t.cpp
🧠 Learnings (11)
📓 Common learnings
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:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
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:03.216Z
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-04-01T21:27:03.216Z
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:03.216Z
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/test_dns_cache-t.cpptest/tap/tests/pgsql-copy_from_stdin_session_parameter-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/set_testing-240-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` header files and link against `libproxysql.a` via custom test harness
Applied to files:
test/tap/tests/test_dns_cache-t.cpptest/tap/tests/pgsql-copy_from_stdin_session_parameter-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/set_testing-240-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/test_dns_cache-t.cpptest/tap/tests/pgsql-copy_from_stdin_session_parameter-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/set_testing-240-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Conditional compilation uses `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, `#ifdef PROXYSQLFFTO`, `#ifdef PROXYSQLTSDB`, and `#ifdef PROXYSQLCLICKHOUSE` to gate feature-specific code
Applied to files:
test/tap/tests/pgsql-copy_from_stdin_session_parameter-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/set_testing-240-t.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:
test/tap/tests/pgsql-copy_from_stdin_session_parameter-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/set_testing-240-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Feature tiers controlled by flags: `PROXYSQL31=1` for v3.1.x (FFTO, TSDB), `PROXYSQLGENAI=1` for v4.0.x (GenAI, MCP, Anomaly Detection). `PROXYSQLGENAI=1` implies `PROXYSQL31=1` which implies `PROXYSQLFFTO=1` and `PROXYSQLTSDB=1`
Applied to files:
test/tap/tests/test_binlog_reader-t.cpptest/tap/tests/set_testing-240-t.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:
test/tap/tests/test_binlog_reader-t.cpptest/tap/tests/set_testing-240-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Refer to `doc/agents/project-conventions.md` for ProxySQL-specific rules on directories, build, test harness, and git workflow
Applied to files:
test/tap/tests/test_binlog_reader-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Admin Interface implementation resides in `ProxySQL_Admin.cpp` and `Admin_Handler.cpp`, with schema versions tracked in `ProxySQL_Admin_Tables_Definitions.h`. Supports runtime config changes without restart via SQL-based configuration with SQLite3 backend
Applied to files:
test/tap/tests/test_binlog_reader-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names use PascalCase with protocol prefixes: `MySQL_`, `PgSQL_`, or `ProxySQL_` (e.g., `MySQL_Protocol`, `PgSQL_Session`, `ProxySQL_Admin`)
Applied to files:
test/tap/tests/set_testing-240-t.cpp
🔇 Additional comments (16)
test/tap/tests/test_dns_cache-t.cpp (1)
263-267: Good stabilization change for DNS metric measurement.Replacing
INVALID_DOMAINwith0.0.0.0before sampling removes DNS side effects in Step 9 and makes the equality checks deterministic.test/infra/docker-pgsql16-single/conf/pgsql/pgsql1/postgresql.conf (1)
3-3: LGTM! Configuration change addresses connection exhaustion.The increase from the default PostgreSQL max_connections (~100) to 120 is appropriate for test infrastructure and aligns with the PR objective to prevent connection slot exhaustion during monitor SSL tests.
test/infra/infra-pgsql17-repl/conf/pgsql/pgsql1/postgresql.conf (1)
8-8: LGTM! Consistent connection limit across test infrastructure.The max_connections setting is consistent with other PostgreSQL instances in this PR and appropriate for the primary server in the replication setup.
test/infra/infra-pgsql17-repl/conf/pgsql/pgsql2/postgresql.conf (1)
8-8: LGTM! Replica connection limit matches primary.Setting the replica's max_connections to match the primary (120) is appropriate for test infrastructure, ensuring consistent behavior and avoiding connection exhaustion on either server.
test/infra/infra-mysql57-binlog/.env (1)
23-23: Change is safe to proceed.The removal of
INFRA_LOGS_PATHfrom.envis correct. Post-init scripts (docker-mysql-post.bashanddocker-orchestrator-post.bash) source.envbut only to readINFRA,INFRA_ID,COMPOSE_PROJECT, andROOT_PASSWORD—none of them useINFRA_LOGS_PATH. The variable is properly set bydocker-compose-init.bashat line 51 before Docker Compose mounts volumes, ensuring the correct path is used.test/infra/infra-mysql57/conf/mysql/mysql1/my.cnf (1)
31-31: Good compatibility fix for MySQL 5.7 test infra.Line 31 adds the required compatibility toggle cleanly and matches the intended test behavior.
test/infra/infra-mysql57-binlog/conf/mysql/mysql3/my.cnf (1)
9-9: LGTM for MySQL 5.7 binlog fixture compatibility.Line 9 is a targeted and appropriate config addition.
test/infra/infra-mysql57/conf/mysql/mysql2/my.cnf (1)
31-31: Consistent fix applied correctly.Line 31 keeps this node aligned with the other MySQL 5.7 fixtures.
test/infra/infra-mysql57-binlog/conf/mysql/mysql1/my.cnf (1)
9-9: Looks good.Line 9 is the expected compatibility flag for this fixture.
test/infra/infra-mysql57-binlog/conf/mysql/mysql2/my.cnf (1)
9-9: Approved — consistent MySQL 5.7 compatibility setting.Line 9 is correct for this infra profile.
test/infra/infra-mysql57/conf/mysql/mysql3/my.cnf (1)
31-31: Good change; consistent across mysql57 nodes.Line 31 is appropriate and low-risk.
test/tap/tests/set_testing-240.h (1)
154-156: Nice hardening ofparseResultnull handling.Lines 154 and 156 correctly guard nullable
row[1]and remove the NULLstrcmpcrash path.test/tap/tests/set_testing-240-t.cpp (4)
186-192: MariaDB skip for unsupportedsession_track_gtidslooks correct.This is a clean guard for an engine-specific unsupported variable and prevents false failures in mixed runs.
329-336: Good null/type guards before decimal trimming.The
find(...)+is_string()checks correctly avoid unsafe parsing when values are missing or non-string.Also applies to: 341-348
361-369:optimizer_switchguard hardening is solid.Checking both iterators and JSON string types before substring comparisons is the right defensive fix.
433-434: The newsession_track_gtidscomparison path is safer and clearer.Using guarded string extraction for
check_session_track_gtids(...)improves resilience when JSON values are missing or non-string.
Comment on lines
+121
to
+123
| // Final attempt: read directly from file to avoid any stream position issues | ||
| const auto& [_, cmd_lines] { get_matching_lines_from_filename(g_proxysql_log_path, command_regex, false, 0) }; | ||
| return !cmd_lines.empty(); |
There was a problem hiding this comment.
Fallback scan never matches with max_lines=0.
Line 122 passes 0 as max_lines, but get_matching_lines_from_filename() keeps only the last max_lines entries. In test/tap/tap/utils.cpp:1684-1750, 0 causes every line to be dropped immediately, so this new fallback always returns an empty match set and the flake fix never activates.
Proposed fix
- const auto& [_, cmd_lines] { get_matching_lines_from_filename(g_proxysql_log_path, command_regex, false, 0) };
+ constexpr size_t fallback_max_lines = 256;
+ const auto& [_, cmd_lines] {
+ get_matching_lines_from_filename(g_proxysql_log_path, command_regex, false, fallback_max_lines)
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Final attempt: read directly from file to avoid any stream position issues | |
| const auto& [_, cmd_lines] { get_matching_lines_from_filename(g_proxysql_log_path, command_regex, false, 0) }; | |
| return !cmd_lines.empty(); | |
| // Final attempt: read directly from file to avoid any stream position issues | |
| constexpr size_t fallback_max_lines = 256; | |
| const auto& [_, cmd_lines] { | |
| get_matching_lines_from_filename(g_proxysql_log_path, command_regex, false, fallback_max_lines) | |
| }; | |
| return !cmd_lines.empty(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/pgsql-copy_from_stdin_session_parameter-t.cpp` around lines
121 - 123, The fallback call to
get_matching_lines_from_filename(g_proxysql_log_path, command_regex, false, 0)
passes max_lines=0 which causes every line to be dropped; change the final
argument to a non-zero "unlimited" value (e.g.
std::numeric_limits<size_t>::max() or SIZE_MAX) so the fallback reads all lines
instead of none, keeping the rest of the call (g_proxysql_log_path,
command_regex, false) and the return check (!cmd_lines.empty()) unchanged.
Comment on lines
+424
to
+450
| // Create GTID-based query rules for sbtest8 | ||
| { | ||
| string query; | ||
| query = "DELETE FROM mysql_query_rules WHERE username='sbtest8'"; | ||
| MYSQL_QUERY(proxysql_admin, query.c_str()); | ||
| mysql_free_result(mysql_store_result(proxysql_admin)); | ||
|
|
||
| query = "INSERT INTO mysql_query_rules (rule_id,active,username,match_digest,destination_hostgroup,apply,gtid_from_hostgroup,comment) " | ||
| "VALUES (1,1,'sbtest8','^SELECT.*FOR UPDATE'," + std::to_string(WHG) + ",1,null,'test_binlog_reader-t')"; | ||
| MYSQL_QUERY(proxysql_admin, query.c_str()); | ||
| mysql_free_result(mysql_store_result(proxysql_admin)); | ||
|
|
||
| query = "INSERT INTO mysql_query_rules (rule_id,active,username,match_digest,destination_hostgroup,apply,gtid_from_hostgroup,comment) " | ||
| "VALUES (2,1,'sbtest8','^SELECT'," + std::to_string(RHG) + ",1," + std::to_string(WHG) + ",'test_binlog_reader-t')"; | ||
| MYSQL_QUERY(proxysql_admin, query.c_str()); | ||
| mysql_free_result(mysql_store_result(proxysql_admin)); | ||
|
|
||
| query = "INSERT INTO mysql_query_rules (rule_id,active,username,match_digest,destination_hostgroup,apply,gtid_from_hostgroup,comment) " | ||
| "VALUES (3,1,'sbtest8','.*'," + std::to_string(WHG) + ",1,null,'test_binlog_reader-t')"; | ||
| MYSQL_QUERY(proxysql_admin, query.c_str()); | ||
| mysql_free_result(mysql_store_result(proxysql_admin)); | ||
|
|
||
| query = "LOAD MYSQL QUERY RULES TO RUNTIME"; | ||
| MYSQL_QUERY(proxysql_admin, query.c_str()); | ||
| mysql_free_result(mysql_store_result(proxysql_admin)); | ||
| fprintf(stderr, "GTID query rules created for sbtest8 (WHG=%d, RHG=%d)\n", WHG, RHG); | ||
| } |
There was a problem hiding this comment.
Restore query rules in teardown to avoid cross-test state leakage.
Line 427 deletes all sbtest8 rules and Lines 431-448 replace runtime rules, but this block never restores baseline state. In shared infra runs, later tests can inherit modified routing behavior.
🔧 Suggested fix
@@
- // Create GTID-based query rules for sbtest8
+ bool gtid_rules_overridden = false;
+ // Create GTID-based query rules for sbtest8
{
@@
query = "LOAD MYSQL QUERY RULES TO RUNTIME";
MYSQL_QUERY(proxysql_admin, query.c_str());
mysql_free_result(mysql_store_result(proxysql_admin));
+ gtid_rules_overridden = true;
fprintf(stderr, "GTID query rules created for sbtest8 (WHG=%d, RHG=%d)\n", WHG, RHG);
}
@@
cleanup:
+ if (gtid_rules_overridden) {
+ mysql_query(proxysql_admin, "LOAD MYSQL QUERY RULES FROM DISK");
+ mysql_free_result(mysql_store_result(proxysql_admin));
+ mysql_query(proxysql_admin, "LOAD MYSQL QUERY RULES TO RUNTIME");
+ mysql_free_result(mysql_store_result(proxysql_admin));
+ }
mysql_close(proxysql_mysql);
mysql_close(proxysql_admin);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/test_binlog_reader-t.cpp` around lines 424 - 450, Before you
overwrite mysql_query_rules for user 'sbtest8', capture the existing rules and
ensure they are restored in the test teardown so other tests aren't affected;
specifically, call the same proxysql_admin/MYSQL_QUERY sequence to SELECT and
store the current rows for username='sbtest8' (use the same proxysql_admin
handle and MYSQL_QUERY helper) before the block that DELETEs and INSERTs, and in
the test's teardown/cleanup routine use those stored rows to DELETE any
test-inserted rules and re-INSERT the original rows (or LOAD MYSQL QUERY RULES
TO RUNTIME after restoring) so the original routing state is reinstated.
renecannao
force-pushed
the
fix/groups-json-orphaned-entries
branch
from
e70a208 to
450c3de
Compare
- set_testing-240-t: Fix crash (json type_error.302) when MySQL returns NULL variable values or fails the session_variables query. Add null guards in parseResult() and all implicit std::string conversions. - Add show_compatibility_56=ON to MySQL 5.7 configs so querying performance_schema.session_variables works reliably. - test_gtid_forwarding-t: Move from legacy-g5 to legacy-binlog-g1 (requires infra with session_track_gtids=OWN_GTID configured). - test_binlog_reader-t: Create own GTID query rules for sbtest8 instead of relying on pre-existing infra rules that may be deleted. - test_ffto_pgsql_error_stats-t: Remove from mysql84-g4 group (no PostgreSQL backend available in mysql84 infrastructure).
…arameter-t and test_dns_cache-t 1. pgsql-copy_from_stdin_session_parameter-t: race condition in log file reading The test's check_logs_for_command() performed a single stream-based read via get_matching_lines() to find the "Switching back to Normal mode" log line. When ProxySQL hadn't flushed the log entry yet (observed window as tight as ~23 microseconds between PQgetResult returning and the check), the read would miss the line, causing a false test failure. The log line was always present in the final file, confirming this is a timing issue. Fix: Replace the single-shot read with a retry loop (up to 10 attempts, 10ms each = 100ms total). If all stream-based retries fail, fall back to get_matching_lines_from_filename() which opens a fresh file handle, avoiding any stream position issues entirely. Add a global g_proxysql_log_path to support the filename-based fallback. 2. test_dns_cache-t: spurious dns_cache_queried counter increment in Step 9 Step 9 disables the DNS cache refresh interval (set to 0) and expects dns_cache_queried to remain unchanged (equal_to). However, INVALID_DOMAIN was still in mysql_servers from Step 8, so when "DO 1" queries were routed through hostgroup 999, the connection routing layer called MySQL_Monitor::dns_lookup() -> DNS_Cache::lookup(), incrementing the counter by exactly 1 regardless of the refresh interval setting. Fix: Before the measurement window in Step 9, replace INVALID_DOMAIN with 0.0.0.0 in mysql_servers. Since 0.0.0.0 is a valid IP address, dns_lookup() returns immediately without consulting the DNS cache (early return at MySQL_Monitor.cpp:6675), so the counter stays unchanged as expected. Both fixes verified: each test passes in isolation against legacy infra.
Closes the pgsql-servers_ssl_params-t portion of #5610. This is the final of four tests from the flake tracking issue. ## Problem 1 — the scheduler caching bug (lib/PgSQL_Monitor.cpp) `pgsql-servers_ssl_params-t` subtests 32 ("Monitor SSL counter increased with use_ssl=1 and no per-server row") and 34 ("Monitor per-server: SSL OK counter resumes advancing after removing the per-server row") were failing deterministically on v3.0. Both expected `PgSQL_Monitor_ssl_connections_OK` to increase within a few seconds of: SET pgsql-monitor_connect_interval=2000; LOAD PGSQL VARIABLES TO RUNTIME; UPDATE pgsql_servers SET use_ssl=1; LOAD PGSQL SERVERS TO RUNTIME; but the counter stayed at 0. Root cause is state caching in the pgsql monitor scheduler loop (`PgSQL_monitor_scheduler_thread()`): T=0 proxysql starts with default pgsql-monitor_connect_interval =120000. The first scheduler tick schedules an initial connect check; compute_next_intvs() sets next_connect_at = T + 120000 ms = T + 120 seconds. T+<30s> test does SET connect_interval=2000 + LOAD PGSQL VARIABLES TO RUNTIME. fetch_updated_conf() starts returning the new 2000 value, but next_connect_at still points at T+120000 because compute_next_intvs() only updates next_<type>_at when the corresponding task type has fired. T+<35s> test reads ssl_connections_OK at the end of its 5-second wait. Counter still 0 because the next scheduled connect check is ~85 seconds in the future. The scheduler is working correctly; what's missing is a bridge between "runtime variables were just refreshed" and "next_<type>_at should reflect the refreshed (shorter) intervals". Fix: in the scheduler loop, track whether the variable version bumped this iteration, and if so, clamp each next_<type>_at down to cur_intv_start + new_interval whenever the refreshed interval would schedule the next check sooner than the currently-cached value. The clamp is one-way (we never push next_<type>_at FURTHER into the future, because growing the interval should not delay an already- imminent check), idempotent, and safe against interval=0 (disabled) which is left to compute_next_intvs() to set to ULONG_MAX. Applied to all four monitor task types: ping, connect, readonly, repl_lag. Same class of bug affected all of them; fixing only connect would leave analogous latent issues for any test that changes the other intervals at runtime. Verified with the raw experiment before committing: configure monitor/monitor + interval=2000 + use_ssl=1 + LOAD, observe counter ticking at the 2-second cadence within 1-2 seconds of LOAD. Before fix: counter stuck at 0 for ~2 minutes (until the cached 120 s interval naturally elapses). ## Problem 2 — test's hardcoded wrong credentials (pgsql-servers_ssl_params-t.cpp) With the scheduler fix alone, the test was still failing because its main() was doing: SET pgsql-monitor_username='postgres'; SET pgsql-monitor_password='postgres'; on the assumption that the backend had a postgres user with password "postgres". But the actual test infra (test/infra/docker-pgsql16-single) RANDOMIZES `POSTGRES_PASSWORD` on every container startup — e.g. `POSTGRES_PASSWORD=05e792e51d`. Hardcoded 'postgres' never matched, so every monitor connect failed with: FATAL: password authentication failed for user "postgres" which increments connect_check_ERR instead of ssl_connections_OK. After the scheduler fix, these auth failures fired every 2 seconds instead of every 120 seconds — but they were still failures, so the counter never advanced. Fix: remove the username/password switch entirely. The default monitor/monitor user is already configured in the infra's pg_hba.conf and authenticates successfully (I verified this manually via `docker exec psql 'host=... user=monitor password=monitor sslmode=require'` from both inside and outside the proxysql container). The corresponding "restore original values" block is also removed since there's nothing to restore. ## Local verification 3 consecutive runs of the full pgsql-servers_ssl_params-t test in legacy-g4 infra with the patched proxysql + patched test binary: attempt 1: PASS attempt 2: PASS attempt 3: PASS === pgsql-servers_ssl_params-t: 3/3 pass === Subtest-level confirmation from the final attempt's TAP log: # Original monitor: user=monitor interval=120000 ms # Initial PgSQL_Monitor_ssl_connections_OK: 33 # After PgSQL_Monitor_ssl_connections_OK: 36 <- +3 in 5 s ok 32 - Monitor SSL counter increased with use_ssl=1 and no per-server row # With TLSv1 per-server pin, ssl OK before wait: 39 # With TLSv1 per-server pin, ssl OK after wait: 39 (delta=0) ok 33 - Monitor per-server: SSL OK counter does NOT advance when per-server row pins ssl_protocol_version_range to TLSv1 # After cleanup, ssl OK recovered from 41 to 44 <- +3 in 5 s ok 34 - Monitor per-server: SSL OK counter resumes advancing after removing the per-server row All three monitor-SSL subtests now exercise the real code path (SSL handshake happening, counter incrementing, per-server pin blocking SSL as designed) instead of observing a no-op. ## Side effect on pgsql-ssl_keylog-t subtest 7 Subtest 7 of pgsql-ssl_keylog-t was marked as SKIP in PR #5612 because it tripped on the same "pgsql monitor isn't making SSL connections" symptom. With this fix merged, the skip's runtime condition (`lines_before_monitor == lines_after_monitor`) will evaluate to false once the monitor is actually producing SSL handshakes, and the test will fall into the `ok(...)` branch automatically. No separate change to that test is needed — the skip was defensive and is dead code after this fix. ## Why mysql monitor is not touched here This fix is scoped to the pgsql monitor scheduler only. The mysql monitor is a different file (lib/MySQL_Monitor.cpp) with a different scheduling architecture (per-thread timers, not a centralized scheduler). If the same class of bug exists there, it would need a separate patch - out of scope for this PR.
- Add proxy_debug(PROXY_DEBUG_MONITOR, 5) calls when clamping fires, so the fix is observable in debug logs. Follows existing pattern from nearby scheduling debug output. - Rename original_monitor_username to monitor_username and clarify it is for diagnostic logging only, since the variable is no longer used for restore after the credential change was removed.
renecannao
force-pushed
the
fix/groups-json-orphaned-entries
branch
2 times, most recently
from
5d51183 to
1ffa870
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/tap/tests/set_testing-240-t.cpp (1)
378-382:⚠️ Potential issue | 🔴 CriticalGuard
sbefore dereferencing in thesql_modebranch.Line 381 and Line 382 still call
s.value()even whens == proxysql_vars["conn"].end(). The earlierdiag()only logs the missing key; it does not make this access safe.Suggested fix
- if (k != mysql_vars.end() && k.value().is_string() && k.value() != el.value()) { // different in mysql + if ( + k != mysql_vars.end() && + s != proxysql_vars["conn"].end() && + k.value().is_string() && + s.value().is_string() && + k.value() != el.value() + ) { // different in mysql std::string e_val { el.value() }; std::string k_val { k.value().get<std::string>() }; - std::string s_val { s.value().is_string() ? s.value().get<std::string>() : std::string("") }; + std::string s_val { s.value().get<std::string>() }; if (el.value() == s.value()) { // but same in proxysql🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/set_testing-240-t.cpp` around lines 378 - 382, The code dereferences iterator/optional s (used from proxysql_vars["conn"]) without verifying it's present; update the sql_mode branch to first check s is valid (e.g., s != proxysql_vars["conn"].end() or s.has_value()) before calling s.value(). Specifically, when building s_val and when comparing el.value() == s.value(), guard both uses with the validity check (adjust the condition that currently reads if (el.value() == s.value()) to something like if (s is valid && el.value() == s.value())), and only call s.value() when s is confirmed present; leave logic for k, el and mysql_vars unchanged.
🧹 Nitpick comments (1)
lib/PgSQL_Monitor.cpp (1)
2536-2611: Well-designed fix for runtime interval responsiveness.The clamping logic correctly ensures that newly-configured smaller intervals take effect immediately after
LOAD PGSQL VARIABLES TO RUNTIME. The one-way clamping (only reducingnext_*_at, never increasing) is the right design choice.The four clamping blocks are repetitive but explicit. If you'd like to reduce duplication, a helper lambda could handle the pattern:
♻️ Optional refactor to reduce duplication
if (vars_refreshed) { + auto clamp_interval = [&](uint64_t& next_at, int32_t interval, const char* name) { + if (interval > 0) { + const uint64_t clamped = cur_intv_start + interval; + if (next_at > clamped) { + proxy_debug(PROXY_DEBUG_MONITOR, 5, + "Clamped %s old=%lu new=%lu interval=%d\n", + name, next_at, clamped, interval); + next_at = clamped; + } + } + }; + clamp_interval(next_intvs.next_ping_at, tasks_conf.ping.params.interval, "next_ping_at"); + clamp_interval(next_intvs.next_connect_at, tasks_conf.connect.params.interval, "next_connect_at"); + clamp_interval(next_intvs.next_readonly_at, tasks_conf.readonly.params.interval, "next_readonly_at"); + clamp_interval(next_intvs.next_repl_lag_at, tasks_conf.repl_lag.params.interval, "next_repl_lag_at"); - if (tasks_conf.ping.params.interval > 0) { - const uint64_t clamped = cur_intv_start + tasks_conf.ping.params.interval; - if (next_intvs.next_ping_at > clamped) { - proxy_debug(PROXY_DEBUG_MONITOR, 5, - "Clamped next_ping_at old=%lu new=%lu interval=%d\n", - next_intvs.next_ping_at, clamped, tasks_conf.ping.params.interval); - next_intvs.next_ping_at = clamped; - } - } - // ... similar blocks for connect, readonly, repl_lag ... },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Monitor.cpp` around lines 2536 - 2611, The four near-identical clamping blocks after vars_refreshed should be DRYed: add a small helper (e.g., a lambda inside the surrounding scope) that takes (uint64_t& next_at, int interval, const char* label) and performs the if interval>0 -> compute clamped = cur_intv_start + interval -> if next_at > clamped log via proxy_debug (include label/old/new/interval) and assign next_at = clamped; then replace the four explicit blocks with four calls supplying next_intvs.next_ping_at / next_connect_at / next_readonly_at / next_repl_lag_at and the corresponding tasks_conf.*.params.interval and labels. Keep vars_refreshed, cur_intv_start and proxy_debug usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/tap/tests/set_testing-240-t.cpp`:
- Around line 378-382: The code dereferences iterator/optional s (used from
proxysql_vars["conn"]) without verifying it's present; update the sql_mode
branch to first check s is valid (e.g., s != proxysql_vars["conn"].end() or
s.has_value()) before calling s.value(). Specifically, when building s_val and
when comparing el.value() == s.value(), guard both uses with the validity check
(adjust the condition that currently reads if (el.value() == s.value()) to
something like if (s is valid && el.value() == s.value())), and only call
s.value() when s is confirmed present; leave logic for k, el and mysql_vars
unchanged.
---
Nitpick comments:
In `@lib/PgSQL_Monitor.cpp`:
- Around line 2536-2611: The four near-identical clamping blocks after
vars_refreshed should be DRYed: add a small helper (e.g., a lambda inside the
surrounding scope) that takes (uint64_t& next_at, int interval, const char*
label) and performs the if interval>0 -> compute clamped = cur_intv_start +
interval -> if next_at > clamped log via proxy_debug (include
label/old/new/interval) and assign next_at = clamped; then replace the four
explicit blocks with four calls supplying next_intvs.next_ping_at /
next_connect_at / next_readonly_at / next_repl_lag_at and the corresponding
tasks_conf.*.params.interval and labels. Keep vars_refreshed, cur_intv_start and
proxy_debug usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 421eb5d4-87aa-4886-be00-cdaa765094eb
📒 Files selected for processing (18)
lib/PgSQL_Monitor.cpptest/infra/docker-pgsql16-single/conf/pgsql/pgsql1/postgresql.conftest/infra/infra-mysql57-binlog/.envtest/infra/infra-mysql57-binlog/conf/mysql/mysql1/my.cnftest/infra/infra-mysql57-binlog/conf/mysql/mysql2/my.cnftest/infra/infra-mysql57-binlog/conf/mysql/mysql3/my.cnftest/infra/infra-mysql57/conf/mysql/mysql1/my.cnftest/infra/infra-mysql57/conf/mysql/mysql2/my.cnftest/infra/infra-mysql57/conf/mysql/mysql3/my.cnftest/infra/infra-pgsql17-repl/conf/pgsql/pgsql1/postgresql.conftest/infra/infra-pgsql17-repl/conf/pgsql/pgsql2/postgresql.conftest/tap/groups/groups.jsontest/tap/tests/pgsql-copy_from_stdin_session_parameter-t.cpptest/tap/tests/pgsql-servers_ssl_params-t.cpptest/tap/tests/set_testing-240-t.cpptest/tap/tests/set_testing-240.htest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_dns_cache-t.cpp
✅ Files skipped from review due to trivial changes (10)
- test/infra/docker-pgsql16-single/conf/pgsql/pgsql1/postgresql.conf
- test/infra/infra-mysql57/conf/mysql/mysql2/my.cnf
- test/infra/infra-mysql57-binlog/conf/mysql/mysql3/my.cnf
- test/infra/infra-mysql57/conf/mysql/mysql1/my.cnf
- test/infra/infra-pgsql17-repl/conf/pgsql/pgsql1/postgresql.conf
- test/infra/infra-mysql57-binlog/conf/mysql/mysql2/my.cnf
- test/infra/infra-mysql57-binlog/conf/mysql/mysql1/my.cnf
- test/infra/infra-pgsql17-repl/conf/pgsql/pgsql2/postgresql.conf
- test/infra/infra-mysql57/conf/mysql/mysql3/my.cnf
- test/infra/infra-mysql57-binlog/.env
🚧 Files skipped from review as they are similar to previous changes (5)
- test/tap/tests/set_testing-240.h
- test/tap/tests/test_dns_cache-t.cpp
- test/tap/tests/pgsql-copy_from_stdin_session_parameter-t.cpp
- test/tap/tests/test_binlog_reader-t.cpp
- test/tap/groups/groups.json
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers controlled by flags:PROXYSQL31=1for v3.1.x (FFTO, TSDB),PROXYSQLGENAI=1for v4.0.x (GenAI, MCP, Anomaly Detection).PROXYSQLGENAI=1impliesPROXYSQL31=1which impliesPROXYSQLFFTO=1andPROXYSQLTSDB=1
Conditional compilation uses#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSEto gate feature-specific code
Class names use PascalCase with protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session,ProxySQL_Admin)
Member variables use snake_case naming convention
Constants and macros use UPPER_SNAKE_CASE naming convention
Require C++17 standard for all source code
Use RAII (Resource Acquisition Is Initialization) pattern for resource management in C++ code
Use jemalloc for memory allocation in C++ code
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
test/tap/tests/pgsql-servers_ssl_params-t.cpplib/PgSQL_Monitor.cpptest/tap/tests/set_testing-240-t.cpp
test/tap/tests/**/{test_*.cpp,*-t.cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow naming pattern
test_*.cppor*-t.cppin test/tap/tests/
Files:
test/tap/tests/pgsql-servers_ssl_params-t.cpptest/tap/tests/set_testing-240-t.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention in lib/ directory
Files:
lib/PgSQL_Monitor.cpp
🧠 Learnings (8)
📓 Common learnings
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:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
📚 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/pgsql-servers_ssl_params-t.cpptest/tap/tests/set_testing-240-t.cpp
📚 Learning: 2026-04-01T21:27:03.216Z
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:03.216Z
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/set_testing-240-t.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:
test/tap/tests/set_testing-240-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` header files and link against `libproxysql.a` via custom test harness
Applied to files:
test/tap/tests/set_testing-240-t.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:
test/tap/tests/set_testing-240-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Feature tiers controlled by flags: `PROXYSQL31=1` for v3.1.x (FFTO, TSDB), `PROXYSQLGENAI=1` for v4.0.x (GenAI, MCP, Anomaly Detection). `PROXYSQLGENAI=1` implies `PROXYSQL31=1` which implies `PROXYSQLFFTO=1` and `PROXYSQLTSDB=1`
Applied to files:
test/tap/tests/set_testing-240-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Conditional compilation uses `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, `#ifdef PROXYSQLFFTO`, `#ifdef PROXYSQLTSDB`, and `#ifdef PROXYSQLCLICKHOUSE` to gate feature-specific code
Applied to files:
test/tap/tests/set_testing-240-t.cpp
🔇 Additional comments (2)
test/tap/tests/pgsql-servers_ssl_params-t.cpp (2)
651-668: Good fix for monitor-auth flakiness in CI.This removes brittle credential assumptions and keeps the test aligned with real infra behavior while retaining useful diagnostics.
678-680: Cleanup note is now accurate and consistent with test behavior.Clear statement of what is restored reduces confusion and future maintenance risk.
- Fix INFRA_LOGS_PATH in infra-mysql57-binlog/.env: removed INFRA_LOGS_PATH=./logs which overrode the proper default from docker-compose-init.bash, causing logs to be written inside test/infra/ instead of ci_infra_logs/ and never cleaned up - Increase PostgreSQL max_connections to 120 (from default 100) in docker-pgsql16-single and infra-pgsql17-repl configs to prevent connection slot exhaustion during monitor SSL tests - Skip session_track_gtids test cases in set_testing-240-t when running against MariaDB, which doesn't support this variable (issues #2134, #3059)
renecannao
force-pushed
the
fix/groups-json-orphaned-entries
branch
from
1ffa870 to
30e86ae
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/tap/tests/set_testing-240-t.cpp (1)
378-382:⚠️ Potential issue | 🔴 CriticalGuard
sbefore dereferencing it in thesql_modebranch.Line 381 and Line 382 still call
s.value()without verifyings != proxysql_vars["conn"].end(). If ProxySQL omitssql_mode, this is undefined behavior and can crash the test thread.🐛 Proposed fix
- if (k != mysql_vars.end() && k.value().is_string() && k.value() != el.value()) { // different in mysql + if ( + k != mysql_vars.end() && + s != proxysql_vars["conn"].end() && + k.value().is_string() && + s.value().is_string() && + k.value() != el.value() + ) { // different in mysql std::string e_val { el.value() }; std::string k_val { k.value().get<std::string>() }; - std::string s_val { s.value().is_string() ? s.value().get<std::string>() : std::string("") }; + std::string s_val { s.value().get<std::string>() }; if (el.value() == s.value()) { // but same in proxysql🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/set_testing-240-t.cpp` around lines 378 - 382, The branch comparing mysql vs proxysql variables dereferences s.value() without checking s exists (the sql_mode path); update the logic around the if that uses s (currently referencing s.value() on lines with el.value() == s.value() and when building s_val) to first verify s != proxysql_vars["conn"].end() (or equivalently check s.has_value()) before calling s.value(), and only construct s_val or compare el.value() to s.value() when that guard passes; adjust the conditional that currently reads if (el.value() == s.value()) to include the s-existence check so no undefined dereference occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/tap/tests/set_testing-240-t.cpp`:
- Around line 378-382: The branch comparing mysql vs proxysql variables
dereferences s.value() without checking s exists (the sql_mode path); update the
logic around the if that uses s (currently referencing s.value() on lines with
el.value() == s.value() and when building s_val) to first verify s !=
proxysql_vars["conn"].end() (or equivalently check s.has_value()) before calling
s.value(), and only construct s_val or compare el.value() to s.value() when that
guard passes; adjust the conditional that currently reads if (el.value() ==
s.value()) to include the s-existence check so no undefined dereference occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69cdf99d-2b39-4d4f-bb4c-b2f0248528bb
📒 Files selected for processing (6)
test/infra/docker-pgsql16-single/conf/pgsql/pgsql1/postgresql.conftest/infra/infra-mysql57-binlog/.envtest/infra/infra-pgsql17-repl/conf/pgsql/pgsql1/postgresql.conftest/infra/infra-pgsql17-repl/conf/pgsql/pgsql2/postgresql.conftest/tap/groups/groups.jsontest/tap/tests/set_testing-240-t.cpp
✅ Files skipped from review due to trivial changes (4)
- test/infra/infra-pgsql17-repl/conf/pgsql/pgsql2/postgresql.conf
- test/infra/docker-pgsql16-single/conf/pgsql/pgsql1/postgresql.conf
- test/infra/infra-pgsql17-repl/conf/pgsql/pgsql1/postgresql.conf
- test/infra/infra-mysql57-binlog/.env
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers controlled by flags:PROXYSQL31=1for v3.1.x (FFTO, TSDB),PROXYSQLGENAI=1for v4.0.x (GenAI, MCP, Anomaly Detection).PROXYSQLGENAI=1impliesPROXYSQL31=1which impliesPROXYSQLFFTO=1andPROXYSQLTSDB=1
Conditional compilation uses#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSEto gate feature-specific code
Class names use PascalCase with protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session,ProxySQL_Admin)
Member variables use snake_case naming convention
Constants and macros use UPPER_SNAKE_CASE naming convention
Require C++17 standard for all source code
Use RAII (Resource Acquisition Is Initialization) pattern for resource management in C++ code
Use jemalloc for memory allocation in C++ code
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
test/tap/tests/set_testing-240-t.cpp
test/tap/tests/**/{test_*.cpp,*-t.cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow naming pattern
test_*.cppor*-t.cppin test/tap/tests/
Files:
test/tap/tests/set_testing-240-t.cpp
🧠 Learnings (11)
📓 Common learnings
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:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
📚 Learning: 2026-04-01T21:27:03.216Z
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:03.216Z
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/groups/groups.jsontest/tap/tests/set_testing-240-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` header files and link against `libproxysql.a` via custom test harness
Applied to files:
test/tap/groups/groups.jsontest/tap/tests/set_testing-240-t.cpp
📚 Learning: 2026-01-20T09:34:27.165Z
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:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
test/tap/groups/groups.json
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Feature tiers controlled by flags: `PROXYSQL31=1` for v3.1.x (FFTO, TSDB), `PROXYSQLGENAI=1` for v4.0.x (GenAI, MCP, Anomaly Detection). `PROXYSQLGENAI=1` implies `PROXYSQL31=1` which implies `PROXYSQLFFTO=1` and `PROXYSQLTSDB=1`
Applied to files:
test/tap/groups/groups.jsontest/tap/tests/set_testing-240-t.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:
test/tap/groups/groups.jsontest/tap/tests/set_testing-240-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/set_testing-240-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Conditional compilation uses `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, `#ifdef PROXYSQLFFTO`, `#ifdef PROXYSQLTSDB`, and `#ifdef PROXYSQLCLICKHOUSE` to gate feature-specific code
Applied to files:
test/tap/tests/set_testing-240-t.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:
test/tap/tests/set_testing-240-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names use PascalCase with protocol prefixes: `MySQL_`, `PgSQL_`, or `ProxySQL_` (e.g., `MySQL_Protocol`, `PgSQL_Session`, `ProxySQL_Admin`)
Applied to files:
test/tap/tests/set_testing-240-t.cpp
📚 Learning: 2026-02-13T09:29:39.713Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5372
File: test/tap/tap/mcp_client.cpp:355-385
Timestamp: 2026-02-13T09:29:39.713Z
Learning: In ProxySQL MCP implementation (test/tap/tap/mcp_client.cpp), the `check_server()` method uses the ping endpoint which is designed to work without authentication. The `ping` method at the `config` endpoint should not require the `Authorization: Bearer` header, unlike tool invocation endpoints which do require authentication when `auth_token_` is set.
Applied to files:
test/tap/tests/set_testing-240-t.cpp
🔇 Additional comments (1)
test/tap/tests/set_testing-240-t.cpp (1)
186-192: Thestrstr()call is case-sensitive, but all instances ofsession_track_gtidsin the test data (set_testing-t.csv) are consistently lowercase. No case variations exist that would be missed by this check, so the skip logic will work correctly as written.
Design for replacing infra-mysql57 (6 containers) with a single-container infra using dbdeployer to run 3 MySQL 5.7 instances. Drops orchestrator (unused by legacy tests). Approach A: entrypoint-driven deployment.
11 tasks covering: Dockerfile, entrypoint, build script, docker-compose, init/destroy scripts, mysql-post, proxy-post, infra-config.sql, and smoke testing.
Ubuntu 22.04 base with dbdeployer v2.2.1 and pre-unpacked MySQL 5.7.44 tarball. Clears reserved ports to allow base-port 3306.
Deploys 3-node GTID replication via dbdeployer, creates all test users (root, monitor, testuser, sbtest1-10, ssluser, user, infra-specific), creates test databases, signals readiness via marker file.
Single service (dbdeployer1) replaces 6 containers (3 mysql + 3 orc). Same hostgroup IDs as infra-mysql57 for compatibility.
Adapted from infra-mysql57. Removes orchestrator patching and SSL setup. Waits for dbdeployer readiness marker before running post-scripts.
Verifies all 3 MySQL nodes on ports 3306/3307/3308, checks replication status, and collects SSL cert bundles. User creation is handled by the container entrypoint.
Adapted from infra-mysql57. Key change: single hostname (dbdeployer1) with 3 ports (3306/3307/3308) instead of 3 hostnames on port 3306. All other config (users, query rules, debug filters) unchanged.
Same logic as infra-mysql57: registers backends in ProxySQL, creates fallback hostgroups 0/1, sets monitor credentials.
Issues found and fixed during smoke testing: - Dockerfile: create /root/opt/mysql before dbdeployer unpack - Dockerfile: use correct tarball filename matching inner dir name - Dockerfile: clear reserved-ports with '0' (empty string not accepted) - Dockerfile: add MySQL client to PATH for docker exec health checks - entrypoint: use --base-port=3305 (dbdeployer assigns base+1 to first node) - entrypoint: use separate -c flags instead of comma-separated --my-cnf-options - entrypoint: use correct sandbox dirs (master/node1/node2 not node1/node2/node3) - entrypoint: connect as root with dbdeployer default password for user creation - entrypoint: alter root@localhost password (not just root@'%') - entrypoint: set read_only=1 on replica nodes
- legacy/env.sh: DEFAULT_MYSQL_INFRA -> infra-dbdeployer57 - legacy/infras.lst: infra-mysql57 -> infra-dbdeployer57 - build.sh: add --network=host for DNS resolution in Docker build
Change WARNING to NOTE for tests in groups.json that aren't on disk. These may be scripts (PHP, Python) not yet built or feature-gated tests — not necessarily compilation failures.
Tests that connect directly to MySQL backends used hardcoded
"mysql1.<infra>" hostname. With dbdeployer, the hostname is
"dbdeployer1.<infra>" and ports differ.
- Add MYSQL_PRIMARY_HOST/PORT to infra-dbdeployer57/.env
- Update control/env-isolated.bash to use these if set, after
sourcing the infra .env (ordering matters)
- Update common/env-isolated.sh with the same override pattern
Fixes: mysql-protocol_compression_level-t,
test_cluster_sync_mysql_servers-t,
test_read_only_actions_offline_hard_servers-t
More explicit name since dbdeployer will also be used for MariaDB and PostgreSQL infras in the future.
Two fixes: 1. Enable CLIENT_SESSION_TRACKING capability via ProxySQL admin before the test runs, then reconnect so the new greeting takes effect. Without this flag, the client library cannot parse GTID session tracking data from OK packets. 2. Relax UUID regex to accept dbdeployer-style synthetic UUIDs (e.g. 00003306-1111-1111-1111-111111111111) in addition to RFC 4122 UUIDs.
The legacy group sets DEFAULT_MYSQL_INFRA but not INFRA_TYPE, so the infra .env was never sourced and MYSQL_PRIMARY_HOST remained unset. Now source .env for both INFRA_TYPE and DEFAULT_MYSQL_INFRA. Fixes test_cluster_sync_mysql_servers-t and test_read_only_actions_offline_hard_servers-t which connect directly to the MySQL backend using cl.mysql_host (from TAP_MYSQLHOST).
Delete query rules by rule_id IN (1,2,3) before inserting, not just by username='sbtest8'. When test_binlog_reader_uses_previous_hostgroup-t runs first, it inserts rule_id=1 with no username, which the old DELETE missed.
The test was calling test_binlog_reader-t as a subprocess, but that test only validates GTID consistency — it never sends COM_BINLOG_DUMP or COM_REGISTER_SLAVE, so the "uses previous hostgroup" assertion (ConnOk-ConnFree for HG 2) could never work. Rewritten to directly test what the test claims: 1. Route all traffic to HG 2 via query rule (apply=1) 2. Connect, send a COM_QUERY (routed to HG 2) 3. On the same connection, send COM_BINLOG_DUMP via mariadb_rpl_open (triggers fast_forward using HG 2 — the previous query's hostgroup) 4. Close connection, verify ConnOk-ConnFree for HG 2 increased Also: test_binlog_reader-t uses rule_ids 100-102 to avoid conflicts with rules set by other tests calling it as a subprocess.
Single-container MySQL 8.4.8 infra using dbdeployer. Same pattern as infra-dbdeployer-mysql57 but with MySQL 8.4 configuration: - innodb_redo_log_capacity instead of innodb_log_file_size - log-replica-updates instead of log-slave-updates - mysql_native_password=ON for compatibility - WHG=2900, RHG=2901
Single-container MySQL 8.4.8 infra with GTID binlog reader support. Same pattern as infra-dbdeployer-mysql57-binlog but with MySQL 8.4: - innodb_redo_log_capacity, log-replica-updates, mysql_native_password=ON - proxysql_binlog_reader v2.3 (supports MySQL 8.4) - PREFIX=14, WHG=1400, RHG=1401
- mysql84 group: infra-mysql84 -> infra-dbdeployer-mysql84 - mysql84-binlog group: infra-mysql84-binlog -> infra-dbdeployer-mysql84-binlog
MySQL 8.4 renamed Slave_IO_Running/Slave_SQL_Running to Replica_IO_Running/Replica_SQL_Running. The old column names caused the replication check to hang.
MySQL 8.4 removed SHOW MASTER STATUS, renamed to SHOW BINARY LOG STATUS. Try the new syntax first, fall back to legacy for MySQL 5.7.
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
test/infra/infra-dbdeployer-mysql84/docker-compose-destroy.bash-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorQuote
dirnameexpansion and$0inpushdto avoid split/glob edge cases.Line 4 should use quoted expansion for safe directory resolution:
-pushd $(dirname $0) &>/dev/null +pushd "$(dirname "$0")" &>/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mysql84/docker-compose-destroy.bash` at line 4, The pushd invocation uses unquoted command substitution which can break on filenames with spaces or glob characters; update the call using quoted expansion around the dirname expression and $0 so pushd receives a single safe path (i.e., call pushd "$(dirname "$0")" instead of pushd $(dirname $0)) to prevent split/glob edge cases in docker-compose-destroy.bash.test/infra/infra-dbdeployer-mysql57/docker-compose-destroy.bash-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorQuote
dirnameexpansion inpushdto avoid path-splitting bugs.Line 4 uses an unquoted command substitution that can break on paths containing spaces. The variables should be quoted to prevent word splitting.
Suggested patch
-pushd $(dirname $0) &>/dev/null +pushd "$(dirname "$0")" &>/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mysql57/docker-compose-destroy.bash` at line 4, The pushd call uses an unquoted command substitution (pushd $(dirname $0) &>/dev/null) which can break on paths with spaces; update the invocation so the dirname expansion and the $0 variable are quoted (i.e., quote both the command substitution result and the inner $0) to prevent word-splitting when calling pushd (look for the pushd and dirname usage in the script).test/infra/infra-dbdeployer-mysql57-binlog/docker-compose-destroy.bash-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorQuote
dirnameexpansion inpushdfor robust path handling.Line 4 uses unquoted command substitution and variable expansion, which can break on paths containing spaces or special characters.
Suggested patch
-pushd $(dirname $0) &>/dev/null +pushd "$(dirname "$0")" &>/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mysql57-binlog/docker-compose-destroy.bash` at line 4, The pushd invocation uses unquoted command substitution and can break on paths with spaces; update the line using pushd and dirname so that the dirname expansion and the $0 variable are quoted (i.e., quote the command substitution and inner $0) to ensure robust path handling and redirection to /dev/null remains intact.test/infra/infra-dbdeployer-mariadb10/docker-compose-destroy.bash-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorQuote
dirnameexpansion inpushdfor shell-safe behavior.Line 4 currently risks path splitting if
$0contains spaces or special characters; quoting removes that edge case.Suggested patch
-pushd $(dirname $0) &>/dev/null +pushd "$(dirname "$0")" &>/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mariadb10/docker-compose-destroy.bash` at line 4, The pushd invocation uses unquoted dirname expansion and can break on paths with spaces; update the pushd call that wraps dirname and the script parameter ($0) so the dirname expansion is quoted (i.e., quote both the dirname expression and the $0 argument) to ensure shell-safe behavior when calling pushd from docker-compose-destroy.bash.docs/superpowers/plans/2026-04-12-infra-dbdeployer-mysql57.md-302-302 (1)
302-302:⚠️ Potential issue | 🟡 MinorLabel the fenced
.envblock.This fence currently trips markdownlint MD040.
Proposed doc fix
-``` +```dotenv🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-12-infra-dbdeployer-mysql57.md` at line 302, The fenced code block for the .env snippet is unlabeled and triggers markdownlint MD040; update the fenced block by adding the language label "dotenv" (i.e., change the opening fence from ``` to ```dotenv) wherever the .env block is rendered in this document (the fenced `.env` block shown around the environment variables).test/infra/infra-dbdeployer-mysql84-binlog/docker-compose-destroy.bash-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorQuote the script directory before
pushd.Line 4 will split/glob if the script path contains spaces or wildcard characters.
Suggested fix
-pushd $(dirname $0) &>/dev/null +pushd "$(dirname "$0")" &>/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mysql84-binlog/docker-compose-destroy.bash` at line 4, The pushd invocation uses unquoted command substitution (pushd $(dirname $0) &>/dev/null) which will split or glob paths with spaces/wildcards; update the call in docker-compose-destroy.bash to quote the dirname expansion so pushd receives a single path (i.e., quote both the $(dirname ...) expansion and the $0 inside it) when invoking pushd and redirecting output, referencing the pushd and dirname usages shown in the diff.test/infra/infra-dbdeployer-mysql57-binlog/.env-29-32 (1)
29-32:⚠️ Potential issue | 🟡 MinorClean up
.envfile to avoid shell syntax that Docker Compose won't evaluate.Docker Compose does not support command substitution (
$(...)) in.envfiles — it will be treated as a literal string. While your wrapper script (docker-compose-init.bash) currently sources.envas a shell script and pre-computesROOT_PASSWORDbefore invoking Compose (avoiding the issue in practice), the file mixes Docker Compose syntax with shell-only constructs (exportkeyword, command substitution), creating unnecessary confusion. Move the computed assignments (ROOT_PASSWORD derivation) to the wrapper where they're already partially done (line 43), and keep.envto plainKEY=valuepairs with only${VAR}/${VAR:-default}interpolation for clarity and resilience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mysql57-binlog/.env` around lines 29 - 32, Remove shell-only constructs from the .env file: delete the command-substitution assignment for ROOT_PASSWORD and the exported TAP_REG_TEST_... hostgroup line (the `$(...)` and `export` usage), and instead leave only plain KEY=value pairs using simple ${VAR} or ${VAR:-default} expansions; move the ROOT_PASSWORD derivation into the wrapper script (the existing docker-compose-init.bash logic that pre-computes ROOT_PASSWORD) and have the wrapper write or export a plain ROOT_PASSWORD=value into the environment before invoking docker-compose so Compose sees a literal KEY=value entry.docs/superpowers/specs/2026-04-12-infra-dbdeployer-mysql57-design.md-34-43 (1)
34-43:⚠️ Potential issue | 🟡 MinorAdd language info strings to these fenced blocks.
These four blocks are still plain triple-backtick fences, which will keep tripping markdownlint MD040.
textis enough for the topology/tree/env examples.Also applies to: 47-57, 65-81, 135-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-04-12-infra-dbdeployer-mysql57-design.md` around lines 34 - 43, The fenced code blocks that show topology/topology examples (for example the block starting with "6 containers:" and the Hostnames lines) are missing language info strings and should be changed from plain triple-backticks to backticks with a language tag (use "text") so markdownlint MD040 is satisfied; update the same style of blocks noted in the comment (the blocks around lines 47-57, 65-81, and 135-141) to begin with ```text instead of ``` while leaving the block contents unchanged.docs/superpowers/specs/2026-04-12-infra-dbdeployer-mysql57-design.md-97-105 (1)
97-105:⚠️ Potential issue | 🟡 MinorUpdate the spec to match the mysql57 entrypoint that landed in this PR.
The example here still documents
--base-port=3306and does not mentionshow_compatibility_56=ON, but the current implementation intest/infra/infra-dbdeployer-mysql57/docker/entrypoint.sh:17-32uses--base-port=3305and enablesshow_compatibility_56=ON. As written, the design doc describes a different topology/config than the one reviewers will actually get.Also applies to: 213-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-04-12-infra-dbdeployer-mysql57-design.md` around lines 97 - 105, The spec example documents a different topology than the actual mysql57 entrypoint: update the replication deployment example to match the entrypoint behavior by changing the base port flag to --base-port=3305 and adding show_compatibility_56=ON to the my-cnf options (to match the logic in entrypoint.sh that enables compatibility mode); apply the same change to the other occurrence mentioned (lines 213-214 in the doc) so both examples reflect the real behavior.test/infra/infra-dbdeployer-mariadb10/bin/docker-proxy-post.bash-10-11 (1)
10-11:⚠️ Potential issue | 🟡 MinorRead
infra-config.sqlrelative toSCRIPT_DIR.Line 11 uses
./conf/proxysql/infra-config.sql, so this helper only works when the caller happens to be in the infra root. Line 5 already made.envlookup script-relative; the SQL file should be resolved the same way or manual/indirect invocations will fail before ProxySQL is configured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mariadb10/bin/docker-proxy-post.bash` around lines 10 - 11, The SQL file is read relative to the current working directory instead of the script location; update the command that injects infra-config.sql so it uses the script directory variable (SCRIPT_DIR) when resolving the path to conf/proxysql/infra-config.sql (the same approach used for the .env lookup), e.g. reference SCRIPT_DIR when reading infra-config.sql in the docker exec heredoc invocation so manual/indirect invocations work regardless of the caller's CWD.
🧹 Nitpick comments (1)
test/infra/infra-dbdeployer-mariadb10/docker-compose-init.bash (1)
1-153: Consider extracting the shared compose-init bootstrap.This is now effectively the same script as the mysql57-binlog/mysql84/mysql84-binlog variants. The
INFRA_IDbug had to be fixed in all of them, which is a good sign the common flow should live intest/infra/common/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mariadb10/docker-compose-init.bash` around lines 1 - 153, This script duplicates bootstrap logic across multiple infra variants and repeats the INFRA_ID extraction bugfix; extract the shared flow into a single sourced script (e.g., test/infra/common/compose-init.bash) and have docker-compose-init.bash (and mysql57-binlog/mysql84/mysql84-binlog variants) source it; move the INFRA_ID detection/export block, COMPOSE_CMD detection, WORKSPACE/REPO_ROOT/SCRIPT_DIR setup, MOUNTED_PATHS directory preparation loop (including the .crt/.key exclusion and postgres UID handling), ENV_FILE creation, compose up/verification and the dbdeployer wait loop into the common script, leaving only variant-specific bits (like post-scripts or COMPOSE_PROJECT naming) in the per-variant files; ensure sourcing preserves existing INFRA_ID from the caller by saving/restoring it the same way the current script does.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-04-12-infra-dbdeployer-mysql57.md`:
- Around line 302-313: The .env template omitted PREFIX so generated query-rule
IDs in conf/proxysql/infra-config.sql collapse; add a PREFIX variable and derive
WHG, RHG, BHG, OHG from it (e.g., set PREFIX and then WHG=${PREFIX}00,
RHG=${PREFIX}01, BHG=${PREFIX}02, OHG=${PREFIX}03) and keep the TAP export
referencing ${WHG} so queries remain infra-scoped and compatible with
infra-mysql57; update the .env block in the doc accordingly.
In `@test/infra/infra-dbdeployer-mariadb10/docker-compose-init.bash`:
- Around line 2-9: The INFRA_ID fallback derives from the parent directory
(basename $(dirname $(pwd))) which yields generic names like "infra" and causes
collisions; change the derivation in docker-compose-init.bash to use the current
working directory name (basename "$(pwd)"), strip the prefixes (s/infra-//;
s/docker-//), and if still empty or "." set INFRA_ID to a more unique value
(e.g., "dev-$USER-$(date +%s)" or include a short git commit/UUID) so the
INFRA_ID variable will be unique per run and avoid compose/network/name
collisions.
In `@test/infra/infra-dbdeployer-mysql57-binlog/conf/proxysql/infra-config.sql`:
- Around line 12-15: The DELETE that currently removes rows with comment='.' OR
comment='' OR comment IS NULL is too broad; restrict its scope to only the
replication mapping being recreated by matching the writer and reader hostgroups
(and/or the ${INFRA} comment) so it only affects rows for this infra. Update the
cleanup logic operating on mysql_replication_hostgroups to include conditions on
writer_hostgroup=${WHG} AND reader_hostgroup=${RHG} (and/or comment LIKE
'%${INFRA}%') so only the specific mapping replaced by the subsequent REPLACE
INTO is deleted, leaving unrelated hostgroup rows untouched.
In `@test/infra/infra-dbdeployer-mysql57-binlog/docker-compose-init.bash`:
- Around line 2-9: The current INFRA_ID derivation uses basename $(dirname
$(pwd)) which picks the parent directory (yields "infra") and causes collisions;
update the logic that sets INFRA_ID (the export INFRA_ID=... line) to derive
from the current working directory name instead (use basename "$(pwd)" or
equivalent) and still run the same sed cleanup (s/infra-//; s/docker-//) so
local runs produce a unique COMPOSE_PROJECT/network/seed; keep the final safety
fallback (if [ -z "${INFRA_ID}" ] || [ "${INFRA_ID}" = "." ] ...) unchanged.
In `@test/infra/infra-dbdeployer-mysql57-binlog/docker/Dockerfile`:
- Line 1: Replace the floating FROM image in the Dockerfile's binlog-reader
stage — currently using proxysql/ci-infra:proxysql-mysqlbinlog — with the
immutable pinned tag used by the MySQL 8.4 variant (e.g.
proxysql/ci-infra:proxysql-mysqlbinlog-v2.3) so the binlog-reader build is
deterministic; update the FROM line in the Dockerfile referencing the
binlog-reader stage accordingly.
In `@test/infra/infra-dbdeployer-mysql57-binlog/docker/entrypoint.sh`:
- Around line 159-175: The loop that checks GTID readers (using
READER_PORTS/READER_PORT, MAX_WAIT and COUNT) currently only warns on timeout
but still prints "OK" and allows the script to proceed to touch
/tmp/dbdeployer_ready; change it so that if any reader fails to bind within
MAX_WAIT the script records that failure (e.g., set a FAILED_READERS flag or
non-zero EXIT_CODE), avoid printing "OK" for a failed check, and after the loop
exit non-zero or skip touching /tmp/dbdeployer_ready unless all readers
succeeded; update the check logic around the while loop and the post-loop
readiness touch to gate readiness on all readers being OK.
In `@test/infra/infra-dbdeployer-mysql57/conf/proxysql/infra-config.sql`:
- Around line 31-38: The mysql_users INSERT list is missing sbtest5, sbtest6,
sbtest9 and sbtest10 which causes auth failures through ProxySQL; add INSERT OR
IGNORE rows into the mysql_users table for 'sbtest5','sbtest6','sbtest9' and
'sbtest10' using the same pattern as existing entries (username and password
equal to the sbtest name, active=1, default_hostgroup=${WHG},
comment='${INFRA}') so the frontend matches the backend provisioned accounts.
In `@test/infra/infra-dbdeployer-mysql57/docker-compose-init.bash`:
- Around line 3-5: The INFRA_ID is being derived from the parent directory
(basename $(dirname $(pwd))) which collapses different infra dirs; change the
derivation to use the current directory basename instead (basename $(pwd)) and
keep the same sed cleanup (s/infra-//; s/docker-//) so INFRA_ID reflects the
actual infra directory; update the export line that sets INFRA_ID accordingly
(referencing INFRA_ID in the script) to prevent cross-group collisions in
project/network/password names.
In `@test/infra/infra-dbdeployer-mysql84-binlog/docker-compose-init.bash`:
- Around line 2-9: The INFRA_ID derivation is using the parent directory
(dirname $(pwd)) which yields a generic "infra" fallback and can cause
collisions; change the assignment to derive from the current working directory
name (basename $(pwd)) and sanitize with the same sed expression, i.e. replace
the $(basename $(dirname $(pwd))) usage with $(basename $(pwd)) so INFRA_ID
comes from the repo/instance folder; also make the final safety default more
unique (e.g. include $USER plus a short timestamp or PID) when INFRA_ID is still
empty or "." to avoid shared compose projects.
In `@test/infra/infra-dbdeployer-mysql84-binlog/docker/entrypoint.sh`:
- Around line 160-179: The current startup loop checks READER_PORTS but only
warns on timeout and still creates the readiness marker (/tmp/dbdeployer_ready),
letting docker-compose-init.bash treat startup as successful; change the logic
in the loop that uses READER_PORTS/READER_PORT/MAX_WAIT/COUNT so that if any
reader port fails to respond within MAX_WAIT you exit with a non-zero status (or
set a failure flag and exit after the loop) and do not touch
/tmp/dbdeployer_ready; ensure the script prints a clear error for which
READER_PORT failed and terminates to prevent signaling readiness when a binlog
reader never came up.
In `@test/infra/infra-dbdeployer-mysql84/docker-compose-init.bash`:
- Around line 2-9: The derived INFRA_ID uses basename $(dirname $(pwd)) which
returns the parent directory ("infra") instead of the current directory, causing
non-unique IDs; change the derivation to use the current working directory name
(e.g. basename "$PWD" or basename "$(pwd)") and then apply the existing sed
stripping (s/infra-//; s/docker-//) so INFRA_ID becomes the current folder name
(like infra-dbdeployer-mysql84) cleaned of prefixes; update the INFRA_ID
assignment in the script (the block that sets export INFRA_ID=...) accordingly.
In `@test/tap/groups/check_groups.py`:
- Around line 88-95: The current check that computes in_groups_not_found =
sorted(groups_tests - executable_tests) and prints a NOTE should instead treat
unknown entries as a failing condition (non-zero exit) or explicitly validate
against a whitelist of feature-gated tests; update the block that handles
in_groups_not_found so that missing tests are reported as errors (e.g., print an
ERROR and exit non-zero) unless the test name is proven to be intentionally
feature-gated via an explicit allowlist you add/consult, and ensure you
reference the variables groups_tests, executable_tests and the
in_groups_not_found set when implementing the change.
In `@test/tap/tap/Makefile`:
- Around line 145-147: The rule using an order-only prerequisite prevents
LIBTAP_ARCHIVES from being marked out-of-date when the Makefile changes; update
the dependency so Makefile is a normal prerequisite for the archive targets
(i.e., change the target header for LIBTAP_ARCHIVES from using the order-only
pipe to a normal dependency) or add Makefile as a normal prerequisite in the
individual archive rules (refer to LIBTAP_ARCHIVES and the rule
"$(LIBTAP_ARCHIVES): | Makefile") so edits to the Makefile will correctly
rebuild libtap_mariadb.a, libtap_mysql57.a and libtap_mysql8.a.
In `@test/tap/tests/test_gtid_forwarding-t.cpp`:
- Around line 58-80: The admin precheck must connect to the admin listener and
abort on failure: change the mysql_real_connect call to use cl.admin_host (not
cl.host), and make the admin queries fail-fast by checking return values of
mysql_real_connect, mysql_query for the SELECT and the subsequent SET and "LOAD
MYSQL VARIABLES TO RUNTIME" calls; if any of those fail, log the error and
immediately fail the test (close mysql handle via mysql_close(admin) and
return/exit). Update checks around MYSQL_RES/mysql_fetch_row to treat missing
rows or parse failures as fatal for the precheck so setup errors aren't hidden.
---
Minor comments:
In `@docs/superpowers/plans/2026-04-12-infra-dbdeployer-mysql57.md`:
- Line 302: The fenced code block for the .env snippet is unlabeled and triggers
markdownlint MD040; update the fenced block by adding the language label
"dotenv" (i.e., change the opening fence from ``` to ```dotenv) wherever the
.env block is rendered in this document (the fenced `.env` block shown around
the environment variables).
In `@docs/superpowers/specs/2026-04-12-infra-dbdeployer-mysql57-design.md`:
- Around line 34-43: The fenced code blocks that show topology/topology examples
(for example the block starting with "6 containers:" and the Hostnames lines)
are missing language info strings and should be changed from plain
triple-backticks to backticks with a language tag (use "text") so markdownlint
MD040 is satisfied; update the same style of blocks noted in the comment (the
blocks around lines 47-57, 65-81, and 135-141) to begin with ```text instead of
``` while leaving the block contents unchanged.
- Around line 97-105: The spec example documents a different topology than the
actual mysql57 entrypoint: update the replication deployment example to match
the entrypoint behavior by changing the base port flag to --base-port=3305 and
adding show_compatibility_56=ON to the my-cnf options (to match the logic in
entrypoint.sh that enables compatibility mode); apply the same change to the
other occurrence mentioned (lines 213-214 in the doc) so both examples reflect
the real behavior.
In `@test/infra/infra-dbdeployer-mariadb10/bin/docker-proxy-post.bash`:
- Around line 10-11: The SQL file is read relative to the current working
directory instead of the script location; update the command that injects
infra-config.sql so it uses the script directory variable (SCRIPT_DIR) when
resolving the path to conf/proxysql/infra-config.sql (the same approach used for
the .env lookup), e.g. reference SCRIPT_DIR when reading infra-config.sql in the
docker exec heredoc invocation so manual/indirect invocations work regardless of
the caller's CWD.
In `@test/infra/infra-dbdeployer-mariadb10/docker-compose-destroy.bash`:
- Line 4: The pushd invocation uses unquoted dirname expansion and can break on
paths with spaces; update the pushd call that wraps dirname and the script
parameter ($0) so the dirname expansion is quoted (i.e., quote both the dirname
expression and the $0 argument) to ensure shell-safe behavior when calling pushd
from docker-compose-destroy.bash.
In `@test/infra/infra-dbdeployer-mysql57-binlog/.env`:
- Around line 29-32: Remove shell-only constructs from the .env file: delete the
command-substitution assignment for ROOT_PASSWORD and the exported
TAP_REG_TEST_... hostgroup line (the `$(...)` and `export` usage), and instead
leave only plain KEY=value pairs using simple ${VAR} or ${VAR:-default}
expansions; move the ROOT_PASSWORD derivation into the wrapper script (the
existing docker-compose-init.bash logic that pre-computes ROOT_PASSWORD) and
have the wrapper write or export a plain ROOT_PASSWORD=value into the
environment before invoking docker-compose so Compose sees a literal KEY=value
entry.
In `@test/infra/infra-dbdeployer-mysql57-binlog/docker-compose-destroy.bash`:
- Line 4: The pushd invocation uses unquoted command substitution and can break
on paths with spaces; update the line using pushd and dirname so that the
dirname expansion and the $0 variable are quoted (i.e., quote the command
substitution and inner $0) to ensure robust path handling and redirection to
/dev/null remains intact.
In `@test/infra/infra-dbdeployer-mysql57/docker-compose-destroy.bash`:
- Line 4: The pushd call uses an unquoted command substitution (pushd $(dirname
$0) &>/dev/null) which can break on paths with spaces; update the invocation so
the dirname expansion and the $0 variable are quoted (i.e., quote both the
command substitution result and the inner $0) to prevent word-splitting when
calling pushd (look for the pushd and dirname usage in the script).
In `@test/infra/infra-dbdeployer-mysql84-binlog/docker-compose-destroy.bash`:
- Line 4: The pushd invocation uses unquoted command substitution (pushd
$(dirname $0) &>/dev/null) which will split or glob paths with spaces/wildcards;
update the call in docker-compose-destroy.bash to quote the dirname expansion so
pushd receives a single path (i.e., quote both the $(dirname ...) expansion and
the $0 inside it) when invoking pushd and redirecting output, referencing the
pushd and dirname usages shown in the diff.
In `@test/infra/infra-dbdeployer-mysql84/docker-compose-destroy.bash`:
- Line 4: The pushd invocation uses unquoted command substitution which can
break on filenames with spaces or glob characters; update the call using quoted
expansion around the dirname expression and $0 so pushd receives a single safe
path (i.e., call pushd "$(dirname "$0")" instead of pushd $(dirname $0)) to
prevent split/glob edge cases in docker-compose-destroy.bash.
---
Nitpick comments:
In `@test/infra/infra-dbdeployer-mariadb10/docker-compose-init.bash`:
- Around line 1-153: This script duplicates bootstrap logic across multiple
infra variants and repeats the INFRA_ID extraction bugfix; extract the shared
flow into a single sourced script (e.g., test/infra/common/compose-init.bash)
and have docker-compose-init.bash (and mysql57-binlog/mysql84/mysql84-binlog
variants) source it; move the INFRA_ID detection/export block, COMPOSE_CMD
detection, WORKSPACE/REPO_ROOT/SCRIPT_DIR setup, MOUNTED_PATHS directory
preparation loop (including the .crt/.key exclusion and postgres UID handling),
ENV_FILE creation, compose up/verification and the dbdeployer wait loop into the
common script, leaving only variant-specific bits (like post-scripts or
COMPOSE_PROJECT naming) in the per-variant files; ensure sourcing preserves
existing INFRA_ID from the caller by saving/restoring it the same way the
current script does.
🪄 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: 3e8f5888-3beb-4fd9-ba2a-267d1b6b3ec1
📒 Files selected for processing (68)
docs/superpowers/plans/2026-04-12-infra-dbdeployer-mysql57.mddocs/superpowers/specs/2026-04-12-infra-dbdeployer-mysql57-design.mdtest/infra/common/env-isolated.shtest/infra/control/env-isolated.bashtest/infra/infra-dbdeployer-mariadb10/.envtest/infra/infra-dbdeployer-mariadb10/bin/docker-mysql-post.bashtest/infra/infra-dbdeployer-mariadb10/bin/docker-proxy-post.bashtest/infra/infra-dbdeployer-mariadb10/conf/proxysql/infra-config.sqltest/infra/infra-dbdeployer-mariadb10/docker-compose-destroy.bashtest/infra/infra-dbdeployer-mariadb10/docker-compose-init.bashtest/infra/infra-dbdeployer-mariadb10/docker-compose.ymltest/infra/infra-dbdeployer-mariadb10/docker/Dockerfiletest/infra/infra-dbdeployer-mariadb10/docker/build.shtest/infra/infra-dbdeployer-mariadb10/docker/entrypoint.shtest/infra/infra-dbdeployer-mysql57-binlog/.envtest/infra/infra-dbdeployer-mysql57-binlog/bin/docker-mysql-post.bashtest/infra/infra-dbdeployer-mysql57-binlog/bin/docker-proxy-post.bashtest/infra/infra-dbdeployer-mysql57-binlog/conf/proxysql/infra-config.sqltest/infra/infra-dbdeployer-mysql57-binlog/docker-compose-destroy.bashtest/infra/infra-dbdeployer-mysql57-binlog/docker-compose-init.bashtest/infra/infra-dbdeployer-mysql57-binlog/docker-compose.ymltest/infra/infra-dbdeployer-mysql57-binlog/docker/Dockerfiletest/infra/infra-dbdeployer-mysql57-binlog/docker/build.shtest/infra/infra-dbdeployer-mysql57-binlog/docker/entrypoint.shtest/infra/infra-dbdeployer-mysql57/.envtest/infra/infra-dbdeployer-mysql57/bin/docker-mysql-post.bashtest/infra/infra-dbdeployer-mysql57/bin/docker-proxy-post.bashtest/infra/infra-dbdeployer-mysql57/conf/proxysql/infra-config.sqltest/infra/infra-dbdeployer-mysql57/docker-compose-destroy.bashtest/infra/infra-dbdeployer-mysql57/docker-compose-init.bashtest/infra/infra-dbdeployer-mysql57/docker-compose.ymltest/infra/infra-dbdeployer-mysql57/docker/Dockerfiletest/infra/infra-dbdeployer-mysql57/docker/build.shtest/infra/infra-dbdeployer-mysql57/docker/entrypoint.shtest/infra/infra-dbdeployer-mysql84-binlog/.envtest/infra/infra-dbdeployer-mysql84-binlog/bin/docker-mysql-post.bashtest/infra/infra-dbdeployer-mysql84-binlog/bin/docker-proxy-post.bashtest/infra/infra-dbdeployer-mysql84-binlog/conf/proxysql/infra-config.sqltest/infra/infra-dbdeployer-mysql84-binlog/docker-compose-destroy.bashtest/infra/infra-dbdeployer-mysql84-binlog/docker-compose-init.bashtest/infra/infra-dbdeployer-mysql84-binlog/docker-compose.ymltest/infra/infra-dbdeployer-mysql84-binlog/docker/Dockerfiletest/infra/infra-dbdeployer-mysql84-binlog/docker/build.shtest/infra/infra-dbdeployer-mysql84-binlog/docker/entrypoint.shtest/infra/infra-dbdeployer-mysql84/.envtest/infra/infra-dbdeployer-mysql84/bin/docker-mysql-post.bashtest/infra/infra-dbdeployer-mysql84/bin/docker-proxy-post.bashtest/infra/infra-dbdeployer-mysql84/conf/proxysql/infra-config.sqltest/infra/infra-dbdeployer-mysql84/docker-compose-destroy.bashtest/infra/infra-dbdeployer-mysql84/docker-compose-init.bashtest/infra/infra-dbdeployer-mysql84/docker-compose.ymltest/infra/infra-dbdeployer-mysql84/docker/Dockerfiletest/infra/infra-dbdeployer-mysql84/docker/build.shtest/infra/infra-dbdeployer-mysql84/docker/entrypoint.shtest/tap/groups/check_groups.pytest/tap/groups/legacy-binlog/env.shtest/tap/groups/legacy-binlog/infras.lsttest/tap/groups/legacy/env.shtest/tap/groups/legacy/infras.lsttest/tap/groups/mysql84-binlog/env.shtest/tap/groups/mysql84-binlog/infras.lsttest/tap/groups/mysql84/env.shtest/tap/groups/mysql84/infras.lsttest/tap/tap/Makefiletest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_binlog_reader_uses_previous_hostgroup-t.cpptest/tap/tests/test_com_binlog_dump_enables_fast_forward-t.cpptest/tap/tests/test_gtid_forwarding-t.cpp
✅ Files skipped from review due to trivial changes (18)
- test/tap/groups/mysql84/infras.lst
- test/tap/groups/legacy/env.sh
- test/tap/groups/mysql84-binlog/infras.lst
- test/tap/groups/legacy-binlog/env.sh
- test/tap/groups/legacy/infras.lst
- test/infra/infra-dbdeployer-mysql57/docker/build.sh
- test/infra/infra-dbdeployer-mysql57-binlog/docker/build.sh
- test/infra/infra-dbdeployer-mariadb10/docker/build.sh
- test/infra/infra-dbdeployer-mysql84-binlog/docker/build.sh
- test/tap/groups/mysql84-binlog/env.sh
- test/infra/infra-dbdeployer-mysql84/docker/build.sh
- test/infra/infra-dbdeployer-mysql84-binlog/docker-compose.yml
- test/infra/infra-dbdeployer-mariadb10/docker-compose.yml
- test/tap/groups/legacy-binlog/infras.lst
- test/infra/infra-dbdeployer-mysql57-binlog/docker-compose.yml
- test/infra/infra-dbdeployer-mysql57/docker-compose.yml
- test/tap/groups/mysql84/env.sh
- test/infra/infra-dbdeployer-mysql84/docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- test/tap/tests/test_binlog_reader-t.cpp
👮 Files not reviewed due to content moderation or server errors (8)
- test/infra/control/env-isolated.bash
- test/tap/tests/test_binlog_reader_uses_previous_hostgroup-t.cpp
- test/infra/infra-dbdeployer-mysql84-binlog/bin/docker-proxy-post.bash
- test/infra/infra-dbdeployer-mysql84-binlog/.env
- test/infra/infra-dbdeployer-mysql57-binlog/bin/docker-proxy-post.bash
- test/infra/infra-dbdeployer-mysql84-binlog/conf/proxysql/infra-config.sql
- test/infra/infra-dbdeployer-mysql57-binlog/bin/docker-mysql-post.bash
- test/infra/infra-dbdeployer-mysql84-binlog/bin/docker-mysql-post.bash
Comment on lines
+302
to
+313
| ``` | ||
| MYSQL_VERSION=5.7 | ||
| WHG=1300 | ||
| RHG=1301 | ||
| BHG=1302 | ||
| OHG=1303 | ||
|
|
||
| # Export hostgroup for TAP tests that need it | ||
| export TAP_REG_TEST_3549_AUTOCOMMIT_TRACKING___MYSQL_SERVER_HOSTGROUP=${WHG} | ||
| ``` | ||
|
|
||
| This is identical to `infra-mysql57/.env` — same hostgroup IDs for drop-in compatibility. |
There was a problem hiding this comment.
Add PREFIX to the .env template.
Task 8 later expands ${PREFIX} in conf/proxysql/infra-config.sql. Following this plan verbatim leaves it unset, so the generated query-rule IDs collapse to 00/01/... instead of staying infra-scoped.
Proposed doc fix
-```
-MYSQL_VERSION=5.7
-WHG=1300
-RHG=1301
-BHG=1302
-OHG=1303
+```dotenv
+MYSQL_VERSION=5.7
+
+PREFIX=13
+WHG=${PREFIX}00
+RHG=${PREFIX}01
+BHG=${PREFIX}02
+OHG=${PREFIX}03
# Export hostgroup for TAP tests that need it
export TAP_REG_TEST_3549_AUTOCOMMIT_TRACKING___MYSQL_SERVER_HOSTGROUP=${WHG}</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 302-302: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-12-infra-dbdeployer-mysql57.md` around lines
302 - 313, The .env template omitted PREFIX so generated query-rule IDs in
conf/proxysql/infra-config.sql collapse; add a PREFIX variable and derive WHG,
RHG, BHG, OHG from it (e.g., set PREFIX and then WHG=${PREFIX}00,
RHG=${PREFIX}01, BHG=${PREFIX}02, OHG=${PREFIX}03) and keep the TAP export
referencing ${WHG} so queries remain infra-scoped and compatible with
infra-mysql57; update the .env block in the doc accordingly.
Comment on lines
+2
to
+9
| # RELIABLY CAPTURE INFRA_ID FROM ENVIRONMENT OR DIRECTORY NAME | ||
| if [ -z "${INFRA_ID}" ]; then | ||
| export INFRA_ID=$(basename $(dirname $(pwd)) | sed 's/infra-//; s/docker-//') | ||
| fi | ||
| # Final safety: if INFRA_ID is still empty or ".", use a default | ||
| if [ -z "${INFRA_ID}" ] || [ "${INFRA_ID}" = "." ]; then | ||
| export INFRA_ID="dev-$USER" | ||
| fi |
There was a problem hiding this comment.
Derive INFRA_ID from the right place.
This fallback currently resolves to infra instead of a unique run ID when INFRA_ID is unset, so local runs can collide on the same compose project, Docker network, and derived root password.
Proposed fix
if [ -z "${INFRA_ID}" ]; then
- export INFRA_ID=$(basename $(dirname $(pwd)) | sed 's/infra-//; s/docker-//')
-fi
-# Final safety: if INFRA_ID is still empty or ".", use a default
-if [ -z "${INFRA_ID}" ] || [ "${INFRA_ID}" = "." ]; then
- export INFRA_ID="dev-$USER"
+ export INFRA_ID="dev-${USER}"
fi🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 4-4: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 4-4: Quote this to prevent word splitting.
(SC2046)
[warning] 4-4: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/infra/infra-dbdeployer-mariadb10/docker-compose-init.bash` around lines
2 - 9, The INFRA_ID fallback derives from the parent directory (basename
$(dirname $(pwd))) which yields generic names like "infra" and causes
collisions; change the derivation in docker-compose-init.bash to use the current
working directory name (basename "$(pwd)"), strip the prefixes (s/infra-//;
s/docker-//), and if still empty or "." set INFRA_ID to a more unique value
(e.g., "dev-$USER-$(date +%s)" or include a short git commit/UUID) so the
INFRA_ID variable will be unique per run and avoid compose/network/name
collisions.
Comment on lines
+12
to
+15
| DELETE FROM mysql_replication_hostgroups WHERE comment LIKE '%${INFRA}'; | ||
| DELETE FROM mysql_replication_hostgroups WHERE writer_hostgroup=${WHG} AND reader_hostgroup=${RHG}; | ||
| DELETE FROM mysql_replication_hostgroups WHERE comment='.' OR comment='' OR comment IS NULL; | ||
| REPLACE INTO mysql_replication_hostgroups (writer_hostgroup,reader_hostgroup,comment) VALUES (${WHG},${RHG},'${INFRA}'); |
There was a problem hiding this comment.
Scope the replication-hostgroup cleanup to this infra only.
Line 14 deletes every blank/NULL-comment row in mysql_replication_hostgroups, not just the ${WHG}/${RHG} mapping being recreated here. In a reused ProxySQL instance that can silently wipe unrelated replication config and reintroduce the kind of multi-group flakiness this PR is trying to remove.
Suggested fix
DELETE FROM mysql_replication_hostgroups WHERE comment LIKE '%${INFRA}';
DELETE FROM mysql_replication_hostgroups WHERE writer_hostgroup=${WHG} AND reader_hostgroup=${RHG};
-DELETE FROM mysql_replication_hostgroups WHERE comment='.' OR comment='' OR comment IS NULL;
REPLACE INTO mysql_replication_hostgroups (writer_hostgroup,reader_hostgroup,comment) VALUES (${WHG},${RHG},'${INFRA}');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| DELETE FROM mysql_replication_hostgroups WHERE comment LIKE '%${INFRA}'; | |
| DELETE FROM mysql_replication_hostgroups WHERE writer_hostgroup=${WHG} AND reader_hostgroup=${RHG}; | |
| DELETE FROM mysql_replication_hostgroups WHERE comment='.' OR comment='' OR comment IS NULL; | |
| REPLACE INTO mysql_replication_hostgroups (writer_hostgroup,reader_hostgroup,comment) VALUES (${WHG},${RHG},'${INFRA}'); | |
| DELETE FROM mysql_replication_hostgroups WHERE comment LIKE '%${INFRA}'; | |
| DELETE FROM mysql_replication_hostgroups WHERE writer_hostgroup=${WHG} AND reader_hostgroup=${RHG}; | |
| REPLACE INTO mysql_replication_hostgroups (writer_hostgroup,reader_hostgroup,comment) VALUES (${WHG},${RHG},'${INFRA}'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/infra/infra-dbdeployer-mysql57-binlog/conf/proxysql/infra-config.sql`
around lines 12 - 15, The DELETE that currently removes rows with comment='.' OR
comment='' OR comment IS NULL is too broad; restrict its scope to only the
replication mapping being recreated by matching the writer and reader hostgroups
(and/or the ${INFRA} comment) so it only affects rows for this infra. Update the
cleanup logic operating on mysql_replication_hostgroups to include conditions on
writer_hostgroup=${WHG} AND reader_hostgroup=${RHG} (and/or comment LIKE
'%${INFRA}%') so only the specific mapping replaced by the subsequent REPLACE
INTO is deleted, leaving unrelated hostgroup rows untouched.
Comment on lines
+2
to
+9
| # RELIABLY CAPTURE INFRA_ID FROM ENVIRONMENT OR DIRECTORY NAME | ||
| if [ -z "${INFRA_ID}" ]; then | ||
| export INFRA_ID=$(basename $(dirname $(pwd)) | sed 's/infra-//; s/docker-//') | ||
| fi | ||
| # Final safety: if INFRA_ID is still empty or ".", use a default | ||
| if [ -z "${INFRA_ID}" ] || [ "${INFRA_ID}" = "." ]; then | ||
| export INFRA_ID="dev-$USER" | ||
| fi |
There was a problem hiding this comment.
Derive INFRA_ID from the right place.
basename $(dirname $(pwd)) resolves to infra when this script is run from the infra directory, so local runs without an explicit INFRA_ID all reuse the same namespace (COMPOSE_PROJECT, network, logs, and password seed).
Proposed fix
if [ -z "${INFRA_ID}" ]; then
- export INFRA_ID=$(basename $(dirname $(pwd)) | sed 's/infra-//; s/docker-//')
-fi
-# Final safety: if INFRA_ID is still empty or ".", use a default
-if [ -z "${INFRA_ID}" ] || [ "${INFRA_ID}" = "." ]; then
- export INFRA_ID="dev-$USER"
+ export INFRA_ID="dev-${USER}"
fi🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 4-4: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 4-4: Quote this to prevent word splitting.
(SC2046)
[warning] 4-4: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/infra/infra-dbdeployer-mysql57-binlog/docker-compose-init.bash` around
lines 2 - 9, The current INFRA_ID derivation uses basename $(dirname $(pwd))
which picks the parent directory (yields "infra") and causes collisions; update
the logic that sets INFRA_ID (the export INFRA_ID=... line) to derive from the
current working directory name instead (use basename "$(pwd)" or equivalent) and
still run the same sed cleanup (s/infra-//; s/docker-//) so local runs produce a
unique COMPOSE_PROJECT/network/seed; keep the final safety fallback (if [ -z
"${INFRA_ID}" ] || [ "${INFRA_ID}" = "." ] ...) unchanged.
| @@ -0,0 +1,46 @@ | |||
| FROM proxysql/ci-infra:proxysql-mysqlbinlog AS binlog-reader | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/infra/infra-dbdeployer-mysql57-binlog/docker/DockerfileRepository: sysown/proxysql
Length of output: 2067
🏁 Script executed:
find test/infra -name "Dockerfile" -type f | grep -i "binlog\|mysql"Repository: sysown/proxysql
Length of output: 288
🏁 Script executed:
find test/infra -name "Dockerfile" -type f | head -20 | xargs -I {} sh -c 'echo "=== {} ===" && head -5 {}'Repository: sysown/proxysql
Length of output: 1312
🏁 Script executed:
cat -n test/infra/infra-dbdeployer-mysql84-binlog/docker/Dockerfile | head -30Repository: sysown/proxysql
Length of output: 1399
🏁 Script executed:
rg "FROM.*proxysql" test/infra --type dockerfile -A 1 -B 1Repository: sysown/proxysql
Length of output: 93
🏁 Script executed:
find test/infra -name "*.sh" -o -name "*.md" | xargs rg -l "proxysql-mysqlbinlog\|binlog.*version" 2>/dev/nullRepository: sysown/proxysql
Length of output: 43
Pin the binlog-reader stage to an immutable image reference.
Using proxysql/ci-infra:proxysql-mysqlbinlog uses a floating tag, making rebuilds non-deterministic. A later push to that tag could change the reader binary without any repo change. The MySQL 8.4 variant pins this to proxysql-mysqlbinlog-v2.3; the MySQL 5.7 variant should be pinned consistently.
🧰 Tools
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/infra/infra-dbdeployer-mysql57-binlog/docker/Dockerfile` at line 1,
Replace the floating FROM image in the Dockerfile's binlog-reader stage —
currently using proxysql/ci-infra:proxysql-mysqlbinlog — with the immutable
pinned tag used by the MySQL 8.4 variant (e.g.
proxysql/ci-infra:proxysql-mysqlbinlog-v2.3) so the binlog-reader build is
deterministic; update the FROM line in the Dockerfile referencing the
binlog-reader stage accordingly.
Comment on lines
+160
to
+179
| # Wait briefly for readers to start, then verify they're listening | ||
| sleep 2 | ||
| for i in 0 1 2; do | ||
| READER_PORT="${READER_PORTS[$i]}" | ||
| echo -n " Checking reader on port ${READER_PORT}..." | ||
| MAX_WAIT=10 | ||
| COUNT=0 | ||
| while ! bash -c "echo > /dev/tcp/127.0.0.1/${READER_PORT}" 2>/dev/null; do | ||
| if [ $COUNT -ge $MAX_WAIT ]; then | ||
| echo " WARNING: reader on port ${READER_PORT} not responding (continuing anyway)" | ||
| break | ||
| fi | ||
| sleep 1 | ||
| COUNT=$((COUNT + 1)) | ||
| done | ||
| echo " OK" | ||
| done | ||
|
|
||
| # Signal readiness via marker file (used by docker-compose-init.bash) | ||
| touch /tmp/dbdeployer_ready |
There was a problem hiding this comment.
Fail startup when a binlog reader never comes up.
On timeout this loop only warns, then the script still touches /tmp/dbdeployer_ready. docker-compose-init.bash treats that marker as success, so tests can start with one or more dead GTID reader ports.
Proposed fix
# Wait briefly for readers to start, then verify they're listening
sleep 2
+reader_failed=0
for i in 0 1 2; do
READER_PORT="${READER_PORTS[$i]}"
echo -n " Checking reader on port ${READER_PORT}..."
MAX_WAIT=10
COUNT=0
+ reader_ok=1
while ! bash -c "echo > /dev/tcp/127.0.0.1/${READER_PORT}" 2>/dev/null; do
if [ $COUNT -ge $MAX_WAIT ]; then
- echo " WARNING: reader on port ${READER_PORT} not responding (continuing anyway)"
+ echo " FAILED"
+ reader_ok=0
+ reader_failed=1
break
fi
sleep 1
COUNT=$((COUNT + 1))
done
- echo " OK"
+ if [ "${reader_ok}" -eq 1 ]; then
+ echo " OK"
+ fi
done
+[ "${reader_failed}" -eq 0 ] || exit 1
+
# Signal readiness via marker file (used by docker-compose-init.bash)
touch /tmp/dbdeployer_ready📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Wait briefly for readers to start, then verify they're listening | |
| sleep 2 | |
| for i in 0 1 2; do | |
| READER_PORT="${READER_PORTS[$i]}" | |
| echo -n " Checking reader on port ${READER_PORT}..." | |
| MAX_WAIT=10 | |
| COUNT=0 | |
| while ! bash -c "echo > /dev/tcp/127.0.0.1/${READER_PORT}" 2>/dev/null; do | |
| if [ $COUNT -ge $MAX_WAIT ]; then | |
| echo " WARNING: reader on port ${READER_PORT} not responding (continuing anyway)" | |
| break | |
| fi | |
| sleep 1 | |
| COUNT=$((COUNT + 1)) | |
| done | |
| echo " OK" | |
| done | |
| # Signal readiness via marker file (used by docker-compose-init.bash) | |
| touch /tmp/dbdeployer_ready | |
| # Wait briefly for readers to start, then verify they're listening | |
| sleep 2 | |
| reader_failed=0 | |
| for i in 0 1 2; do | |
| READER_PORT="${READER_PORTS[$i]}" | |
| echo -n " Checking reader on port ${READER_PORT}..." | |
| MAX_WAIT=10 | |
| COUNT=0 | |
| reader_ok=1 | |
| while ! bash -c "echo > /dev/tcp/127.0.0.1/${READER_PORT}" 2>/dev/null; do | |
| if [ $COUNT -ge $MAX_WAIT ]; then | |
| echo " FAILED" | |
| reader_ok=0 | |
| reader_failed=1 | |
| break | |
| fi | |
| sleep 1 | |
| COUNT=$((COUNT + 1)) | |
| done | |
| if [ "${reader_ok}" -eq 1 ]; then | |
| echo " OK" | |
| fi | |
| done | |
| [ "${reader_failed}" -eq 0 ] || exit 1 | |
| # Signal readiness via marker file (used by docker-compose-init.bash) | |
| touch /tmp/dbdeployer_ready |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/infra/infra-dbdeployer-mysql84-binlog/docker/entrypoint.sh` around lines
160 - 179, The current startup loop checks READER_PORTS but only warns on
timeout and still creates the readiness marker (/tmp/dbdeployer_ready), letting
docker-compose-init.bash treat startup as successful; change the logic in the
loop that uses READER_PORTS/READER_PORT/MAX_WAIT/COUNT so that if any reader
port fails to respond within MAX_WAIT you exit with a non-zero status (or set a
failure flag and exit after the loop) and do not touch /tmp/dbdeployer_ready;
ensure the script prints a clear error for which READER_PORT failed and
terminates to prevent signaling readiness when a binlog reader never came up.
Comment on lines
+2
to
+9
| # RELIABLY CAPTURE INFRA_ID FROM ENVIRONMENT OR DIRECTORY NAME | ||
| if [ -z "${INFRA_ID}" ]; then | ||
| export INFRA_ID=$(basename $(dirname $(pwd)) | sed 's/infra-//; s/docker-//') | ||
| fi | ||
| # Final safety: if INFRA_ID is still empty or ".", use a default | ||
| if [ -z "${INFRA_ID}" ] || [ "${INFRA_ID}" = "." ]; then | ||
| export INFRA_ID="dev-$USER" | ||
| fi |
There was a problem hiding this comment.
Derive INFRA_ID from the right place.
With the current expression, running this from test/infra/infra-dbdeployer-mysql84 without INFRA_ID set produces infra, not a unique run ID. That makes compose names, networks, and password seeds collide across local runs.
Proposed fix
if [ -z "${INFRA_ID}" ]; then
- export INFRA_ID=$(basename $(dirname $(pwd)) | sed 's/infra-//; s/docker-//')
-fi
-# Final safety: if INFRA_ID is still empty or ".", use a default
-if [ -z "${INFRA_ID}" ] || [ "${INFRA_ID}" = "." ]; then
- export INFRA_ID="dev-$USER"
+ export INFRA_ID="dev-${USER}"
fi🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 4-4: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 4-4: Quote this to prevent word splitting.
(SC2046)
[warning] 4-4: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/infra/infra-dbdeployer-mysql84/docker-compose-init.bash` around lines 2
- 9, The derived INFRA_ID uses basename $(dirname $(pwd)) which returns the
parent directory ("infra") instead of the current directory, causing non-unique
IDs; change the derivation to use the current working directory name (e.g.
basename "$PWD" or basename "$(pwd)") and then apply the existing sed stripping
(s/infra-//; s/docker-//) so INFRA_ID becomes the current folder name (like
infra-dbdeployer-mysql84) cleaned of prefixes; update the INFRA_ID assignment in
the script (the block that sets export INFRA_ID=...) accordingly.
Comment on lines
+88
to
95
| # --- Check 1: tests in groups.json but not found on disk (WARNING) --- | ||
| in_groups_not_found = sorted(groups_tests - executable_tests) | ||
| if in_groups_not_found: | ||
| print(f"NOTE: {len(in_groups_not_found)} test(s) in groups.json are not yet available on disk " | ||
| f"(not built, or feature-gated):") | ||
| for t in in_groups_not_found: | ||
| print(f" - {t}") | ||
| print() |
There was a problem hiding this comment.
Don't demote unknown groups.json entries to a passing note.
This makes typos and stale test names non-fatal. A misspelled key in groups.json will now exit 0 whenever the executable is absent, so this check no longer protects the mapping itself. If feature-gated tests need special handling, they should be identified more precisely than treating every missing-on-disk entry as informational.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/groups/check_groups.py` around lines 88 - 95, The current check that
computes in_groups_not_found = sorted(groups_tests - executable_tests) and
prints a NOTE should instead treat unknown entries as a failing condition
(non-zero exit) or explicitly validate against a whitelist of feature-gated
tests; update the block that handles in_groups_not_found so that missing tests
are reported as errors (e.g., print an ERROR and exit non-zero) unless the test
name is proven to be intentionally feature-gated via an explicit allowlist you
add/consult, and ensure you reference the variables groups_tests,
executable_tests and the in_groups_not_found set when implementing the change.
Comment on lines
+145
to
+147
| # Rebuild archives when the Makefile itself changes (order-only). | ||
| LIBTAP_ARCHIVES := libtap_mariadb.a libtap_mysql57.a libtap_mysql8.a | ||
|
|
||
| $(LIBTAP_ARCHIVES): preclean-libtap-archives | Makefile | ||
|
|
||
| .PHONY: preclean-libtap-archives | ||
| preclean-libtap-archives: | ||
| rm -f $(LIBTAP_ARCHIVES) | ||
| $(LIBTAP_ARCHIVES): | Makefile |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In GNU Make, does an order-only prerequisite like target: | Makefilecausetargetto rebuild whenMakefile has a newer timestamp?
💡 Result:
No, an order-only prerequisite like target: | Makefile does not cause target to rebuild when Makefile has a newer timestamp. Order-only prerequisites ensure the prerequisite is built before the target if needed, but their timestamps are never checked when determining if the target is out of date. The official GNU Make documentation explicitly states: "Order-only prerequisites are never checked when determining if the target is out of date; even order-only prerequisites marked as phony will not cause the target to be rebuilt."
Citations:
- 1: https://www.gnu.org/s/make/manual/html_node/Prerequisite-Types.html
- 2: https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html
Order-only prerequisite won't rebuild archives after Makefile edits.
Line 147 uses an order-only prerequisite (| Makefile), which enforces build order but does not mark libtap_*.a out-of-date when Makefile changes. This contradicts the intent stated in Line 145 and can leave stale archives in incremental builds.
Proposed fix
-# Rebuild archives when the Makefile itself changes (order-only).
+# Rebuild archives when the Makefile itself changes.
LIBTAP_ARCHIVES := libtap_mariadb.a libtap_mysql57.a libtap_mysql8.a
-$(LIBTAP_ARCHIVES): | Makefile
+$(LIBTAP_ARCHIVES): Makefile📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Rebuild archives when the Makefile itself changes (order-only). | |
| LIBTAP_ARCHIVES := libtap_mariadb.a libtap_mysql57.a libtap_mysql8.a | |
| $(LIBTAP_ARCHIVES): preclean-libtap-archives | Makefile | |
| .PHONY: preclean-libtap-archives | |
| preclean-libtap-archives: | |
| rm -f $(LIBTAP_ARCHIVES) | |
| $(LIBTAP_ARCHIVES): | Makefile | |
| # Rebuild archives when the Makefile itself changes. | |
| LIBTAP_ARCHIVES := libtap_mariadb.a libtap_mysql57.a libtap_mysql8.a | |
| $(LIBTAP_ARCHIVES): Makefile |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tap/Makefile` around lines 145 - 147, The rule using an order-only
prerequisite prevents LIBTAP_ARCHIVES from being marked out-of-date when the
Makefile changes; update the dependency so Makefile is a normal prerequisite for
the archive targets (i.e., change the target header for LIBTAP_ARCHIVES from
using the order-only pipe to a normal dependency) or add Makefile as a normal
prerequisite in the individual archive rules (refer to LIBTAP_ARCHIVES and the
rule "$(LIBTAP_ARCHIVES): | Makefile") so edits to the Makefile will correctly
rebuild libtap_mariadb.a, libtap_mysql57.a and libtap_mysql8.a.
Comment on lines
+58
to
+80
| MYSQL* admin = mysql_init(NULL); | ||
| if (admin && mysql_real_connect(admin, cl.host, cl.admin_username, cl.admin_password, NULL, cl.admin_port, NULL, 0)) { | ||
| // Read current server_capabilities and enable CLIENT_SESSION_TRACKING if not set | ||
| if (mysql_query(admin, "SELECT variable_value FROM global_variables WHERE variable_name='mysql-server_capabilities'") == 0) { | ||
| MYSQL_RES* res = mysql_store_result(admin); | ||
| if (res) { | ||
| MYSQL_ROW row = mysql_fetch_row(res); | ||
| if (row && row[0]) { | ||
| uint32_t caps = (uint32_t)strtoul(row[0], NULL, 10); | ||
| if (!(caps & CLIENT_SESSION_TRACKING)) { | ||
| caps |= CLIENT_SESSION_TRACKING; | ||
| std::string q = "SET mysql-server_capabilities=" + std::to_string(caps); | ||
| diag("Enabling CLIENT_SESSION_TRACKING: %s", q.c_str()); | ||
| mysql_query(admin, q.c_str()); | ||
| mysql_query(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); | ||
| } else { | ||
| diag("CLIENT_SESSION_TRACKING already enabled in server_capabilities"); | ||
| } | ||
| } | ||
| mysql_free_result(res); | ||
| } | ||
| } | ||
| mysql_close(admin); |
There was a problem hiding this comment.
Use the admin endpoint and fail fast if the precheck cannot apply.
This branch connects to cl.host instead of cl.admin_host, and every admin query here is best-effort. If the admin listener is on a different host, or the SET / LOAD fails, the test just reconnects and later reports missing GTIDs, which hides the real setup failure.
Suggested fix
- MYSQL* admin = mysql_init(NULL);
- if (admin && mysql_real_connect(admin, cl.host, cl.admin_username, cl.admin_password, NULL, cl.admin_port, NULL, 0)) {
+ MYSQL* admin = mysql_init(NULL);
+ if (!admin) {
+ diag("mysql_init() failed for admin connection");
+ return -1;
+ }
+ if (mysql_real_connect(admin, cl.admin_host, cl.admin_username, cl.admin_password, NULL, cl.admin_port, NULL, 0)) {
// Read current server_capabilities and enable CLIENT_SESSION_TRACKING if not set
if (mysql_query(admin, "SELECT variable_value FROM global_variables WHERE variable_name='mysql-server_capabilities'") == 0) {
MYSQL_RES* res = mysql_store_result(admin);
@@
- mysql_query(admin, q.c_str());
- mysql_query(admin, "LOAD MYSQL VARIABLES TO RUNTIME");
+ if (mysql_query(admin, q.c_str()) != 0 ||
+ mysql_query(admin, "LOAD MYSQL VARIABLES TO RUNTIME") != 0) {
+ diag("Failed to enable CLIENT_SESSION_TRACKING: %s", mysql_error(admin));
+ mysql_close(admin);
+ return -1;
+ }
} else {
diag("CLIENT_SESSION_TRACKING already enabled in server_capabilities");
}
@@
- mysql_close(admin);
- }
+ mysql_close(admin);
+ } else {
+ diag("Admin connect failed: %s", mysql_error(admin));
+ mysql_close(admin);
+ return -1;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/test_gtid_forwarding-t.cpp` around lines 58 - 80, The admin
precheck must connect to the admin listener and abort on failure: change the
mysql_real_connect call to use cl.admin_host (not cl.host), and make the admin
queries fail-fast by checking return values of mysql_real_connect, mysql_query
for the SELECT and the subsequent SET and "LOAD MYSQL VARIABLES TO RUNTIME"
calls; if any of those fail, log the error and immediately fail the test (close
mysql handle via mysql_close(admin) and return/exit). Update checks around
MYSQL_RES/mysql_fetch_row to treat missing rows or parse failures as fatal for
the precheck so setup errors aren't hidden.
- Replace the external binlog reader with a helper function which
uses mariadb replication functions.
- The helper opens replication sessions, sends `COM_REGISTER_SLAVE`,
and fetches heartbeats to verify the stream is active.
- It runs the same two-session flow required for TAP tests:
- `test_com_register_slave_enables_fast_forward-t`
- `test_binlog_reader_uses_previous_hostgroup-t`
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
With many concurrent test groups running against dbdeployer containers, MySQL's default max_connect_errors=100 gets triggered, blocking the ProxySQL host IP. This caused cascading failures (42+ tests in g1 groups) with "Host is blocked because of many connection errors". Set max_connect_errors=100000 in all 5 dbdeployer infras.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
test/infra/infra-dbdeployer-mysql57-binlog/docker/entrypoint.sh (1)
162-179:⚠️ Potential issue | 🟠 MajorGate infra readiness on binlog-reader startup success.
Line 169 only warns on timeout, Line 175 still prints
OK, and Line 179 always marks infra ready. This can mask a broken reader startup and push failures downstream into TAP.🔧 Proposed fix
# Wait briefly for readers to start, then verify they're listening sleep 2 +READERS_OK=1 for i in 0 1 2; do READER_PORT="${READER_PORTS[$i]}" echo -n " Checking reader on port ${READER_PORT}..." MAX_WAIT=10 COUNT=0 while ! bash -c "echo > /dev/tcp/127.0.0.1/${READER_PORT}" 2>/dev/null; do if [ $COUNT -ge $MAX_WAIT ]; then - echo " WARNING: reader on port ${READER_PORT} not responding (continuing anyway)" + echo " TIMEOUT" + READERS_OK=0 break fi sleep 1 COUNT=$((COUNT + 1)) done - echo " OK" + if [ $COUNT -lt $MAX_WAIT ]; then + echo " OK" + fi done # Signal readiness via marker file (used by docker-compose-init.bash) -touch /tmp/dbdeployer_ready +if [ $READERS_OK -ne 1 ]; then + echo "ERROR: one or more binlog readers failed to start" + exit 1 +fi +touch /tmp/dbdeployer_ready🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mysql57-binlog/docker/entrypoint.sh` around lines 162 - 179, The loop that checks readers (using READER_PORTS, READER_PORT, MAX_WAIT, COUNT) currently only warns on timeout, still echoes "OK" and always creates the readiness marker (touch /tmp/dbdeployer_ready), which hides failures; change the logic so that for each port you detect a real success before printing "OK" (and do not print "OK" if the while loop timed out), track an overall failure flag when any reader times out, and if any reader failed exit non-zero (or skip creating the readiness marker) instead of always touching /tmp/dbdeployer_ready so downstream steps only proceed when all binlog-readers actually started.test/infra/infra-dbdeployer-mysql84-binlog/docker/entrypoint.sh (1)
163-180:⚠️ Potential issue | 🟠 MajorDon't mark the sandbox ready when a reader never binds.
Lines 169-171 only break the inner loop. Line 176 still prints
OK, and Line 180 always touches/tmp/dbdeployer_ready, sodocker-compose-init.bashcan start tests against a half-broken binlog setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mysql84-binlog/docker/entrypoint.sh` around lines 163 - 180, The script currently prints "OK" and touches /tmp/dbdeployer_ready even if a reader never bound; modify the reader-check loop (the for loop iterating READER_PORTS and the while that tests /dev/tcp/127.0.0.1/${READER_PORT}) to record a failure (e.g., set a flag like READER_FAILED=1) when the MAX_WAIT is exceeded instead of silently continuing, ensure you only print "OK" when the port actually responded, and only create the readiness marker (/tmp/dbdeployer_ready) if no readers failed (otherwise exit non-zero or skip touching the file) so docker-compose-init.bash won't start against a half-broken binlog setup.
🧹 Nitpick comments (2)
test/infra/infra-dbdeployer-mysql84/docker/entrypoint.sh (2)
10-10: Replacels | headwithfindfor more robust directory selection.Line 10 and line 41 both pipe
lsoutput tohead -1to select the first entry. This approach is non-deterministic when multiple entries exist—file ordering depends on locale and filesystem behavior. Usefindinstead for portable, consistent selection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mysql84/docker/entrypoint.sh` at line 10, The current assignment to MYSQL_VERSION uses `ls | head -1`, which is non-deterministic; replace both occurrences (the variable MYSQL_VERSION assignments around lines 10 and 41) with a robust find-based selection: use `find /root/opt/mysql -maxdepth 1 -mindepth 1 -type d` (or `-type f` if you expect files) piped to a deterministic ordering (e.g., `sort`) and then pick the first entry and extract its basename so MYSQL_VERSION gets a consistent, portable value; update both places where `ls … | head -1` is used.
51-63: Use argv array formysqlcommand instead of command string expansion.Lines 62 and 83 execute
${MYSQL_CMD}from a string, which relies on shell word-splitting. Use an array command to make invocation robust and shellcheck-clean.Suggested refactor
-MYSQL_CMD="mysql -h127.0.0.1 -uroot -p${DBDEPLOYER_ROOT_PASS}" +MYSQL_CMD=(mysql -h127.0.0.1 -uroot "-p${DBDEPLOYER_ROOT_PASS}") @@ - while ! ${MYSQL_CMD} -P${PORT} -e "SELECT 1" >/dev/null 2>&1; do + while ! "${MYSQL_CMD[@]}" -P"${PORT}" -e "SELECT 1" >/dev/null 2>&1; do @@ - ${MYSQL_CMD} -P${PORT} <<SQL + "${MYSQL_CMD[@]}" -P"${PORT}" <<SQL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-dbdeployer-mysql84/docker/entrypoint.sh` around lines 51 - 63, The script builds MYSQL_CMD as a plain string and later invokes it via ${MYSQL_CMD}, which is fragile; change MYSQL_CMD to an array (e.g., MYSQL_CMD=(mysql -h127.0.0.1 -uroot -p"${DBDEPLOYER_ROOT_PASS}")) and invoke it with the array expansion "${MYSQL_CMD[@]}" when adding flags like -P"${PORT}" and -e "SELECT 1"; update every invocation of ${MYSQL_CMD} (the wait loop and the other call around line 83) to use the array form to avoid word-splitting and satisfy shellcheck while keeping NODE_PORTS/NODE_NAMES/COUNT/MAX_WAIT logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/infra/infra-dbdeployer-mariadb10/docker/entrypoint.sh`:
- Around line 68-70: The script injects ROOT_PASSWORD and INFRA directly into
SQL heredocs (variables ROOT_PASSWORD and INFRA), risking syntax breaks or
injection if values contain single quotes; create a small shell helper (e.g.,
escape_sql_literal) that replaces each single quote in a value with '\''
(SQL-safe single-quote escaping) and use that escaped value when interpolating
into all SQL heredocs (the blocks that use ROOT_PASSWORD and INFRA around lines
68 and 75-105) so the injected literals are always properly quoted and safe.
- Around line 10-13: The script uses `ls | head` to set MYSQL_VERSION which is
brittle under set -e; replace the ls pipeline with safe globbing or an explicit
directory scan (e.g., iterate /root/opt/mysql/* and pick the first entry) and
ensure the command substitution cannot fail (avoid pipelines that return
non-zero). Apply the same change to the other block noted (the section that
currently uses ls at lines ~33-37) so both places use a guarded check for an
empty directory before extracting the filename into MYSQL_VERSION (and any
related variables like the tarball name), and keep the existing explicit if [ -z
... ] checks to emit the error and exit when no files are present.
In `@test/infra/infra-dbdeployer-mysql57/docker/entrypoint.sh`:
- Around line 137-138: Delay creation of the readiness marker
(/tmp/dbdeployer_ready) until replication threads are confirmed running: update
entrypoint.sh to query the MySQL instance (e.g., using mysql -e "SHOW
SLAVE/REPLICA STATUS") in a short retry loop and only create the marker after
verifying Slave_IO_Running and Slave_SQL_Running are both "Yes" (or if the
instance is primary/no replica status exists, proceed immediately); replace the
unconditional touch /tmp/dbdeployer_ready with this gated check so downstream
checks see healthy replication before readiness is signaled.
In `@test/infra/infra-dbdeployer-mysql84/docker/entrypoint.sh`:
- Around line 83-127: Validate and fail-fast on the environment values used
inside the SQL heredoc: before the SQL block in entrypoint.sh add a small
validator (e.g., function validate_env_value) and call it for ROOT_PASSWORD and
INFRA to reject unsafe characters (single quote ', semicolon ;, newline,
backslash \, null byte, and other SQL metacharacters) or enforce an allowed
character class (e.g., alphanumerics, underscore, dash) and exit 1 with a clear
error if validation fails; do this so the CREATE/ALTER USER heredoc that
interpolates ${ROOT_PASSWORD} and ${INFRA} cannot be corrupted by
malicious/invalid input.
---
Duplicate comments:
In `@test/infra/infra-dbdeployer-mysql57-binlog/docker/entrypoint.sh`:
- Around line 162-179: The loop that checks readers (using READER_PORTS,
READER_PORT, MAX_WAIT, COUNT) currently only warns on timeout, still echoes "OK"
and always creates the readiness marker (touch /tmp/dbdeployer_ready), which
hides failures; change the logic so that for each port you detect a real success
before printing "OK" (and do not print "OK" if the while loop timed out), track
an overall failure flag when any reader times out, and if any reader failed exit
non-zero (or skip creating the readiness marker) instead of always touching
/tmp/dbdeployer_ready so downstream steps only proceed when all binlog-readers
actually started.
In `@test/infra/infra-dbdeployer-mysql84-binlog/docker/entrypoint.sh`:
- Around line 163-180: The script currently prints "OK" and touches
/tmp/dbdeployer_ready even if a reader never bound; modify the reader-check loop
(the for loop iterating READER_PORTS and the while that tests
/dev/tcp/127.0.0.1/${READER_PORT}) to record a failure (e.g., set a flag like
READER_FAILED=1) when the MAX_WAIT is exceeded instead of silently continuing,
ensure you only print "OK" when the port actually responded, and only create the
readiness marker (/tmp/dbdeployer_ready) if no readers failed (otherwise exit
non-zero or skip touching the file) so docker-compose-init.bash won't start
against a half-broken binlog setup.
---
Nitpick comments:
In `@test/infra/infra-dbdeployer-mysql84/docker/entrypoint.sh`:
- Line 10: The current assignment to MYSQL_VERSION uses `ls | head -1`, which is
non-deterministic; replace both occurrences (the variable MYSQL_VERSION
assignments around lines 10 and 41) with a robust find-based selection: use
`find /root/opt/mysql -maxdepth 1 -mindepth 1 -type d` (or `-type f` if you
expect files) piped to a deterministic ordering (e.g., `sort`) and then pick the
first entry and extract its basename so MYSQL_VERSION gets a consistent,
portable value; update both places where `ls … | head -1` is used.
- Around line 51-63: The script builds MYSQL_CMD as a plain string and later
invokes it via ${MYSQL_CMD}, which is fragile; change MYSQL_CMD to an array
(e.g., MYSQL_CMD=(mysql -h127.0.0.1 -uroot -p"${DBDEPLOYER_ROOT_PASS}")) and
invoke it with the array expansion "${MYSQL_CMD[@]}" when adding flags like
-P"${PORT}" and -e "SELECT 1"; update every invocation of ${MYSQL_CMD} (the wait
loop and the other call around line 83) to use the array form to avoid
word-splitting and satisfy shellcheck while keeping
NODE_PORTS/NODE_NAMES/COUNT/MAX_WAIT logic unchanged.
🪄 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: 7ae5457e-8adc-4b1d-aecd-b7648f62114a
📒 Files selected for processing (5)
test/infra/infra-dbdeployer-mariadb10/docker/entrypoint.shtest/infra/infra-dbdeployer-mysql57-binlog/docker/entrypoint.shtest/infra/infra-dbdeployer-mysql57/docker/entrypoint.shtest/infra/infra-dbdeployer-mysql84-binlog/docker/entrypoint.shtest/infra/infra-dbdeployer-mysql84/docker/entrypoint.sh
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
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:03.216Z
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.
🪛 Shellcheck (0.11.0)
test/infra/infra-dbdeployer-mariadb10/docker/entrypoint.sh
[info] 10-10: Use find instead of ls to better handle non-alphanumeric filenames.
(SC2012)
[info] 33-33: Use find instead of ls to better handle non-alphanumeric filenames.
(SC2012)
[info] 54-54: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 75-75: Double quote to prevent globbing and word splitting.
(SC2086)
test/infra/infra-dbdeployer-mysql57/docker/entrypoint.sh
[info] 10-10: Use find instead of ls to better handle non-alphanumeric filenames.
(SC2012)
[info] 40-40: Use find instead of ls to better handle non-alphanumeric filenames.
(SC2012)
[info] 61-61: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 82-82: Double quote to prevent globbing and word splitting.
(SC2086)
test/infra/infra-dbdeployer-mysql57-binlog/docker/entrypoint.sh
[info] 10-10: Use find instead of ls to better handle non-alphanumeric filenames.
(SC2012)
[info] 42-42: Use find instead of ls to better handle non-alphanumeric filenames.
(SC2012)
[info] 62-62: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 82-82: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 155-155: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 156-156: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 157-157: Double quote to prevent globbing and word splitting.
(SC2086)
test/infra/infra-dbdeployer-mysql84-binlog/docker/entrypoint.sh
[info] 10-10: Use find instead of ls to better handle non-alphanumeric filenames.
(SC2012)
[info] 43-43: Use find instead of ls to better handle non-alphanumeric filenames.
(SC2012)
[info] 63-63: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 83-83: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 156-156: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 157-157: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 158-158: Double quote to prevent globbing and word splitting.
(SC2086)
test/infra/infra-dbdeployer-mysql84/docker/entrypoint.sh
[info] 10-10: Use find instead of ls to better handle non-alphanumeric filenames.
(SC2012)
[info] 41-41: Use find instead of ls to better handle non-alphanumeric filenames.
(SC2012)
[info] 62-62: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 83-83: Double quote to prevent globbing and word splitting.
(SC2086)
🔇 Additional comments (5)
test/infra/infra-dbdeployer-mysql57-binlog/docker/entrypoint.sh (2)
55-72: Node readiness wait loop is robust.The per-node timeout with fail-fast exit prevents silent bootstrap hangs and gives clear diagnostics.
74-131: User/bootstrap SQL setup looks consistent for binlog infra.
SQL_LOG_BIN=0, required service users, and test DB/user provisioning are coherent with the binlog test topology expectations.test/infra/infra-dbdeployer-mariadb10/docker/entrypoint.sh (1)
140-148: Good readiness-marker sequencing.Creating
/tmp/dbdeployer_readyonly after user provisioning and replicaread_onlysetup is the right ordering for the init flow.test/infra/infra-dbdeployer-mysql57/docker/entrypoint.sh (1)
82-126: User provisioning is consistent with ProxySQL post-setup expectations.This block correctly provisions
root,testuser,${INFRA},monitor,user,ssluser, andsbtest*accounts with credentials that match the referenced infra SQL/post scripts.test/infra/infra-dbdeployer-mysql84-binlog/docker/entrypoint.sh (1)
10-14:or after
Comment on lines
+10
to
+13
| MYSQL_VERSION=$(ls /root/opt/mysql/ | head -1) | ||
| if [ -z "${MYSQL_VERSION}" ]; then | ||
| echo "ERROR: No MariaDB tarball found in /root/opt/mysql/" | ||
| exit 1 |
There was a problem hiding this comment.
ls | head is brittle under set -e; fallback checks may never run.
At Line 10 and Line 33, a failed ls can terminate the script before your explicit if [ -z ... ] checks, so error handling becomes inconsistent.
Suggested hardening
-# Detect the unpacked MariaDB version
-MYSQL_VERSION=$(ls /root/opt/mysql/ | head -1)
-if [ -z "${MYSQL_VERSION}" ]; then
+# Detect the unpacked MariaDB version
+shopt -s nullglob
+mysql_versions=(/root/opt/mysql/*)
+if [ ${`#mysql_versions`[@]} -eq 0 ]; then
echo "ERROR: No MariaDB tarball found in /root/opt/mysql/"
exit 1
fi
+MYSQL_VERSION="$(basename "${mysql_versions[0]}")"
echo "Using MariaDB version: ${MYSQL_VERSION}"
@@
-# Determine the sandbox directory
-SANDBOX_DIR=$(ls -d /root/sandboxes/rsandbox_* | head -1)
-if [ -z "${SANDBOX_DIR}" ]; then
+# Determine the sandbox directory
+sandbox_dirs=(/root/sandboxes/rsandbox_*)
+if [ ${`#sandbox_dirs`[@]} -eq 0 ]; then
echo "ERROR: Sandbox directory not found"
exit 1
fi
+SANDBOX_DIR="${sandbox_dirs[0]}"
echo "Sandbox directory: ${SANDBOX_DIR}"Also applies to: 33-37
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 10-10: Use find instead of ls to better handle non-alphanumeric filenames.
(SC2012)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/infra/infra-dbdeployer-mariadb10/docker/entrypoint.sh` around lines 10 -
13, The script uses `ls | head` to set MYSQL_VERSION which is brittle under set
-e; replace the ls pipeline with safe globbing or an explicit directory scan
(e.g., iterate /root/opt/mysql/* and pick the first entry) and ensure the
command substitution cannot fail (avoid pipelines that return non-zero). Apply
the same change to the other block noted (the section that currently uses ls at
lines ~33-37) so both places use a guarded check for an empty directory before
extracting the filename into MYSQL_VERSION (and any related variables like the
tarball name), and keep the existing explicit if [ -z ... ] checks to emit the
error and exit when no files are present.
Comment on lines
+68
to
+70
| ROOT_PASSWORD="${ROOT_PASSWORD:-default_password}" | ||
| INFRA="${INFRA:-infra-dbdeployer-mariadb10}" | ||
|
|
There was a problem hiding this comment.
Escape env-derived SQL literals before injecting into heredoc.
ROOT_PASSWORD and INFRA are interpolated directly into SQL strings. A single quote in either value breaks SQL at startup (and opens injection risk if env input is not fully trusted).
Suggested fix
ROOT_PASSWORD="${ROOT_PASSWORD:-default_password}"
INFRA="${INFRA:-infra-dbdeployer-mariadb10}"
+
+escape_sql_literal() {
+ printf '%s' "$1" | sed "s/'/''/g"
+}
+ROOT_PASSWORD_SQL="$(escape_sql_literal "${ROOT_PASSWORD}")"
+INFRA_SQL="$(escape_sql_literal "${INFRA}")"
@@
-CREATE USER IF NOT EXISTS 'root'@'%' IDENTIFIED BY '${ROOT_PASSWORD}';
-SET PASSWORD FOR 'root'@'%' = PASSWORD('${ROOT_PASSWORD}');
-SET PASSWORD FOR 'root'@'localhost' = PASSWORD('${ROOT_PASSWORD}');
+CREATE USER IF NOT EXISTS 'root'@'%' IDENTIFIED BY '${ROOT_PASSWORD_SQL}';
+SET PASSWORD FOR 'root'@'%' = PASSWORD('${ROOT_PASSWORD_SQL}');
+SET PASSWORD FOR 'root'@'localhost' = PASSWORD('${ROOT_PASSWORD_SQL}');
@@
-CREATE USER IF NOT EXISTS '${INFRA}'@'%' IDENTIFIED BY '${INFRA}';
-GRANT ALL PRIVILEGES ON *.* TO '${INFRA}'@'%';
+CREATE USER IF NOT EXISTS '${INFRA_SQL}'@'%' IDENTIFIED BY '${INFRA_SQL}';
+GRANT ALL PRIVILEGES ON *.* TO '${INFRA_SQL}'@'%';Also applies to: 75-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/infra/infra-dbdeployer-mariadb10/docker/entrypoint.sh` around lines 68 -
70, The script injects ROOT_PASSWORD and INFRA directly into SQL heredocs
(variables ROOT_PASSWORD and INFRA), risking syntax breaks or injection if
values contain single quotes; create a small shell helper (e.g.,
escape_sql_literal) that replaces each single quote in a value with '\''
(SQL-safe single-quote escaping) and use that escaped value when interpolating
into all SQL heredocs (the blocks that use ROOT_PASSWORD and INFRA around lines
68 and 75-105) so the injected literals are always properly quoted and safe.
Comment on lines
+137
to
+138
| # Signal readiness via marker file (used by docker-mysql-post.bash) | ||
| touch /tmp/dbdeployer_ready |
There was a problem hiding this comment.
Gate readiness on replication health before creating /tmp/dbdeployer_ready.
At Line 138, readiness is signaled before confirming replica SQL/IO threads are fully running. Since downstream post-check only warns, this can still leak flaky starts into test execution.
Proposed fix
# Set read_only=1 on replicas (ports 3307, 3308) to match infra-mysql57 behavior
# Use ROOT_PASSWORD since we just changed the root password above
echo "Setting read_only on replicas..."
for PORT in 3307 3308; do
mysql -h127.0.0.1 -uroot -p"${ROOT_PASSWORD}" -P${PORT} -e "SET GLOBAL read_only=1; SET GLOBAL super_read_only=1;"
echo " port ${PORT}: read_only=1"
done
+# Wait for replication threads to be fully running before signaling readiness
+echo "Waiting for replication threads on replicas..."
+for PORT in 3307 3308; do
+ echo -n "Checking replication on port ${PORT}..."
+ MAX_WAIT=60
+ COUNT=0
+ while true; do
+ SLAVE_STATUS=$(mysql -h127.0.0.1 -uroot -p"${ROOT_PASSWORD}" -P${PORT} -e "SHOW SLAVE STATUS\G" 2>/dev/null || true)
+ IO_RUNNING=$(echo "${SLAVE_STATUS}" | awk '/Slave_IO_Running:/ {print $2}')
+ SQL_RUNNING=$(echo "${SLAVE_STATUS}" | awk '/Slave_SQL_Running:/ {print $2}')
+ if [ "${IO_RUNNING}" = "Yes" ] && [ "${SQL_RUNNING}" = "Yes" ]; then
+ echo " OK"
+ break
+ fi
+ if [ "${COUNT}" -ge "${MAX_WAIT}" ]; then
+ echo " TIMEOUT (IO: ${IO_RUNNING:-N/A}, SQL: ${SQL_RUNNING:-N/A})"
+ exit 1
+ fi
+ echo -n "."
+ sleep 1
+ COUNT=$((COUNT + 1))
+ done
+done
+
# Signal readiness via marker file (used by docker-mysql-post.bash)
touch /tmp/dbdeployer_ready🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/infra/infra-dbdeployer-mysql57/docker/entrypoint.sh` around lines 137 -
138, Delay creation of the readiness marker (/tmp/dbdeployer_ready) until
replication threads are confirmed running: update entrypoint.sh to query the
MySQL instance (e.g., using mysql -e "SHOW SLAVE/REPLICA STATUS") in a short
retry loop and only create the marker after verifying Slave_IO_Running and
Slave_SQL_Running are both "Yes" (or if the instance is primary/no replica
status exists, proceed immediately); replace the unconditional touch
/tmp/dbdeployer_ready with this gated check so downstream checks see healthy
replication before readiness is signaled.
Comment on lines
+83
to
+127
| ${MYSQL_CMD} -P${PORT} <<SQL | ||
| SET SQL_LOG_BIN=0; | ||
|
|
||
| -- root user with dynamic password (both remote and localhost) | ||
| CREATE USER IF NOT EXISTS 'root'@'%' IDENTIFIED WITH 'mysql_native_password' BY '${ROOT_PASSWORD}'; | ||
| ALTER USER 'root'@'%' IDENTIFIED WITH 'mysql_native_password' BY '${ROOT_PASSWORD}'; | ||
| ALTER USER 'root'@'localhost' IDENTIFIED WITH 'mysql_native_password' BY '${ROOT_PASSWORD}'; | ||
| GRANT ALL PRIVILEGES ON *.* TO 'root'@'%' WITH GRANT OPTION; | ||
|
|
||
| -- Monitor user | ||
| CREATE USER IF NOT EXISTS 'monitor'@'%' IDENTIFIED WITH 'mysql_native_password' BY 'monitor'; | ||
| GRANT USAGE, REPLICATION CLIENT ON *.* TO 'monitor'@'%'; | ||
|
|
||
| -- testuser | ||
| CREATE USER IF NOT EXISTS 'testuser'@'%' IDENTIFIED WITH 'mysql_native_password' BY 'testuser'; | ||
| GRANT ALL PRIVILEGES ON *.* TO 'testuser'@'%'; | ||
|
|
||
| -- Cluster specific user | ||
| CREATE USER IF NOT EXISTS '${INFRA}'@'%' IDENTIFIED WITH 'mysql_native_password' BY '${INFRA}'; | ||
| GRANT ALL PRIVILEGES ON *.* TO '${INFRA}'@'%'; | ||
|
|
||
| -- Databases | ||
| CREATE DATABASE IF NOT EXISTS sysbench; | ||
| CREATE DATABASE IF NOT EXISTS test; | ||
| CREATE DATABASE IF NOT EXISTS t1; | ||
| CREATE DATABASE IF NOT EXISTS jdbc_test; | ||
|
|
||
| -- sbtest users | ||
| $(for j in $(seq 1 10); do | ||
| echo "CREATE USER IF NOT EXISTS 'sbtest${j}'@'%' IDENTIFIED BY 'sbtest${j}';" | ||
| for db in sysbench test t1 jdbc_test; do | ||
| echo "GRANT ALL PRIVILEGES ON ${db}.* TO 'sbtest${j}'@'%';" | ||
| done | ||
| done) | ||
|
|
||
| -- ssluser | ||
| CREATE USER IF NOT EXISTS 'ssluser'@'%' IDENTIFIED WITH 'mysql_native_password' BY 'ssluser'; | ||
| GRANT ALL PRIVILEGES ON *.* TO 'ssluser'@'%'; | ||
|
|
||
| -- user (generic) | ||
| CREATE USER IF NOT EXISTS 'user'@'%' IDENTIFIED WITH 'mysql_native_password' BY 'user'; | ||
| GRANT ALL PRIVILEGES ON *.* TO 'user'@'%'; | ||
|
|
||
| FLUSH PRIVILEGES; | ||
| SQL |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where runtime env vars are defined and whether unsafe characters are possible.
set -euo pipefail
echo "== Candidate env definitions =="
fd -a '.env$' test/infra | xargs -r rg -n '^(ROOT_PASSWORD|INFRA)='
echo
echo "== Heredoc SQL interpolation sites in entrypoint =="
rg -n "ROOT_PASSWORD|INFRA|<<SQL|CREATE USER|ALTER USER" test/infra/infra-dbdeployer-mysql84/docker/entrypoint.shRepository: sysown/proxysql
Length of output: 1448
Guard SQL interpolation for ROOT_PASSWORD and INFRA environment variables.
Lines 87–102 directly interpolate ROOT_PASSWORD and INFRA into SQL without validation. If either variable contains single quotes, semicolons, or other SQL metacharacters, the heredoc will fail or execute unintended statements. Add validation before line 83 to fail fast on unsafe characters.
Proposed validation
ROOT_PASSWORD="${ROOT_PASSWORD:-default_password}"
INFRA="${INFRA:-infra-dbdeployer-mysql84}"
+
+# Fail fast on unsafe SQL characters
+case "${ROOT_PASSWORD}" in
+ *"'"*|*";"*|*$'\n'*|*$'\r'*)
+ echo "ERROR: ROOT_PASSWORD contains unsupported characters for SQL interpolation" >&2
+ exit 1
+ ;;
+esac
+case "${INFRA}" in
+ *[!a-zA-Z0-9_-]*)
+ echo "ERROR: INFRA must contain only alphanumeric characters, underscores, and hyphens" >&2
+ exit 1
+ ;;
+esac🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 83-83: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/infra/infra-dbdeployer-mysql84/docker/entrypoint.sh` around lines 83 -
127, Validate and fail-fast on the environment values used inside the SQL
heredoc: before the SQL block in entrypoint.sh add a small validator (e.g.,
function validate_env_value) and call it for ROOT_PASSWORD and INFRA to reject
unsafe characters (single quote ', semicolon ;, newline, backslash \, null byte,
and other SQL metacharacters) or enforce an allowed character class (e.g.,
alphanumerics, underscore, dash) and exit 1 with a clear error if validation
fails; do this so the CREATE/ALTER USER heredoc that interpolates
${ROOT_PASSWORD} and ${INFRA} cannot be corrupted by malicious/invalid input.
ProxySQL's replication lag monitor runs SHOW SLAVE STATUS using the monitor user, which requires SUPER (MySQL 5.7, MariaDB) or REPLICATION SLAVE (MySQL 8.4+). Without this, every monitor check fails with "Access denied; you need SUPER, SLAVE MONITOR privilege", generating connection errors that eventually trigger max_connect_errors and block the ProxySQL host IP entirely. - MySQL 5.7 + MariaDB: add SUPER - MySQL 8.4: add REPLICATION SLAVE (SUPER is deprecated)
Use MariaDB replication helper for binlog TAP tests
The MariaDB proxy-post was deleting and re-inserting root and testuser with default_hostgroup=1700 (MariaDB), overwriting the MySQL proxy-post's correct configuration of HG 1300 (MySQL). This caused tests to route queries to MariaDB instead of MySQL, resulting in: - "connection is locked to hostgroup 1700 but trying to reach 1300" - "Table 'test.sbtest1' doesn't exist" (tables on MySQL, query on MariaDB) The old infra-mariadb10 proxy-post didn't do this — it only loaded infra-config.sql (with INSERT OR IGNORE) and added fallback hostgroups. Match that behavior.
MySQL 8.4 defaults local_infile to OFF. The old infra-mysql84 my.cnf had local_infile=ON. Without this, test_unsupported_queries-t fails because LOAD DATA LOCAL INFILE is disabled on the backend.
The MariaDB docker-mysql-post.bash was deleting dbservers-cert-bundle.pem and caservers-cert-bundle.pem, then failing to recreate them because MariaDB doesn't generate SSL certs in the sandbox datadir. This destroyed the MySQL CA cert that was already collected by the MySQL docker-mysql-post (which runs first), causing mysql-test_ssl_CA-t to fail with "self-signed certificate in certificate chain". The fix: skip cert bundle handling entirely in the MariaDB post script.
test_backend_conn_ping-t hardcoded mysql1/mysql2/mysql3 hostnames. With dbdeployer, the hostname is dbdeployer1 with ports 3306/3307/3308. Query ProxySQL's runtime_mysql_servers for the actual server list.
fast_forward_grace_close_libmysql-t calculated the throttled read rate using total_bytes across ALL binlog files but only read the FIRST file. With dbdeployer, the binlog accumulates data from all earlier tests in the group, making total_bytes much larger than the first file. The resulting high rate meant the first file was read instantly, reaching EOF before the grace close could trigger. Fix: use the size of the first binlog file (the one actually read).
…points The real fix for the blocked host errors was granting SUPER/REPLICATION SLAVE to the monitor user. Inflating max_connect_errors just masked the problem. Let MySQL use its default (100).
The mysql-test_ssl_CA-t test generates many SSL connection failures (testing wrong CA bundles), which accumulate against max_connect_errors. MySQL's default of 100 is too low — the SSL test alone can generate hundreds of failed connection attempts. Set to 10000 as a reasonable threshold that won't mask real issues but tolerates SSL test failures.
dbdeployer generates separate SSL certs per MySQL node (master, node1, node2), each with a different CA. The docker-mysql-post.bash only collected from node1, so the cert bundle was missing the master's CA. This caused mysql-test_ssl_CA-t to fail with "certificate signature failure" on HG 1300 (writer/master on port 3306) while HG 1301 (reader including node1 on port 3307) succeeded. Collect CA certs from all 3 nodes into the cert bundle.
…oyer SSL certs: dbdeployer generates separate SSL certs per MySQL node. Collect CA certs from ALL nodes (master, node1, node2) instead of just node1. Also revert max_connect_errors to default — the real fix is having correct certs so SSL connections don't fail. Binlog size: fast_forward_grace_close test needs enough binlog data for throttled reads to exceed the 8s grace close period. Generate 50 x 50KB events instead of 3, creating enough per-event sleeps.
|
Phase 2 of the test switches use_ssl from 1 to 0 and checks that the NON-SSL monitor counter increases. A fixed 4-second wait was not always enough for the monitor to complete a cycle with the new config. Poll up to 6 intervals (12s) instead of a fixed 2x wait.
Signed-off-by: René Cannaò <rene@proxysql.com>
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
2 participants
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
INFRA_LOGS_PATHininfra-mysql57-binlog/.env— removedINFRA_LOGS_PATH=./logsthat overrode the proper default fromdocker-compose-init.bash, causing logs to be written insidetest/infra/instead ofci_infra_logs/and never cleaned upsession_track_gtidstest cases inset_testing-240-twhen running against MariaDB, which doesn't support this variable (Causal Consistency Reads & Tracking GTID on MariaDB/Galera #2134, Mariadb and session_track_gtids - appears to be support from Mariadb but does not function #3059)max_connectionsto 120 (from default 100) to prevent connection slot exhaustion during monitor SSL testsset_testing-240-t/.hfor JSON value access (parseResult,optimizer_switch,sql_mode,session_track_gtids)show_compatibility_56=ONto MySQL 5.7 configs soperformance_schema.session_variablesqueries worktest_gtid_forwarding-tfromlegacy-g5tolegacy-binlog-g1(requires binlog infra)test_binlog_reader-tcreate its own GTID query rules instead of relying on infra configtest_ffto_pgsql_error_stats-tfrommysql84-g4(PostgreSQL test in MySQL-only group)groups.jsonwith complete group assignmentspgsql-copy_from_stdin_session_parameter-tandtest_dns_cache-tTest plan
test_binlog_reader-tvialegacy-binlog-g1— PASS (3/3)pgsql-monitor_ssl_connections_test-tvialegacy-g4— PASS (7/7)set_testing-240-tvialegacy-g2— PASS (0 failures, ~120k assertions)Summary by CodeRabbit
New Features
Chores
Tests
Monitoring
Documentation