Horton

Re: BitmapHeapScan streaming read user and prelim refactoring

Melanie Plageman <melanieplageman@gmail.com>

View thread View on postgr.es
From: Melanie Plageman <melanieplageman@gmail.com>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Andres Freund <andres@anarazel.de>, Pg Hackers <pgsql-hackers@postgresql.org>, Thomas Munro <thomas.munro@gmail.com>, Heikki Linnakangas <hlinnaka@iki.fi>, Nazir Bilal Yavuz <byavuz81@gmail.com>
Date: 2024-03-01T16:51:44Z
Lists: pgsql-hackers 
On Fri, Mar 1, 2024 at 9:05 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> On 3/1/24 02:18, Melanie Plageman wrote:
> > On Thu, Feb 29, 2024 at 6:44 PM Tomas Vondra
> > <tomas.vondra@enterprisedb.com> wrote:
> >>
> >> On 2/29/24 23:44, Tomas Vondra wrote:
> >> 1) On master there's clear difference between eic=0 and eic=1 cases, but
> >> on the patched build there's literally no difference - for example the
> >> "uniform" distribution is clearly not great for prefetching, but eic=0
> >> regresses to eic=1 poor behavior).
> >
> > Yes, so eic=0 and eic=1 are identical with the streaming read API.
> > That is, eic 0 does not disable prefetching. Thomas is going to update
> > the streaming read API to avoid issuing an fadvise for the last block
> > in a range before issuing a read -- which would mean no prefetching
> > with eic 0 and eic 1. Not doing prefetching with eic 1 actually seems
> > like the right behavior -- which would be different than what master
> > is doing, right?
>
> I don't think we should stop doing prefetching for eic=1, or at least
> not based just on these charts. I suspect these "uniform" charts are not
> a great example for the prefetching, because it's about distribution of
> individual rows, and even a small fraction of rows may match most of the
> pages. It's great for finding strange behaviors / corner cases, but
> probably not a sufficient reason to change the default.

Yes, I would like to see results from a data set where selectivity is
more correlated to pages/heap fetches. But, I'm not sure I see how
that is related to prefetching when eic = 1.

> I think it makes sense to issue a prefetch one page ahead, before
> reading/processing the preceding one, and it's fairly conservative
> setting, and I assume the default was chosen for a reason / after
> discussion.

Yes, I suppose the overhead of an fadvise does not compare to the IO
latency of synchronously reading that block. Actually, I bet the
regression I saw by accidentally moving BitmapAdjustPrefetchIterator()
after table_scan_bitmap_next_block() would be similar to the
regression introduced by making eic = 1 not prefetch.

When you think about IO concurrency = 1, it doesn't imply prefetching
to me. But, I think we want to do the right thing and have parity with
master.

> My suggestion would be to keep the master behavior unless not practical,
> and then maybe discuss changing the details later. The patch is already
> complicated enough, better to leave that discussion for later.

Agreed. Speaking of which, we need to add back use of tablespace IO
concurrency for the streaming read API (which is used by
BitmapHeapScan in master).

> > With very low selectivity, you are less likely to get readahead
> > (right?) and similarly less likely to be able to build up > 8kB IOs --
> > which is one of the main value propositions of the streaming read
> > code. I imagine that this larger read benefit is part of why the
> > performance is better at higher selectivities with the patch. This
> > might be a silly experiment, but we could try decreasing
> > MAX_BUFFERS_PER_TRANSFER on the patched version and see if the
> > performance gains go away.
>
> Sure, I can do that. Do you have any particular suggestion what value to
> use for MAX_BUFFERS_PER_TRANSFER?

I think setting it to 1 would be the same as always master -- doing
only 8kB reads. The only thing about that is that I imagine the other
streaming read code has some overhead which might end up being a
regression on balance even with the prefetching if we aren't actually
using the ranges/vectored capabilities of the streaming read
interface. Maybe if you just run it for one of the very obvious
performance improvement cases? I can also try this locally.

> I'll also try to add a better version of uniform, where the selectivity
> matches more closely to pages, not rows.

This would be great.

- Melanie

Commits

  1. Fix bitmapheapscan incorrect recheck of NULL tuples

  2. Increase default maintenance_io_concurrency to 16

  3. Separate TBM[Shared|Private]Iterator and TBMIterateResult

  4. Improve read_stream.c advice for dense streams.

  5. Increase default effective_io_concurrency to 16

  6. Delay extraction of TIDBitmap per page offsets

  7. Add lossy indicator to TBMIterateResult

  8. Move BitmapTableScan per-scan setup into a helper

  9. Add and use BitmapHeapScanDescData struct

  10. Fix bitmap table scan crash on iterator release

  11. Bitmap Table Scans use unified TBMIterator

  12. Add common interface for TBMIterators

  13. Make table_scan_bitmap_next_block() async-friendly

  14. Move EXPLAIN counter increment to heapam_scan_bitmap_next_block

  15. Refactor tidstore.c iterator buffering.

  16. BitmapHeapScan: Remove incorrect assert and reset field

  17. Change BitmapAdjustPrefetchIterator to accept BlockNumber

  18. BitmapHeapScan: Use correct recheck flag for skip_fetch

  19. BitmapHeapScan: Push skip_fetch optimization into table AM

  20. BitmapHeapScan: postpone setting can_skip_fetch

  21. BitmapHeapScan: begin scan after bitmap creation

  22. Fix EXPLAIN Bitmap heap scan to count pages with no visible tuples

  23. Remove redundant snapshot copying from parallel leader to workers

  24. Remove some obsolete smgrcloseall() calls.

  25. Remove the "snapshot too old" feature.

  26. Compute XID horizon for page level index vacuum on primary.