iframe-proxy

nauu · 2026-05-14T12:16:08Z

Changelog category (leave one):

Bug Fix (user-visible misbehavior in an official stable release)

When max_memory_usage_soft_limit is not set explicitly and is auto-calculated from max_memory_usage_soft_limit_ratio, initializeKeeperMemorySoftLimit called config.setUInt64() to cache the result into the writable MapConfiguration layer(PRIO_APPLICATION = -100).

On config reload triggered by CgroupsMemoryUsageObserver(e.g., when the cgroup memory limit increases), LayeredConfiguration::replace() only swaps the file layer (PRIO_DEFAULT = 0) but leaves MapConfiguration intact.

Because MapConfiguration has higher priority, the stale startup value shadows the new file config, and updateKeeperMemorySoftLimit reads the old value instead of recalculating.

Fix: extract calculateMemorySoftLimit and remove the config.setUInt64() call, so each reload recalculates from the current config and getMemoryAmount().

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

Fixed the max_memory_usage_soft_limit was not updated on-the-fly when the configuration changed. The soft memory limit was calculated once at startup and cached, so subsequent config reloads had no effect. After this fix, the limit is recalculated and applied immediately on every config reload, no Keeper restart required.

Version info

Merged into: 26.5.1.893

clickhouse-gh · 2026-05-14T12:16:48Z

Ergus

The fix as is seems correct (LGTM), but there are a couple or corner cases/paths not covered in tests:

If we set max_memory_usage_soft_limit directly in the config then calculateMemorySoftLimit takes the hasProperty branch and there is no test for that:
We need a test that:
Sets an explicit value at startup and verifies it is respected
Changes the explicit value on reload and verifies the new value is picked up
If we change ratio at startup. After the PR's fix, this should work: the MapConfiguration is clean (as there is not call to setUInt64 now), so hasProperty("...soft_limit") is false at startup (correct ratio path), and true after reload with an explicit value (correct explicit path).
When calculateMemorySoftLimit returns 0 (i.e ratio = 0). Callers of getKeeperMemorySoftLimit() must treat 0 as "no limit".

nauu · 2026-05-14T15:35:37Z

The fix as is seems correct (LGTM), but there are a couple or corner cases/paths not covered in tests:

If we set max_memory_usage_soft_limit directly in the config then calculateMemorySoftLimit takes the hasProperty branch and there is no test for that:
We need a test that:

Sets an explicit value at startup and verifies it is respected

Changes the explicit value on reload and verifies the new value is picked up

If we change ratio at startup. After the PR's fix, this should work: the MapConfiguration is clean (as there is not call to setUInt64 now), so hasProperty("...soft_limit") is false at startup (correct ratio path), and true after reload with an explicit value (correct explicit path).
as "no limit".

I haven't change any other existing logic in this PR, however I've added more test cases

When calculateMemorySoftLimit returns 0 (i.e ratio = 0). Callers of getKeeperMemorySoftLimit() must treat 0

I thought this is guaranteed by this part

ClickHouse/src/Coordination/KeeperServer.cpp

Line 795 in b6844b2

bool KeeperServer::isExceedingMemorySoftLimit() const

nauu · 2026-05-15T04:33:35Z

The failed test 02346_text_index_queries has a known issue #104923

It is unrelated to this PR.

Ergus · 2026-05-15T12:38:10Z

I haven't change any other existing logic in this PR, however I've added more test cases

I know, but as this part is a fragile defined behavior, the extra tests simplify detectin conflicting changes in apparently unrelated parts in the future.

When calculateMemorySoftLimit returns 0 (i.e ratio = 0). Callers of getKeeperMemorySoftLimit() must treat 0

I thought this is guaranteed by this part

ClickHouse/src/Coordination/KeeperServer.cpp

Line 795 in b6844b2

bool KeeperServer::isExceedingMemorySoftLimit() const

Indeed, but like in crypto: "Don't Trust, Verify" ;)

