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: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Thomas Munro <thomas.munro@gmail.com>, Andres Freund <andres@anarazel.de>, Pg Hackers <pgsql-hackers@postgresql.org>, Nazir Bilal Yavuz <byavuz81@gmail.com>, Tomas Vondra <tv@fuzzy.cz>
Date: 2024-10-15T00:34:24Z
Lists: pgsql-hackers

Attachments

On Mon, Aug 26, 2024 at 9:37 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> v23-0003-Make-table_scan_bitmap_next_block-async-friendly.patch
>
> > @@ -1955,19 +1954,26 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths
>>
> > + * `blockno` is only used in bitmap table scan code to validate that the
> > + * prefetch block is staying ahead of the current block.
> >   *
> >   * Note, this is an optionally implemented function, therefore should only be
> >   * used after verifying the presence (at plan time or such).
> >   */
> >  static inline bool
> >  table_scan_bitmap_next_block(TableScanDesc scan,
> > -                                                      struct TBMIterateResult *tbmres,
> > +                                                      BlockNumber *blockno, bool *recheck,
> >                                                        long *lossy_pages,
> >                                                        long *exact_pages)
> >  {
>
> The new comment doesn't really explain what *blockno means. Is it an
> input or output parameter? How is it used with the prefetching?

I've updated this comment.

> v23-0004-Add-common-interface-for-TBMIterators.patch
>
> > +void
> > +tbm_begin_iterate(TBMIterator *iterator, TIDBitmap *tbm,
> > +                               dsa_area *dsa, dsa_pointer dsp)
> > +{
> > +     Assert(iterator);
> > +
> > +     iterator->private_iterator = NULL;
> > +     iterator->shared_iterator = NULL;
> > +     iterator->exhausted = false;
> > +
> > +     /* Allocate a private iterator and attach the shared state to it */
> > +     if (DsaPointerIsValid(dsp))
> > +             iterator->shared_iterator = tbm_attach_shared_iterate(dsa, dsp);
> > +     else
> > +             iterator->private_iterator = tbm_begin_private_iterate(tbm);
> > +}
>
> Hmm, I haven't looked at how this is used the later patches, but a
> function signature where some parameters are used or not depending on
> the situation seems a bit awkward. Perhaps it would be better to let the
> caller call tbm_attach_shared_iterate(dsa, dsp) or
> tbm_begin_private_iterate(tbm), and provide a function to turn that into
> a TBMIterator? Something like:
>
> TBMIterator *tbm_iterator_from_shared_iterator(TBMSharedIterator *);
> TBMIterator *tbm_iterator_from_private_iterator(TBMPrivateIterator *);

I thought about this a lot. I've changed the struct a bit but have
left the tbm_begin_iterate() interface mostly the same. I actually
think having some parameters that are used only for parallel and some
only for serial is better than exposing the private/shared versions of
the iterators only for tbm_begin_iterate() (which I would have to do
if the caller calls
tbm_attach_shared_iterate()/tbm_begin_private_iterate() directly).

> Does tbm_iterator() really need the 'exhausted' flag? The private and
> shared variants work without one.

I got rid of this.

> v23-0006-BitmapHeapScan-uses-unified-iterator.patch
>
> Makes sense. (Might be better to squash this with the previous patch)

Done.

> v23-0008-Push-current-scan-descriptor-into-specialized-sc.patch
> v23-0009-Remove-ss_current-prefix-from-ss_currentScanDesc.patch
>
> LGTM. I would squash these together.

I wanted to make a change that was lower impact than a new top level
scan descriptor. So, I made a few changes to the design to allow that.
Those rendered v23-0008 and v23-0009 moot.

See v24-0003 and v24-0007 for the new design. I could use input on
whether or not this strategy seems acceptable.

> v23-0010-Add-scan_in_progress-to-BitmapHeapScanState.patch
>
> > --- a/src/include/nodes/execnodes.h
> > +++ b/src/include/nodes/execnodes.h
> > @@ -1804,6 +1804,7 @@ typedef struct ParallelBitmapHeapState
> >   *           prefetch_target    current target prefetch distance
> >   *           prefetch_maximum   maximum value for prefetch_target
> >   *           initialized                is node is ready to iterate
> > + *           scan_in_progress   is this a rescan
> >   *           pstate                     shared state for parallel bitmap scan
> >   *           recheck                    do current page's tuples need recheck
> >   *           blockno                    used to validate pf and current block in sync
> > @@ -1824,6 +1825,7 @@ typedef struct BitmapHeapScanState
> >       int                     prefetch_target;
> >       int                     prefetch_maximum;
> >       bool            initialized;
> > +     bool            scan_in_progress;
> >       ParallelBitmapHeapState *pstate;
> >       bool            recheck;
> >       BlockNumber blockno;
>
> Hmm, the "is this a rescan" comment sounds inaccurate, because it is set
> as soon as the scan is started, not only when rescanning. Other than
> that, LGTM.

It turns out I didn't need scan_in_progress once I rearranged some
things anyway.

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