-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[DONOTMERGE] PgSQL pool starvation / worker imbalance investigation #6189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v3.0
Are you sure you want to change the base?
Changes from all commits
75fca7f
0ebd380
79d9b9a
dadb52d
a534df4
5dc7f5b
b465d37
61c7215
13a1d3b
9a57c3d
426f3aa
997e81d
ab31f05
f321fb4
4dd829e
9e254d9
331b843
007df57
ea8bb5b
575f58a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,5 +1,7 @@ | ||||||
| #include "Base_Thread.h" | ||||||
|
|
||||||
| #include <algorithm> | ||||||
|
|
||||||
| #include "cpp.h" | ||||||
|
|
||||||
| #include <unistd.h> | ||||||
|
|
@@ -40,6 +42,7 @@ | |||||
| const uint64_t nulls = partition_pool_nulls; | ||||||
| partition_pool_attempts = 0; | ||||||
| partition_pool_nulls = 0; | ||||||
| partition_pool_nulls_prev = (unsigned int)nulls; | ||||||
|
|
||||||
| // Low-volume ticks carry no signal; leave gate and streak unchanged. | ||||||
| if (attempts < PARTITION_GATE_MIN_ATTEMPTS) { | ||||||
|
|
@@ -281,7 +284,7 @@ | |||||
| * than run unconditionally on every iteration. | ||||||
| */ | ||||||
| template<typename S> | ||||||
| void Base_Thread::ProcessAllSessions_Partition() { | ||||||
|
Check failure on line 287 in lib/Base_Thread.cpp
|
||||||
| size_t running_end = 0; | ||||||
| size_t idle_begin = mysql_sessions->len; | ||||||
| size_t idx = 0; | ||||||
|
|
@@ -328,10 +331,59 @@ | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Promote the longest-waiting B session (smallest max_connect_time) to | ||||||
| // running_end so the CONNECTING_SERVER pass serves it first. Gated by a | ||||||
| // minimum B-band size to avoid churn on tiny bands. | ||||||
| if (idle_begin > running_end + PARTITION_FAIRNESS_MIN_B | ||||||
| // Order the B band oldest-first, occasionally. | ||||||
| // | ||||||
| // max_connect_time is stamped as curtime + connect_timeout_server_max when | ||||||
| // a session enters PROCESSING_QUERY, before it tries the pool -- so it is a | ||||||
| // fixed offset from when the session started waiting, and ascending order | ||||||
| // is FIFO by arrival. Band B is therefore not just "sessions connecting": | ||||||
| // with connect_timeout_server_max non-zero (default 10000) it holds every | ||||||
| // session with a pending query, including all the ones that failed a pool | ||||||
| // checkout. | ||||||
| // | ||||||
| // Promoting only the single oldest session, as the fallback below does, | ||||||
| // rescues one waiter per pass and leaves the rest in arbitrary order, which | ||||||
| // lets a subset lose the checkout race repeatedly. A full sort gives | ||||||
| // approximate FIFO across the whole band. It is rate-limited because doing | ||||||
| // it every iteration was measured at ~12% throughput loss. | ||||||
| const size_t b_len = (idle_begin > running_end) ? (idle_begin - running_end) : 0; | ||||||
| const bool sort_due = (curtime >= last_partition_sort_time + PARTITION_SORT_MIN_INTERVAL_US); | ||||||
| if (b_len > 1 && sort_due) { | ||||||
| last_partition_sort_time = curtime; | ||||||
| // Every element in [running_end, idle_begin) satisfied is_B, so | ||||||
| // mybe->server_myds is non-null and max_connect_time is non-zero. | ||||||
| auto cmp = [](void* a, void* b) { | ||||||
|
Check failure on line 355 in lib/Base_Thread.cpp
|
||||||
| return static_cast<S*>(a)->mybe->server_myds->max_connect_time | ||||||
| < static_cast<S*>(b)->mybe->server_myds->max_connect_time; | ||||||
| }; | ||||||
|
|
||||||
| // Only the front of the band can be served this pass -- the pool is | ||||||
| // far smaller than the client count, so most of the band is scrap we | ||||||
| // were never going to reach anyway. Fully sorting all of it (the | ||||||
| // std::sort this replaces) spends O(n log n) to order elements whose | ||||||
| // relative order will never be observed before the next sort. | ||||||
| // | ||||||
| // nth_element partitions the N smallest to the front in O(n) average, | ||||||
| // unordered among themselves; sorting just that front slice is | ||||||
| // O(N log N). N = 10% of the band: small enough to be cheap even at | ||||||
| // full band size, large enough that a session just past the cutoff | ||||||
| // this pass is very likely inside it on the next one. | ||||||
| void** begin = mysql_sessions->pdata + running_end; | ||||||
| void** end = mysql_sessions->pdata + idle_begin; | ||||||
| size_t top_n = b_len / 5; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For every starvation band of at least five sessions, dividing by 5 selects 20% of the band, despite the surrounding algorithm and commit description specifying a 10% prefix. This doubles the Useful? React with 👍 / 👎.
Comment on lines
+368
to
+373
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Align the comment with the sorted-prefix size. The comment states 📝 Proposed fix for the comment- // nth_element partitions the N smallest to the front in O(n) average,
- // unordered among themselves; sorting just that front slice is
- // O(N log N). N = 10% of the band: small enough to be cheap even at
- // full band size, large enough that a session just past the cutoff
- // this pass is very likely inside it on the next one.
+ // nth_element partitions the N smallest to the front in O(n) average,
+ // unordered among themselves; sorting just that front slice is
+ // O(N log N). N = 20% of the band: small enough to be cheap even at
+ // full band size, large enough that a session just past the cutoff
+ // this pass is very likely inside it on the next one.🤖 Prompt for AI AgentsThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: When the B band is sorted, Prompt for AI agents
Suggested change
|
||||||
| if (top_n < 1) top_n = 1; | ||||||
| if (top_n >= b_len) { | ||||||
| std::sort(begin, end, cmp); | ||||||
| } else { | ||||||
| void** nth = begin + top_n; | ||||||
| std::nth_element(begin, nth, end, cmp); | ||||||
| std::sort(begin, nth, cmp); | ||||||
| } | ||||||
| } | ||||||
| // Fallback when the band was not sorted this pass: promote the | ||||||
| // longest-waiting B session so the CONNECTING_SERVER pass serves it first. | ||||||
| // Gated by a minimum B-band size to avoid churn on tiny bands. | ||||||
| else if (idle_begin > running_end + PARTITION_FAIRNESS_MIN_B | ||||||
| && oldest_idx != SIZE_MAX && oldest_idx != running_end) { | ||||||
| void* p = mysql_sessions->pdata[running_end]; | ||||||
| mysql_sessions->pdata[running_end] = mysql_sessions->pdata[oldest_idx]; | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4223,7 +4223,45 @@ | |
| unsigned int w=rand_fast()%(GloMTH->num_threads); | ||
| MySQL_Thread *thr=GloMTH->mysql_threads[w].worker; | ||
| if (resume_mysql_sessions->len) { | ||
| idle_thread_assigns_sessions_to_worker_thread(thr); | ||
| // Power of two choices: sample two workers and give the whole | ||
| // batch to the less loaded one. | ||
| // | ||
| // Picking a single worker at random and moving the entire | ||
| // resume queue there is unstable. Measured on the PgSQL path | ||
| // under an equivalent workload: 1598 sessions on one worker | ||
| // and 2 on the other, at 1600 clients. The split is | ||
| // established while connections ramp up and is never | ||
| // corrected, because once sessions stop going idle the | ||
| // migration that would rebalance them stops too. | ||
| // | ||
| // A fair coin cannot fix it, and splitting the batch evenly | ||
| // between two workers was tried and did not either: a worker | ||
| // exports all of its idle sessions every loop iteration, and a | ||
| // light worker's loop is fast because poll() and session | ||
| // processing are both O(sessions), so it re-exports new | ||
| // arrivals almost immediately. Directing the whole batch at | ||
| // the lighter worker gives a restoring force rather than | ||
| // merely matching that bleed. This produced an exact 800/800 | ||
| // split where every other approach produced 1598/2. | ||
| // | ||
| // Dirty reads are deliberate: a load hint, not an invariant. | ||
| // A stale value costs at most one misdirected batch, and | ||
| // locking to read two counters would cost more than it saves. | ||
| // resume_mysql_sessions is included because those sessions are | ||
| // already promised to that worker but not yet absorbed. | ||
| unsigned int nthr = GloMTH->num_threads; | ||
| if (nthr > 1) { | ||
|
Check failure on line 4253 in lib/MySQL_Thread.cpp
|
||
| unsigned int w2 = (w + 1) % nthr; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The "two choices" sampler only samples the randomly picked worker and its array successor Prompt for AI agents |
||
| MySQL_Thread *thr2 = GloMTH->mysql_threads[w2].worker; | ||
| unsigned int load1 = thr->mysql_sessions->len | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: While workers register and resume sessions, this idle thread reads their plain, non-atomic Prompt for AI agents |
||
| + thr->myexchange.resume_mysql_sessions->len; | ||
| unsigned int load2 = thr2->mysql_sessions->len | ||
| + thr2->myexchange.resume_mysql_sessions->len; | ||
| idle_thread_assigns_sessions_to_worker_thread( | ||
| (load2 < load1) ? thr2 : thr, 0); | ||
| } else { | ||
| idle_thread_assigns_sessions_to_worker_thread(thr, 0); | ||
| } | ||
| } else { | ||
| idle_thread_check_if_worker_thread_has_unprocess_resumed_sessions_and_signal_it(thr); | ||
| } | ||
|
|
@@ -4361,14 +4399,20 @@ | |
| * | ||
| * @param thr The worker thread to which idle sessions will be assigned. | ||
| */ | ||
| void MySQL_Thread::idle_thread_assigns_sessions_to_worker_thread(MySQL_Thread *thr) { | ||
| void MySQL_Thread::idle_thread_assigns_sessions_to_worker_thread(MySQL_Thread *thr, unsigned int max_sessions) { | ||
| bool send_signal = false; | ||
| // send_signal variable will control if we need to signal or not | ||
| // the worker thread | ||
| pthread_mutex_lock(&thr->myexchange.mutex_resumes); | ||
| if (shutdown==0 && thr->shutdown==0) | ||
| if (resume_mysql_sessions->len) { | ||
| while (resume_mysql_sessions->len) { | ||
| // max_sessions == 0 means "move everything", preserving the original | ||
| // behaviour for any caller that does not care. | ||
| unsigned int to_move = resume_mysql_sessions->len; | ||
| if (max_sessions && max_sessions < to_move) { | ||
| to_move = max_sessions; | ||
| } | ||
| while (to_move--) { | ||
| MySQL_Session *mysess=(MySQL_Session *)resume_mysql_sessions->remove_index_fast(0); | ||
| thr->myexchange.resume_mysql_sessions->add(mysess); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3: The new
max_sessionsparameter is never exercised with a non-zero value: both call sites in the changed code pass0(idle_thread_assigns_sessions_to_worker_thread((load2 < load1) ? thr2 : thr, 0)andidle_thread_assigns_sessions_to_worker_thread(thr, 0)), so the cap branchif (max_sessions && max_sessions < to_move)is dead and the feature is untested. Either drop the parameter until a caller uses it, or add a test exercising the cap to avoid shipping a rounding-only API.Prompt for AI agents