nauu · 2026-05-15T14:40:12Z

I haven't change any other existing logic in this PR, however I've added more test cases

I know, but as this part is a fragile defined behavior, the extra tests simplify detectin conflicting changes in apparently unrelated parts in the future.

When calculateMemorySoftLimit returns 0 (i.e ratio = 0). Callers of getKeeperMemorySoftLimit() must treat 0

I thought this is guaranteed by this part

ClickHouse/src/Coordination/KeeperServer.cpp

Line 795 in b6844b2

bool KeeperServer::isExceedingMemorySoftLimit() const

Indeed, but like in crypto: "Don't Trust, Verify" ;)

Hi @Ergus thanks your reply .

In the original logic, when ratio = 0, memory_soft_limit remains 0 and setUInt64 is never called . so the behavior is identical to what we have now.

The only caller of getKeeperMemorySoftLimit() is isExceedingMemorySoftLimit, which already guards with mem_soft_limit > 0.

Could you clarify what specific test case you have in mind here? I am happy to add it if there's a concrete scenario to cover.

rschu1ze · 2026-05-18T20:29:24Z

@nauu Normal users won't be able to decipher the changelog entry Fixed Keeper max_memory_usage_soft_limit not hot updating after config reload. Please rewrite and expand.

Ergus · 2026-05-19T10:49:19Z

@rschu1ze

I would rephrase the changelog more or less like this:

Fixed on-the-fly update of soft memory limit when the configuration changes by  recalculating and applying the value on every config reload without requiring keeper restart.

Ergus · 2026-05-19T10:59:12Z


-    LOG_INFO(log, "keeper_server.max_memory_usage_soft_limit is set to {}", formatReadableSizeWithBinarySuffix(memory_soft_limit));
+void KeeperContext::initializeKeeperMemorySoftLimit(Poco::Util::AbstractConfiguration & config, Poco::Logger * log)


The name of this function is misleading now because it is not changing the memory_soft_limit

Ergus · 2026-05-19T11:25:07Z


-    LOG_INFO(log, "keeper_server.max_memory_usage_soft_limit is set to {}", formatReadableSizeWithBinarySuffix(memory_soft_limit));
+void KeeperContext::initializeKeeperMemorySoftLimit(Poco::Util::AbstractConfiguration & config, Poco::Logger * log)


Also I think that the config argument can be const now. Which requires updatign the comment in the header

Ergus · 2026-05-19T11:32:57Z

+    const UInt64 expected_reload = static_cast<UInt64>(static_cast<double>(getMemoryAmount()) * 0.5);
+    ASSERT_EQ(ctx->getKeeperMemorySoftLimit(), expected_reload);
+}
+


I would recommend to add a test for explicit value -> ratio-only config on reload.

Similar to ExplicitZeroFallsBackToRatio bit where max_memory_usage_soft_limit_ratio=0

Ergus · 2026-05-19T11:33:55Z

-            config.setUInt64("keeper_server.max_memory_usage_soft_limit", memory_soft_limit);
-        }
-    }
+    memory_soft_limit = calculateMemorySoftLimit(config);


Add a log line in this function so the actual stored value is always observable.

Ergus · 2026-05-19T11:34:40Z

