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
Index support for compress chunk #4821
Index support for compress chunk #4821
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4821 +/- ##
==========================================
+ Coverage 89.59% 89.63% +0.03%
==========================================
Files 227 226 -1
Lines 51623 51220 -403
==========================================
- Hits 46252 45910 -342
+ Misses 5371 5310 -61
Continue to review full report at Codecov.
|
Needs more tests, probably good to put them into a separate test file.
|
489ecc5
to
d85c7e1
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.
I think we are missing check for collation. Can you add tests with different collation. In the index check the collation must match for an index to be elegible for datatypes that are collation sensitive.
d85c7e1
to
5b347c0
Compare
Looks like we still need more tests:
|
ce44416
to
15d28f5
Compare
15d28f5
to
3984c45
Compare
3984c45
to
bbd312e
Compare
tsl/src/compression/compression.c
Outdated
else | ||
{ | ||
break; | ||
} |
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.
You're not resetting current_direction
here, this will lead to an index still being chosen if the last column didn't match, right?
We have to fix some things:
- add a test for this bug. Try to reproduce it on the unfixed version and verify that the last column is chosen incorrectly.
- Reset
current_direction
at the start of the loop to avoid bugs like this. - Rewrite this as early exit from the loop:
if (att_num == 0 || index_info->ii_IndexAttrNumbers[i] != att_num)
{
break;
}
Long if branches are hard to read, so early exit with break/return is preferrable.
tsl/src/compression/compression.c
Outdated
bool is_null_first = | ||
COMPRESSIONCOL_IS_SEGMENT_BY(keys[i]) ? false : keys[i]->orderby_nullsfirst; | ||
|
||
if (att_num > 0 && index_info->ii_IndexAttrNumbers[i] == att_num) |
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.
Attnum can't be zero here, right? The orderby/segmentby keys must be present in the table. We should check this with an assertion.
bbd312e
to
33b8b38
Compare
bool is_null_first = | ||
COMPRESSIONCOL_IS_SEGMENT_BY(keys[i]) ? false : keys[i]->orderby_nullsfirst; | ||
|
||
if (att_num == 0 || index_info->ii_IndexAttrNumbers[i] != att_num) |
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.
Can att_num == 0
happen in practice? This means that the orderby/segmentby column is not found in the uncompressed chunk. That would be either a program error or a data corruption. Let's use Ensure
macro to check this, not if
.
33b8b38
to
447b682
Compare
de7c95d
to
9655ec7
Compare
It allows to override tuplesort with indexscan if compression setting keys matches with Index keys. Moreover this feature has Enable/Disable Toggle. To Disable from the client use the following command, SET timescaledb.enable_compression_indexscan = 'OFF'
9655ec7
to
e45a557
Compare
Index support for compress chunk
It allows to override tuplesort with indexscan
if compression setting keys matches with Index keys.
Moreover this feature has Enable/Disable Toggle.
To Disable from the client use the following command,
SET timescaledb.enable_compression_indexscan = 'OFF'
Fixes #4617