Add optional GTID tracking from OK packets - #6035
Conversation
📝 WalkthroughWalkthroughThe change adds configurable, validated GTID ingestion from MySQL OK responses. It propagates the setting through sessions and hostgroup management. Read-write locking and snapshots protect GTID state. Unit and integration tests cover propagation, inactive endpoints, cleanup, concurrency, and causal reads. ChangesGTID OK-response ingestion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MySQL_Session
participant MySQL_HostGroups_Manager
participant GTID_Server_Data
participant Reader_Endpoint
MySQL_Session->>MySQL_HostGroups_Manager: update_gtid_from_ok(mysrvc, gtid)
MySQL_HostGroups_Manager->>GTID_Server_Data: add_gtid_from_ok(gtid)
GTID_Server_Data-->>MySQL_HostGroups_Manager: return update status
MySQL_HostGroups_Manager->>Reader_Endpoint: expose updated GTID state
Reader_Endpoint-->>MySQL_Session: process causal read
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/tap/tests/unit/gtid_server_data_unit-t.cpp`:
- Around line 506-515: Update the reader thread in the concurrent snapshot test
to capture and count one initial snapshot before calling start.store(). Reuse
the existing snapshot coherence check for this initial capture, then retain the
current sampling loop afterward so the reader always records more than one
snapshot.
🪄 Autofix
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 Plus
Run ID: 5e3e04b8-4ce6-4d77-9361-24b8fcbe9c40
📒 Files selected for processing (14)
include/GTID_Server_Data.hinclude/MySQL_HostGroups_Manager.hinclude/MySQL_Thread.hinclude/proxysql_structs.hlib/GTID_Server_Data.cpplib/MySQL_HostGroups_Manager.cpplib/MySQL_Session.cpplib/MySQL_Thread.cpptest/tap/groups/groups.jsontest/tap/tests/Makefiletest/tap/tests/proxysql_reference_select_config_file.cnftest/tap/tests/test_cluster_sync-t.cpptest/tap/tests/test_gtid_from_ok-t.cpptest/tap/tests/unit/gtid_server_data_unit-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Gitar
🧰 Additional context used
📓 Path-based instructions (4)
include/**/*.h
📄 CodeRabbit inference engine (CLAUDE.md)
Header include guards use the
#ifndef __CLASS_*_Hconvention.
Files:
include/MySQL_Thread.hinclude/MySQL_HostGroups_Manager.hinclude/proxysql_structs.hinclude/GTID_Server_Data.h
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
include/MySQL_Thread.hlib/MySQL_Thread.cpptest/tap/tests/test_cluster_sync-t.cppinclude/MySQL_HostGroups_Manager.hinclude/proxysql_structs.htest/tap/tests/test_gtid_from_ok-t.cpplib/MySQL_Session.cppinclude/GTID_Server_Data.hlib/MySQL_HostGroups_Manager.cpptest/tap/tests/unit/gtid_server_data_unit-t.cpplib/GTID_Server_Data.cpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
test/tap/tests/**/*.cpp: Test files intest/tap/tests/must follow the naming patterntest_*.cppor*-t.cpp.
To add a new TAP test, add the<testname>-t.cppfile and register it intest/tap/tests/Makefile/groups.json; no special Makefile target is needed becausemake <testname>-tis generated by pattern rule.
Files:
test/tap/tests/test_cluster_sync-t.cpptest/tap/tests/test_gtid_from_ok-t.cpptest/tap/tests/unit/gtid_server_data_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in
test/tap/tests/unit/must usetest_globals.handtest_init.hwith the custom unit-test harness.
Files:
test/tap/tests/unit/gtid_server_data_unit-t.cpp
🧠 Learnings (2)
📚 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_cluster_sync-t.cpptest/tap/tests/test_gtid_from_ok-t.cpptest/tap/tests/unit/gtid_server_data_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/gtid_server_data_unit-t.cpp
🪛 ast-grep (0.45.1)
lib/MySQL_HostGroups_Manager.cpp
[error] 5814-5814: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: sprintf(buf,"%llu", snapshot.events_read)
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').
(dangerous-buffer-functions-cpp)
🔇 Additional comments (18)
include/GTID_Server_Data.h (1)
6-13: LGTM!Also applies to: 38-45
lib/GTID_Server_Data.cpp (1)
4-8: LGTM!Also applies to: 180-184, 202-202, 218-218, 230-230, 283-329, 377-385, 446-446, 516-521
include/MySQL_HostGroups_Manager.h (1)
1240-1240: LGTM!test/tap/tests/unit/gtid_server_data_unit-t.cpp (1)
17-28: LGTM!Also applies to: 348-460, 462-504, 517-554
test/tap/tests/Makefile (1)
311-312: LGTM!test/tap/tests/proxysql_reference_select_config_file.cnf (1)
85-85: LGTM!test/tap/tests/test_cluster_sync-t.cpp (1)
2476-2476: LGTM!include/MySQL_Thread.h (1)
655-655: LGTM!include/proxysql_structs.h (1)
1392-1392: LGTM!Also applies to: 1748-1748
lib/MySQL_Thread.cpp (1)
508-508: LGTM!Also applies to: 1443-1443, 2650-2650, 4936-4936
lib/MySQL_HostGroups_Manager.cpp (3)
5784-5786: LGTM!Also applies to: 5813-5815
1689-1726: 🩺 Stability & AvailabilityNo locking issue in
GTID_Server_Data.gtid_existsacquiresexecuted_rwlockin read mode, andadd_gtid_from_okacquires it in write mode.> Likely an incorrect or invalid review comment.
1783-1800: 🩺 Stability & AvailabilityNo watcher leak occurs on reactivation. Failure paths stop, close, free, and clear
sd->wbefore the inactive entry can be retried.> Likely an incorrect or invalid review comment.lib/MySQL_Session.cpp (1)
5776-5786: LGTM!test/tap/tests/test_gtid_from_ok-t.cpp (3)
34-334: LGTM!
336-663: LGTM!
1-5: 📐 Maintainability & Code QualityNo Makefile change is required.
test/tap/tests/Makefileregisterstest_gtid_from_ok-t, andtest/tap/groups/groups.jsonalso includes it.> Likely an incorrect or invalid review comment.test/tap/groups/groups.json (1)
389-389: LGTM!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb7ae79425
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 14 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/GTID_Server_Data.cpp (1)
377-386: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the manual unlock sites with an RAII write-lock guard.
The write lock is released at seven separate return points inside
read_next_gtid(). The current paths are all correct, but the lock is held acrossmalloc,strtok_r,free, and the whole message parse. Any future early return inside those branches deadlocks everygtid_exists()reader.Use a scope guard so the unlock cannot be missed. The repository guidelines require RAII for resource management.
♻️ Proposed guard
Add a small helper near the top of the file:
namespace { class RWLock_WrGuard { public: explicit RWLock_WrGuard(pthread_rwlock_t& lock) : lock_(lock) { pthread_rwlock_wrlock(&lock_); } ~RWLock_WrGuard() { pthread_rwlock_unlock(&lock_); } RWLock_WrGuard(const RWLock_WrGuard&) = delete; RWLock_WrGuard& operator=(const RWLock_WrGuard&) = delete; private: pthread_rwlock_t& lock_; }; } // namespaceThen simplify the entry and every exit:
bool GTID_Server_Data::read_next_gtid() { - pthread_rwlock_wrlock(&executed_rwlock); + RWLock_WrGuard guard(executed_rwlock); if (len==0) { - pthread_rwlock_unlock(&executed_rwlock); return false; }Remove the remaining six
pthread_rwlock_unlock(&executed_rwlock);calls in this function.Also applies to: 454-454, 467-475, 536-540
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/GTID_Server_Data.cpp` around lines 377 - 386, Replace the manual lock management in GTID_Server_Data::read_next_gtid() with a non-copyable RAII write-lock guard defined near the top of the file. Have the guard acquire executed_rwlock on construction and release it on destruction, then remove the explicit pthread_rwlock_unlock calls and simplify the function entry while preserving all existing return behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/GTID_Server_Data.cpp`:
- Around line 298-317: Normalize the UUID string in add_gtid_from_ok before
inserting it into gtid_executed: after removing hyphens and validating
hexadecimal content, fold every character to lowercase using std::tolower and
include the required cctype header. Keep validation and numeric GTID parsing
unchanged so both ingestion paths use the same lowercase key.
In `@test/tap/tests/test_gtid_from_ok-t.cpp`:
- Around line 7-24: Add the POSIX <strings.h> header to the include list in
test_gtid_from_ok-t.cpp so strcasecmp is declared before use; leave the existing
test logic unchanged.
---
Nitpick comments:
In `@lib/GTID_Server_Data.cpp`:
- Around line 377-386: Replace the manual lock management in
GTID_Server_Data::read_next_gtid() with a non-copyable RAII write-lock guard
defined near the top of the file. Have the guard acquire executed_rwlock on
construction and release it on destruction, then remove the explicit
pthread_rwlock_unlock calls and simplify the function entry while preserving all
existing return behavior.
🪄 Autofix
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 Plus
Run ID: 458e5374-a765-4d1a-98a8-34769e582f99
📒 Files selected for processing (14)
include/GTID_Server_Data.hinclude/MySQL_HostGroups_Manager.hinclude/MySQL_Thread.hinclude/proxysql_structs.hlib/GTID_Server_Data.cpplib/MySQL_HostGroups_Manager.cpplib/MySQL_Session.cpplib/MySQL_Thread.cpptest/tap/groups/groups.jsontest/tap/tests/Makefiletest/tap/tests/proxysql_reference_select_config_file.cnftest/tap/tests/test_cluster_sync-t.cpptest/tap/tests/test_gtid_from_ok-t.cpptest/tap/tests/unit/gtid_server_data_unit-t.cpp
🚧 Files skipped from review as they are similar to previous changes (11)
- test/tap/tests/test_cluster_sync-t.cpp
- include/MySQL_HostGroups_Manager.h
- test/tap/groups/groups.json
- test/tap/tests/Makefile
- include/proxysql_structs.h
- test/tap/tests/proxysql_reference_select_config_file.cnf
- include/MySQL_Thread.h
- lib/MySQL_Thread.cpp
- include/GTID_Server_Data.h
- test/tap/tests/unit/gtid_server_data_unit-t.cpp
- lib/MySQL_HostGroups_Manager.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
lib/MySQL_Session.cpptest/tap/tests/test_gtid_from_ok-t.cpplib/GTID_Server_Data.cpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
test/tap/tests/**/*.cpp: Test files intest/tap/tests/must follow the naming patterntest_*.cppor*-t.cpp.
To add a new TAP test, add the<testname>-t.cppfile and register it intest/tap/tests/Makefile/groups.json; no special Makefile target is needed becausemake <testname>-tis generated by pattern rule.
Files:
test/tap/tests/test_gtid_from_ok-t.cpp
🧠 Learnings (1)
📚 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_gtid_from_ok-t.cpp
🔇 Additional comments (12)
lib/GTID_Server_Data.cpp (4)
4-7: LGTM!
179-201: LGTM!
217-217: LGTM!Also applies to: 229-229
281-286: LGTM!Also applies to: 322-331
lib/MySQL_Session.cpp (2)
5776-5778: LGTM!
8403-8410: LGTM!test/tap/tests/test_gtid_from_ok-t.cpp (6)
35-105: LGTM!
107-221: LGTM!
223-353: LGTM!
355-651: LGTM!
655-682: LGTM!
1-5: 📐 Maintainability & Code QualityNo registration change is required.
test_gtid_from_ok-tis listed intest/tap/tests/Makefileandtest/tap/groups/groups.json.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-08-11-gtid-sonar-cleanup.md`:
- Around line 330-335: Update the test sequence using the existing debug-build
flow so test/infra/control/start-proxysql-isolated.bash runs after the rebuilt
binary and before run-tests-isolated.bash; keep ensure-infras.bash as currently
ordered and recreate only the ProxySQL container without substituting docker
restart.
🪄 Autofix
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 Plus
Run ID: fee31d49-4e83-4e59-bc21-35b2ff60af69
📒 Files selected for processing (8)
docs/superpowers/plans/2026-08-11-gtid-sonar-cleanup.mddocs/superpowers/specs/2026-08-11-gtid-sonar-cleanup-design.mdinclude/MySQL_HostGroups_Manager.hlib/GTID_Server_Data.cpplib/MySQL_HostGroups_Manager.cpplib/MySQL_Thread.cpptest/tap/tests/test_gtid_from_ok-t.cpptest/tap/tests/unit/gtid_server_data_unit-t.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- include/MySQL_HostGroups_Manager.h
- lib/MySQL_Thread.cpp
- test/tap/tests/unit/gtid_server_data_unit-t.cpp
- lib/GTID_Server_Data.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Gitar
🧰 Additional context used
📓 Path-based instructions (2)
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
test/tap/tests/**/*.cpp: Test files intest/tap/tests/must follow the naming patterntest_*.cppor*-t.cpp.
To add a new TAP test, add the<testname>-t.cppfile and register it intest/tap/tests/Makefile/groups.json; no special Makefile target is needed becausemake <testname>-tis generated by pattern rule.
Files:
test/tap/tests/test_gtid_from_ok-t.cpp
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
test/tap/tests/test_gtid_from_ok-t.cpplib/MySQL_HostGroups_Manager.cpp
🧠 Learnings (18)
📓 Common learnings
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.
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5861
File: lib/ProxySQL_Cluster.cpp:2251-2255
Timestamp: 2026-07-13T08:29:05.757Z
Learning: In ProxySQL (lib/ProxySQL_Cluster.cpp and related cluster sync code), the MySQL server status value `SHUNNED_AWS_BGD` is runtime-only. Both `SHUNNED` and `SHUNNED_AWS_BGD` are normalized to `ONLINE` before being exposed/checksummed for cluster synchronization, so case-mismatched or unexpected status strings for these states are not expected to reach the `mysql_servers_v2` insert path (e.g., in `pull_mysql_servers_v2_from_peer`) during normal cluster sync operation.
Learnt from: peterlyoo
Repo: sysown/proxysql PR: 5925
File: lib/MySQL_Session.cpp:0-0
Timestamp: 2026-07-10T02:12:40.310Z
Learning: In lib/MySQL_Session.cpp, MySQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_qpo() has an early-return path for query cache hits (GloMyQC->get(...) keyed on client_myds->myconn->userinfo->hash) that occurs before the `__exit_set_destination_hostgroup` label. Any per-query session state mutation driven by qpo (e.g. qpo->destination_schema) that is placed after that label will be skipped entirely on a cache hit. The destination_schema switch (client_myds->myconn->userinfo->set_schemaname) is therefore applied right after the qpo->OK_msg/qpo->error_msg early-return checks (before the __exit_set_destination_hostgroup label and before the locked_on_hostgroup rejection check), not after the hostgroup-lock validation, specifically to avoid this cache-hit bypass. This placement was decided in PR `#5925` (commit 652ffa124) after discussion.
Learnt from: peterlyoo
Repo: sysown/proxysql PR: 5925
File: lib/MySQL_Session.cpp:0-0
Timestamp: 2026-07-10T02:12:40.310Z
Learning: In lib/MySQL_Session.cpp, mysql_query_rules.attributes.destination_schema (query-rule-driven session schema switching) is applied unconditionally, without the `transaction_persistent_hostgroup == -1` guard used for `destination_hostgroup`. This is intentional: switching a session's default schema mid-transaction via COM_INIT_DB has the same semantics as a client issuing `USE <schema>` mid-transaction through ProxySQL — it does not commit or invalidate the transaction and the sticky backend connection is preserved. Guarding on `transaction_persistent_hostgroup` was considered but rejected because it would make the destination_schema rule silently inert during an active transaction, which was judged more surprising than the current behavior.
📚 Learning: 2026-08-11T12:56:09.846Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 6033
File: docs/superpowers/plans/2026-08-11-ed25519-authentication.md:469-469
Timestamp: 2026-08-11T12:56:09.846Z
Learning: In `docs/superpowers/plans/2026-08-11-ed25519-authentication.md`, the historical-artifact notice states that embedded expected outputs are plan-time values. Review-driven changes can modify the MariaDB Ed25519 implementation and TAP assertion counts after the plan is written. The shipped implementation and tests are authoritative, so reviewers must not require retroactive synchronization of plan-time expected outputs.
Applied to files:
docs/superpowers/specs/2026-08-11-gtid-sonar-cleanup-design.mddocs/superpowers/plans/2026-08-11-gtid-sonar-cleanup.md
📚 Learning: 2026-04-11T13:17:55.508Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.508Z
Learning: When using GitHub-flavored Markdown headings, be aware that an em-dash surrounded by spaces (written as ` — `) affects the generated anchor/slug: GitHub replaces spaces with hyphens and removes non-alphanumeric punctuation, which can produce double hyphens (e.g., `## Foo — bar` → anchor `#foo--bar`, not `#foo-bar`). If you reference these anchors (e.g., internal links), ensure the expected slug matches this behavior.
Applied to files:
docs/superpowers/specs/2026-08-11-gtid-sonar-cleanup-design.mddocs/superpowers/plans/2026-08-11-gtid-sonar-cleanup.md
📚 Learning: 2026-04-11T13:17:55.509Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.509Z
Learning: When reviewing GitHub-flavored Markdown links/anchors, remember that heading-to-anchor slug generation treats spaces as hyphens and removes punctuation. If a heading contains an em-dash surrounded by spaces (e.g. ` — `), the slugs can legitimately include a double hyphen where the two surrounding space-runs become `-` on either side of the removed em-dash (e.g. `...vocabulary--read...`). Do not flag double-hyphens in anchor links for em-dash-containing headings as errors; they reflect GitHub’s correct slug behavior.
Applied to files:
docs/superpowers/specs/2026-08-11-gtid-sonar-cleanup-design.mddocs/superpowers/plans/2026-08-11-gtid-sonar-cleanup.md
📚 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:
docs/superpowers/plans/2026-08-11-gtid-sonar-cleanup.mdtest/tap/tests/test_gtid_from_ok-t.cpp
📚 Learning: 2026-07-08T13:19:04.649Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-07-08T13:19:04.649Z
Learning: When swapping in a rebuilt proxysql binary, rerun `test/infra/control/start-proxysql-isolated.bash` to recreate only the ProxySQL container; do not rely on `ensure-infras.bash` or `docker restart` to pick up the new binary.
Applied to files:
docs/superpowers/plans/2026-08-11-gtid-sonar-cleanup.md
📚 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_gtid_from_ok-t.cpp
📚 Learning: 2026-07-08T13:19:04.649Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-07-08T13:19:04.649Z
Learning: Applies to test/tap/tests/**/*.cpp : To add a new TAP test, add the `<testname>-t.cpp` file and register it in `test/tap/tests/Makefile`/`groups.json`; no special Makefile target is needed because `make <testname>-t` is generated by pattern rule.
Applied to files:
test/tap/tests/test_gtid_from_ok-t.cpp
📚 Learning: 2026-07-08T13:19:04.649Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-07-08T13:19:04.649Z
Learning: The proxysql binary under test must be a DEBUG build when running the isolated TAP harness.
Applied to files:
test/tap/tests/test_gtid_from_ok-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/test_gtid_from_ok-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/test_gtid_from_ok-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_gtid_from_ok-t.cpplib/MySQL_HostGroups_Manager.cpp
📚 Learning: 2026-07-08T13:19:04.649Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-07-08T13:19:04.649Z
Learning: Applies to **/*.{cpp,h,hpp} : Use C++17, and gate conditional code with `#ifdef PROXYSQL31`, `#ifdef PROXYSQL40`, `#ifdef PROXYSQLFFTO`, `#ifdef PROXYSQLTSDB`, and `#ifdef PROXYSQLCLICKHOUSE`; `PROXYSQLGENAI` must not guard core code outside `plugins/genai/`.
Applied to files:
test/tap/tests/test_gtid_from_ok-t.cpp
📚 Learning: 2026-07-22T14:10:08.098Z
Learnt from: burnison
Repo: sysown/proxysql PR: 5946
File: lib/MySQL_Thread.cpp:4483-4485
Timestamp: 2026-07-22T14:10:08.098Z
Learning: In `lib/MySQL_Thread.cpp`, `MySQL_Thread::ProcessAllSessions_Healthy0()` intentionally logs the live backend MySQL thread ID when `sess->mybe->server_myds->myconn` is attached; it logs `connection 0` when no backend is attached at unhealthy client-session close time. Do not require preserving a historical backend ID for that case.
Applied to files:
lib/MySQL_HostGroups_Manager.cpp
📚 Learning: 2026-07-13T08:29:05.757Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5861
File: lib/ProxySQL_Cluster.cpp:2251-2255
Timestamp: 2026-07-13T08:29:05.757Z
Learning: In ProxySQL (lib/ProxySQL_Cluster.cpp and related cluster sync code), the MySQL server status value `SHUNNED_AWS_BGD` is runtime-only. Both `SHUNNED` and `SHUNNED_AWS_BGD` are normalized to `ONLINE` before being exposed/checksummed for cluster synchronization, so case-mismatched or unexpected status strings for these states are not expected to reach the `mysql_servers_v2` insert path (e.g., in `pull_mysql_servers_v2_from_peer`) during normal cluster sync operation.
Applied to files:
lib/MySQL_HostGroups_Manager.cpp
📚 Learning: 2026-07-10T02:12:40.310Z
Learnt from: peterlyoo
Repo: sysown/proxysql PR: 5925
File: lib/MySQL_Session.cpp:0-0
Timestamp: 2026-07-10T02:12:40.310Z
Learning: In lib/MySQL_Session.cpp, MySQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_qpo() has an early-return path for query cache hits (GloMyQC->get(...) keyed on client_myds->myconn->userinfo->hash) that occurs before the `__exit_set_destination_hostgroup` label. Any per-query session state mutation driven by qpo (e.g. qpo->destination_schema) that is placed after that label will be skipped entirely on a cache hit. The destination_schema switch (client_myds->myconn->userinfo->set_schemaname) is therefore applied right after the qpo->OK_msg/qpo->error_msg early-return checks (before the __exit_set_destination_hostgroup label and before the locked_on_hostgroup rejection check), not after the hostgroup-lock validation, specifically to avoid this cache-hit bypass. This placement was decided in PR `#5925` (commit 652ffa124) after discussion.
Applied to files:
lib/MySQL_HostGroups_Manager.cpp
📚 Learning: 2026-07-22T21:24:52.599Z
Learnt from: burnison
Repo: sysown/proxysql PR: 5948
File: lib/MySQL_Session.cpp:6850-6850
Timestamp: 2026-07-22T21:24:52.599Z
Learning: In `include/MySQL_Thread.h`, `MySQL_Thread::status_variables.stvar` is intentionally per-worker-thread storage. Writers use non-atomic direct updates for hot-path counters, while `MySQL_Threads_Handler::get_status_variable()` in `lib/MySQL_Thread.cpp` aggregates values using `__sync_fetch_and_add(..., 0)`. New `stvar` counters should follow this established contract unless their ownership becomes cross-thread.
Applied to files:
lib/MySQL_HostGroups_Manager.cpp
📚 Learning: 2026-07-10T02:12:40.310Z
Learnt from: peterlyoo
Repo: sysown/proxysql PR: 5925
File: lib/MySQL_Session.cpp:0-0
Timestamp: 2026-07-10T02:12:40.310Z
Learning: In lib/MySQL_Session.cpp, mysql_query_rules.attributes.destination_schema (query-rule-driven session schema switching) is applied unconditionally, without the `transaction_persistent_hostgroup == -1` guard used for `destination_hostgroup`. This is intentional: switching a session's default schema mid-transaction via COM_INIT_DB has the same semantics as a client issuing `USE <schema>` mid-transaction through ProxySQL — it does not commit or invalidate the transaction and the sticky backend connection is preserved. Guarding on `transaction_persistent_hostgroup` was considered but rejected because it would make the destination_schema rule silently inert during an active transaction, which was judged more surprising than the current behavior.
Applied to files:
lib/MySQL_HostGroups_Manager.cpp
🔇 Additional comments (7)
lib/MySQL_HostGroups_Manager.cpp (1)
1727-1744: LGTM!Also applies to: 1746-1760, 1806-1814
test/tap/tests/test_gtid_from_ok-t.cpp (3)
18-18: LGTM!Also applies to: 368-380, 382-416, 418-434, 436-461
463-511: LGTM!Also applies to: 513-555
557-601: LGTM!Also applies to: 603-650, 652-708, 710-724
docs/superpowers/plans/2026-08-11-gtid-sonar-cleanup.md (2)
359-363: 📐 Maintainability & Code QualityStage the reviewed documents or state that they are already committed.
The commit command stages only source and test files. This review also contains
docs/superpowers/plans/2026-08-11-gtid-sonar-cleanup.mdanddocs/superpowers/specs/2026-08-11-gtid-sonar-cleanup-design.md. If these documents are part of the final change and are uncommitted, the command omits them.
1-329: LGTM!Also applies to: 336-358, 364-401
docs/superpowers/specs/2026-08-11-gtid-sonar-cleanup-design.md (1)
1-37: LGTM!
|
CI failed: Downstream CI-builds workflow runs failed, one encountering a TLS certificate verification error while communicating with the GitHub API and another failing during execution.OverviewA total of 2 log failures were analyzed across jobs. The failures stem from a downstream CI-builds workflow run failing or encountering connectivity issues (TLS certificate verification failure for api.github.com), causing the parent workflow waiting via FailuresDownstream CI-Builds Workflow Failure and TLS Certificate Error (confidence: high)
Summary
Code Review ✅ Approved 3 resolved / 3 findingsAdds optional GTID tracking from MySQL OK packets with cross-hostgroup endpoint sharing and cluster synchronization, addressing the hot-path string build, lock reentrancy, and UUID parsing findings. ✅ 3 resolved✅ Bug: to_string() called under read lock may not be reentrant
✅ Edge Case: add_gtid_from_ok only accepts a single uuid:trxid GTID
✅ Performance: Per-OK-packet string build + lock in hot path
Tip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
1 participant
Summary
mysql-update_gtid_from_okvariable, disabled by default and synchronized through ProxySQL Clustergtid_exists()to consult records whose binlog reader is inactiveMotivation
A MySQL endpoint can appear in both writer and reader hostgroups. Writes may reach it through the writer hostgroup while
min_gtidrouting evaluates the same endpoint in the reader hostgroup. GTIDs learned from OK packets therefore need endpoint-wide visibility rather than hostgroup-local visibility.This implements the behavior discussed in #5983.
Behavioral contract
session_track_gtids=OWN_GTIDfor this feature.Validation
test_gtid_from_ok-ton MySQL 5.7 binlog infrastructure: 14/14test_gtid_from_ok-ton MySQL 8.4 infrastructure: 14/14git diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Tests