Skip to content

fix: fix server level max temporary size limit#87140

Merged
vdimir merged 8 commits intoClickHouse:masterfrom
JiaQiTang98:fix-temporary-data
Sep 22, 2025
Merged

fix: fix server level max temporary size limit#87140
vdimir merged 8 commits intoClickHouse:masterfrom
JiaQiTang98:fix-temporary-data

Conversation

@JiaQiTang98
Copy link
Copy Markdown
Contributor

@JiaQiTang98 JiaQiTang98 commented Sep 16, 2025 •
edited by vdimir
Loading

Changelog category (leave one):

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

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

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Sep 16, 2025 •
edited
Loading

Workflow [PR], commit [477858c]

Summary: ✅

@JiaQiTang98 JiaQiTang98 added the can be tested Allows running workflows for external contributors label Sep 16, 2025
@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label Sep 16, 2025
@JiaQiTang98 JiaQiTang98 added bug Confirmed user-visible misbehaviour in official release pr-backport Changes, backported to release branch. Do not use manually - automated use only! pr-must-backport Pull request should be backported intentionally. Use this label with great care! and removed pr-must-backport Pull request should be backported intentionally. Use this label with great care! labels Sep 18, 2025
@JiaQiTang98 JiaQiTang98 requested a review from vdimir September 18, 2025 00:23
@vdimir vdimir self-assigned this Sep 19, 2025
@vdimir vdimir removed the pr-backport Changes, backported to release branch. Do not use manually - automated use only! label Sep 19, 2025
vdimir
vdimir approved these changes Sep 19, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@vdimir vdimir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, it's really a great catch! So basically, max_temporary_data_on_disk_size was not properly tested and barely usable. Have you checked by the chance if the metrics were correct before and are now fixed by this change?

tests/queries/0_stateless/03622_max_temporary_data_size_on_disk.config.xml
@@ -0,0 +1,35 @@
<clickhouse>
Copy link
Copy Markdown
Member

@vdimir vdimir Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it would be better to add this test case to the integration test suite at https://github.com/ClickHouse/ClickHouse/tree/master/tests/integration/test_temporary_data rather than as a stateless test that runs a separate server. However, this is fine for now - we can refactor it in a separate PR if we decide it's necessary.

src/Interpreters/TemporaryDataOnDisk.h
void freeAlloc();

TemporaryDataOnDiskScope * parent;
std::shared_ptr<TemporaryDataOnDiskScope> parent;
Copy link
Copy Markdown
Member

@vdimir vdimir Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there cases where TemporaryDataBuffer's lifetime is greater than TemporaryDataOnDiskScope? I assume the initial intent was to keep the buffer's lifetime shorter than the scope lifetime, so I'm asking just to better understanding.

Copy link
Copy Markdown
Contributor Author

@JiaQiTang98 JiaQiTang98 Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For MergeSortingTransform, its TemporaryDataOnDiskScope is created here.

ClickHouse/src/Processors/QueryPlan/SortingStep.cpp

Line 364 in 19a5a49

tmp_data_on_disk = data->childScope(CurrentMetrics::TemporaryFilesForSort);

And In MergeSortingTransform, we create tmp_stream use raw pointer of tmp_data

ClickHouse/src/Processors/Transforms/MergeSortingTransform.cpp

Line 246 in 19a5a49

TemporaryBlockStreamHolder tmp_stream(shared_header_without_constants, tmp_data.get(), reserve_size);

Subsequently, we use tmp_stream to create BufferingToFileSink. This BufferingToFileSink is destroyed in SortingTransform at a point later than the destruction of MergeSortingTransform, which causes the lifetime of TemporaryDataBuffer to exceed that of TemporaryDataOnDiskScope

ClickHouse/src/Processors/Transforms/MergeSortingTransform.cpp

Line 255 in 19a5a49

auto sink = std::make_shared<BufferingToFileSink>(shared_header_without_constants, std::move(tmp_stream), log);

And I also encountered a Fatal error here with raw pointer

@vdimir vdimir added pr-must-backport Pull request should be backported intentionally. Use this label with great care! and removed bug Confirmed user-visible misbehaviour in official release labels Sep 19, 2025
@JiaQiTang98
Copy link
Copy Markdown
Contributor Author

JiaQiTang98 commented Sep 22, 2025

Thanks for the fix, it's really a great catch! So basically, max_temporary_data_on_disk_size was not properly tested and barely usable. Have you checked by the chance if the metrics were correct before and are now fixed by this change?

Yeah, I have checked the metrics were correct after the fix

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Sep 22, 2025

@vdimir, can we use MemoryTracker for this purpose?

https://clickhouse-inc.slack.com/archives/C02F2LML5UG/p1737064859711219?thread_ts=1737058291.049439&cid=C02F2LML5UG

@vdimir vdimir added this pull request to the merge queue Sep 22, 2025
@vdimir
Copy link
Copy Markdown
Member

vdimir commented Sep 22, 2025

@vdimir, can we use MemoryTracker for this purpose?

clickhouse-inc.slack.com/archives/C02F2LML5UG/p1737064859711219?thread_ts=1737058291.049439&cid=C02F2LML5UG

@alexey-milovidov I initially tried using MemoryTracker, but found that class specifically designed for RAM accounting and more complex than what we need here. So, we decided to implement separate counters for external memory instead. We could revisit this approach, though I'm not sure if it would be worth it.

