Re: BitmapHeapScan streaming read user and prelim refactoring
Andres Freund <andres@anarazel.de>
Hi, On 2025-04-14 09:58:19 -0700, James Hunter wrote: > Of course, late feedback is a burden, but I think our discussion here > (and, in the future, if you try to "ReadStream" BTree index pages, > themselves) illustrates my point. FWIW, it's quite conceivable that we'll want to use non-readstream prefetching for some cases where "heuristic" prefetching is easier. I think prefetching of btree pages is one of those cases. The read stream based API provides a single point of adjusting the readahead logic, implementing read-combining etc, without needing to know about that in the user of the read stream. > I see two orthogonal problems, in processing Bitmap Heap pages in > parallel: (1) we need to prefetch enough pages, far enough in advance, > to hide read latency; (2) later, every parallel worker needs to be > given a set of pages to process, in a way that minimizes contention. > > The easiest way to hand out work to parallel workers (and often the > best) is to maintain a single, shared, global work queue. Just put > whatever pages you prefetch into a FIFO queue, and let each worker > pull one piece of "work" off that queue. In this was, there's no > "ramp-down" problem. If you just issue prefetch requests separately you'll get no read combining - and it turns out that that is a really rather significant loss, both on the storage layer and just due to the syscall overhead. So you do need to perform batching when issuing IO. Which in turn requires a bit of rampup logic etc. > If you find that contention on this shared queue becomes a bottleneck, > then you just pull *n* pieces of work, in a batch. Then the > "ramp-down" problem is limited to "n", instead of just 1. Often, one > can find a suitable value of n that simultaneously makes contention > effectively zero, while avoiding "ramp-down" problems; say n = 10. > > So much for popping from the shared queue. Pushing to the shared queue > is also easy, because you have async reads. Anytime a worker pops a > (batch of) work item(s) off the shared queue, it checks to see if the > queue is still large enough. If not, it issues the appropriate > prefetch / "ReadStream" calls. > > A single shared queue is easiest, but sometimes there's no way to > prevent it from becoming a bottleneck. In that case, one typically > partitions the input at startup, gives each worker its own partition, > and waits for all workers to complete. In this, second, model, workers > are entirely independent, so there is no contention: we scale out > perfectly. The problem, as you've pointed out, is that one worker > might finish its own work long before another; and then the worker > that finished its work is idle and therefore wasted. > > This is why a single shared queue is so nice, because it avoids > workers being idle. But I am confused by your proposal, which seems to > be trying to get the behavior of a single shared queue, but > implemented with the added complexity of multiple queues. > > Why not just use a single queue? Accessing buffers in a maximally interleaved way, which is what a single queue would give you, adds a good bit of overhead when you have a lot of memory, because e.g. TLB hit rate is minimized. > > These are now > > real IOs running in the background and for the *exact* blocks you will > > consume; posix_fadvise() was just a stepping towards AIO that > > tolerated sloppy synchronisation including being entirely wrong. > > It has never been clear to me why prefetching the exact blocks you'll > later consume is seen as a *benefit*, rather than a *cost*. I'm not > aware of any prefetch interface, other than PG's "ReadStream," that > insists on this. But that's a separate discussion... It (randomly ordered): a) reduces wasted IO b) makes things like IO combining a lot easier c) makes it a lot easier to adaptively control readahead distance, because you have information about 1) the # in-flight IOs 2) the # completed IOs 3) current position 4) reada d) provides *per-stream* control over readahead. You want to be only as aggressive about prefetching as you need to be, which very well can differ between different parts of a query tree. e) provides one central point to implement readahead logic As I said above, that's not to say that we'll only ever want to do readahead via a the read stream interface. Greetings, Andres Freund
Commits
-
Fix bitmapheapscan incorrect recheck of NULL tuples
- aea916fe555a 18.0 landed
-
Increase default maintenance_io_concurrency to 16
- cc6be07ebde2 18.0 landed
-
Separate TBM[Shared|Private]Iterator and TBMIterateResult
- 944e81bf99db 18.0 landed
-
Improve read_stream.c advice for dense streams.
- 7ea8cd15661e 18.0 landed
-
Increase default effective_io_concurrency to 16
- ff79b5b2aba0 18.0 landed
-
Delay extraction of TIDBitmap per page offsets
- bfe56cdf9a4e 18.0 landed
-
Add lossy indicator to TBMIterateResult
- b8778c4cd8bc 18.0 landed
-
Move BitmapTableScan per-scan setup into a helper
- a5358c14b2fe 18.0 landed
-
Add and use BitmapHeapScanDescData struct
- f7a8fc10ccb8 18.0 landed
-
Fix bitmap table scan crash on iterator release
- 754c610e13b8 18.0 landed
-
Bitmap Table Scans use unified TBMIterator
- 1a0da347a7ac 18.0 landed
-
Add common interface for TBMIterators
- 7f9d4187e7ba 18.0 landed
-
Make table_scan_bitmap_next_block() async-friendly
- de380a62b5da 18.0 landed
-
Move EXPLAIN counter increment to heapam_scan_bitmap_next_block
- 7bd7aa4d3067 18.0 landed
-
Refactor tidstore.c iterator buffering.
- f6bef362cac8 18.0 cited
-
BitmapHeapScan: Remove incorrect assert and reset field
- a3e6c6f92991 17.0 landed
-
Change BitmapAdjustPrefetchIterator to accept BlockNumber
- 92641d8d651e 17.0 landed
-
BitmapHeapScan: Use correct recheck flag for skip_fetch
- 1fdb0ce9b109 17.0 landed
-
BitmapHeapScan: Push skip_fetch optimization into table AM
- 04e72ed617be 17.0 landed
-
BitmapHeapScan: postpone setting can_skip_fetch
- fe1431e39cdd 17.0 landed
-
BitmapHeapScan: begin scan after bitmap creation
- 1577081e9614 17.0 landed
-
Fix EXPLAIN Bitmap heap scan to count pages with no visible tuples
- f3e4581acdc8 12.19 landed
- 992189a3e94d 13.15 landed
- 262757b73286 14.12 landed
- d3d95f583995 15.7 landed
- 1f4eb734200a 16.3 landed
- 0960ae1967d0 17.0 landed
-
Remove redundant snapshot copying from parallel leader to workers
- 84c18acaf690 17.0 landed
-
Remove some obsolete smgrcloseall() calls.
- 6a8ffe812d19 17.0 cited
-
Remove the "snapshot too old" feature.
- f691f5b80a85 17.0 cited
-
Compute XID horizon for page level index vacuum on primary.
- 558a9165e081 12.0 cited