Improved parallel DecompressChunk worker selection #5870

jnidzwetzki · 2023-07-12T12:05:01Z

This PR improves the way the number of parallel workers for the DecompressChunk node are calculated. Since
1a93c2d, no partial paths for small chunks are generated, which could cause a fallback to a sequential plan and a performance regression. This patch ensures that a partial path is created again to get the 2.11.x behavior back.

Disable-check: force-changelog-file

Benchmarks

Tsbench against the last commit before this PR: https://grafana.ops.savannah-dev.timescale.com/d/NdmLnOk4z/compare-benchmark-runs?orgId=1&var-branch=All&var-run1=2549&var-run2=2551&var-threshold=0.02

Tsbench against 2.11.1 (a version without the commit 1a93c2d): https://grafana.ops.savannah-dev.timescale.com/d/NdmLnOk4z/compare-benchmark-runs?orgId=1&var-branch=All&var-run1=2514&var-run2=2551&var-threshold=0.02

Note: Disabling parallel plans for some queries might be beneficial. For example, when ht_chunk_compressed_2k is processed. Using this table, we have a lot of small chunks, and we have a huge overhead by the parallel logic. Here, we see some regression compared to the last commit but almost the same execution times as for 2.11.1.

Planner regression

Another result from benchmarking: it seems we have some kind of planner regression introduced in ecc1b7b. It is hidden on the tsbench dashboards due to some missing commits in tsbench and the fact this particular query becomes faster when it is executed sequentially. So, we have the planner regression which is compensated by the sequentially executed query. This should be addressed in a follow-up PR.

Commit `ecc1b7b`

tsbench=#  explain (verbose, analyze) SELECT * FROM ht_chunk_partially_compressed_2k WHERE device = 3 ORDER BY time, device DESC LIMIT 1;

 Planning Time: 1399.066 ms
 Execution Time: 674.978 ms

Before (`d6030a3`)

tsbench=#  explain (verbose, analyze) SELECT * FROM ht_chunk_partially_compressed_2k WHERE device = 3 ORDER BY time, device DESC LIMIT 1;

 Planning Time: 894.385 ms
 Execution Time: 717.762 ms

Differential Flame Graphs

Differential Flame Graphs for the query and the commits d6030a3 and ecc1b7b

Tsbench run

https://grafana.ops.savannah-dev.timescale.com/d/NdmLnOk4z/compare-benchmark-runs?orgId=1&var-branch=main&var-run1=2620&var-run2=2621&var-threshold=0.02

codecov · 2023-07-12T12:20:03Z

Codecov Report

Merging #5870 (8600266) into main (2eb0a38) will increase coverage by 0.01%.
Report is 8 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5870      +/-   ##
==========================================
+ Coverage   87.08%   87.09%   +0.01%     
==========================================
  Files         243      243              
  Lines       55917    55877      -40     
  Branches    12377    12359      -18     
==========================================
- Hits        48694    48666      -28     
+ Misses       4894     4858      -36     
- Partials     2329     2353      +24

Files Changed	Coverage Δ
src/import/allpaths.c	`73.64% <ø> (-0.39%)`	⬇️
tsl/src/nodes/decompress_chunk/decompress_chunk.c	`89.91% <100.00%> (+0.44%)`	⬆️

... and 36 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jnidzwetzki · 2023-07-13T07:18:31Z

test/sql/updates/post.compression.sql

@@ -2,7 +2,7 @@
 -- Please see the included NOTICE for copyright information and
 -- LICENSE-APACHE for a copy of the license.

-SELECT * FROM compress ORDER BY time DESC, small_cardinality;
+SELECT * FROM compress ORDER BY time DESC, small_cardinality, large_cardinality, some_double, some_int, some_custom, some_bool;


Test output was not deterministic so far.

github-actions · 2023-07-13T14:36:14Z

@RafiaSabih, @fabriziomello: please review this pull request.

Powered by pull-review

svenklemm · 2023-07-14T11:16:17Z

tsl/src/nodes/decompress_chunk/decompress_chunk.c

