Skip to content

24.8.14 Backport of #79147 - Remove guard pages from fibers and alternative stack for signals in threads#2035

Open
mkmkme wants to merge 1 commit into
customizations/24.8.14from
backports/24.8/79147
Open

24.8.14 Backport of #79147 - Remove guard pages from fibers and alternative stack for signals in threads#2035
mkmkme wants to merge 1 commit into
customizations/24.8.14from
backports/24.8/79147

Conversation

@mkmkme

@mkmkme mkmkme commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Note for the reviewer

This PR originally changed two files and I had conflicts in both of them. One of them, FiberStack.cpp, was missing from the original 24.8 branch because it was added later.

Initially I landed the changes on FiberStack.h itself and introduced the missing memoryGuardInstall / memoryGuardRemove functions to Common/memory.h, creating Common/memory.cpp as well. This, however, broke the build for a missing jemalloc.h because FiberStack.h got an include for Common/memory.h:

In file included from /home/mk/git/altinity-clickhouse-24.8-79147/src/Core/ExternalTable.cpp:18:
In file included from /home/mk/git/altinity-clickhouse-24.8-79147/src/Core/ExternalTable.h:3:
In file included from /home/mk/git/altinity-clickhouse-24.8-79147/src/Client/Connection.h:11:
In file included from /home/mk/git/altinity-clickhouse-24.8-79147/src/IO/ReadBufferFromPocoSocketChunked.h:4:
In file included from /home/mk/git/altinity-clickhouse-24.8-79147/src/IO/ReadBufferFromPocoSocket.h:5:
In file included from /home/mk/git/altinity-clickhouse-24.8-79147/src/Common/AsyncTaskExecutor.h:5:
In file included from /home/mk/git/altinity-clickhouse-24.8-79147/src/Common/FiberStack.h:7:
/home/mk/git/altinity-clickhouse-24.8-79147/src/Common/memory.h:13:14: fatal error: 'jemalloc/jemalloc.h' file not found
   13 | #    include <jemalloc/jemalloc.h>
      |              ^~~~~~~~~~~~~~~~~~~~~
1 error generated.

Because of that, I decided to split FiberStack to a header and implementation file as it is in the upstream. When I did that, I got multiple compiler errors about missing fmt and formatReadable* functions because the files using those functions previously got those from transitive includes by FiberStack.h. After I moved most of inclusions to FiberStack.cpp, the functions became undefined.

Before adding the extra includes to Keeper.cpp, ClientBase.cpp, Connection.cpp and AsyncTaskExecutor.cpp I verified those includes existed in the upstream's versions of those files at the moment of the original PR.

Unfortunately all of this led to the situation where the PR got quite large in comparison to the original one.

Changelog category (leave one):

  • CI Fix or Improvement (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Remove guard pages for threads and async_socket_for_remote/use_hedge_requests. Change the allocation method in FiberStack from mmap to aligned_alloc. Since this splits VMAs and under heavy load vm.max_map_count can be reached (ClickHouse#79147 by @CheSema)

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Performance tests
  • Aarch64 tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • OAuth (5m)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

…ack-protection

stack allocation in Fiber and alternative stack for signals
@altinity-robot

altinity-robot commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

This is an automated comment for commit 3691792 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check nameDescriptionStatus
BuildsThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS❌ failure
Successful checks
Check nameDescriptionStatus
AST fuzzerRuns randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help✅ success
Stateful testsRuns stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc✅ success
Stateless testsRuns stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc✅ success
Stress testRuns stateless functional tests concurrently from several clients to detect concurrency-related errors✅ success

@ilejn

ilejn commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Honestly I am a bit shocked because I expected something like this https://github.com/Altinity/ClickHouse/tree/backports/24.8.14/79147_tiny
Kind of a sketch, requires minor fixes, but it is compilable.

This is a compact change, it keeps ::munmap and everything that is natural for 24.8

Anyway, the PR looks good to me.

@mkmkme

mkmkme commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Honestly I am a bit shocked because I expected something like this https://github.com/Altinity/ClickHouse/tree/backports/24.8.14/79147_tiny

This is a compact change, it keeps ::munmap and everything that is natural for 24.8

Anyway, the PR looks good to me.

I was considering that, but wanted to keep it as close to upstream as possible.
We can take your approach instead since you've done it anyway :)

@ilejn

ilejn commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

I was considering that, but wanted to keep it as close to upstream as possible. We can take your approach instead since you've done it anyway :)

We already have this upstream PR in 25.3 #917
Since this backport is in line with 917, lets go further.

@CarlosFelipeOR

CarlosFelipeOR commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

❌ QA: Build fails — needs a fix before further verification

Verdict: Not approvable in its current state. This PR does not build on either architecture, so no functional or regression testing can run yet.

Build failure

Both BuilderDebRelease (amd64) and BuilderDebAarch64 (arm64) fail identically; everything downstream (stateless, integration, regression, etc.) is SKIPPED as a result.

src/Common/AsyncTaskExecutor.h:126:1: error: unknown type name 'String'; did you mean 'std::string'?
  126 | String getSocketTimeoutExceededMessageByTimeoutType(AsyncEventTimeoutType type, Poco::Timespan timeout, const String & socket_description);

Logs: release · aarch64

Root cause

AsyncTaskExecutor.h uses String (line 126) but obtained it only transitively through FiberStack.h. This PR empties FiberStack.h (moving its includes into the new FiberStack.cpp), so AsyncTaskExecutor.h no longer sees the String alias. The <fmt/format.h> include added to AsyncTaskExecutor.cpp covers the fmt::format usage but not String — and since the declaration lives in the header, the fix belongs in the header.

Suggested fix

// src/Common/AsyncTaskExecutor.h
#include <base/types.h>   // provides the `String` alias (using String = std::string)

This keeps the header self-contained and fixes every consumer in the ExternalTable.h → Connection.h → … → AsyncTaskExecutor.h chain, not just the .cpp.

Backport review (otherwise looks good)

Differential vs upstream ClickHouse#79147: the core logic of FiberStack.cpp and ThreadStatus.cpp matches the upstream final state exactly. The larger diff is justified — it reconstructs prerequisites the 24.8 branch lacked (splitting FiberStack into .h/.cpp, backporting the memoryGuardInstall/memoryGuardRemove infra into memory.cpp).


Root-cause diagnosis and the suggested fix were produced with AI-assisted analysis (static; confirmed by inspecting the header includes, not yet by a local rebuild).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants