Horton

Re: BitmapHeapScan streaming read user and prelim refactoring

Tomas Vondra <tomas@vondra.me>

View thread View on postgr.es
From: Tomas Vondra <tomas@vondra.me>
To: Melanie Plageman <melanieplageman@gmail.com>, 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-16T15:09:13Z
Lists: pgsql-hackers 
Hi,

I took a quick look on the first couple parts of this series, up to
0007. I only have a couple minor review comments:

1) 0001

I find it a bit weird that we use ntuples to determine if it's exact or
lossy. Not an issue caused by this patch, of course, but maybe we could
improve that somehow? I mean this:

    if (tbmres->ntuples >= 0)
        (*exact_pages)++;
    else
        (*lossy_pages)++;

Also, maybe consider manually wrapping long lines - I haven't tried, but
I guess pgindent did not wrap this. I mean this line, for example:

... whitespace ... &node->stats.lossy_pages, &node->stats.exact_pages))


2) 0003

The "tidr" name is not particularly clear, IMO. Maybe just "range" would
be better?

+  struct
+  {
+      ItemPointerData rs_mintid;
+      ItemPointerData rs_maxtid;
+  }   tidr;

Isn't it a bit strange that this patch remove tmbres from
heapam_scan_bitmap_next_block, when it was already removed from
table_scan_bitmap_next_tuple in the previous commit? Does it make sense
to separate it like this? (Maybe it does, not sure.)

I find this not very clear:

+ *    recheck     do current page's tuples need recheck
+ *    blockno     used to validate pf and current block in sync
+ *    pfblockno   used to validate pf stays ahead of current block

The "blockno" sounds weird - shouldn't it say "are in sync"? Also, not
clear what "pf" stands for without context (sure, it's "prefetch"). But
Why not to not write "prefetch_blockno" or something like that?


3) 0006

Seems unnecessary to keep this as separate commit, it's  just log
message improvement. I'd merge it to the earlier patch.

4) 0007

Seems fine to me in principle. This adds "subclass" similar to what we
do for plan nodes, except that states are not derived from Node. But
it's the same approach.

Why not to do

    scan = (HeapScanDesc *) bscan;

instead of

    scan = &bscan->rs_heap_base;

I think the simple cast is what we do for the plan nodes, and we even do
it this way in the opposite direction in a couple places:

    BitmapHeapScanDesc *bscan = (BitmapHeapScanDesc *) scan;

BTW would it be a good idea for heapam_scan_bitmap_next_block to check
the passed "scan" has SO_TYPE_BITMAPSCAN? We haven't been checking that
so far I think, but we only had a single struct ...


regards

-- 
Tomas Vondra

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.