+void KeeperContext::initializeKeeperMemorySoftLimit(Poco::Util::AbstractConfiguration & config, Poco::Logger * log)
+{
+    const auto limit = calculateMemorySoftLimit(config);
+    LOG_INFO(log, "keeper_server.max_memory_usage_soft_limit is set to {}", formatReadableSizeWithBinarySuffix(limit));


I think that this is the log line needed in updateKeeperMemorySoftLimit

Ergus · 2026-05-19T11:36:43Z

The failing test is unrelated.

rschu1ze · 2026-05-19T11:39:27Z

@nauu Can you please write an integration test instead? A C++ unit test seems too low level for the job.

rschu1ze · 2026-05-19T15:31:11Z

@@ -104,3 +108,61 @@ def test_soft_limit_create(started_cluster):
        return

    raise Exception("all records are inserted but no error occurs")
+
+
+def test_soft_limit_hot_reload(started_cluster):


Needs a test-level comment for the next guy who will read this test in a couple of weeks from now, basically what is the test supposed to test.

clickhouse-gh · 2026-05-19T19:44:58Z

LLVM Coverage Report

Changed lines: 56.00% (14/25) | lost baseline coverage: 4 line(s) · Uncovered code

Full report · Diff report

Ergus

I liked the unit tests, but I agree that with integration tests it is simpler to test.

Fix max_memory_usage_soft_limit hot reload

nauu added the can be tested Allows running workflows for external contributors label May 14, 2026

clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label May 14, 2026

Ergus reviewed May 14, 2026

View reviewed changes

clickhouse-gh Bot reviewed May 14, 2026

View reviewed changes

Comment thread src/Coordination/KeeperContext.cpp Outdated

Algunenano assigned Ergus May 15, 2026

alexey-milovidov mentioned this pull request May 17, 2026

Stop the bleeding in function_prop_fuzzer #105146

Merged

1 task

Ergus requested changes May 19, 2026

View reviewed changes

rschu1ze reviewed May 19, 2026

View reviewed changes

fix max_memory_usage_soft_limit hot reload

6aba3b6

nauu force-pushed the fix-keeper branch from 9c7d79a to 6aba3b6 Compare May 19, 2026 15:27

rschu1ze reviewed May 19, 2026

View reviewed changes

add test-level comment

f173997

clickhouse-gh Bot reviewed May 19, 2026

View reviewed changes

Comment thread tests/integration/test_keeper_memory_soft_limit/test.py Outdated

separate test

0c9b874

nauu force-pushed the fix-keeper branch from 81a882b to 0c9b874 Compare May 19, 2026 16:17

Ergus reviewed May 20, 2026

View reviewed changes

Comment thread tests/integration/test_keeper_memory_soft_limit_ratio/configs/keeper_config1.xml

Comment thread tests/integration/test_keeper_memory_soft_limit_ratio/configs/keeper_config1.xml

Ergus approved these changes May 20, 2026

View reviewed changes

Ergus added this pull request to the merge queue May 20, 2026

Merged via the queue into ClickHouse:master with commit b9e7652 May 20, 2026
165 checks passed

robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label May 20, 2026

DavidHe-2008 pushed a commit to DavidHe-2008/ClickHouse that referenced this pull request Jun 1, 2026

Merge pull request ClickHouse#104940 from nauu/fix-keeper

1ed77d0

Fix max_memory_usage_soft_limit hot reload


		LOG_INFO(log, "keeper_server.max_memory_usage_soft_limit is set to {}", formatReadableSizeWithBinarySuffix(memory_soft_limit));
		void KeeperContext::initializeKeeperMemorySoftLimit(Poco::Util::AbstractConfiguration & config, Poco::Logger * log)

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

nauu commented May 14, 2026 • edited by robot-clickhouse Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Version info

Uh oh!

clickhouse-gh Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Final Verdict

Uh oh!

Ergus left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nauu commented May 14, 2026

Uh oh!

nauu commented May 15, 2026

Uh oh!

Ergus commented May 15, 2026

Uh oh!

nauu commented May 15, 2026

Uh oh!

rschu1ze commented May 18, 2026

Uh oh!

Ergus commented May 19, 2026

Uh oh!

Ergus May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Ergus May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Ergus May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Ergus May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Ergus May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Ergus commented May 19, 2026

Uh oh!

rschu1ze May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rschu1ze May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clickhouse-gh Bot commented May 19, 2026

LLVM Coverage Report

Uh oh!

Ergus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nauu commented May 14, 2026 •

edited by robot-clickhouse

Loading

clickhouse-gh Bot commented May 14, 2026 •

edited

Loading

Ergus left a comment •

edited

Loading

rschu1ze May 19, 2026 •

edited

Loading