Conversation
refactor: Extract GTID interval logic into `Gtid_Interval` class
This PR adds full support for ranges in `proxysql_mysqlbinlog` update messages, while also improving efficiency when dealing with GTID gaps for any given UUID. Single GTID updates are transparently supported, so older `proxysql_mysqlbinlog` version will continue to work with newer ProxySQL releases. See sysown/proxysql_mysqlbinlog#33 for details.
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
|
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:
📝 WalkthroughWalkthroughReplaces legacy typedef-based GTID representations and free helper functions with class-based Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Parser as GTID Parser
participant GSD as GTID_Server_Data
participant GSet as GTID_Set
participant HGM as HostGroupsMgr
Parser->>GSD: read_next_gtid(tokenized message)
GSD->>GSet: gtid_executed.add(uuid, token)
GSet-->>GSD: add result (bool)
HGM->>GSD: query gtid_exists(uuid, trxid)
GSD->>GSet: gtid_executed.has_gtid(uuid, trxid)
GSet-->>HGM: membership bool
HGM->>HGM: gtid_executed.to_string() for serialization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the GTID management system by introducing the TrxId_Interval and GTID_Set classes to replace the previous typedef-based implementation. These classes centralize logic for interval merging, membership validation, and serialization to the MySQL GTID format. The refactor includes updates to GTID_Server_Data and MySQL_HostGroups_Manager, the removal of legacy helper functions, and the addition of comprehensive unit tests. Review feedback highlights a potential null pointer dereference in GTID_Server_Data.cpp, portability concerns with sscanf format strings, and opportunities for optimization in the GTID_Set copy and add methods. A safety check for UUID string lengths was also recommended to prevent potential runtime exceptions during formatting.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/proxysql_gtid.h (1)
1-2:⚠️ Potential issue | 🟡 MinorInclude guard format does not match repository rule.
#ifndef PROXYSQL_GTID/#define PROXYSQL_GTIDdoes not follow the required include-guard pattern for headers underinclude/.Suggested guard format
-#ifndef PROXYSQL_GTID -#define PROXYSQL_GTID +#ifndef __CLASS_PROXYSQL_GTID_H +#define __CLASS_PROXYSQL_GTID_H ... -#endif /* PROXYSQL_GTID */ +#endif /* __CLASS_PROXYSQL_GTID_H */As per coding guidelines "include/**/.{h,hpp}: Include guards must follow the pattern
#ifndef_CLASS_H".Also applies to: 56-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/proxysql_gtid.h` around lines 1 - 2, The current include guard macros PROXYSQL_GTID in proxysql_gtid.h don't follow the repo pattern; replace them with the required format by changing `#ifndef` PROXYSQL_GTID and `#define` PROXYSQL_GTID to `#ifndef` __CLASS_PROXYSQL_GTID_H and `#define` __CLASS_PROXYSQL_GTID_H, and update the trailing `#endif` to include a comment like /* __CLASS_PROXYSQL_GTID_H */ so the guard matches the "#ifndef __CLASS_*_H" rule used for headers under include/.
🧹 Nitpick comments (3)
include/MySQL_HostGroups_Manager.h (1)
26-27: Minor: Redundant include.
GTID_Server_Data.his already included viaBase_HostGroups_Manager.h(line 26). The direct include on line 27 is redundant, though harmless due to include guards.♻️ Suggested cleanup
`#include` "proxysql.h" `#include` "cpp.h" `#include` "Base_HostGroups_Manager.h" -#include "GTID_Server_Data.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/MySQL_HostGroups_Manager.h` around lines 26 - 27, Remove the redundant direct include of GTID_Server_Data.h from the top of include/MySQL_HostGroups_Manager.h since GTID_Server_Data.h is already pulled in via Base_HostGroups_Manager.h; keep the Base_HostGroups_Manager.h include and the MySQL_HostGroups_Manager declarations intact, and delete the extra `#include` "GTID_Server_Data.h" line to avoid needless duplication while preserving include guards.lib/proxysql_gtid.cpp (1)
22-36: Potential portability issue with%ldformat specifier.
trxid_tisint64_t, butsscanfwith%ldassumeslongwhich is only guaranteed to be 32-bit minimum. On 32-bit platforms or Windows, this could truncate values.Consider using the portable format macro from
<cinttypes>:♻️ Portable format specifier
+#include <cinttypes> + // Initializes a trxid interval from a C string buffer, in [trxid]{-[trxid]} format. TrxId_Interval::TrxId_Interval(const char *s) { trxid_t _start = 0, _end = 0; - if (sscanf(s, "%ld-%ld", &_start, &_end) == 2) { + if (sscanf(s, "%" SCNd64 "-%" SCNd64, &_start, &_end) == 2) { start = _start; end = _end; - } else if (sscanf(s, "%ld", &_start) == 1) { + } else if (sscanf(s, "%" SCNd64, &_start) == 1) { start = _start; end = _start; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/proxysql_gtid.cpp` around lines 22 - 36, TrxId_Interval::TrxId_Interval uses sscanf with "%ld" but trxid_t is int64_t, causing truncation on some platforms; fix by using the portable int64 scanf macro from <cinttypes> (SCNd64) and update the sscanf format strings in TrxId_Interval::TrxId_Interval to use "%" SCNd64 "-%" SCNd64 for the two-value parse and "%" SCNd64 for the single-value parse, and add the <cinttypes> include if not present so sscanf reads into trxid_t (_start, _end) correctly across platforms.include/proxysql_gtid.h (1)
12-12: Rename classes to include the required protocol prefix.
TrxId_IntervalandGTID_Setare PascalCase, but they miss the requiredMySQL_,PgSQL_, orProxySQL_prefix.As per coding guidelines "Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)".
Also applies to: 36-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/proxysql_gtid.h` at line 12, The classes TrxId_Interval and GTID_Set must be renamed to include the required protocol prefix; change TrxId_Interval to ProxySQL_TrxId_Interval and GTID_Set to ProxySQL_GTID_Set, then update all references (constructors, destructors, forward declarations, typedefs, method signatures, use-sites, and any serialization/parsing helpers) to the new names so compilation and linkage succeed; ensure header guards, comments, and any .cpp usages are updated accordingly and run a full build to catch remaining references.
🤖 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/unit/gtid_set_unit-t.cpp`:
- Around line 14-17: The test source is missing the required test harness
headers; modify the includes in gtid_set_unit-t.cpp so that after the existing
`#include` "tap.h" you add `#include` "test_globals.h" and `#include` "test_init.h"
before the existing `#include` "proxysql_gtid.h" so the unit test links against
the custom harness and libproxysql.a (look for the include block at the top of
the file).
---
Outside diff comments:
In `@include/proxysql_gtid.h`:
- Around line 1-2: The current include guard macros PROXYSQL_GTID in
proxysql_gtid.h don't follow the repo pattern; replace them with the required
format by changing `#ifndef` PROXYSQL_GTID and `#define` PROXYSQL_GTID to `#ifndef`
__CLASS_PROXYSQL_GTID_H and `#define` __CLASS_PROXYSQL_GTID_H, and update the
trailing `#endif` to include a comment like /* __CLASS_PROXYSQL_GTID_H */ so the
guard matches the "#ifndef __CLASS_*_H" rule used for headers under include/.
---
Nitpick comments:
In `@include/MySQL_HostGroups_Manager.h`:
- Around line 26-27: Remove the redundant direct include of GTID_Server_Data.h
from the top of include/MySQL_HostGroups_Manager.h since GTID_Server_Data.h is
already pulled in via Base_HostGroups_Manager.h; keep the
Base_HostGroups_Manager.h include and the MySQL_HostGroups_Manager declarations
intact, and delete the extra `#include` "GTID_Server_Data.h" line to avoid
needless duplication while preserving include guards.
In `@include/proxysql_gtid.h`:
- Line 12: The classes TrxId_Interval and GTID_Set must be renamed to include
the required protocol prefix; change TrxId_Interval to ProxySQL_TrxId_Interval
and GTID_Set to ProxySQL_GTID_Set, then update all references (constructors,
destructors, forward declarations, typedefs, method signatures, use-sites, and
any serialization/parsing helpers) to the new names so compilation and linkage
succeed; ensure header guards, comments, and any .cpp usages are updated
accordingly and run a full build to catch remaining references.
In `@lib/proxysql_gtid.cpp`:
- Around line 22-36: TrxId_Interval::TrxId_Interval uses sscanf with "%ld" but
trxid_t is int64_t, causing truncation on some platforms; fix by using the
portable int64 scanf macro from <cinttypes> (SCNd64) and update the sscanf
format strings in TrxId_Interval::TrxId_Interval to use "%" SCNd64 "-%" SCNd64
for the two-value parse and "%" SCNd64 for the single-value parse, and add the
<cinttypes> include if not present so sscanf reads into trxid_t (_start, _end)
correctly across platforms.
🪄 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: 42b30b17-dfce-42b1-96d0-36b03a21f494
📒 Files selected for processing (13)
include/Base_HostGroups_Manager.hinclude/GTID_Server_Data.hinclude/MySQL_HostGroups_Manager.hinclude/proxysql_gtid.hlib/GTID_Server_Data.cpplib/Makefilelib/MySQL_HostGroups_Manager.cpplib/proxysql_gtid.cpptest/tap/groups/groups.jsontest/tap/tests/unit/Makefiletest/tap/tests/unit/gtid_set_unit-t.cpptest/tap/tests/unit/gtid_trxid_interval_unit-t.cpptest/tap/tests/unit/gtid_utils_unit-t.cpp
💤 Files with no reviewable changes (1)
- test/tap/tests/unit/gtid_utils_unit-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
include/Base_HostGroups_Manager.hinclude/MySQL_HostGroups_Manager.hlib/MySQL_HostGroups_Manager.cpptest/tap/tests/unit/gtid_set_unit-t.cppinclude/GTID_Server_Data.htest/tap/tests/unit/gtid_trxid_interval_unit-t.cpplib/GTID_Server_Data.cpplib/proxysql_gtid.cppinclude/proxysql_gtid.h
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards must follow the pattern
#ifndef_CLASS*_H
Files:
include/Base_HostGroups_Manager.hinclude/MySQL_HostGroups_Manager.hinclude/GTID_Server_Data.hinclude/proxysql_gtid.h
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
lib/MySQL_HostGroups_Manager.cpptest/tap/tests/unit/gtid_set_unit-t.cpptest/tap/tests/unit/gtid_trxid_interval_unit-t.cpplib/GTID_Server_Data.cpplib/proxysql_gtid.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention
Files:
lib/MySQL_HostGroups_Manager.cpplib/GTID_Server_Data.cpplib/proxysql_gtid.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Files:
test/tap/tests/unit/gtid_set_unit-t.cpptest/tap/tests/unit/gtid_trxid_interval_unit-t.cpp
🧠 Learnings (10)
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
lib/Makefileinclude/Base_HostGroups_Manager.hinclude/MySQL_HostGroups_Manager.hinclude/GTID_Server_Data.hlib/proxysql_gtid.cppinclude/proxysql_gtid.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Applied to files:
lib/Makefiletest/tap/tests/unit/Makefiletest/tap/groups/groups.jsoninclude/Base_HostGroups_Manager.hinclude/MySQL_HostGroups_Manager.htest/tap/tests/unit/gtid_set_unit-t.cpptest/tap/tests/unit/gtid_trxid_interval_unit-t.cpplib/proxysql_gtid.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:
lib/Makefileinclude/MySQL_HostGroups_Manager.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Applied to files:
lib/Makefileinclude/Base_HostGroups_Manager.hinclude/MySQL_HostGroups_Manager.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Admin interface uses SQLite3 backend for SQL-based configuration and schema versions are tracked in ProxySQL_Admin_Tables_Definitions.h
Applied to files:
lib/Makefileinclude/MySQL_HostGroups_Manager.h
📚 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:
lib/Makefileinclude/Base_HostGroups_Manager.hinclude/MySQL_HostGroups_Manager.h
📚 Learning: 2026-02-13T05:55:42.693Z
Learnt from: mevishalr
Repo: sysown/proxysql PR: 5364
File: lib/MySQL_Logger.cpp:1211-1232
Timestamp: 2026-02-13T05:55:42.693Z
Learning: In ProxySQL, the MySQL_Logger and PgSQL_Logger destructors run after all worker threads have been joined during shutdown. The sequence in src/main.cpp is: (1) join all worker threads, (2) call ProxySQL_Main_shutdown_all_modules() which deletes the loggers. Therefore, there is no concurrent thread access during logger destruction, and lock ordering in the destructors cannot cause deadlocks.
Applied to files:
include/Base_HostGroups_Manager.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Feature tiers are controlled by build flags: PROXYSQL31=1 for v3.1.x, PROXYSQLGENAI=1 for v4.0.x; PROXYSQLGENAI=1 implies PROXYSQL31=1
Applied to files:
include/MySQL_HostGroups_Manager.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Applied to files:
test/tap/tests/unit/gtid_set_unit-t.cpptest/tap/tests/unit/gtid_trxid_interval_unit-t.cpp
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/gtid_set_unit-t.cpptest/tap/tests/unit/gtid_trxid_interval_unit-t.cpp
🔇 Additional comments (16)
lib/Makefile (1)
100-100: GTID object inclusion is correctly wired into the static library build.Adding
proxysql_gtid.ooto_OBJ_CXXensures the new GTID implementation is compiled and packaged intolibproxysql.afor downstream linkage.test/tap/tests/unit/Makefile (1)
288-289: New GTID unit tests are correctly registered in the unit-test suite.Both targets are added to
UNIT_TESTS, so they are built via the existing pattern rule and executed through the normalallflow.lib/MySQL_HostGroups_Manager.cpp (1)
5554-5555: GTID string serialization update looks correct.Using
GTID_Set::to_string()here is consistent with the new class-based GTID model and keeps the existing result-row behavior intact.test/tap/groups/groups.json (1)
372-374: LGTM!The test group configuration correctly replaces the removed
gtid_utils_unit-twith the two new GTID unit test binaries, properly mapping them to theunit-tests-g1group.include/GTID_Server_Data.h (1)
6-6: LGTM!The header correctly transitions from the legacy
gtid_set_ttypedef to the newGTID_Setclass abstraction. The include forproxysql_gtid.hprovides the necessary type definition.Also applies to: 20-20
include/Base_HostGroups_Manager.h (1)
21-22: LGTM!The include change correctly switches from direct
proxysql_gtid.hinclusion toGTID_Server_Data.h, which provides the necessary GTID types transitively. The removal of free-function declarations (gtid_executed_to_string,addGtid) aligns with the migration toGTID_Setmethods.test/tap/tests/unit/gtid_set_unit-t.cpp (1)
291-312: LGTM! Comprehensive test coverage.The test suite provides thorough coverage of
GTID_Setoperations including interval addition, membership checks, serialization, clearing, and deep copying. The assertion count (62) is properly documented with per-function breakdowns.lib/GTID_Server_Data.cpp (4)
263-265: LGTM!Clean delegation to the new
GTID_Set::has_gtid()method, replacing the previous manual iteration logic.
302-308: Good documentation.The wire format comment clearly documents the three message types (
ST=,I1=,I2=) and their formats, aiding future maintainability.
353-355: LGTM!The bootstrap parsing now correctly uses
GTID_Set::add(uuid, subtoken)which handles both single trxid and range formats internally viaTrxId_Interval(const char*)constructor.
377-381: LGTM!The I1/I2 message parsing correctly uses
GTID_Set::add()with the string token, which internally constructs aTrxId_Intervalfrom the parsed format.test/tap/tests/unit/gtid_trxid_interval_unit-t.cpp (1)
1-202: LGTM! Well-structured test coverage.The test suite comprehensively covers
TrxId_Intervaloperations:
- All constructor variants (range, single, string literal, std::string, negative values)
- Membership checks (
containsfor both trxid and interval)- Serialization (
to_string)- Mutation operations (
append,merge) with proper acceptance/rejection cases- Comparison operators
The 48 planned assertions are well-documented with per-function breakdowns.
lib/proxysql_gtid.cpp (2)
148-193: LGTM! Well-implemented interval merge logic.The
add()implementation correctly handles:
- Fast-path append for sequential additions
- Containment check to avoid duplicates
- Merge with existing intervals
- Post-merge consolidation via sort + merge pass
The algorithm maintains a sorted, non-overlapping interval list efficiently.
231-248: LGTM!The
to_string()implementation correctly formats the 32-character UUID string into the standard8-4-4-4-12hyphenated format by inserting hyphens at the appropriate positions.include/proxysql_gtid.h (2)
46-50:add(...)overload set is clean and practical.The overloads for interval/trxid/string inputs give a clear and ergonomic API surface for callers.
23-33: Apply proper const-qualification to method signatures in TrxId_Interval and GTID_Set classes.Return-type const qualifiers are redundant for value returns; query and comparison methods should be marked
constinstead.Proposed signature cleanup
- const bool contains(const TrxId_Interval& other); - const bool contains(trxid_t trxid); - const std::string to_string(void); - const bool append(const TrxId_Interval& other); - const bool merge(const TrxId_Interval& other); + bool contains(const TrxId_Interval& other) const; + bool contains(trxid_t trxid) const; + std::string to_string() const; + bool append(const TrxId_Interval& other); + bool merge(const TrxId_Interval& other); - const int cmp(const TrxId_Interval& other); - const bool operator<(const TrxId_Interval& other); - const bool operator==(const TrxId_Interval& other); - const bool operator!=(const TrxId_Interval& other); + int cmp(const TrxId_Interval& other) const; + bool operator<(const TrxId_Interval& other) const; + bool operator==(const TrxId_Interval& other) const; + bool operator!=(const TrxId_Interval& other) const; - GTID_Set copy(); + GTID_Set copy() const; - const bool has_gtid(const std::string& uuid, const trxid_t trxid); - const std::string to_string(void); + bool has_gtid(const std::string& uuid, trxid_t trxid) const; + std::string to_string() const;Also applies to: lines 43, 52–53
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 `@lib/proxysql_gtid.cpp`:
- Around line 150-155: The fast-path in GTID_Set::add() returns true when
it->second.back().append(iv) succeeds even if the tail interval's end didn't
actually change; capture the tail interval's current end before calling
it->second.back().append(iv), call append(iv), then compare the previous end to
the new end and only return true if the end changed (otherwise treat as no-op
and let the later containment check run). Update the code around
it->second.back().append(iv) to perform this pre/post-end comparison so
duplicates like [10,20], [15,20], or 20 are not reported as mutations.
- Around line 61-64: The comparisons in TrxId_Interval::append and
TrxId_Interval::merge use end+1 and start-1 which can overflow trxid_t at
int64_t limits; fix by guarding those adjacency checks: instead of computing
end+1 or start-1 unconditionally, first test if end == INT64_MAX (for end+1) or
start == INT64_MIN (for start-1) and handle those edge cases (treat as
non-adjacent or use adjusted logic), or rewrite the condition as a safe range
comparison (e.g., check other.start <= end or (end < INT64_MAX && other.start ==
end + 1)); apply the same pattern for other.end/start in merge so no signed
overflow occurs when evaluating (end+1) or (start-1).
- Around line 22-35: The TrxId_Interval constructor can leave start/end
uninitialized and dereference a null s; change
TrxId_Interval::TrxId_Interval(const char*) to validate the input string (check
s!=nullptr), initialize start and end to a safe sentinel (or set a boolean valid
flag) before parsing, and only accept the parsed interval when sscanf returns a
success (otherwise mark the interval invalid or throw); update GTID_Set::add
overloads that forward strings into TrxId_Interval to detect and propagate
parsing failure instead of creating an indeterminate object. Also fix
TrxId_Interval::append adjacency logic to avoid overflow by checking end !=
INT64_MAX before computing end+1 (treat INT64_MAX as non-adjacent or handle
specially). Ensure all callers check the interval’s validity flag or handle
exceptions accordingly.
🪄 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: f090b7ba-b1d1-4a1e-b345-56a22ec20d7e
📒 Files selected for processing (2)
lib/proxysql_gtid.cpptest/tap/tests/unit/gtid_set_unit-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
lib/proxysql_gtid.cpptest/tap/tests/unit/gtid_set_unit-t.cpp
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
lib/proxysql_gtid.cpptest/tap/tests/unit/gtid_set_unit-t.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention
Files:
lib/proxysql_gtid.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Files:
test/tap/tests/unit/gtid_set_unit-t.cpp
🧠 Learnings (8)
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
lib/proxysql_gtid.cpptest/tap/tests/unit/gtid_set_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_set_unit-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Applied to files:
test/tap/tests/unit/gtid_set_unit-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Applied to files:
test/tap/tests/unit/gtid_set_unit-t.cpp
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/gtid_set_unit-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Applied to files:
test/tap/tests/unit/gtid_set_unit-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/unit/gtid_set_unit-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/unit/gtid_set_unit-t.cpp
🪛 Clang (14.0.6)
lib/proxysql_gtid.cpp
[error] 1-1: 'cstdio' file not found
(clang-diagnostic-error)
test/tap/tests/unit/gtid_set_unit-t.cpp
[error] 14-14: 'tap.h' file not found
(clang-diagnostic-error)
- Use MySQL GTID set formatting in `GTID_Set::to_string()` - Initialize `TrxId_Interval` safely on null or invalid C-string input - Return false for no-op tail additions in `GTID_Set::add()` - Simplify `GTID_Set::copy()`` Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
wazir-ahmed
force-pushed
the
feature/gtid-range-update
branch
from
7ae0add to
a95acd0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/proxysql_gtid.cpp (1)
22-38:⚠️ Potential issue | 🟠 MajorReject malformed text ranges instead of coercing them into real intervals.
Bad input is still turned into GTID data here (
nullptr/"foo"=>[0,0],"10-"=>[10,10]), and the stringGTID_Set::add()overloads then persist it as a successful mutation. That silently corrupts the set and can later serialize invalid output like:0; parse failure needs to remain invalid all the way up the API.Also applies to: 46-47, 209-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/proxysql_gtid.cpp` around lines 22 - 38, The TrxId_Interval constructor currently coerces malformed input into [0,0] or partial intervals; change TrxId_Interval::TrxId_Interval(const char *s) to treat parse failures as invalid by (a) requiring sscanf to match the full expected format and not accepting partial matches or nullptr, and (b) signaling failure (e.g. set a new boolean member like valid = false or throw std::invalid_argument) instead of leaving start/end as 0; then update callers—particularly GTID_Set::add() overloads—to check that the TrxId_Interval is valid before persisting the interval and reject/propagate the parse error so malformed strings never become stored GTID entries.
🧹 Nitpick comments (1)
test/tap/tests/unit/gtid_set_unit-t.cpp (1)
56-65: Add a malformed-text regression next to the happy-path string tests.These cases only validate valid ranges, so they will not catch junk input being materialized as a real interval. One negative assertion here would lock down the intended
false/no-mutation behavior once the parser is fixed.🧪 Suggested regression
static void test_add_cstring_range() { GTID_Set gs; ok(gs.add(UUID_A, "9-22"), "add C string range: new range"); ok(gs.add(UUID_B, "31-50"), "add C string range: new range for UUID B"); + ok(!gs.add(UUID_A, "foo"), "add C string range: malformed input returns false"); + ok(gs.map[UUID_A].size() == 1, "add C string range: malformed input does not create a new interval"); ok(gs.map.size() == 2, "add C string range: two UUIDs"); auto it_a = gs.map[UUID_A].begin();Also bump
plan(62)toplan(64).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/unit/gtid_set_unit-t.cpp` around lines 56 - 65, Add a negative regression to test_add_cstring_range that asserts GTID_Set::add returns false and does not create a map entry when given a malformed C-string (e.g., junk or invalid range format) so malformed input is rejected; update the test's plan count from plan(62) to plan(64) accordingly. Locate the test_add_cstring_range function and add one ok(...) asserting add(UUID_A, "bad-input") == false (or that gs.map size remains unchanged / no new interval for UUID_A), and increment the overall plan() call to 64.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/proxysql_gtid.cpp`:
- Around line 22-38: The TrxId_Interval constructor currently coerces malformed
input into [0,0] or partial intervals; change
TrxId_Interval::TrxId_Interval(const char *s) to treat parse failures as invalid
by (a) requiring sscanf to match the full expected format and not accepting
partial matches or nullptr, and (b) signaling failure (e.g. set a new boolean
member like valid = false or throw std::invalid_argument) instead of leaving
start/end as 0; then update callers—particularly GTID_Set::add() overloads—to
check that the TrxId_Interval is valid before persisting the interval and
reject/propagate the parse error so malformed strings never become stored GTID
entries.
---
Nitpick comments:
In `@test/tap/tests/unit/gtid_set_unit-t.cpp`:
- Around line 56-65: Add a negative regression to test_add_cstring_range that
asserts GTID_Set::add returns false and does not create a map entry when given a
malformed C-string (e.g., junk or invalid range format) so malformed input is
rejected; update the test's plan count from plan(62) to plan(64) accordingly.
Locate the test_add_cstring_range function and add one ok(...) asserting
add(UUID_A, "bad-input") == false (or that gs.map size remains unchanged / no
new interval for UUID_A), and increment the overall plan() call to 64.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7d82495-2ec7-40ae-a032-e1327a23ddba
📒 Files selected for processing (2)
lib/proxysql_gtid.cpptest/tap/tests/unit/gtid_set_unit-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
lib/proxysql_gtid.cpptest/tap/tests/unit/gtid_set_unit-t.cpp
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
lib/proxysql_gtid.cpptest/tap/tests/unit/gtid_set_unit-t.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention
Files:
lib/proxysql_gtid.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Files:
test/tap/tests/unit/gtid_set_unit-t.cpp
🧠 Learnings (10)
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
lib/proxysql_gtid.cpptest/tap/tests/unit/gtid_set_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
lib/proxysql_gtid.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Applied to files:
lib/proxysql_gtid.cpptest/tap/tests/unit/gtid_set_unit-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:
lib/proxysql_gtid.cpptest/tap/tests/unit/gtid_set_unit-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:
lib/proxysql_gtid.cpptest/tap/tests/unit/gtid_set_unit-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:
lib/proxysql_gtid.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_set_unit-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Applied to files:
test/tap/tests/unit/gtid_set_unit-t.cpp
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/gtid_set_unit-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Applied to files:
test/tap/tests/unit/gtid_set_unit-t.cpp
🪛 Clang (14.0.6)
lib/proxysql_gtid.cpp
[error] 1-1: 'cstdio' file not found
(clang-diagnostic-error)
test/tap/tests/unit/gtid_set_unit-t.cpp
[error] 14-14: 'tap.h' file not found
(clang-diagnostic-error)
… message I1/I2 now strictly handle single trxid values (parsed via atoll). New I3/I4 messages handle range-based trxid updates (parsed as TrxId_Interval strings). This ensures backwards compatibility: an old ProxySQL receiving I3/I4 will safely ignore them via default:break, while a new ProxySQL correctly distinguishes single vs range messages. Also fixes events_read being incremented for unrecognized message types, and disconnects on unknown message types instead of silently ignoring them.
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 `@lib/GTID_Server_Data.cpp`:
- Line 318: The parser currently accepts an I4 interval and reuses uuid_server
even when no prior I1/I3 established that UUID; modify the handling in the
function that processes I4 (e.g., the method handling "I4" tokens or function
parseI4/appendInterval in GTID_Server_Data.cpp) to first check a boolean/state
set by processing I1/I3 (e.g., have_seen_I1_or_I3 or reuse_uuid_initialized) and
reject/return an error if that flag is not set; do not rely on uuid_server being
populated from ST= parsing alone, and ensure the same guard is applied to the
related code paths referenced around lines 403-405 so the code fails closed when
the first post-bootstrap range is I4.
- Line 273: GTID UUID strings must be canonicalized the same way before being
stored and looked up; update places that call gtid_executed.add(...) and
gtid_executed.has_gtid(...) (and the gtid_exists() path) to normalize the UUID
argument (e.g., remove hyphens and normalize case to match the ST= bootstrap
canonical form used by GTID_Set) before passing it to add() or has_gtid(); apply
the same normalization in both the call at the shown return line using
gtid_executed.has_gtid((std::string)gtid_uuid, gtid_trxid) and the other
affected block (around lines 388-401) so the GTID_Set lookups are consistent.
- Around line 395-411: In the '3' case handling I3 frames (in
GTID_Server_Data.cpp) guard against strchr returning nullptr: after a =
strchr(rec_msg+3, ':') check if a is NULL before computing ul or accessing a+1;
if NULL, call proxy_warning (matching the default error message with rec_msg[1],
port, address, mysql_port), set active = false and return false to avoid
UB/crash. Only when a is non-NULL compute ul, strncpy into uuid_server,
null-terminate, then call gtid_executed.add((std::string)uuid_server, a+1) and
increment events_read; similarly keep existing case '4' behavior 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: e92424c8-a750-4caf-9973-ad47e0ee9849
📒 Files selected for processing (1)
lib/GTID_Server_Data.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
lib/GTID_Server_Data.cpp
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
lib/GTID_Server_Data.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention
Files:
lib/GTID_Server_Data.cpp
🪛 Cppcheck (2.20.0)
lib/GTID_Server_Data.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (1)
lib/GTID_Server_Data.cpp (1)
84-92: Fail-closed disconnect after parser deactivation looks good.Cleaning up the watcher as soon as
dump()marks the stream inactive keeps the reader from continuing on a protocol-bad socket.
Tests read_next_gtid() by buffer-stuffing, covering: - ST= bootstrap with single trxids and ranges - I1/I2 single trxid messages - I3/I4 range-based messages - I1 rejects range input (atoll parses only first number) - Unknown message type sets active=false and returns false - read_all_gtids() stops processing on unknown message - Empty buffer and incomplete message edge cases - Mixed message sequence with full GTID state verification
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/tap/tests/unit/gtid_server_data_unit-t.cpp (1)
32-40: Make buffer helper explicitly control cached UUID reset
stuff_buffer()replaces wire bytes but leavesuuid_serveruntouched. Because I2/I4 parsing consumes cached UUID state, this creates hidden coupling unless each call site is intentional. Consider making UUID reset explicit in the helper API and opt-out only where reuse is required.Suggested refactor
-static void stuff_buffer(GTID_Server_Data &sd, const std::string &msg) { +static void stuff_buffer(GTID_Server_Data &sd, const std::string &msg, bool reset_uuid = true) { size_t needed = msg.size(); if (needed > sd.size) { sd.resize(needed); } memcpy(sd.data, msg.c_str(), needed); sd.len = needed; sd.pos = 0; + if (reset_uuid) { + memset(sd.uuid_server, 0, sizeof(sd.uuid_server)); + } }- stuff_buffer(sd, msg2); + stuff_buffer(sd, msg2, false); // I2 reuses UUID from previous I1- stuff_buffer(sd, msg2); + stuff_buffer(sd, msg2, false); // I4 reuses UUID from previous I3- stuff_buffer(sd, m2); + stuff_buffer(sd, m2, false); // I2 reuses UUID_A ... - stuff_buffer(sd, m4); + stuff_buffer(sd, m4, false); // I4 reuses UUID_B🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/unit/gtid_server_data_unit-t.cpp` around lines 32 - 40, The helper stuff_buffer currently writes new wire bytes into GTID_Server_Data but leaves the cached UUID state (uuid_server/have_uuid) intact, causing hidden coupling; change stuff_buffer to explicitly control clearing the cached UUID by adding a boolean parameter (e.g., bool reset_uuid = true) and, when true, clear or invalidate the cached UUID fields on sd (reset sd.uuid_server and/or set sd.have_uuid = false) before writing the buffer; keep an opt-out (false) for callers that intentionally reuse the cached UUID and update call sites accordingly to pass false only where reuse is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/tap/tests/unit/gtid_server_data_unit-t.cpp`:
- Around line 32-40: The helper stuff_buffer currently writes new wire bytes
into GTID_Server_Data but leaves the cached UUID state (uuid_server/have_uuid)
intact, causing hidden coupling; change stuff_buffer to explicitly control
clearing the cached UUID by adding a boolean parameter (e.g., bool reset_uuid =
true) and, when true, clear or invalidate the cached UUID fields on sd (reset
sd.uuid_server and/or set sd.have_uuid = false) before writing the buffer; keep
an opt-out (false) for callers that intentionally reuse the cached UUID and
update call sites accordingly to pass false only where reuse is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 929c2ca2-7362-45a3-937a-993c42b140bd
📒 Files selected for processing (3)
test/tap/groups/groups.jsontest/tap/tests/unit/Makefiletest/tap/tests/unit/gtid_server_data_unit-t.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- test/tap/tests/unit/Makefile
- test/tap/groups/groups.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
test/tap/tests/unit/gtid_server_data_unit-t.cpp
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
test/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 use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Files:
test/tap/tests/unit/gtid_server_data_unit-t.cpp
🧠 Learnings (4)
📚 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
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Applied to files:
test/tap/tests/unit/gtid_server_data_unit-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Applied to files:
test/tap/tests/unit/gtid_server_data_unit-t.cpp
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/gtid_server_data_unit-t.cpp
🪛 Clang (14.0.6)
test/tap/tests/unit/gtid_server_data_unit-t.cpp
[error] 14-14: 'tap.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (2)
test/tap/tests/unit/gtid_server_data_unit-t.cpp (2)
45-287: Coverage quality is strong for protocol/state transitionsNice work covering success paths, UUID-reuse semantics, unknown-message disconnect behavior,
read_all_gtids()stop conditions, and incomplete/empty buffer handling. This is a solid safety net for the GTID parser behavior.
14-17: No action required—harness integration is correct and includes follow the established pattern.The file is properly registered in
test/tap/tests/unit/Makefile(line 290) and uses the correct headers. Like the similar pure data-structure GTID tests (gtid_set_unit-t.cpp,gtid_trxid_interval_unit-t.cpp),gtid_server_data_unit-t.cppintentionally includes onlytap.hand the project header—omittingtest_globals.handtest_init.his correct for data-structure parsing tests that do not depend on ProxySQL runtime globals.
|
3 participants
Summary by CodeRabbit