-
Notifications
You must be signed in to change notification settings - Fork 884
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 GUC for segmentwise recompression #7413
base: main
Are you sure you want to change the base?
Conversation
f582e31
to
796d690
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, but I would suggest to make that message NOTICE
instead of DEBUG1
to avoid flaky tests.
ERROR: segmentwise recompression functionality disabled, enable it by first setting timescaledb.enable_segmentwise_recompression to on | ||
\set ON_ERROR_STOP 1 | ||
-- When GUC is OFF, entire chunk should be fully uncompressed and compressed instead | ||
SET client_min_messages TO DEBUG; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be very flaky once in main, so it might be better to not use debug level in tests. You might be able to avoid it by making the message in question a notice rather than debug1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the kind of flakiness I meant. Look at recompress_chunk_segmentwise
. Debug outputs are rarely stable and will change depending on the run or the environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, makes sense. I'll change that to NOTICE
instead.
tsl/src/compression/api.c
Outdated
elog(DEBUG1, | ||
"segmentwise recompression is disabled, performing full recompression on " | ||
"chunk \"%s.%s\"", | ||
NameStr(chunk->fd.schema_name), | ||
NameStr(chunk->fd.table_name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might make more sense as a NOTICE
rather than DEBUG1
. The message will only be shown when you have disabled the GUC, so it should not affect the default setup, but would be a good hint for users that disable segmentwise recompression.
Add GUC option to enable or disable segmentwise recompression. If disabled, then a full recompression is done instead when recompression is attempted through `compress_chunk`. If `recompress_chunk_segmentwise` is used when GUC is disabled, then an error is thrown.
796d690
to
bcaea08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7413 +/- ##
==========================================
+ Coverage 80.06% 82.62% +2.55%
==========================================
Files 190 229 +39
Lines 37181 42740 +5559
Branches 9450 10739 +1289
==========================================
+ Hits 29770 35314 +5544
- Misses 2997 3143 +146
+ Partials 4414 4283 -131 ☔ View full report in Codecov by Sentry. |
Add GUC option to enable or disable segmentwise recompression. If disabled, then a full recompression is done instead when recompression is attempted through
compress_chunk
. Ifrecompress_chunk_segmentwise
is used when GUC is disabled, then an error is thrown.Closes #7381.