implement segmentwise batch sorted merge

[natalya-aksman]

Implement segmentwise batch sorted merge for queries matching compressed index order over unordered chunks.
For example this query should support Batch sorted merge over compressed unordered chunks for t_segbydev_orderbytime:

 SELECT * FROM t_segbydev_orderbytime ORDER BY dev, time;

Compressed data is always ordered by segmentby values, the batches for Batch sorted merge are consumed segment-by-segment and we want to do heap sort of tuples by orderby keys for each segment.

BSM cost method has been updated for segmentwise BSM.

This PR is a part of ongoing project to improve Batch sorted merge coverage: https://github.com/timescale/eng-database/issues/811.

We may consider further optimizations in the executor for Batch sorted merge in a separate PR, these optimizations have been excluded from this PR:
We can avoid comparing segmentby keys in binary_heap comparator for each tuple as they are the same within each segment.
We will stop requesting new batches after new segment is added to the heap, we will process all batches from the current segment already on the heap and when only the batch from the new segment remains on the heap, we will make new segment the current segment.

[github-actions]

@Poroma-Banerjee, @kpan2034: please review this pull request.

Powered by pull-review

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[akuzm]

Why do we need any special handling for segmentby keys? It should just work transparently if you put them as a prefix of the heap keys, no?

[natalya-aksman]

1. We don't want segment keys on the heap as they do not need to be merged, they are already sorted in correct order. It will slow heapsort down unnecessarily, also, it will eat up memory and we are trying to keep heap memory down to keep BSM viable.

2. Segment keys can be passed by reference, i.e. they can be of text type etc. Actually, orderby keys can also be of text type, can they? If yes, then we have a bug in BSM as we always copy heap keys by value. I'll check that.

[akuzm]

It will slow heapsort down unnecessarily, also, it will eat up memory and we are trying to keep heap memory down to keep BSM viable.

Why would it use more memory? We already keep the full uncompressed current tuple in every batch, segmentby is already there. The uptake of source batches also shouldn't be influenced by the way we do comparison. It's the prefix key, so if the next batch compares greater on segmentby, we just stop there.

About the performance characteristics, let's measure if it's actually important. This batch heap queue has a lot of code already, and it's pretty complicated and bug-prone. It's poorly tested too, despite all our effort. We found another serious bug in batch sorted merge just last month. So I'd be happy if we can just make a planner change that works well enough. Maybe it just splits in two PRs trivially, if my idea is right.

[akuzm]

This reminds me of a kind of adjacent change that I wanted to do a long time ago: decide when to stop intake of the source batches based on comparing the compressed tuple, even before any decompression takes place. IIRC now we have to decompress and materialize the first tuple always, so this wastes some time. Also because of this the batch sorted merge has a special weird interface in the "compressed batch" module, compressed_batch_save_first_tuple.

[natalya-aksman]

The uptake of source batches also shouldn't be influenced by the way we do comparison. It's the prefix key, so if the next batch compares greater on segmentby, we just stop there.

We will be doing unnecessary extra keys comparison for each binary_heap comparison we do as we compare heap entries for the same segment. I.e. instead of comparing on, say, (orderby1, orderby2) columns for each heap comparison we will have to compare on (segby1, segby2, orderby1, orderby2) where (segby1, segby2) are the same for all tuples in the current processed segment.

I'm also looking at this comment which reasons the need for HeapEntry for better performance:

timescaledb/tsl/src/nodes/columnar_scan/batch_queue_heap.c

Line 33 in 62575c3

* This is the actual entries of the heap we're going to compare. We're using

If we want to follow this reasoning it won't make sense to add unneeded to fields to HeapEntry.

I can put this (supposedly) optimized method under a GUC and implement HeapEntry in a regular way, just include segmentby keys into it and let binary_heap compare it in a normal way.

We can then compare performance and see if simplified logic doesn't lead to noticeable performance hit.

[akuzm]

The point is that the current execution logic transparently handles the case when you have to check the segmentby. That's why we can start with a simple planner change, that works "well enough" by itself and then work on optimizing the execution side. Do I get this right? If this doesn't work transparently, and the execution side must have changes as well, then there's probably not much value in making some simplified versions of it.

[akuzm]

Letting the heap consume all batches and then merge them would make BSM unusable.

Right, that's obviously not practical. I was thinking about using the "last batch compressed tuple" in batch_queue_heap_needs_next_batch instead of the last_batch_first_tuple_entry. This means the last batch will not be decompressed until it's needed. This also means that the "comparison for heap property" becomes a separate thing from "comparison to stop intake of new batches". Your change also splits these concepts, that's why I remembered about it.

[natalya-aksman]

The point is that the current execution logic transparently handles the case when you have to check the segmentby. That's why we can start with a simple planner change, that works "well enough" by itself and then work on optimizing the execution side. Do I get this right?

That's a valid point. Yes, I can split planner changes only into a separate PR, and execution optimizations can come in the next PR and be under a GUC for good measure. Could be a separate exec method for segmentwise BSM, which will only compare non-segmentby fields on a heap.

LLM Fuzzer found a bug with copying HeapEntry for segmentby keys by value only, that's why I split SegmentEntryColumn from HeapEntryColumn, and that execution change will have to stay I think.
I cannot find that LLM Fuzzer report now, but hopefully it will flag the issue again if I put segment key values into HeapEntryColumn again in a split PR.
And hopefully I can reproduce it for orderby columns and then fix it as a long-standing BSM bug.

I.e., action plan: new PR with only planning changes plus tests, and then we go from there.

[natalya-aksman]

Actually it's simpler to use this PR for planner-only changes as it has all the new tests.
The new PR with executor optimizations can be added later.

[natalya-aksman]

Also, that LLM Fuzzer bug was about a different thing. It's OK to copy by value into HeapEntries as they are just pointers into decompressed tuples fields used for comparison.

[natalya-aksman]

I need to update cost estimate for segmentwise BSM, I forgot about it. -- DONE.

[natalya-aksman]

This is just to avoid unnecessary checks during planning for compressed metadata columns i.e. for columns with destination_attno <= 0.