Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove ThreadStatus::untracked_memory_limit_increase #40321

Merged

Conversation

azat
Copy link
Collaborator

@azat azat commented Aug 17, 2022

It looks useless nowadays, because operator new cannot throw
MEMORY_LIMIT_EXCEEDED today, and so any code that works on Exception is
likely safe.

Refs: #40249 (cc @KochetovNicolai )
Refs: #24483 (cc @kitaisreal @alexey-milovidov )

Will mark as draft until #40424 will not be resolved

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

@robot-clickhouse robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Aug 17, 2022
@azat
Copy link
Collaborator Author

azat commented Aug 19, 2022

Stress test (debug) — Killed by signal (in clickhouse-server.log)

Integration tests (asan) [2/3] — fail: 1, passed: 695, flaky: 0

Memory limit (for query) exceeded: would use 4.00 MiB (attempt to allocate chunk of 4194364 bytes), maximum: 1000.00 B

Stateless tests (ubsan) — Tests are not finished, fail: 1, passed: 55, skipped: 1

2022-08-17 18:11:14 02355_column_type_name_lc:                                              [ FAIL ] 0.00 sec. - server died
2022-08-17 18:11:14 Server does not respond to health check
2022.08.17 18:10:44.508670 [ 2921 ] {1b92236c-c8de-4c03-b2c7-22667075537c} <Debug> executeQuery: (from [::1]:34604) SELECT 'Running test stateless/02355_column_type_name_lc.sql from pid=898'  (stage: Complete)
...
2022.08.17 18:11:25.764928 [ 2921 ] {1b92236c-c8de-4c03-b2c7-22667075537c} <Trace> ContextAccess (default): Access granted: SELECT(dummy) ON system.one

Timeout was 30 seconds, sadly but this was under UBSan, and query profiler is not available there, but in logs there are messages about flushing system tables, and I'm pretty sure the the IO is culprit

It looks useless nowadays, because operator new cannot throw
MEMORY_LIMIT_EXCEEDED today, and so any code that works on Exception is
likely safe.

Refs: ClickHouse#40249
Refs: ClickHouse#24483
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
@azat azat force-pushed the mem/untracked_memory_limit_increase branch from 2d90bc1 to ec2e671 Compare August 22, 2022 10:26
@azat azat marked this pull request as ready for review August 22, 2022 10:26
@azat
Copy link
Collaborator Author

azat commented Aug 23, 2022

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

Indeed an OOM (and dmesg works):

[Mon Aug 22 17:40:27 2022] Out of memory: Killed process 109666 (clickhouse-serv) total-vm:100680400kB, anon-rss:65557992kB, file-rss:231920kB, shmem-rss:0kB, UID:0 pgtables:161300kB oom_score_adj:0

Even though this is BC check, it includes #40249, let's see the logs:

2022.08.22 17:40:19.892916 [ 123209 ] {} <Error> ServerErrorHandler: Code: 241. DB::Exception: Memory limit (total) exceeded: would use 62.74 GiB (attempt to allocate chunk of 1048591 bytes), maximum: 51.44 GiB. OvercommitTracker decision: Memory overcommit isn't used. OvercommitTracker isn't set. (MEMORY_LIMIT_EXCEEDED), Stack trace (when copying this message, always include the lines below):
2022.08.22 17:40:20.509908 [ 103714 ] {} <Trace> AsynchronousMetrics: MemoryTracking: was 62.76 GiB, peak 62.76 GiB, will set to 62.76 GiB (RSS), difference: -9.85 MiB
WITH toTimeZone(event_time, 'Europe/Moscow') AS dt
SELECT
    arrayStringConcat(arrayMap(x -> demangle(addressToSymbol(x)), trace), '\n') AS sym,
    formatReadableSize(size) AS size_,
    trace_type,
    thread_id,
    query_id
FROM system.trace_log
WHERE (revision = /* get only the traces from the previous server, from https://github.com/ClickHouse/ClickHouse/commit/09a2ff88435f79e5279745bbe1dc0e5e401df38d */ revision()) AND (trace_type = 'MemoryPeak') AND (size > (/* max_server_memory_usage= */ 55228753920 + /* 100MB */ 100000000000.))
ORDER BY dt DESC
LIMIT 10

Query id: 4dd67317-8951-4fa1-b952-df00a4c1e823

Ok.

As usual, zero allocations after max_server_memory_usage had been reached.
Have few ideas, though none of I would not call perspective, will take a look more.

@alexey-milovidov alexey-milovidov self-assigned this Sep 4, 2022
@alexey-milovidov alexey-milovidov merged commit dbca269 into ClickHouse:master Sep 4, 2022
@azat azat deleted the mem/untracked_memory_limit_increase branch September 4, 2022 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-not-for-changelog This PR should not be mentioned in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants