Accounting of temporary data on the disk, pt1 #40893

vdimir · 2022-09-01T12:27:11Z

Changelog category (leave one):

New Feature

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

Support limiting of temporary data stored on disk using settings max_temporary_data_on_disk_size_for_user/max_temporary_data_on_disk_size_for_query .

Use new interface for:

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

vdimir · 2022-09-15T10:23:04Z

@kssenii refactoring of partial merge join to use new interface is pretty tricky. Probably it's better to make it in several steps. Let's finish this PR with sorting and aggregation to have it in master, and I'll continue the rest of TODOs in separate PRs.

kssenii · 2022-09-20T13:28:05Z

src/Interpreters/TemporaryDataOnDisk.h

+
+
+/*
+ * Used to account amount of temporary data written to dicsk.


Suggested change

* Used to account amount of temporary data written to dicsk.

* Used to account amount of temporary data written to disk.

kssenii · 2022-09-20T13:35:50Z

src/Interpreters/TemporaryDataOnDisk.cpp

+    extern const int NOT_ENOUGH_SPACE;
+}
+
+void TemporaryDataOnDiskScope::deltaAlloc(int comp_delta, int uncomp_delta)


Probably compressed instead of comp would be more readable, takes a few seconds to understand what comp is.

kssenii · 2022-09-20T13:37:10Z

src/Interpreters/TemporaryDataOnDisk.cpp

+
+void TemporaryDataOnDiskScope::deltaAlloc(int comp_delta, int uncomp_delta)
+{
+    size_t current_consuption = stat.compressed_size.fetch_add(comp_delta) + comp_delta;


Why comp_delta is added twice? should it be just size_t current_consuption = stat.compressed_size.fetch_add(comp_delta)?

kssenii · 2022-09-20T13:39:01Z

src/Interpreters/TemporaryDataOnDisk.cpp

+    if (parent)
+        parent->deltaAlloc(comp_delta, uncomp_delta);
+
+    if (comp_delta > 0 && limit && current_consuption > limit)
+        throw Exception("Memory limit exceeded", ErrorCodes::MEMORY_LIMIT_EXCEEDED);


should we catch exception from parent->deltaAlloc and restore just updated stat before rethrowing?

src/Interpreters/TemporaryDataOnDisk.cpp

kssenii · 2022-09-20T15:25:32Z

src/Interpreters/Context.cpp

+                "Disk '{}' is not local and can't be used for temporary files", disk->getName());
+    }
+
+    shared->temp_data_on_disk = std::make_shared<TemporaryDataOnDiskScope>(volume, max_size);


what if shared->temp_data_on_disk was already set?

It can be set only here in setTemporaryStorage and setTemporaryStorage is called once.

src/Interpreters/Context.h

kssenii · 2022-09-20T15:31:50Z

src/Interpreters/TemporaryDataOnDisk.cpp

+        parent->deltaAlloc(comp_delta, uncomp_delta);
+
+    if (comp_delta > 0 && limit && current_consuption > limit)
+        throw Exception("Memory limit exceeded", ErrorCodes::MEMORY_LIMIT_EXCEEDED);


why exception is about memory limit if the data is stored on disk?

Changed to ErrorCodes::TOO_MANY_ROWS_OR_BYTES

kssenii · 2022-09-20T15:33:42Z

src/Interpreters/TemporaryDataOnDisk.cpp

+    if (out_writer)
+    {
+        out_writer->finalize();
+        updateAlloc(0);


did not understand why zero update alloc updateAlloc(0) is needed, looks noop.

The finalize probably can flush some buffers, so amount of bytes can be changed. Argument is amount of rows and it's a bit inconsistent (doesn't take buffered data into account), so removed num rows at all.

src/Interpreters/TemporaryDataOnDisk.cpp

kssenii · 2022-09-27T10:06:08Z

src/Disks/DiskDecorator.h

@@ -107,6 +107,13 @@ class DiskDecorator : public IDisk
    bool supportsChmod() const override { return delegate->supportsChmod(); }
    void chmod(const String & path, mode_t mode) override { delegate->chmod(path, mode); }

+    const IDisk & getNestedDisk() const


maybe getNestedDiskOrThis()?

src/Processors/Transforms/AggregatingTransform.cpp

src/Interpreters/Aggregator.h

src/Disks/DiskDecorator.h

src/Processors/Transforms/MergeSortingTransform.cpp

This reverts commit 46e4f02.

vdimir · 2022-09-30T09:27:15Z

CI failures:

Integration tests (release) [1/2] — fail: 1, passed: 1025, flaky: 0

test_disks_app_func - #41933

Integration tests (release) [2/2] — fail: 1, passed: 1139, flaky: 0

test_cluster_copier/test.py::test_copy_with_recovering - #30399

Stateless tests (aarch64) — fail: 1, passed: 4488, skipped: 19

02267_file_globs_schema_inference - known issue fixed in master

Stress test (debug) — Download script failed

requests.exceptions.HTTPError: 404 Client Error: Not Found for url: https://github.com/ClickHouse/ClickHouse/releases/download/v22.9.3.18-stable/clickhouse-common-static_22.9.3.18_amd64.deb

Stress test (msan) — Sanitizer assert (in stderr.log)

#41548

Stress test (tsan) — Backward compatibility check: OOM killer (or signal 9) in clickhouse-server.log

alexey-milovidov · 2022-10-01T00:33:17Z

src/Core/Settings.h

@@ -399,6 +399,9 @@ static constexpr UInt64 operator""_GiB(unsigned long long value)
    M(UInt64, max_network_bandwidth_for_user, 0, "The maximum speed of data exchange over the network in bytes per second for all concurrently running user queries. Zero means unlimited.", 0)\
    M(UInt64, max_network_bandwidth_for_all_users, 0, "The maximum speed of data exchange over the network in bytes per second for all concurrently running queries. Zero means unlimited.", 0) \
    \
+    M(UInt64, max_temp_data_on_disk_size_for_user, 0, "The maximum amount of data consumed by temporary files on disk in bytes for all concurrently running user queries. Zero means unlimited.", 0)\


temp -> temporary please.

Need "temporary" instead of "temp" for the following reasons:

We hate abbrs wholeheartedly.

If you abbreviate "temporary", it is difficult to remember if it should be "temp" or "tmp" or "t".

There are other settings with the full "temporary" word.

The "temp" abbreviation can be confused with the words "tempo" and "temperature" and "Temple OS".

@alexey-milovidov

Let's rename it before it is in release #41984

Also, I noticed that there's no setting for global. I'll add it. Where should it be configured? In config.xml, I suppose, not in settings? What section?

config.xml, similarly to max_memory_usage_for_server.

robot-ch-test-poll2 added the pr-feature Pull request with new product feature label Sep 1, 2022

kssenii self-assigned this Sep 1, 2022

vdimir changed the title ~~Account temoray data on disk~~ Accounting of temoray data on the disk Sep 1, 2022

vdimir force-pushed the vdimir/track-tmp-disk branch 2 times, most recently from 9692119 to 0829dc4 Compare September 1, 2022 16:04

kssenii changed the title ~~Accounting of temoray data on the disk~~ Accounting of temporary data on the disk Sep 1, 2022

vdimir force-pushed the vdimir/track-tmp-disk branch from 0b030d9 to ddfa074 Compare September 2, 2022 14:00

vdimir force-pushed the vdimir/track-tmp-disk branch from 00ff6b4 to fb1a319 Compare September 14, 2022 11:36

vdimir marked this pull request as ready for review September 15, 2022 10:20

vdimir changed the title ~~Accounting of temporary data on the disk~~ Accounting of temporary data on the disk, pt1 Sep 15, 2022

kssenii reviewed Sep 20, 2022

View reviewed changes

vdimir force-pushed the vdimir/track-tmp-disk branch 6 times, most recently from 7d526aa to e40e832 Compare September 27, 2022 08:03

kssenii reviewed Sep 27, 2022

View reviewed changes

vdimir force-pushed the vdimir/track-tmp-disk branch from e40e832 to 78251d3 Compare September 28, 2022 10:22

vdimir added 10 commits September 29, 2022 09:51

Check if tmp volume contains only local disks

367c77f

Upd ru doc for tmp_policy

3a2508a

[wip] Common interface for temporary data on disk

ac39bbb

Use abstraction for temporary data on disk in Sort and Aggregation

c0898ce

Temp data on disk: build

15c7a3b

Do not require tmp_data in Aggregator

a56a10f

Upd log message

0093752

Fix MergeSortingTransform

8587695

Fix reading block info in NativeReader with header in ctor

efe0f99

Allow to create temporaty streams on leaf TemporaryDataOnDisk

9f3f345

vdimir added 9 commits September 29, 2022 09:51

Fix typos

dd20fa3

better TemporaryDataOnDisk

0f1a7c2

wip

74d4532

Revert "wip"

6f8e8b9

This reverts commit 46e4f02.

Create reader only after the TemporaryDataOnDisk::read

cb4397d

fix after rebase

0605f6e

Handle decorator checking tmp disk type

1234b70

fix Context::shutdown

14cd2b6

fixes for TemporaryDataOnDisk

f495361

vdimir force-pushed the vdimir/track-tmp-disk branch from 78251d3 to f495361 Compare September 29, 2022 10:09

vdimir merged commit 895afde into master Sep 30, 2022

vdimir deleted the vdimir/track-tmp-disk branch September 30, 2022 09:27

alexey-milovidov mentioned this pull request Oct 1, 2022

Use MemoryTracker to track and limit disk usage for temporary data for queries. #38422

Open

alexey-milovidov reviewed Oct 1, 2022

View reviewed changes

RunningXie mentioned this pull request Mar 1, 2024

Feature grace hash join young-scott/ClickHouse#11

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accounting of temporary data on the disk, pt1 #40893

Accounting of temporary data on the disk, pt1 #40893

vdimir commented Sep 1, 2022 •

edited

Loading

vdimir commented Sep 15, 2022

kssenii Sep 20, 2022

kssenii Sep 20, 2022

kssenii Sep 20, 2022

kssenii Sep 20, 2022

kssenii Sep 20, 2022

vdimir Sep 28, 2022

kssenii Sep 20, 2022

vdimir Sep 28, 2022

kssenii Sep 20, 2022

vdimir Sep 28, 2022

kssenii Sep 27, 2022

vdimir commented Sep 30, 2022 •

edited

Loading

alexey-milovidov Oct 1, 2022

alexey-milovidov Oct 1, 2022 •

edited

Loading

vdimir Oct 1, 2022 •

edited

Loading

alexey-milovidov Oct 1, 2022



		/*
		* Used to account amount of temporary data written to dicsk.

Accounting of temporary data on the disk, pt1 #40893

Accounting of temporary data on the disk, pt1 #40893

Conversation

vdimir commented Sep 1, 2022 • edited Loading

Changelog category (leave one):

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

vdimir commented Sep 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdimir commented Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

alexey-milovidov Oct 1, 2022 • edited Loading

Choose a reason for hiding this comment

vdimir Oct 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdimir commented Sep 1, 2022 •

edited

Loading

vdimir commented Sep 30, 2022 •

edited

Loading

alexey-milovidov Oct 1, 2022 •

edited

Loading

vdimir Oct 1, 2022 •

edited

Loading