Merged via the queue into ClickHouse:master with commit 0225be9 Sep 22, 2025
123 checks passed
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Sep 22, 2025
@robot-ch-test-poll4 robot-ch-test-poll4 mentioned this pull request Sep 22, 2025
Merged
@robot-ch-test-poll4 robot-ch-test-poll4 mentioned this pull request Sep 22, 2025
Merged
robot-ch-test-poll4 added a commit that referenced this pull request Sep 22, 2025
@robot-ch-test-poll4
Merge pull request #87385 from ClickHouse/cherrypick/25.9/87140
d3e581a 
Cherry pick #87140 to 25.9: fix: fix server level max temporary size limit
robot-clickhouse added a commit that referenced this pull request Sep 22, 2025
@robot-clickhouse
Backport #87140 to 25.9: fix: fix server level max temporary size limit
41520c6
@robot-ch-test-poll4 robot-ch-test-poll4 mentioned this pull request Sep 22, 2025
Merged
robot-ch-test-poll2 added a commit that referenced this pull request Sep 22, 2025
@robot-ch-test-poll2
Merge pull request #87383 from ClickHouse/cherrypick/25.7/87140
66034ab 
Cherry pick #87140 to 25.7: fix: fix server level max temporary size limit
robot-clickhouse added a commit that referenced this pull request Sep 22, 2025
@robot-clickhouse
Backport #87140 to 25.7: fix: fix server level max temporary size limit
fce211f
@robot-ch-test-poll2 robot-ch-test-poll2 mentioned this pull request Sep 22, 2025
Merged
robot-ch-test-poll2 added a commit that referenced this pull request Sep 22, 2025
@robot-ch-test-poll2
Merge pull request #87384 from ClickHouse/cherrypick/25.8/87140
9fdbe5b 
Cherry pick #87140 to 25.8: fix: fix server level max temporary size limit
robot-clickhouse added a commit that referenced this pull request Sep 22, 2025
@robot-clickhouse
Backport #87140 to 25.8: fix: fix server level max temporary size limit
84b349f
@robot-ch-test-poll2 robot-ch-test-poll2 mentioned this pull request Sep 22, 2025
Merged
clickhouse-gh bot added a commit that referenced this pull request Sep 22, 2025
@clickhouse-gh
Merge pull request #87386 from ClickHouse/backport/25.9/87140
35d0660 
Backport #87140 to 25.9: fix: fix server level max temporary size limit
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Sep 22, 2025
@JiaQiTang98 JiaQiTang98 added v25.4-must-backport and removed pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore labels Sep 22, 2025
clickhouse-gh bot added a commit that referenced this pull request Sep 22, 2025
@clickhouse-gh
Merge pull request #87393 from ClickHouse/backport/25.7/87140
53fafdb 
Backport #87140 to 25.7: fix: fix server level max temporary size limit
clickhouse-gh bot added a commit that referenced this pull request Sep 22, 2025
@clickhouse-gh
Merge pull request #87394 from ClickHouse/backport/25.8/87140
abe8ba5 
Backport #87140 to 25.8: fix: fix server level max temporary size limit
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR labels Sep 22, 2025
azat added a commit that referenced this pull request Sep 23, 2025
@azat
Merge remote-tracking branch 'u/backport/25.9/87449' into cherrypick/…
cda044d 
…25.9/87449

* u/backport/25.9/87449:
  Backport #87426 to 25.9: Ignore only not found errors for s3_plain_rewritable (and some other S3 code)
  Backport #87231 to 25.9: Fix "Too large size passed to allocator" UB in JOIN due to mixed const and non-const blocks
  Backport #87198 to 25.9: Disable the setting by default
  Backport #87392 to 25.9: Fix EmbeddedRocksDB upgrade
  Backport #87140 to 25.9: fix: fix server level max temporary size limit
  Backport #87178 to 25.9: PR: fix LEFT/INNER ... RIGHT ... JOINS chain
  Update autogenerated version to 25.10.1.1 and contributors
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 removed the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Sep 25, 2025
robot-clickhouse-ci-1 added a commit that referenced this pull request Sep 25, 2025
@robot-clickhouse-ci-1
Merge pull request #87381 from ClickHouse/cherrypick/25.3/87140
ecfb389 
Cherry pick #87140 to 25.3: fix: fix server level max temporary size limit
robot-clickhouse added a commit that referenced this pull request Sep 25, 2025
@robot-clickhouse
Backport #87140 to 25.3: fix: fix server level max temporary size limit
998b977
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 mentioned this pull request Sep 25, 2025
Merged
robot-clickhouse-ci-1 added a commit that referenced this pull request Sep 25, 2025
@robot-clickhouse-ci-1
Merge pull request #87382 from ClickHouse/cherrypick/25.6/87140
cbc30c1 
Cherry pick #87140 to 25.6: fix: fix server level max temporary size limit
robot-clickhouse added a commit that referenced this pull request Sep 25, 2025
@robot-clickhouse
Backport #87140 to 25.6: fix: fix server level max temporary size limit
3e82034
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 mentioned this pull request Sep 25, 2025
Merged
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Sep 25, 2025
clickhouse-gh bot added a commit that referenced this pull request Sep 25, 2025
@clickhouse-gh
Merge pull request #87636 from ClickHouse/backport/25.6/87140
7c43717 
Backport #87140 to 25.6: fix: fix server level max temporary size limit
vdimir added a commit that referenced this pull request Oct 1, 2025
@vdimir
Merge pull request #87635 from ClickHouse/backport/25.3/87140
af680b5 
Backport #87140 to 25.3: fix: fix server level max temporary size limit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vdimir vdimir vdimir approved these changes

Assignees

@vdimir vdimir

Labels

can be tested Allows running workflows for external contributors pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP pr-bugfix Pull request with bugfix, not backported by default pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The statistic value of temporary data size always increases

7 participants

@JiaQiTang98 @alexey-milovidov @vdimir @robot-clickhouse @robot-ch-test-poll2 @robot-clickhouse-ci-1 @robot-clickhouse-ci-2