Re: index prefetching

Hi,

I went through this patch series again, mostly to learn about the
various recent improvements, look for stale comments/docs, etc. And to
see if something seems "surprising".

I only found a couple minor points;

1) I got a bit confused by tableam_index_pos_advance and how it
interacts with tableam_index_pos_advance, and how it affects the "valid"
flag of the position. I mean that:

- if tableam_index_pos_advance succeeds, the position keeps valid=true
(which seems logical) and points at a valid item in the batch

- if tableam_index_pos_advance fails, the position still has valid=true
(which may seem a bit surprising), and we do different things. If the
ringbuffer is full (or we're throttling), we undo the position advance
(thus making it point at a valid item again). But for BATCH_POS_DONE
we don't do that, we leave it pointing kinda "out of bounds".

The BatchRingItemPos definition says:

/* Position references a valid IndexScanDescData.batchbuf[] entry? */
bool valid;

and this seems to contradict the spirit of that, no? Because we're
leaving the position with valid=true, yet it's pointing outside the
current batch.

I'm not claiming this is broken, I just keep getting confused/distracted
by this. I recall we discussed it in the past (a couple months back?).
Maybe it'd be worth a comment somewhere?

2) The explanation (both in docs and comments) when we start prefetching
was talking about number of distinct blocks, but that's quite not what
xs_blkswitch_count counts. [prefetch-start-docs.patch]

3) It's probably worth mentioning that nearest-neighbor searches disable
index-only scans in the relevant sgml docs. [gist-ios-ordering.patch]

4) I struggle to understand why read_stream_grow_repeats needs +1
elements in stream->repeats. It seems pretty confusing, and also
"infecting" various other places with the +1. Smells like off-by-one
bugs in the future.

5) I noticed show_scan_io_usage may not be sufficiently careful about
checking iss_Instrument. I've been unable to make it fail with the
in-core scans, but I suspect it might dereference NULL with custom
scans or whatever. It probably should do the checks similarly to
show_indexscan_info. That's my patch, so it's up to me to get that
fixed (that patch probably needs more improvements).

I've been running a bunch of stress tests and benchmarks, looking for
crashes and regressions, but that found nothing significant so far.

regards

--
Tomas Vondra