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
