Fix jemalloc verifySetup on builds without background_thread mallctl#104330
Conversation
|
Workflow [PR], commit [8196609] Summary: ✅ AI ReviewSummaryThis PR hardens jemalloc handling on builds where optional mallctls are unavailable by introducing fallible reads ( Final VerdictStatus: ✅ Approve |
Per review (ClickHouse#104330): collapse the je_mallctl boilerplate by implementing getValue on top of tryGetValue, and chassert that the read succeeded. This makes the contracts explicit: - tryGetValue is the fallible API for optional mallctls. - getValue means "this mallctl must exist; failing is a programming error" and trips chassert in debug if violated. Drop the now-incompatible "GetValueReturnsZeroOnUnknownMallctl" gtest since calling getValue on a missing mallctl is no longer defined. The remaining tests still cover the regression from ClickHouse#102183.
Previously implemented Jemalloc::getValue on top of tryGetValue and chasserted that the read succeeded. CI for PR ClickHouse#104330 showed that this contract aborts the server at startup: AsynchronousMetrics::update reads jemalloc stats via getValue, and on builds where any individual mallctl is unavailable (e.g. background-thread mallctls on macOS, or any future optional name) the chassert fires inside saveJemallocMetricImpl and debug builds abort with signal 6. Switch back to a non-fatal contract that better matches the call sites: - getValue returns a zero-initialized value when je_mallctl fails. AsynchronousMetrics will report 0 for an unreadable metric instead of aborting the entire server. - tryGetValue stays the explicit API for callers that must distinguish "missing" from "present" (verifySetup already uses it). - MibCache::getValue zero-initializes its local for the same reason, so a failing je_mallctlbymib no longer returns uninitialized memory. Restore the gtest pinning the zero-default contract for getValue, which was dropped in 2da2008 when that contract was briefly undefined. Closes ClickHouse#102183.
| size_t value_size = sizeof(T); | ||
| je_mallctl(name, &value, &value_size, nullptr, 0); | ||
| T value{}; | ||
| tryGetValue(name, value); |
There was a problem hiding this comment.
| tryGetValue(name, value); | |
| [[maybe_unused]] auto success = tryGetValue(name, value); | |
| chassert(success, fmt::format("Failed to get jemalloc value for '{}'", name)); |
There was a problem hiding this comment.
They should be converted to tryGetValue and not add value to async metrics if they don't exist. Otherwise we are pushing random values there.
Jemalloc verifySetup compared getValue for background_thread and max_background_threads against server settings. On Darwin jemalloc is built without JEMALLOC_BACKGROUND_THREAD, so je_mallctl returns ENOENT and the old code read uninitialized memory, tripping chassert in debug. Skip those checks when the mallctl is unavailable; zero-initialize getValue on failure. Closes ClickHouse#102183.
…thread Covers the regression from issue ClickHouse#102183: on builds without JEMALLOC_BACKGROUND_THREAD (e.g. macOS), je_mallctl for "background_thread" returns ENOENT. The new gtest exercises three guarantees from the fix: - tryGetValue reports failure for an unknown mallctl and does not write through the output pointer. - getValue returns a zero-initialized value when the mallctl is missing. - verifySetup does not abort or warn when the only potential mismatch comes from optional background-thread mallctls that are absent on the running jemalloc build.
Per review (ClickHouse#104330): collapse the je_mallctl boilerplate by implementing getValue on top of tryGetValue, and chassert that the read succeeded. This makes the contracts explicit: - tryGetValue is the fallible API for optional mallctls. - getValue means "this mallctl must exist; failing is a programming error" and trips chassert in debug if violated. Drop the now-incompatible "GetValueReturnsZeroOnUnknownMallctl" gtest since calling getValue on a missing mallctl is no longer defined. The remaining tests still cover the regression from ClickHouse#102183.
Previously implemented Jemalloc::getValue on top of tryGetValue and chasserted that the read succeeded. CI for PR ClickHouse#104330 showed that this contract aborts the server at startup: AsynchronousMetrics::update reads jemalloc stats via getValue, and on builds where any individual mallctl is unavailable (e.g. background-thread mallctls on macOS, or any future optional name) the chassert fires inside saveJemallocMetricImpl and debug builds abort with signal 6. Switch back to a non-fatal contract that better matches the call sites: - getValue returns a zero-initialized value when je_mallctl fails. AsynchronousMetrics will report 0 for an unreadable metric instead of aborting the entire server. - tryGetValue stays the explicit API for callers that must distinguish "missing" from "present" (verifySetup already uses it). - MibCache::getValue zero-initializes its local for the same reason, so a failing je_mallctlbymib no longer returns uninitialized memory. Restore the gtest pinning the zero-default contract for getValue, which was dropped in 2da2008 when that contract was briefly undefined. Closes ClickHouse#102183.
saveJemallocMetricImpl now uses Jemalloc::tryGetValue and only inserts into AsynchronousMetricValues when je_mallctl succeeds. Optional or build-dependent stats (e.g. background_thread.* on macOS) no longer appear as misleading zeros. jemalloc.epoch is only published when epoch can be read without forcing an update (same tryGetValue rule). MergeTree arena derived bytes metrics are emitted only when both pactive and pdirty reads succeed.
chassert when je_mallctl fails, so absent mallctl names are only used where call sites explicitly tolerate failure. Use tryGetValue for best-effort reads that must not abort debug builds: MemoryTracker (prof.active, opt.prof_prefix when flushing profiles), MemoryWorker (arenas.narenas before per-arena dirty_decay_ms), and KeeperJemallocStatusHandler (opt.prof, prof.active, prof.lg_sample for JSON). Remove the try/catch around the old getValue path there because getValue does not throw. AsynchronousMetrics jemalloc stats already used tryGetValue. Drop the gtest that expected getValue to return zero for a bogus mallctl; that conflicts with the strict getValue contract.
02cb141 to
758c206
Compare
…alue, so a failed tryGetValue still produced an engaged optional with value- initialized data (false/0) and never appended to errors. On failure, return std::nullopt and add the mallctl name to the JSON errors array so /jemalloc/status does not report fake profiling state and clients can see which mallctls failed.
…o fix-jemalloc-verify-setup-darwin
MibCache::getValue ignored the je_mallctlbymib return code, so after the
previous value{} zero-initialization a failed MIB read silently returned
0/false (e.g. /jemalloc/status reporting thread_active_init=false on
builds without prof support).
Track the MIB validity from je_mallctlnametomib at construction, add a
fallible MibCache::tryGetValue, and make MibCache::getValue chassert on
read failure to match the free-function Jemalloc::getValue contract.
KeeperJemallocStatusHandler switches to tryGetValue for prof.thread_active_init
so an absent mallctl is surfaced via the JSON "errors" array instead of
asserting in debug.
…o fix-jemalloc-verify-setup-darwin
`prof.lg_sample`; reading them through the strict `MibCache::getValue` / `Jemalloc::getValue` introduced earlier in PR ClickHouse#104330 chasserts during debug-build startup before the existing `background_thread` guards are reached. Match the `background_thread` pattern: read with `tryGetValue` and only compare against the expected value when the mallctl exists. setup() is already symmetric (the corresponding writes are no-ops when the mallctls are absent), so skipping verification is the correct behavior rather than a workaround.
Expose `FailPointInjection::hasAnyFailPointBeenRegistered` so hot paths can reuse the existing libfiu relaxed-atomic guard before doing local failpoint-specific checks. Use it in `QueryMetricLog::finishQuery` to skip the test-only query-id prefix comparison when no failpoint has ever been enabled in the process. CI: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=104330&sha=d53d6108634f5740d1ff713e32213dbec4267213&name_0=PR&name_1=Stateless%20tests%20%28amd_tsan%2C%20parallel%2C%201%2F2%29 PR: #104330
A late periodic `QueryMetricLog::collectMetric` can update `last_collect_time` past the `BlockIO::onFinish` finish timestamp before `QueryMetricLog::finishQuery` emits the final row. Do not apply periodic timestamp deduplication to final rows. Update `03203_system_query_metric_log` to check that a `system.query_metric_log` row exists at the `system.query_log` `QueryFinish` timestamp, instead of requiring that timestamp to be the maximum metric timestamp. Add a failpoint regression test for the race. CI: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=104330&sha=d53d6108634f5740d1ff713e32213dbec4267213&name_0=PR&name_1=Stateless%20tests%20%28amd_tsan%2C%20parallel%2C%201%2F2%29 PR: ClickHouse#104330
|
@sayondeep can you address the last 2 comments from reviewer bot? |
…of.* The strict MibCache::getValue / Jemalloc::getValue contract introduced earlier in PR ClickHouse#104330 leaves several optional `prof.*` reads aborting debug builds on jemalloc builds without JEMALLOC_PROF: - `setProfileSamplingRate` reads `prof.lg_sample` (reachable from non-default `jemalloc_profiler_sampling_rate` config in `setup`). - `ThreadStatus::detachFromGroup` reads `prof.thread_active_init` (reachable from query-level `jemalloc_enable_profiler=1`). - The gtest `VerifySetupNoAbortWhenBackgroundThreadMallctlMissing` unconditionally reads `prof.thread_active_init`, so the test itself asserts on the exact build profile it is meant to cover. Route all three through tryGetValue: treat a missing mallctl as a no-op (the matching writes in `setup` are already no-ops), and let the test fall back to `default_enable_global_profiler` when prof is absent.
…o fix-jemalloc-verify-setup-darwin
|
This was fixed by #105146. Let's update the branch. |
…o fix-jemalloc-verify-setup-darwin
LLVM Coverage ReportChanged lines: 78.15% (118/151) | lost baseline coverage: 3 line(s) · Uncovered code |

Motivation
On macOS (and any jemalloc build without
JEMALLOC_BACKGROUND_THREAD),je_mallctlforbackground_threadandmax_background_threadsreturnsENOENT.Jemalloc::setupalready no-ops for those writes, butJemalloc::verifySetupstill read viagetValueand compared the result toServerSettings, using uninitialized memory when the read failed. That produced a falsejemalloc_enable_background_threadsmismatch and aborted debug builds (see #102183).Changes
tryGetValueand use it inverifySetupso background-thread mallctls are only checked whenje_mallctlsucceeds.getValuewhen mallctl fails.Changelog category:
Changelog entry:
Fix a fatal logical error at server startup on macOS (and similar jemalloc builds) where
Jemalloc::verifySetupincorrectly reported ajemalloc_enable_background_threadsmismatch because optionalbackground_thread/max_background_threadsmallctls are absent; verification is skipped when those mallctls are unavailable, andgetValueno longer leaves the output uninitialized on failure. Closes #102183.Documentation entry for user-facing changes