Skip to content

Update compression stats when merging chunks #7909

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

erimatnor
Copy link
Contributor

Merge compression chunk size stats when merging chunks. The merged chunk's stats is simply the sum of the stats of all compressed chunks that are merged. This ensures that the aggregate stats do not change due to the merge. Still, the stats might not accurately represent the merged chunk since merging both compressed and non-compressed chunks will leave some data uncompressed and this data won't be reflected in the stats of the merged chunk.

@erimatnor erimatnor force-pushed the update-compression-stats-on-merge-chunks branch 2 times, most recently from eb21756 to f33b3c9 Compare April 3, 2025 15:06
Copy link

codecov bot commented Apr 3, 2025

Codecov Report

Attention: Patch coverage is 82.45614% with 10 lines in your changes missing coverage. Please review.

Project coverage is 81.88%. Comparing base (59f50f2) to head (1c375af).
Report is 878 commits behind head on main.

Files with missing lines Patch % Lines
src/ts_catalog/compression_chunk_size.c 82.92% 2 Missing and 5 partials ⚠️
tsl/src/chunk.c 81.25% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7909      +/-   ##
==========================================
+ Coverage   80.06%   81.88%   +1.81%     
==========================================
  Files         190      249      +59     
  Lines       37181    46177    +8996     
  Branches     9450    11575    +2125     
==========================================
+ Hits        29770    37813    +8043     
- Misses       2997     3801     +804     
- Partials     4414     4563     +149     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@erimatnor erimatnor force-pushed the update-compression-stats-on-merge-chunks branch from f33b3c9 to 13a19e7 Compare April 4, 2025 10:36
Merge compression chunk size stats when merging chunks. The merged
chunk's stats is simply the sum of the stats of all compressed chunks
that are merged. This ensures that the aggregate stats do not change
due to the merge. Still, the stats might not accurately represent the
merged chunk since merging both compressed and non-compressed chunks
will leave some data uncompressed and this data won't be reflected in
the stats of the merged chunk.
@erimatnor erimatnor force-pushed the update-compression-stats-on-merge-chunks branch from 13a19e7 to 1c375af Compare April 8, 2025 12:28
@erimatnor erimatnor marked this pull request as ready for review April 8, 2025 14:44
Copy link
Contributor

@mkindahl mkindahl left a comment

Choose a reason for hiding this comment

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

Patch looks fine. It's a little hard to follow and verify the stats, so you might want to have tests that actually save away the data before the merge and then do the computations in SQL to verify that they are what is expected after the merge.

TupleInfo *ti = ts_scan_iterator_tuple_info(&iterator);
bool should_free;
HeapTuple tuple = ts_scanner_fetch_heap_tuple(ti, false, &should_free);
memcpy(form, GETSTRUCT(tuple), sizeof(FormData_compression_chunk_size));
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Will avoid problems if the type of form changes.

Suggested change
memcpy(form, GETSTRUCT(tuple), sizeof(FormData_compression_chunk_size));
memcpy(form, GETSTRUCT(tuple), sizeof(*form));

Comment on lines +286 to +292
select * from _timescaledb_catalog.compression_chunk_size order by chunk_id;
chunk_id | compressed_chunk_id | uncompressed_heap_size | uncompressed_toast_size | uncompressed_index_size | compressed_heap_size | compressed_toast_size | compressed_index_size | numrows_pre_compression | numrows_post_compression | numrows_frozen_immediately
----------+---------------------+------------------------+-------------------------+-------------------------+----------------------+-----------------------+-----------------------+-------------------------+--------------------------+----------------------------
1 | 6 | 8192 | 0 | 32768 | 16384 | 8192 | 16384 | 1 | 1 | 1
3 | 7 | 8192 | 0 | 32768 | 16384 | 8192 | 16384 | 1 | 1 | 1
(2 rows)

Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to use \x on for these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants