Skip to content

Add memory Metric for Projections PK keys#102587

Merged
tavplubix merged 3 commits into
ClickHouse:masterfrom
npakeer:add_proj_pk_async_metrics
Apr 22, 2026
Merged

Add memory Metric for Projections PK keys#102587
tavplubix merged 3 commits into
ClickHouse:masterfrom
npakeer:add_proj_pk_async_metrics

Conversation

@npakeer
Copy link
Copy Markdown
Contributor

@npakeer npakeer commented Apr 13, 2026

Changelog category (leave one):

  • Improvement

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

Metrics to Track memory used by Projections Primary Keys and Projections Index Granularity for all tables.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Apr 13, 2026

Workflow [PR], commit [99a5aa8]

Summary:


AI Review

Summary

This PR extends system.asynchronous_metrics with four projection-specific memory metrics (TotalProjectionPrimaryKeyBytesInMemory, TotalProjectionPrimaryKeyBytesInMemoryAllocated, TotalProjectionIndexGranularityBytesInMemory, TotalProjectionIndexGranularityBytesInMemoryAllocated) and adds an integration test that validates increase/decrease behavior across inserts and TRUNCATE. I did not find new correctness, safety, concurrency, or performance issues in the current PR head, and existing bot-raised gaps appear addressed.

ClickHouse Rules
Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsChangesHistory.cpp
PR metadata quality
Safe rollout
Compilation time
No large/binary files
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-improvement Pull request with some product improvements label Apr 13, 2026
Comment thread tests/integration/test_asynchronous_metrics_pk_bytes_fields/test.py
Comment thread tests/integration/test_asynchronous_metrics_pk_bytes_fields/test.py
@tavplubix
Copy link
Copy Markdown
Member

btw do we already count marks and index granularity from projection parts?

Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

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

Do not merge before fixing "Logical error: '!res.empty()' (STID: 2508-3ea0)"

@npakeer
Copy link
Copy Markdown
Contributor Author

npakeer commented Apr 15, 2026

btw do we already count marks and index granularity from projection parts?

@tavplubix From what I understood, MarkCache memory usage metric is current metric, and is automatically incremented when marks file for any part(including projection parts) is loaded, because the increment of the loaded bytes happens with in the Cache Class during the load.

IndexGranularity, and PK metrics are asynchronous metrics and they are calculated separately for each interval i.e "asynchronous_metrics_update_period_s " and these metrics for projections were missed. This PR calculates PK and index granularity for projections in separate metrics.

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Apr 16, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.00% 84.00% +0.00%
Functions 90.90% 90.90% +0.00%
Branches 76.50% 76.50% +0.00%

Changed lines: 100.00% (22/22) · Uncovered code

Full report · Diff report

@npakeer npakeer marked this pull request as ready for review April 17, 2026 10:38
@azat azat self-assigned this Apr 22, 2026
return int(node.query(query_proj_ig_bytes_allocated).strip())

# metrics should increase after first insert
proj_pk_bytes_before, proj_pk_bytes_after = query_until_condition(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of this we can use SYSTEM RELOAD ASYNCHRONOUS METRICS

@azat
Copy link
Copy Markdown
Member

azat commented Apr 22, 2026

Also make sense to add index_granularity_bytes_in_memory and index_granularity_bytes_in_memory_allocated to system.projection_parts

@azat
Copy link
Copy Markdown
Member

azat commented Apr 22, 2026

Do not merge before fixing "Logical error: '!res.empty()' (STID: 2508-3ea0)"

Has been fixed already, likely #102604

@tavplubix tavplubix enabled auto-merge April 22, 2026 13:18
@tavplubix tavplubix added this pull request to the merge queue Apr 22, 2026
Merged via the queue into ClickHouse:master with commit af89ac9 Apr 22, 2026
317 of 318 checks passed
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 22, 2026
@npakeer
Copy link
Copy Markdown
Contributor Author

npakeer commented Apr 26, 2026

Also make sense to add index_granularity_bytes_in_memory and index_granularity_bytes_in_memory_allocated to system.projection_parts

@azat Thanks for the review. I will work on adding these

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 pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants