Skip to content

Limit number of analyze for one query#38185

Merged
alexey-milovidov merged 8 commits intoClickHouse:masterfrom
vdimir:analyze_stuck
Jul 31, 2022
Merged

Limit number of analyze for one query#38185
alexey-milovidov merged 8 commits intoClickHouse:masterfrom
vdimir:analyze_stuck

Conversation

@vdimir
Copy link
Copy Markdown
Member

@vdimir vdimir commented Jun 17, 2022 •
edited by alexey-milovidov
Loading

Changelog category (leave one):

  • Performance Improvement

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

  • Break on analyze stuck on complex query

Ref #21557

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

@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-bugfix Pull request with bugfix, not backported by default label Jun 17, 2022
@vdimir vdimir mentioned this pull request Jun 17, 2022
Closed
@vdimir vdimir marked this pull request as draft June 17, 2022 18:02
alexey-milovidov
alexey-milovidov reviewed Jun 18, 2022
View reviewed changes
alexey-milovidov
alexey-milovidov reviewed Jun 18, 2022
View reviewed changes
alexey-milovidov
alexey-milovidov reviewed Jun 18, 2022
View reviewed changes
src/Interpreters/InterpreterSelectQuery.cpp Outdated
Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov Jun 18, 2022

Choose a reason for hiding this comment

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

Also, we planned to use this counter to inhibit the second analysis step.

Copy link
Copy Markdown
Member Author

@vdimir vdimir Jun 19, 2022

Choose a reason for hiding this comment

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

In which cases second analysis step should be inhibited?

Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov Jun 20, 2022

Choose a reason for hiding this comment

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

It was said that the second analysis step is only an optimization. (Although I'm not sure)
If we already have made too many analyses, we can avoid further optimizations.

Also you can remove exponential blowup by memorizing the ASTs that were already optimized.

Copy link
Copy Markdown
Member Author

@vdimir vdimir Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Changed it to use max_pipeline_depth / 10 as a the threshold to disable the second analyze and max_pipeline_depth to cancel the query at all.

UPD: I've decided that it's not really good idea, it over-complicates logic, so just throw an exception.

@alexey-milovidov alexey-milovidov self-assigned this Jun 18, 2022
@alexey-milovidov alexey-milovidov added pr-performance Pull request with some performance improvements and removed pr-bugfix Pull request with bugfix, not backported by default labels Jun 19, 2022
@vdimir vdimir marked this pull request as ready for review July 5, 2022 11:51
@vdimir vdimir force-pushed the analyze_stuck branch 2 times, most recently from 885e058 to b0d043b Compare July 7, 2022 10:44
@alexey-milovidov alexey-milovidov merged commit ccef227 into ClickHouse:master Jul 31, 2022
@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Jul 31, 2022 •
edited
Loading

@kssenii
RabbitMQ tests have failed:
https://s3.amazonaws.com/clickhouse-test-reports/38185/1e974b55ea63430a059f70f06892865b4caafd7e/integration_tests__release__[2/2].html

@Felixoid
any hypothesis about the DNS issue and how to avoid it?
https://s3.amazonaws.com/clickhouse-test-reports/38185/1e974b55ea63430a059f70f06892865b4caafd7e/performance_comparison_[3/4]/runlog.log

@vdimir vdimir deleted the analyze_stuck branch August 1, 2022 09:32
@tavplubix tavplubix mentioned this pull request Aug 1, 2022
Merged
@tavplubix
Copy link
Copy Markdown
Member

tavplubix commented Aug 2, 2022

@alexey-milovidov, RabbitMQ tests failure was actually related to changes in this PR:
https://play.clickhouse.com/play?user=play#c2VsZWN0IAp0b1N0YXJ0T2ZEYXkoY2hlY2tfc3RhcnRfdGltZSkgYXMgZCwKY291bnQoKSwgZ3JvdXBVbmlxQXJyYXkocHVsbF9yZXF1ZXN0X251bWJlciksIGFueShyZXBvcnRfdXJsKQpmcm9tIGNoZWNrcyB3aGVyZSAnMjAyMi0wNi0wMScgPD0gY2hlY2tfc3RhcnRfdGltZSBhbmQgdGVzdF9uYW1lIGxpa2UgJyV0ZXN0X3N0b3JhZ2VfcmFiYml0bXElJyBhbmQgdGVzdF9zdGF0dXMgaW4gKCdGQUlMJykgZ3JvdXAgYnkgZCBvcmRlciBieSBkIGRlc2M=

@tavplubix tavplubix mentioned this pull request Aug 2, 2022
Merged
@tavplubix tavplubix mentioned this pull request Aug 2, 2022
Merged
@vdimir
Copy link
Copy Markdown
Member Author

vdimir commented Aug 2, 2022

@tavplubix thanks for noticing, I'll check

@vdimir
Copy link
Copy Markdown
Member Author

vdimir commented Aug 3, 2022

StorageRabbitMQ uses a global context for storage, and that's why the counter overflowed. Probably it's better to find another solution than the counter in context, but for now, I can't come up with it.

ClickHouse/src/Storages/RabbitMQ/StorageRabbitMQ.cpp

Lines 1056 to 1058 in 1c0d267

// Only insert into dependent views and expect that input blocks contain virtual columns
InterpreterInsertQuery interpreter(insert, rabbitmq_context, false, true, true);
auto block_io = interpreter.execute();

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Aug 18, 2022

@vdimir please resubmit this PR.

@vdimir
Copy link
Copy Markdown
Member Author

vdimir commented Aug 18, 2022

@alexey-milovidov
Here it is #40334, the same code, but with disabling for StorageRabbitMQ, because it reuses the same query context.

@kssenii
Copy link
Copy Markdown
Member

kssenii commented Aug 18, 2022

but with disabling for StorageRabbitMQ, because it reuses the same query context

for writing to materialised view? may be just create a new query context there each time?

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

Reviewers

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

Assignees

@alexey-milovidov alexey-milovidov

Labels

pr-performance Pull request with some performance improvements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vdimir @alexey-milovidov @tavplubix @kssenii @robot-ch-test-poll1