@@ -1572,7 +1572,23 @@ create_compressed_scan_paths(PlannerInfo *root, RelOptInfo *compressed_rel, Comp

 	/* create parallel scan path */
 	if (compressed_rel->consider_parallel)
-		ts_create_plain_partial_paths(root, compressed_rel);
+	{


Since this is the only caller for ts_create_plain_partial_paths we should either move the code into that function or get rid of the function

The function is still called in set_plain_rel_pathlist(). However, it is used only in non-TSL code. So, I removed the export of the function.

akuzm · 2023-08-02T11:08:12Z

src/import/allpaths.c

+static void
+create_plain_partial_paths(PlannerInfo *root, RelOptInfo *rel)
 {


This is a matter of taste, but I would still leave the ts_ prefix so that it's harder to confuse with grep/go to definition.

Changed back to ts_create_plain_partial_paths as suggested.

akuzm · 2023-08-02T11:09:17Z

test/sql/updates/post.compression.sql

@@ -17,7 +17,7 @@ WHERE
  hypertable.table_name = 'compress'
  AND chunk.compressed_chunk_id IS NULL;

-SELECT * FROM compress ORDER BY time DESC, small_cardinality;
+SELECT * FROM compress ORDER BY time DESC, small_cardinality, large_cardinality, some_double, some_int, some_custom, some_bool;


ORDER BY compress would order by all columns.

We have to use the same ordering as the existing up/downgrade tests to generate the same output. Otherwise, the tests will fail. Therefore, we need an ordering of time DESC and large_cardinality ASC. I think that is not possible with the proposed change.

tsl/src/nodes/decompress_chunk/decompress_chunk.c

shhnwz · 2023-08-09T05:37:19Z

@jnidzwetzki ,
Please rebase the branch, last week considerable changes went into decompress code path.

This PR improves the way the number of parallel workers for the DecompressChunk node are calculated. Since 1a93c2d, no partial paths for small relations are generated, which could cause a fallback to a sequential plan and a performance regression. This patch ensures that for all relations, a partial path is created again.

jnidzwetzki · 2023-08-09T06:15:30Z

@shhnwz The PR is now rebased

shhnwz

Looks Good To Me.

github-actions bot assigned jnidzwetzki Jul 12, 2023

jnidzwetzki added force-auto-backport Automatically backport this PR or fix of this issue, even if it's not marked as "bug" compression labels Jul 12, 2023

jnidzwetzki force-pushed the parallel_worker branch 3 times, most recently from e248784 to 267a239 Compare July 12, 2023 12:10

jnidzwetzki force-pushed the parallel_worker branch 13 times, most recently from 6054cd3 to 2122b26 Compare July 12, 2023 19:20

jnidzwetzki removed the force-auto-backport Automatically backport this PR or fix of this issue, even if it's not marked as "bug" label Jul 12, 2023

jnidzwetzki force-pushed the parallel_worker branch 2 times, most recently from 65d1a9b to 023c494 Compare July 13, 2023 07:17

jnidzwetzki commented Jul 13, 2023

View reviewed changes

jnidzwetzki marked this pull request as ready for review July 13, 2023 14:35

github-actions bot requested review from fabriziomello and RafiaSabih July 13, 2023 14:36

svenklemm reviewed Jul 14, 2023

View reviewed changes

jnidzwetzki force-pushed the parallel_worker branch from 023c494 to 587ada9 Compare July 17, 2023 07:06

jnidzwetzki mentioned this pull request Jul 17, 2023

Ensure partial paths are always created #5855

Closed

akuzm reviewed Aug 2, 2023

View reviewed changes

tsl/src/nodes/decompress_chunk/decompress_chunk.c Outdated Show resolved Hide resolved

akuzm approved these changes Aug 2, 2023

View reviewed changes

jnidzwetzki force-pushed the parallel_worker branch from 587ada9 to 27ab266 Compare August 7, 2023 08:56

jnidzwetzki force-pushed the parallel_worker branch from 27ab266 to 8600266 Compare August 9, 2023 06:14

shhnwz approved these changes Aug 10, 2023

View reviewed changes

jnidzwetzki enabled auto-merge (rebase) August 10, 2023 09:57

jnidzwetzki merged commit 9a2dfbf into timescale:main Aug 10, 2023
36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved parallel DecompressChunk worker selection #5870

Improved parallel DecompressChunk worker selection #5870

jnidzwetzki commented Jul 12, 2023 •

edited

codecov bot commented Jul 12, 2023 •

edited

jnidzwetzki Jul 13, 2023

github-actions bot commented Jul 13, 2023

svenklemm Jul 14, 2023

jnidzwetzki Jul 17, 2023

akuzm Aug 2, 2023

jnidzwetzki Aug 7, 2023

akuzm Aug 2, 2023

jnidzwetzki Aug 7, 2023

shhnwz commented Aug 9, 2023

jnidzwetzki commented Aug 9, 2023

shhnwz left a comment

Improved parallel DecompressChunk worker selection #5870

Improved parallel DecompressChunk worker selection #5870

Conversation

jnidzwetzki commented Jul 12, 2023 • edited

Benchmarks

Planner regression

Commit ecc1b7b

Before (d6030a3)

Differential Flame Graphs

Tsbench run

codecov bot commented Jul 12, 2023 • edited

Codecov Report

jnidzwetzki Jul 13, 2023

Choose a reason for hiding this comment

github-actions bot commented Jul 13, 2023

svenklemm Jul 14, 2023

Choose a reason for hiding this comment

jnidzwetzki Jul 17, 2023

Choose a reason for hiding this comment

akuzm Aug 2, 2023

Choose a reason for hiding this comment

jnidzwetzki Aug 7, 2023

Choose a reason for hiding this comment

akuzm Aug 2, 2023

Choose a reason for hiding this comment

jnidzwetzki Aug 7, 2023

Choose a reason for hiding this comment

shhnwz commented Aug 9, 2023

jnidzwetzki commented Aug 9, 2023

shhnwz left a comment

Choose a reason for hiding this comment

jnidzwetzki commented Jul 12, 2023 •

edited

Commit `ecc1b7b`

Before (`d6030a3`)

codecov bot commented Jul 12, 2023 •

edited