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

Add warning for poor compression ratio #7756

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

Conversation

kpan2034
Copy link
Contributor

@kpan2034 kpan2034 commented Feb 19, 2025

Adds functionality to emit a warning to the user when the compression
ratio is "bad".

Currently, "bad" is defined to be when size after compression is higher
than after compression.

@kpan2034
Copy link
Contributor Author

This change breaks roughly half our tests, since compressing chunks with only a few rows leads to a bad compression ratio. I considered adding this behind a GUC, which should probably be on by default, but except for testing I don't think there's ever a case where we want it to be OFF.

We can either:

  • Add a GUC that enables this warning, and set it to OFF for all our tests.
  • Update the affected tests with the warning that is emitted.

However, if we'd like to do more than just emit a warning in the future, then perhaps a GUC makes more sense, since that is something users might like to turn off. What are your thoughts on this @antekresic?

@philkra philkra modified the milestone: v2.19.0 Feb 20, 2025
@kpan2034 kpan2034 force-pushed the add-compression-warning branch 3 times, most recently from 076e1b0 to cabda33 Compare March 4, 2025 10:14
Copy link

codecov bot commented Mar 4, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.92%. Comparing base (59f50f2) to head (b8f1a65).
Report is 802 commits behind head on main.

Files with missing lines Patch % Lines
tsl/src/compression/api.c 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7756      +/-   ##
==========================================
+ Coverage   80.06%   81.92%   +1.85%     
==========================================
  Files         190      247      +57     
  Lines       37181    45390    +8209     
  Branches     9450    11343    +1893     
==========================================
+ Hits        29770    37187    +7417     
- Misses       2997     3733     +736     
- Partials     4414     4470      +56     

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

@kpan2034 kpan2034 force-pushed the add-compression-warning branch from cabda33 to 1889c9b Compare March 4, 2025 10:29
Adds functionality to emit a warning to the user when the compression
ratio is "bad".

Currently, "bad" is defined to be when size after compression is higher
than after compression.
@kpan2034 kpan2034 force-pushed the add-compression-warning branch from 1889c9b to b8f1a65 Compare March 4, 2025 13:29
@kpan2034 kpan2034 marked this pull request as ready for review March 4, 2025 13:39
@kpan2034 kpan2034 requested a review from svenklemm March 4, 2025 13:39
@philkra philkra added this to the v2.20.0 milestone Mar 18, 2025
compression_ratio < POOR_COMPRESSION_THRESHOLD ?
WARNING :
DEBUG1,
"size before compression: " INT64_FORMAT
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs better wording, its too dry and doesn't even include name of the chunk.

Copy link
Member

Choose a reason for hiding this comment

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

agree with ante here that this needs a better error message
poor compression rate detected in chunk "xyz", the numbers could be moved into errdetail. Additionally you should probably use ereport here

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.

4 participants