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: Heikki Linnakangas <hlinnaka@iki.fi>, Andres Freund <andres@anarazel.de>, Pg Hackers <pgsql-hackers@postgresql.org>, Thomas Munro <thomas.munro@gmail.com>, Nazir Bilal Yavuz <byavuz81@gmail.com>
Date: 2024-03-25T16:07:09Z
Lists: pgsql-hackers

Attachments

On Sun, Mar 24, 2024 at 06:37:20PM -0400, Melanie Plageman wrote:
> On Sun, Mar 24, 2024 at 5:59 PM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
> >
> > BTW when you say "up to 'Make table_scan_bitmap_next_block() async
> > friendly'" do you mean including that patch, or that this is the first
> > patch that is not one of the independently useful patches.
> 
> I think the code is easier to understand with "Make
> table_scan_bitmap_next_block() async friendly". Prior to that commit,
> table_scan_bitmap_next_block() could return false even when the bitmap
> has more blocks and expects the caller to handle this and invoke it
> again. I think that interface is very confusing. The downside of the
> code in that state is that the code for prefetching is still in the
> BitmapHeapNext() code and the code for getting the current block is in
> the heap AM-specific code. I took a stab at fixing this in v9's 0013,
> but the outcome wasn't very attractive.
> 
> What I will do tomorrow is reorder and group the commits such that all
> of the commits that are useful independent of streaming read are first
> (I think 0014 and 0015 are independently valuable but they are on top
> of some things that are only useful to streaming read because they are
> more recently requested changes). I think I can actually do a bit of
> simplification in terms of how many commits there are and what is in
> each. Just to be clear, v9 is still reviewable. I am just going to go
> back and change what is included in each commit.

So, attached v10 does not include the new version of streaming read API.
I focused instead on the refactoring patches commit regrouping I
mentioned here.

I realized "Remove table_scan_bitmap_next_block()" can't easily be moved
down below "Push BitmapHeapScan prefetch code into heapam.c" because we
have to do BitmapAdjustPrefetchTarget() and
BitmapAdjustPrefetchIterator() on either side of getting the next block
(via table_scan_bitmap_next_block()).

"Push BitmapHeapScan prefetch code into heapam.c" isn't very nice
because it adds a lot of bitmapheapscan specific members to
TableScanDescData and HeapScanDescData. I thought about wrapping all of
those members in some kind of BitmapHeapScanTableState struct -- but I
don't like that because the members are spread out across
HeapScanDescData and TableScanDescData so not all of them would go in
BitmapHeapScanTableState. I could move the ones I put in
HeapScanDescData back into TableScanDescData and then wrap that in a
BitmapHeapScanTableState. I haven't done that in this version.

I did manage to move "Unify parallel and serial BitmapHeapScan iterator
interfaces" down below the line of patches which are only useful if
the streaming read user also goes in.

In attached v10, all patches up to and including "Unify parallel and
serial BitmapHeapScan iterator interfaces" (0010) are proposed for
master with or without the streaming read API.

0010 does add additional indirection and thus pointer dereferencing for
accessing the iterators, which doesn't feel good. But, it does simplify
the code.

Perhaps it is worth renaming the existing TableScanDescData->rs_parallel
(a ParallelTableScanDescData) to something like rs_seq_parallel. It is
only for sequential scans and scans of tables when building indexes but
the comments say it is for parallel scans in general. There is a similar
member in HeapScanDescData called rs_parallelworkerdata.

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