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

Do not acquire lock for total_bytes/total_rows for Buffer engine #24066

Merged
merged 2 commits into from
May 13, 2021

Conversation

azat
Copy link
Collaborator

@azat azat commented May 12, 2021

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Do not acquire lock for total_bytes/total_rows for Buffer engine

Detailed description / Documentation draft:
When Buffer() is under preassure, acquiring per-layer lock may take
significant time. And so the following query may take significant amount of time:

SELECT total_bytes, total_rows FROM system.tables WHERE engine='Buffer'

@robot-clickhouse robot-clickhouse added the pr-improvement Pull request with some product improvements label May 12, 2021
@kitaisreal kitaisreal self-assigned this May 12, 2021
azat added 2 commits May 12, 2021 23:38
When Buffer() is under preassure, acquiring per-layer lock may take
significant time. And so the following query may take significant amount of time:

    SELECT total_bytes, total_rows FROM system.tables WHERE engine='Buffer'
@azat azat force-pushed the buffer-total-lock-contention branch from 9b7b7db to 074b57f Compare May 12, 2021 20:38
@azat
Copy link
Collaborator Author

azat commented May 13, 2021

AST fuzzer (MSan) — Lost connection to server. See the logs.

SIGKILL

Functional stateless tests (release, DatabaseReplicated) — fail: 1, passed: 2935, skipped: 108

Unrelated

@kitaisreal kitaisreal merged commit f3ee14d into ClickHouse:master May 13, 2021
@azat azat deleted the buffer-total-lock-contention branch May 13, 2021 18:59
@@ -606,6 +606,9 @@ class BufferBlockOutputStream : public IBlockOutputStream
if (!buffer.data)
{
buffer.data = sorted_block.cloneEmpty();

storage.total_writes.rows += buffer.data.rows();
storage.total_writes.bytes += buffer.data.allocatedBytes();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is useless

Comment on lines +626 to +632
size_t old_rows = buffer.data.rows();
size_t old_bytes = buffer.data.allocatedBytes();

appendBlock(sorted_block, buffer.data);

storage.total_writes.rows += (buffer.data.rows() - old_rows);
storage.total_writes.bytes += (buffer.data.allocatedBytes() - old_bytes);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like it is better to check this for overflow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-improvement Pull request with some product improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants