-
Notifications
You must be signed in to change notification settings - Fork 897
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
Don't copy compressed slot to compressed batch struct #6806
Conversation
There is overhead associated with copying the heap tuple and (un)pinning the respective heap buffers, which becomes apparent in vectorized aggregation.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6806 +/- ##
==========================================
+ Coverage 80.06% 80.92% +0.85%
==========================================
Files 190 194 +4
Lines 37181 36854 -327
Branches 9450 9645 +195
==========================================
+ Hits 29770 29823 +53
- Misses 2997 3182 +185
+ Partials 4414 3849 -565 ☔ View full report in Codecov by Sentry. |
// fprintf(stderr, "segmentby column [%d]: value %p, null %d\n", | ||
// attr, (void*) decompressed_tuple->tts_values[attr], | ||
// decompressed_tuple->tts_isnull[attr]); |
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.
probably wanna remove that
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.
Should remove the commented out debug statements before merging
There is overhead associated with copying the heap tuple and (un)pinning the respective heap buffers, which becomes apparent in vectorized aggregation.
Instead of this, it is enough to copy the by-reference segmentby values to the per-batch context.
Also we have to copy in the rare case where the compressed data is inlined into the compressed row and not toasted.
No changes in performance, as expected: https://grafana.ops.savannah-dev.timescale.com/d/fasYic_4z/compare-akuzm?orgId=1&var-branch=All&var-run1=3407&var-run2=3411&var-threshold=0&var-use_historical_thresholds=true&var-threshold_expression=2.5%20%2A%20percentile_cont%280.90%29&var-exact_suite_version=false
Disable-check: force-changelog-file