Skip to content

Limit number of analyze for one query, att. 2#40334

Merged
vdimir merged 9 commits intomasterfrom
vdimir/analyze-stuck-limit
Sep 1, 2022
Merged

Limit number of analyze for one query, att. 2#40334
vdimir merged 9 commits intomasterfrom
vdimir/analyze-stuck-limit

Conversation

@vdimir
Copy link
Copy Markdown
Member

@vdimir vdimir commented Aug 18, 2022

Changelog category (leave one):

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

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

  • Limit number of analyze for one query with setting max_analyze_depth

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

@robot-ch-test-poll robot-ch-test-poll added the pr-bugfix Pull request with bugfix, not backported by default label Aug 18, 2022
@vdimir vdimir mentioned this pull request Aug 18, 2022
Merged
tavplubix
tavplubix reviewed Aug 18, 2022
View reviewed changes
src/Storages/RabbitMQ/StorageRabbitMQ.cpp Outdated
Comment on lines +245 to +246
/// Since we are reusing the same context for all queries executed simultaneously, we don't want to used shared `analyze_count`
modified_context->setSetting("max_analyze_depth", 0);
Copy link
Copy Markdown
Member

@tavplubix tavplubix Aug 18, 2022

Choose a reason for hiding this comment

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

Seems like the same issue is possible with StorageNATS, but maybe we don't have enough tests to reproduce it:

ClickHouse/src/Storages/NATS/StorageNATS.cpp

Line 91 in 7a4be3b

nats_context->makeQueryContext();

cc: @kssenii

@vdimir vdimir force-pushed the vdimir/analyze-stuck-limit branch 4 times, most recently from 993fa21 to 5b412fc Compare August 26, 2022 14:24
vdimir
vdimir commented Aug 26, 2022
View reviewed changes
src/Storages/NATS/StorageNATS.cpp Outdated
Copy link
Copy Markdown
Member Author

@vdimir vdimir Aug 26, 2022

Choose a reason for hiding this comment

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

It's better to create a new context, but I'll leave it like that for now and fix it in follow-up PR because it's difficult to debug two issues simultaneously.

@vdimir vdimir force-pushed the vdimir/analyze-stuck-limit branch 2 times, most recently from 56a9421 to a3a549c Compare August 29, 2022 09:44
@vdimir
Copy link
Copy Markdown
Member Author

vdimir commented Aug 30, 2022

Stress test (address) — Cannot start clickhouse-server
https://s3.amazonaws.com/clickhouse-test-reports/40334/a3a549c35f2e4ab6185a638d1b64efd034f23a7f/stress_test__address_.html

Stress test (memory)
BaseDaemon: (version 22.9.1.1, build id: C331C93166EF6A5ED419CE3B847200F8AB69C82A) (from thread 1643) (query_id: 84500b47-c545-4599-8e42-303935c195e9) (query: select * from file('data.capnp', 'CapnProto', 'val1 char') settings format_schema='nonexist:Message') Received signal Trace/breakpoint trap (5)

#40748

Stress test (thread) — Download script failed
requests.exceptions.HTTPError: 404 Client Error: Not Found for url: https://github.com/ClickHouse/ClickHouse/releases/download/v22.8.3.13-lts/clickhouse-common-static_22.8.3.13_amd64.deb

@vdimir vdimir marked this pull request as ready for review August 30, 2022 11:32
@vdimir vdimir force-pushed the vdimir/analyze-stuck-limit branch from a3a549c to 7d8e025 Compare August 31, 2022 09:57
@vdimir vdimir merged commit f2cf7d7 into master Sep 1, 2022
@vdimir vdimir deleted the vdimir/analyze-stuck-limit branch September 1, 2022 12:40
@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Sep 11, 2022

Issue #21557 is not fixed.

@vdimir
Copy link
Copy Markdown
Member Author

vdimir commented Sep 12, 2022 •
edited
Loading

Indeed, I've rechecked and query from #21557 have a problem with not analyze, but with infinite rewrite on AST optimization stage (enable_optimize_predicate_expression).
Fix #41223 contains the same query in the test as mentioned in the issue without simplification so that we can be sure that the fix solves the case. With that patch, EXPLAIN SYNTAX still takes 30 sec in debug, (but it's reasonable, at least not infinite). And it takes 1-2 sec in release build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tavplubix tavplubix tavplubix left review comments

Assignees

No one assigned

Labels

pr-bugfix Pull request with bugfix, not backported by default

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vdimir @alexey-milovidov @tavplubix @robot-ch-test-poll