Implement Fast Forward Grace Close Feature to Prevent Data Loss#5203
Implement Fast Forward Grace Close Feature to Prevent Data Loss#5203renecannao merged 9 commits intov3.0from
Conversation
Problem: In fast forward mode, ProxySQL forwards packets directly from client to backend without buffering them. If the backend connection closes unexpectedly (e.g., due to server crash, network failure, or other issues), ProxySQL immediately closes the client session. This can result in data loss because the client may have sent additional data that hasn't been fully transmitted yet, as ProxySQL does not wait for the output buffers to drain. Solution: Implement a configurable grace period for session closure in fast forward mode. When the backend closes unexpectedly, instead of closing the session immediately, ProxySQL waits for a configurable timeout (fast_forward_grace_close_ms, default 5000ms) to allow any pending client output data to be sent. During this grace period: - If the client output buffers become empty, the session closes gracefully. - If the timeout expires, the session closes anyway to prevent indefinite hanging. Changes: - Added global variable mysql_thread___fast_forward_grace_close_ms (0-3600000ms) - Added session flags: backend_closed_in_fast_forward, fast_forward_grace_start_time - Added data stream flag: defer_close_due_to_fast_forward - Modified MySQL_Data_Stream::read_from_net() to detect backend EOF and initiate grace close if client buffers are not empty - Modified MySQL_Session::handler() FAST_FORWARD case to implement grace close logic with timeout and buffer checks - Added extensive inline documentation explaining the feature and its mechanics This prevents data loss in fast forward scenarios while maintaining bounded session lifetime.
- Rename and modify test to use MySQL C API mysql_binlog_* functions - Implement throttled binlog reading with 5 iterations (no limit, 2s, 5s, 20s, 60s targets) - Add diagnostics for debugging binlog fetch issues - Set RPL options for file, position, server_id, and non-blocking flag - Update Makefile to compile with MySQL client library
Summary of ChangesHello @renecannao, 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 addresses a critical data loss scenario in ProxySQL's fast forward mode. Previously, if a backend connection closed unexpectedly while client data was still being transmitted, the session would terminate immediately, leading to incomplete data transmission. The new "Fast Forward Grace Close" feature introduces a configurable grace period. During this period, ProxySQL defers the session closure, allowing any pending client output buffers to fully drain. The session will close either when all buffers are empty or after the configured grace timeout, ensuring data integrity in high-throughput environments. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature to prevent data loss in fast-forward mode by implementing a grace period for session closure. The implementation is mostly sound, with new configuration variables, session state, and logic to handle the grace period. The inclusion of a TAP test is also a great addition. My review focuses on improving maintainability by reducing code duplication and removing leftover debug code to ensure the implementation is robust and clean for production use.
lib/MySQL_Session.cpp
Outdated
| if (mybe->server_myds->status == MYSQL_SERVER_STATUS_OFFLINE_HARD || mybe->server_myds->fd == -1) { | ||
| if (!backend_closed_in_fast_forward) { | ||
| backend_closed_in_fast_forward = true; | ||
| cerr << __FILE__ << ":" << __LINE__ << " grace_start_time from " << fast_forward_grace_start_time << " to " << thread->curtime << endl; |
There was a problem hiding this comment.
This debug print to cerr should be removed. Such statements can negatively impact performance and clutter logs in production environments, and should not be present in release code.
| if (backend_closed_in_fast_forward) { | ||
| if ( | ||
| ( mybe->server_myds == nullptr || ( mybe->server_myds && mybe->server_myds->PSarrayIN->len == 0 ) ) | ||
| && | ||
| (client_myds->PSarrayOUT->len == 0 && (client_myds->queueOUT.head - client_myds->queueOUT.tail) == 0) | ||
| ) { | ||
| // buffers empty, close | ||
| handler_ret = -1; | ||
| } else if (thread->curtime - fast_forward_grace_start_time > (unsigned long long)mysql_thread___fast_forward_grace_close_ms * 1000) { | ||
| // timeout, close | ||
| handler_ret = -1; | ||
| } | ||
| } |
There was a problem hiding this comment.
This grace period logic for checking if buffers are empty or if a timeout has occurred is very similar to the logic in MySQL_Thread::process_data_on_data_stream. Duplicating this complex logic in multiple places increases the risk of bugs and makes maintenance harder. Consider creating a single helper function within the MySQL_Session class to encapsulate this check and call it from both locations.
| if (myds_type == MYDS_BACKEND && sess && sess->session_fast_forward && ssl_recv_bytes==0) { | ||
| if (PSarrayIN->len > 0 || sess->client_myds->PSarrayOUT->len > 0 || queue_data(sess->client_myds->queueOUT) > 0) { | ||
| if (sess->backend_closed_in_fast_forward == false) { | ||
| sess->backend_closed_in_fast_forward = true; | ||
| //cerr << __FILE__ << ":" << __LINE__ << " grace_start_time from " << sess->fast_forward_grace_start_time << " to " << sess->thread->curtime << endl; | ||
| sess->fast_forward_grace_start_time = sess->thread->curtime; | ||
| sess->client_myds->defer_close_due_to_fast_forward = true; | ||
| } | ||
| //return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic to initiate the fast forward grace period is duplicated in three places within this function (for SSL EOF, non-SSL EOF, and POLLHUP). This makes the code harder to maintain. Consider refactoring this logic into a private helper function to avoid repetition.
Additionally, the commented-out //return 0; on line 597 (and in the other duplicated blocks) is confusing. If it's a remnant of a previous implementation, it should be removed to improve clarity.
| //return 0; | ||
| } | ||
| } | ||
| proxy_debug(PROXY_DEBUG_NET, 5, "Received EOF, shutting down soft socket -- Session=%p, Datastream=%p", sess, this); |
There was a problem hiding this comment.
The format string for this proxy_debug call is missing a newline character (\n) at the end. This can cause log entries to be improperly formatted or interleaved with subsequent log messages. Please add a \n to the end of the format string. A similar issue exists on line 676.
proxy_debug(PROXY_DEBUG_NET, 5, "Received EOF, shutting down soft socket -- Session=%p, Datastream=%p\n", sess, this);|
Retest this please |
Added `CREATE DATABASE IF NOT EXISTS test`
|
Summary
This pull request implements the fast forward grace close feature in ProxySQL. The feature prevents data loss by allowing pending client output buffers to drain before closing sessions when the backend closes unexpectedly during fast forward mode.
Problem
In fast forward mode, ProxySQL buffers packets to minimize latency. However, if the backend connection closes unexpectedly while client data is still being transmitted, the remaining data in the pipeline can be lost because the session terminates immediately. This can lead to incomplete data transmission and potential data loss in high-throughput scenarios.
Solution
Introduce a configurable grace period where, upon detecting an unexpected backend closure in fast forward mode, ProxySQL defers session closure to allow all pending client output buffers to drain. If the buffers empty within the grace timeout, the session closes cleanly; otherwise, it closes after the timeout to prevent indefinite hanging.
Implementation Details
New Configuration Variable
mysql_thread___fast_forward_grace_close_ms: Controls the grace period timeout (0-3600000 ms, default 5000 ms). A value of 0 disables the feature.New Flags and State
backend_closed_in_fast_forwardandfast_forward_grace_start_timeto track grace state and timer.defer_close_due_to_fast_forwardto prevent immediate closure.Core Logic Changes
Code Flow
Testing
fast_forward_grace_close.cppgenerates large binlog data on the backend, connects via ProxySQL, and reads it with throttling to trigger fast forward mode closure.test/tap/groups/groups.jsonfor inclusion in the TAP suite.test/tap/tests/Makefileto link with MySQL client library for binlog APIs.Files Changed
include/MySQL_Data_Stream.h: Addeddefer_close_due_to_fast_forwardflag.include/MySQL_Session.h: Added session flags.include/MySQL_Thread.h: Added variable declaration.include/proxysql_structs.h: Added variable extern.lib/MySQL_Session.cpp: Grace logic in FAST_FORWARD handler.lib/MySQL_Thread.cpp: Variable initialization and refresh.lib/mysql_data_stream.cpp: EOF detection and polling management.test/tap/tests/fast_forward_grace_close.cpp: New TAP test with extensive documentation.test/tap/groups/groups.json: Added test entry.test/tap/tests/Makefile: Updated for test compilation.Backwards Compatibility
Related Commits
This implementation is ready for review and addresses the data loss issue in fast forward mode while maintaining performance and compatibility.