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

Use a stateful detoaster for compressed data #6383

Merged
merged 1 commit into from
Dec 19, 2023
Merged

Conversation

akuzm
Copy link
Member

@akuzm akuzm commented Dec 7, 2023

The normal Postgres detoasting code locks and opens toast tables and indexes for each toast value, which can take a large percentage CPU time on simple queries. Since in decompression we're working with one table at a time, the toast table and index are the same for every datum, so we don't have to redo this work.

Gives up to 60% speedup on some queries to compressed hypertables: https://grafana.ops.savannah-dev.timescale.com/d/fasYic_4z/compare-akuzm?orgId=1&var-branch=All&var-run1=3012&var-run2=3018&var-threshold=0&var-use_historical_thresholds=true&var-threshold_expression=2.0%20*%20percentile_disc(0.90)&var-exact_suite_version=false

Disable-check: force-changelog-file

Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Attention: 36 lines in your changes are missing coverage. Please review.

Comparison is base (301612a) 87.33% compared to head (94dd213) 87.27%.
Report is 1 commits behind head on main.

Files Patch % Lines
tsl/src/nodes/decompress_chunk/detoaster.c 74.46% 20 Missing and 16 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6383      +/-   ##
==========================================
- Coverage   87.33%   87.27%   -0.07%     
==========================================
  Files         184      185       +1     
  Lines       41591    41698     +107     
  Branches     9238     9263      +25     
==========================================
+ Hits        36324    36392      +68     
- Misses       3602     3626      +24     
- Partials     1665     1680      +15     

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

@akuzm akuzm marked this pull request as ready for review December 11, 2023 15:34
Copy link

@antekresic, @mahipv: please review this pull request.

Powered by pull-review

Copy link
Member

@svenklemm svenklemm left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1398,6 +1398,7 @@ tsl_recompress_chunk_segmentwise(PG_FUNCTION_ARGS)
index_endscan(index_scan);
UnregisterSnapshot(snapshot);
index_close(index_rel, AccessExclusiveLock);
row_decompressor_close(&decompressor);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we have row_compressor_finish and now row_decompressor_close which is kinda out of sync.

Also, maybe we could put more of the decompressor finish work in the function like freeing the estate, bistate, closing indexes and anything else necessary. If not in this PR, just add a TODO on the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nit: we have row_compressor_finish and now row_decompressor_close which is kinda out of sync.

I think "finish" might give a wrong impression that it finishes compression, so I renamed it to "close" as well.

Also, maybe we could put more of the decompressor finish work in the function like freeing the estate, bistate, closing indexes and anything else necessary. If not in this PR, just add a TODO on the function.

Makes sense, somehow I didn't notice them. I moved them into the "close" function as well.

return query select sum(length(s1)) from longstr;
end; $$ language plpgsql volatile;

select sum(t) from generate_series(1, 30) x, lateral test(x * x * x) t;
Copy link
Contributor

Choose a reason for hiding this comment

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

For sake of coverage, lets decompress all the chunks using decompress_chunk as well. Maybe its enough to do it once after this select.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

Copy link
Contributor

@antekresic antekresic left a comment

Choose a reason for hiding this comment

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

lgtm

The normal Postgres detoasting code locks and opens toast tables and
indexes for each toast value, which can take a large percentage CPU time
on simple queries. Since in decompression we're working with one table
at a time, the toast table and index are the same for every datum, so we
don't have to redo this work.
@akuzm akuzm enabled auto-merge (rebase) December 19, 2023 12:14
@akuzm akuzm merged commit a64ae61 into timescale:main Dec 19, 2023
42 of 43 checks passed
@akuzm akuzm deleted the detoaster branch December 19, 2023 12:23
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.

None yet

3 participants