refactor: Extract GTID interval logic into Gtid_Interval class
#5224
Conversation
Summary of ChangesHello @proton-lisandro-pin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces foundational changes to ProxySQL's GTID management system, specifically to enable support for ranged GTID updates. It redefines how GTID intervals are represented and manipulated internally, moving from basic pairs to a more robust Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Question: none of the files in I'll be happy to add tests for |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant changes to support ranged GTID updates by refactoring gtid_interval_t from a std::pair into a dedicated Gtid_Interval class and reworking the addGtid logic. The changes are well-structured and lay a good foundation for parsing GTID ranges. My review focuses on improving the C++ implementation of the new Gtid_Interval class and the associated functions, with suggestions for enhancing const correctness, readability, and adherence to C++ best practices. These improvements will make the new code more robust and easier to maintain.
proton-lisandro-pin
force-pushed
the
ranged_gtid_updates
branch
2 times, most recently
from
dd126a2 to
b1a781c
Compare
proton-lisandro-pin
force-pushed
the
ranged_gtid_updates
branch
9 times, most recently
from
c8ac908 to
93dfdf8
Compare
proton-lisandro-pin
force-pushed
the
ranged_gtid_updates
branch
2 times, most recently
from
fda6306 to
893fc40
Compare
wazir-ahmed
left a comment
wazir-ahmed
left a comment
There was a problem hiding this comment.
Looks good overall. Just a few minor comments to address.
lib/GTID_Server_Data.cpp
Outdated
| // merge overlapping GTID ranges, if any | ||
| it->second.sort(); | ||
| auto a = it->second.begin(); | ||
| while (a != it->second.end()) { | ||
| auto b = std::next(a); | ||
| if (b == it->second.end()) { | ||
| break; | ||
| } | ||
| if (a->merge(*b)) { | ||
| it->second.erase(b); | ||
| continue; | ||
| } | ||
| a++; | ||
| } |
There was a problem hiding this comment.
Performing sort+merge outside this function scope, after inserting all intervals, perhaps in read_all_gtids(), would be more efficient. WDYT?
There was a problem hiding this comment.
The sort+merge is necessary to efficiently handle GTID gaps, and i was planning to have this logic encapsulated in gtid_interval_t once it gets converted to a class 😞
However, i just realized that most GTID ranges added will extend the last interval in the list, as GTID updates are streamed in sequence, so we can optimize. Fixed to avoid sorting+merging when that's the case.
There was a problem hiding this comment.
With this patch gtid_interval_t has an merge method which does the job of extending an existing GTID. But this can still lead to creation of adjacent intervals, such as 1-5 and 6-10. We need the run the sort+merge to combine intervals like this. But the question is, how frequent we need to this operation?
- If GTIDs are mostly streamed in sequence, then calling
mergemethod inaddGtid()will take care of that and extent the existing interval. - We only need to run
sort+mergeto cleanup the formation of adjacent intervals and we don't need to that during insertion of each interval (addGtid).
I'm suggesting, read_all_gtids() would be good place for this, especially considering the batch update feature in mysqlbinlog reader. For each read(), which much contains n number of txid (or in future txid intervals), we can run sort+merge once after inserting all txid.
class.
There was a problem hiding this comment.
i was planning to have this logic encapsulated in
gtid_interval_tonce it gets converted to a class
We should put sort+merge in gtid_set_t once it gets converted into a class.
There was a problem hiding this comment.
With this patch gtid_interval_t has an merge method which does the job of extending an existing GTID. But this can still lead to creation of adjacent intervals, such as 1-5 and 6-10. We need the run the sort+merge to combine intervals like this.
No, merge actually merges intervals 😄 So 6-10 merged with 1-5 will correctly yield 1-10, and compact the range.
I have unit tests showcasing this, but i'm not quite sure where to contribute them. ProxySQL doesn't seem to have a lot of unit coverage.
But the question is, how frequent we need to this operation?
Not often, in practice - in my testing GTIDs streamed to ProxySQL come always mostly ordered. The problem is that, even when rare, the existing implementation will not deal with gaps or out-of-order updates, and quickly baloon memory usage in such scenarios.
The overhead added by this sort+merge approach is minimal and guarantees optimal GTID management, which is worth the added complexity IMHO.
I'm suggesting, read_all_gtids() would be good place for this, especially considering the batch update feature in mysqlbinlog reader. For each read(), which much contains n number of txid (or in future txid intervals), we can run sort+merge once after inserting all txid.
class.
(...)
We should put sort+merge in gtid_set_t once it gets converted into a class.
Still WIP, but proton-lisandro-pin@a2cfdac should explain the intended approach better.
The end result is that GTID management concerns are decoupled from the code ingesting GTID updates (see f.ex. line 334) from the mysqlbinlog client.
There was a problem hiding this comment.
No, merge actually merges intervals 😄 So 6-10 merged with 1-5 will correctly yield 1-10, and compact the range.
Calling merge for each individual message from mysqlbinlog is not enough. If ProxySQL receives, txid in the following sequence for example, it would lead to formation of adjacent intervals.
I2=1 // 1-1
I2=6 // 1-1, 6-6
I2=7 // 1-1, 6-7
I2=2 // 1-2, 6-7
I2=3 // 1-3, 6-7
I2=4 // 1-4, 6-7
I2=8 // 1-4, 6-8
I2=9 // 1-4, 6-9
I2=10 // 1-4, 6-10
I2=5 // 1-5, 6-10
We need the run the sort+merge logic to combine intervals like this. This is what I was describing before.
There was a problem hiding this comment.
the existing implementation will not deal with gaps or out-of-order updates, and quickly baloon memory usage in such scenarios.
- The
forloop at the end of existingaddGtid()function is intended to detect the intervals that are adjacent and merge them. It would convert{ 1-5, 6-10 }to{ 1-10 }. - But it is not prefect. For example, if ProxySQL receives
I2=6beforeI2=1, then that would lead to the formation of intervals{ 6-10, 1-5 }which won't be considered as adjacent intervals by the current implementation. So sorting is necessary before iterating through the intervals (your implementation fixes this).
proton-lisandro-pin
force-pushed
the
ranged_gtid_updates
branch
from
893fc40 to
979dfeb
Compare
|
|
@proton-lisandro-pin please resolve the conflicts. |
wazir-ahmed
changed the title
Gtid_Interval class
3 participants
Initial work to support ranged GTID updates from proxysql_mysqlbinlog.
This PR reworks
gtid_interval_tso GTID intervals can be compared/merged, and modifies the logic to add new GTIDs togtid_set_tso it understand range updates. Notably, it will automatically de-dupe and compact overlapping ranges,when possible.
A PR to properly parse ranges on
I1=andI2=GTID update messages will follow; until then, this PR changesno functionality within ProxySQL.
See sysown/proxysql_mysqlbinlog#33 for details.