fix: fix server level max temporary size limit

[JiaQiTang98]

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):

• Fixed incorrect accounting of temporary data deallocations in max_temporary_data_on_disk_size limit tracking, close The statistic value of temporary data size always increases #87118

[clickhouse-gh]

Workflow [PR], commit [477858c]

Summary: ✅

[vdimir]

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?

[vdimir]

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.

[vdimir]

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.

[JiaQiTang98]

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

[JiaQiTang98]

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]

@vdimir, can we use MemoryTracker for this purpose?

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

[vdimir]

@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.