iframe-proxy

serxa · 2024-05-26T21:02:26Z

The current memory tracker has an error of up to untracked_memory_limit = 4MB per thread. Consider a ClickHouse server running with 8GB RAM and a standard max_server_memory_usage_to_ram_ratio = 0.9 server setting. If there are 1000 threads (and every thread has 800KB on average) OOM can happen before any query is stopped with MEMORY_LIMIT_EXCEEDED.

We need total untracked memory to be proportional to current memory usage regardless of the number of threads. To make it fit into 10% RAM we want an error to be less than e.g. 6%. To achieve it we need untracked_memory_limit to be updated dynamically and be proportional to current thread memory usage for threads with small memory footprint:
untracked_memory_limit = clamp(already_tracked / 16, 4KB, 4MB).

Changelog category (leave one):

Not for changelog

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

This PR was reverted

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

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

CI Settings

NOTE: If your merge the PR with modified CI you MUST KNOW what you are doing
NOTE: Checked options will be applied if set before CI RunConfig/PrepareRunConfig step

do not test (only style check)
disable merge-commit (no merge from master before tests)
disable CI cache (job reuse)
allow: batch 1 for multi-batch jobs
allow: batch 2
allow: batch 3
allow: batch 4, 5 and 6

robot-clickhouse-ci-1 · 2024-05-26T21:04:57Z

This is an automated comment for commit 2c193a7 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 name	Description	Status
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	⏳ pending

Successful checks

Check name	Description	Status
A Sync	If it fails, ask a maintainer for help	✅ success
ClickHouse build check	Builds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process	✅ success
Docs check	Builds and tests the documentation	✅ success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	✅ success
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integration tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	✅ success
Mergeable Check	Checks if all other necessary checks are successful	✅ success
PR Check	Checks correctness of the PR's body	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Style check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success
Unit tests	Runs the unit tests for different release types	✅ success

…ents and tests)

…e into precise-memtracking

… test

serxa · 2024-05-31T12:54:31Z

I'm not sure what's wrong with the Upgrade check. Does anyone have an idea? I've added a new setting to the history and it does not show up, but the check is still failing.

Changed settings are not reflected in settings changes history (see changed_settings.txt) - FAIL

   ┌─name─────────────────────────────────────┬─new_value─┬─old_value─┐
1. │ enable_vertical_final                    │ 0         │ 1         │
2. │ output_format_parquet_use_custom_encoder │ 1         │ 0         │
   └──────────────────────────────────────────┴───────────┴───────────┘

New settings are not reflected in settings changes history (see new_settings.txt) - FAIL

    ┌─name──────────────────────────────────────────┐
 1. │ azure_max_blocks_in_multipart_upload          │
 2. │ allow_experimental_join_condition             │
 3. │ prefer_external_sort_block_bytes              │
 4. │ cross_join_min_rows_to_compress               │
 5. │ cross_join_min_bytes_to_compress              │
 6. │ cast_string_to_dynamic_use_inference          │
 7. │ allow_experimental_dynamic_type               │
 8. │ allow_deprecated_error_prone_window_functions │
 9. │ input_format_force_null_for_omitted_fields    │
10. │ input_format_tsv_crlf_end_of_line             │
    └───────────────────────────────────────────────┘

serxa · 2024-06-03T13:30:10Z

Okay, merge master and CI tests rerun fixed the problem. Tests 01676_clickhouse_client_autocomplete and 02456_progress_tty seem to be flaky. I think this PR is now ready to be merged.

Algunenano · 2024-06-07T11:43:49Z

This has broken test_fetch_memory_usage/test.py::test_huge_column -> https://s3.amazonaws.com/clickhouse-test-reports/0/9ec1306016568a0a89f6ac2b93face43481d04c4/integration_tests__asan__old_analyzer__[6_6].html

Algunenano · 2024-06-07T11:46:57Z

…ory_limit is constant

…ed_memory_limit is constant

The current memory tracker has an error of up to `untracked_memory_limit = 4MB` per thread. Consider a ClickHouse server running with 8GB RAM and a standard `max_server_memory_usage_to_ram_ratio = 0.9` server setting. If there are 1000 threads (and every thread has 800KB on average) OOM can happen before any query is stopped with `MEMORY_LIMIT_EXCEEDED`. We need total untracked memory to be proportional to current memory usage regardless of the number of threads. To make it fit into 10% RAM we want an error to be less than e.g. 6%. To achieve it we need `untracked_memory_limit` to be updated dynamically and be proportional to current thread memory usage for threads with small memory footprint: `untracked_memory_limit = clamp(already_tracked / 16, 4KB, 4MB)` Resubmit of: ClickHouse#64423 Co-authored-by: Azat Khuzhin <a3at.mail@gmail.com>

add dynamic untracked memory limits for more precise memory tracking

cea82aa

robot-clickhouse-ci-1 added the pr-feature Pull request with new product feature label May 26, 2024

serxa and others added 10 commits May 27, 2024 10:44

fix tests

e0c8ae8

fix

54735e6

disable precise memory tracking for some tests

c973add

Automatic style fix

18dce41

turn off dynamic untracked limit memory for not-attached threads (cli…

1c9f4da

…ents and tests)

Merge branch 'precise-memtracking' of github.com:ClickHouse/ClickHous…

d7a9760

…e into precise-memtracking

fix syntax error

d07c646

disable precise memory tracking to avoid memory_exceeded exception in…

28e71af

… test

Automatic style fix

c083896

add hysteresis of untracked memory

47b45fd

serxa marked this pull request as ready for review May 31, 2024 08:47

serxa added 3 commits May 31, 2024 09:11

Merge branch 'master' into precise-memtracking

b1de84e

adjust settings history changes

3f74783

Merge branch 'master' into precise-memtracking

6375f3a

divanik self-assigned this May 31, 2024

Merge branch 'master' into precise-memtracking

e06c4a8

serxa requested a review from divanik June 3, 2024 13:30