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

Optimize decompression planning #7568

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

akuzm
Copy link
Member

@akuzm akuzm commented Jan 6, 2025

The EquivalenceMember lookup is the most costly part, so share it between different uses.

Switch batch sorted merge to use the generic pathkey matching code.

Also cache some intermediate data in the CompressionInfo struct.

Disable-check: force-changelog-file

The EquivalenceMember lookup is the most costly part, so share it
between different uses.

Switch batch sorted merge to use the generic pathkey matching code.

Also cache some intermediate data in the CompressionInfo struct.
@@ -212,7 +212,7 @@ decompress_chunk_begin(CustomScanState *node, EState *estate, int eflags)
node->ss.ss_ScanTupleSlot->tts_tupleDescriptor);
}
}
/* Sort keys should only be present when sorted_merge_append is used */
/* Sort keys should only be present when batch sorted merge is used. */
Copy link
Member

Choose a reason for hiding this comment

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

Even without batch sortetd merge we might still want to push down ordering and skip the ordering after decompression.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we do that, but these are the keys for the sorting performed inside the DecompressChunk node itself. The only case when it does that is for batch sorted merge, otherwise the sorting is performed by the underlying compressed scan.

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 96.29630% with 5 lines in your changes missing coverage. Please review.

Project coverage is 82.30%. Comparing base (59f50f2) to head (780258e).
Report is 688 commits behind head on main.

Files with missing lines Patch % Lines
tsl/src/nodes/decompress_chunk/decompress_chunk.c 96.24% 0 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7568      +/-   ##
==========================================
+ Coverage   80.06%   82.30%   +2.24%     
==========================================
  Files         190      238      +48     
  Lines       37181    43706    +6525     
  Branches     9450    10963    +1513     
==========================================
+ Hits        29770    35974    +6204     
- Misses       2997     3402     +405     
+ Partials     4414     4330      -84     

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

@akuzm
Copy link
Member Author

akuzm commented Jan 9, 2025

Tsbench has some 10% speedups on planning queries: https://grafana.ops.savannah-dev.timescale.com/d/uP2MnQk4z/query-run-times?orgId=1&var-suite=lazy_decompression&var-query=8e50d2074a29289e8ec3280d4ee535bc

This also uncovers a major planning regression which is caused by the notorious quadratic equivalence member search in the Postgres sorted plan creation. I think we'll have to live with it for now and fix this upstream. It happens on queries like EXPLAIN SELECT * FROM ht_chunk_compressed_2k ORDER BY time_bucket('1d', time) DESC, device LIMIT 1;. The reason is that we now have many per-chunk Sorts under MergeAppend, instead of one big Sort above Append. Each per-chunk sort requires a EM search (prepare_sort_from_pathkeys).

Another regression is in select min(vendor_id + passenger_count + trip_distance + pickup_longitude + pickup_latitude + rate_code + dropoff_longitude + dropoff_latitude + payment_type + fare_amount + extra + mta_tax + tip_amount + tolls_amount + improvement_surcharge + total_amount) from rides where true;
This is because sort + limit 1 initplan for min() is now chosen instead of aggregation. This is a known problem that will be fixed here #6879 (missed costing for projection under sort).

The regression in SELECT * FROM ht_metrics_compressed ORDER BY time_bucket('1d', time) DESC, device LIMIT 1; happens because now the parallel plan is not chosen. This will be also fixed by #6879

@akuzm
Copy link
Member Author

akuzm commented Jan 9, 2025

I initially didn't want to introduce any plan changes with this PR, but it's split out of #6879, so I had to import one small part from there -- we now can sort above decompression not only by plain columns, but also by expressions (e.g. time_bucket), which gives rise to these (arguably more efficient) MergeAppend over per-chunk Sort plans.

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.

2 participants