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-02T22:52:49Z
Lists: pgsql-hackers 
On Sat, Mar 2, 2024 at 5:41 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
>
>
> On 3/2/24 23:28, Melanie Plageman wrote:
> > On Sat, Mar 2, 2024 at 10:05 AM Tomas Vondra
> > <tomas.vondra@enterprisedb.com> wrote:
> >>
> >> Here's a PDF with charts for a dataset where the row selectivity is more
> >> correlated to selectivity of pages. I'm attaching the updated script,
> >> with the SQL generating the data set. But the short story is all rows on
> >> a single page have the same random value, so the selectivity of rows and
> >> pages should be the same.
> >>
> >> The first page has results for the original "uniform", the second page
> >> is the new "uniform-pages" data set. There are 4 charts, for
> >> master/patched and 0/4 parallel workers. Overall the behavior is the
> >> same, but for the "uniform-pages" it's much more gradual (with respect
> >> to row selectivity). I think that's expected.
> >
> > Cool! Thanks for doing this. I have convinced myself that Thomas'
> > forthcoming patch which will eliminate prefetching with eic = 0 will
> > fix the eic 0 blue line regressions. The eic = 1 with four parallel
> > workers is more confusing. And it seems more noticeably bad with your
> > randomized-pages dataset.
> >
> > Regarding your earlier question:
> >
> >> Just to be sure we're on the same page regarding what eic=1 means,
> >> consider a simple sequence of pages: A, B, C, D, E, ...
> >>
> >> With the current "master" code, eic=1 means we'll issue a prefetch for B
> >> and then read+process A. And then issue prefetch for C and read+process
> >> B, and so on. It's always one page ahead.
> >
> > Yes, that is what I mean for eic = 1
> >
> >> As for how this is related to eic=1 - I think my point was that these
> >> are "adversary" data sets, most likely to show regressions. This applies
> >> especially to the "uniform" data set, because as the row selectivity
> >> grows, it's more and more likely it's right after to the current one,
> >> and so a read-ahead would likely do the trick.
> >
> > No, I think you're right that eic=1 should prefetch. As you say, with
> > high selectivity, a bitmap plan is likely not the best one anyway, so
> > not prefetching in order to preserve the performance of those cases
> > seems silly.
> >
>
> I was just trying to respond do this from an earlier message:
>
> > 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.
>
> And in that same message you also said "Not doing prefetching with eic 1
> actually seems like the right behavior". Hence my argument we should not
> stop prefetching for eic=1.
>
> But maybe I'm confused - it seems agree eic=1 should prefetch, and that
> uniform data set may not be a good argument against that.

Yep, we agree. I was being confusing and wrong :) I just wanted to
make sure the thread had a clear consensus that, yes, it is the right
thing to do to prefetch blocks for bitmap heap scans when
effective_io_concurrency = 1.

- 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.