perf: session-partition gate (supersedes #5776 and #5799)#5819
Merged
Conversation
Replace ProcessAllSessions_SortingSessions with ProcessAllSessions_Partition.
The old function did an implicit one-pass swap-sort over mysql_sessions by
max_connect_time; the new one explicitly partitions sessions into three
contiguous blocks in pdata according to their backend state:
[0, running_end) A: running a query against the backend
[running_end, idle_begin) B: acquiring a backend (max_connect_time != 0)
[idle_begin, len) C: idle (no backend, or holding a conn while
parked in WAITING_CLIENT_DATA)
After the 3-way A/B/C partition, sweep block B once and promote any session whose wait_time exceeds K_WAIT * avg(wait_time) toward the front of the B band. This gives sessions that have been waiting longer than the block average a chance at the next released backend conn, reducing head-of-line starvation when many clients share few conns. - Loop 1 (partition pass) now also accumulates sum_wait and b_count over block B, where wait_time = curtime - CurrentQuery.start_time in microseconds. CurrentQuery.start_time is guaranteed non-zero for any session in B; the (st && curtime > st) guard is defensive. - Loop 2 (O(b_count)) is a one-sided in-place partition: any B session with wt > K_WAIT * avg(wt) is swapped toward running_end. The promoted group is moved together but not sorted internally; same on the non-promoted side. The next pass re-evaluates with fresh wt.
In ProcessAllSessions_Partition the two-loop K_WAIT * avg(wait_time) promotion is replaced with a single Lomuto-style sweep over the B band using max_connect_time as the ordering key. This is the same algorithm as the pre-PR ProcessAllSessions_SortingSessions, just scoped to the B band that Pass 1 has already isolated.
Add -fno-omit-frame-pointer to the optimized build flags so perf --call-graph fp produces reliable call stacks across proxysql functions. Runtime cost well under 1% in our measurements; DWARF unwinding (the only alternative without FPs) is both slower and prone to truncation / unresolved frames in production traces. Enables clean perf profiles and reliable bpftrace ustack() output without any further build-time changes.
…5792) The previous code in get_sslstatus() called ERR_clear_error() unconditionally after every SSL operation: int err = SSL_get_error(ssl, n); ERR_clear_error(); // <- always This is wrong on two axes. (1) The OpenSSL idiom is to clear the error queue *before* an SSL op so it is empty *prior to* the call; clearing *after* SSL_get_error() loses any details for the current error path and does not protect subsequent ops. (2) SSL_ERROR_NONE, SSL_ERROR_WANT_READ, SSL_ERROR_WANT_WRITE do not place anything on the queue, so clearing on those paths is pure overhead -- visible in profiles under sustained TLS workloads. Remove the unconditional clear; drain the queue only on the actual error classifications (ZERO_RETURN / SYSCALL / SSL / default) so the next SSL op on this thread starts clean. Applied symmetrically in lib/PgSQL_Data_Stream.cpp and lib/mysql_data_stream.cpp.
The per-thread cached_connections cache in PgSQL_Thread/MySQL_Thread greedily kept every released backend conn local to the thread that released it. At high client count this self-reinforced into one thread hoarding all backend conns while sibling worker threads starved on the shared pool, manifesting as a throughput cliff at ~200 clients with 2 worker threads and a 50-conn backend pool. Change push_MyConn_local() to cache only 1 release in every N (N = mysql_threads / pgsql_threads); push the rest straight to the shared HGM pool. At N=1 (single worker) the behaviour is unchanged. At N>1 the cache still amortizes the bulk of the HGM wrlock cost while ensuring released conns flow to peer threads regularly enough that the pool is never drained on one side. See #5791 for the empirical analysis. Mechanism eliminates the 2-thread cliff at c=200 and recovers ~80% throughput at c=200, ~50% at c=1000 vs the always-cache baseline.
The Lomuto-style sort of the B band (sessions waiting on a backend) by max_connect_time, added on this branch, was measured to be a net loss for throughput under sustained workloads: at 1 worker thread, 500 clients, 50-conn backend pool, 4 KB rows under SSL, removing the sort gained ~12% qps and ~20% average latency. The earlier Pass 1 (3-way partition into running / waiting / idle blocks) is kept and remains net beneficial on its own (block A is processed first in the outer loop, so end-of-query conn releases become available to block B waiters in the same iteration). If the sort is reintroduced later for tail-latency-sensitive workloads, it should be gated by an explicit starvation-age signal, not run unconditionally on every outer iteration.
…NULL ratio ProcessAllSessions_Partition (Pass 1, the 3-way A/B/C session classification) is cheap and beneficial when block B (sessions waiting for a backend) is non-empty: it lets block A finish queries and release conns to block B waiters in the same outer iteration. When no sessions are contending for backends, partition is unnecessary work. Detect the contention state with a per-worker counter pair tracked at the get_MyConn_from_pool() call site: every call increments partition_pool_attempts; calls that returned NULL (couldn't acquire, session must wait) also increment partition_pool_nulls. At the top of each process_all_sessions outer iteration, the worker computes the NULL ratio and updates a hysteresis state machine: 3 consecutive iterations with ratio >= 5% flips partition_active to true; 3 consecutive iterations below the threshold flips it back. Mechanism: * Cheapest possible signal: 1 int increment per pool acquire, 1 branch per outer iteration. * Hysteresis (3 consecutive iterations either direction) filters transient bursts without needing min-sample / asymmetric-threshold tuning. * Single threshold 5% works in both directions by virtue of the streak filter -- the ratio has to be sustained near the boundary for flapping to occur, which doesn't match any realistic workload. Empirical verification at 1 worker thread, 500c clients, 4 KB rows, 50-conn backend pool, SSL on, 120 s: 20c (no contention): 1576 tps, 13 ms avg latency, gate stays OFF 100c (some): 1487 tps, 67 ms, gate stays ON 500c (heavy): 1487 tps, 336 ms, gate stays ON The gate transitions automatically; no admin variable to learn. Applied symmetrically to MySQL_Thread / MySQL_Session and PgSQL_Thread / PgSQL_Session. See #5791 for analysis.
The two-line split of `MySrvC *mysrvc=(MySrvC *)c->parent;` into a NULL-init plus assignment landed in 5b5b937 as an unrelated debug- session artifact. Reverting to the pre-5b5b937 single-line form.
…teration; widen ratio to uint64_t Three closely related changes: * Move the four partition_* members and note_pool_attempt() from MySQL_Thread / PgSQL_Thread into Base_Thread. Eliminates 22 lines of duplicated gate logic across the two protocol implementations. * Run update_partition_gate() once per process_all_sessions outer iteration, before the sess_sort / len > 3 / partition_active check. Per-tick counters can no longer accumulate stale across ticks where partition is skipped (sess_sort off, IDLE_THREADS maintenance thread, or session count below threshold), which in 5b5b937 could cause spurious gate flips after sess_sort toggled back on. * Widen the ratio comparison to uint64_t so the multiplications cannot overflow unsigned int (reachable in combination with the staleness issue above). Function-local HI_NUM/HI_DEN/STREAK locals are promoted to named static constexpr class members (PARTITION_GATE_NULL_RATIO_NUM/DEN/STREAK). No new behavior introduced -- purely correctness fixes plus a behavior-preserving refactor.
Add PARTITION_GATE_MIN_ATTEMPTS (=4). When a tick has fewer pool acquires than this, leave both gate state and streak counter untouched. Filters the "2/2 NULL = 100% stressed" noise case without drifting the gate OFF on transient idle ticks (which would happen if low- volume ticks were treated as not-stressed). Pairs with the existing mysql_sessions->len > 3 guard at the partition call site.
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughGates a new single-pass session-partition pass with hysteresis driven by pool-attempt metrics, instruments pool lookups, limits per-thread local idle-connection caching with a 1-in-N sampler, and refines OpenSSL error-queue draining. ChangesSession Partitioning Gating and Bounded Connection Caching
Sequence DiagramsequenceDiagram
participant Client as process_all_sessions
participant Gate as update_partition_gate
participant Thread as MySQL_Thread/PgSQL_Thread
participant Partitioner as ProcessAllSessions_Partition
Client->>Gate: read & clear pool-attempt counters
Gate-->>Client: partition_active (true/false)
Thread->>Client: check sess_sort and size>threshold
alt partition_active && sess_sort && size>threshold
Thread->>Partitioner: run partition pass (single-pass in-place)
else
Thread->>Thread: skip partition path
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![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 several performance and fairness optimizations. It replaces the session sorting logic with a single-pass O(n) partitioning algorithm gated by a pool-stress hysteresis state machine. It also implements bounded local connection caching (1-in-N releases) to prevent connection hoarding and worker starvation. Additionally, OpenSSL error queue clearing is optimized to only run on actual error classifications. The reviewer feedback suggests using ERR_clear_error() instead of manually looping ERR_get_error() to clear the OpenSSL error queue, and recommends using std::swap for a more idiomatic and potentially optimized swap operation during session partitioning.
| default: | ||
| // drain the queue; any consumer that wanted the details should have read them | ||
| // before returning to this point | ||
| while (ERR_get_error()) { /* discard */ } |
There was a problem hiding this comment.
Instead of manually draining the OpenSSL error queue using a while (ERR_get_error()) loop, you can use the standard OpenSSL function ERR_clear_error(). It is more efficient and idiomatic for clearing the thread's error queue.
ERR_clear_error();| default: | ||
| // drain the queue; any consumer that wanted the details should have read them | ||
| // before returning to this point | ||
| while (ERR_get_error()) { /* discard */ } |
There was a problem hiding this comment.
Instead of manually draining the OpenSSL error queue using a while (ERR_get_error()) loop, you can use the standard OpenSSL function ERR_clear_error(). It is more efficient and idiomatic for clearing the thread's error queue.
ERR_clear_error();
Comment on lines
+298
to
+300
| void* p = mysql_sessions->pdata[idx]; | ||
| mysql_sessions->pdata[idx] = mysql_sessions->pdata[running_end]; | ||
| mysql_sessions->pdata[running_end] = p; |
There was a problem hiding this comment.
Using std::swap is more idiomatic in C++ and allows the compiler to optimize the swap operation. It also improves code readability and maintainability.
std::swap(mysql_sessions->pdata[idx], mysql_sessions->pdata[running_end]);
Comment on lines
+309
to
+311
| void* p = mysql_sessions->pdata[idx]; | ||
| mysql_sessions->pdata[idx] = mysql_sessions->pdata[idle_begin]; | ||
| mysql_sessions->pdata[idle_begin] = p; |
There was a problem hiding this comment.
Using std::swap is more idiomatic in C++ and allows the compiler to optimize the swap operation. It also improves code readability and maintainability.
std::swap(mysql_sessions->pdata[idx], mysql_sessions->pdata[idle_begin]);
Contributor
Author
…fter_free Run-with --fix from test/tap/groups/lint_groups_json.py. Unblocks the 'lint' check on PR #5819.
|
This was referenced Jun 4, 2026
This was referenced Jul 8, 2026
Closed
This was referenced Jul 8, 2026
dbnski
pushed a commit
to dbnski/proxysql
that referenced
this pull request
Jul 9, 2026
…wn#5883) ProxySQL mirroring is fire-and-forget / best-effort and carries no delivery guarantee; under load it drops a variable, sometimes large fraction of mirrored writes (measured ~25-45% loss in isolation). mysql-mirror1-t asserted an exact doubled row count, and later a tight ~20% margin, which only held while loss stayed near zero -- making the test inherently flaky. Assert instead that mirroring is functioning: the row count is strictly above the primary-only baseline (some mirrored rows landed) and no more than the fully-doubled ceiling. This still catches a completely broken mirror while tolerating best-effort loss. Verified: passes 5/5 in isolation where the exact-count assertion failed reliably. Investigation showed the loss is long-standing best-effort behaviour, not a regression -- the partition gate (sysown#5819) does not engage on this low-load path and the core mirror code is unchanged since 2017-2020.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidated successor to #5776 and #5799. Carries the pieces from both that survived measurement, drops the ones that didn't, and cleans up the duplicated state.
What's in this PR
Builds on top of the work from #5776 (Pass 1 A/B/C session partition) and #5799 (NULL-ratio gate, 1-in-N local conn cache, SSL fix, frame-pointer flag), then refactors the gate.
max_connect_time-fno-omit-frame-pointerfor usableperf --call-graph fpERR_clear_errorplacement inget_sslstatus()(#5792)get_MyConn_from_pool()NULL-ratio + 3-tick hysteresisMySQL_Thread/PgSQL_ThreadBase_Thread::update_partition_gate()— one implementation, one set of constants, called once per outer iteration.PARTITION_GATE_MIN_ATTEMPTS = 4floorEmpirical results (from #5799 — sustained 4 KB rows, SSL on, 120 s per cell)
Open items
Watchdog: 10 missed MySQL thread heartbeatsat 500 c / 50 pool sustained). Inherited here since this stack carries the same payload. Investigation in progress; will update before merge.Supersedes
Close those after this lands.
Summary by CodeRabbit
Performance Improvements
Bug Fixes