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
diff --git a/doc/src/sgml/indices.sgml b/doc/src/sgml/indices.sgml
index 55f39b0df2f..e590727646f 100644
--- a/doc/src/sgml/indices.sgml
+++ b/doc/src/sgml/indices.sgml
@@ -1167,7 +1167,11 @@ CREATE INDEX mytable_cat_data ON mytable (category, data);
<para>
The index type must support index-only scans. B-tree indexes always
do. GiST and SP-GiST indexes support index-only scans for some
- operator classes but not others. Other index types have no support.
+ operator classes but not others, and never for
+ <quote>nearest-neighbor</quote> searches, that is, scans that return
+ rows in the order of an ordering operator (see
+ <xref linkend="indexes-type-gist"/> and
+ <xref linkend="indexes-type-spgist"/>). Other index types have no support.
The underlying requirement is that the index must physically store, or
else be able to reconstruct, the original data value for each index
entry. As a counterexample, GIN indexes cannot support index-only
diff --git a/doc/src/sgml/perform.sgml b/doc/src/sgml/perform.sgml
index 4f3b4e76078..df684a2051a 100644
--- a/doc/src/sgml/perform.sgml
+++ b/doc/src/sgml/perform.sgml
@@ -1108,8 +1108,9 @@ EXPLAIN (ANALYZE, IO, BUFFERS OFF) SELECT * FROM tbl WHERE id < 100000 AND id
<literal>I/O</literal> line is shown only when the prefetching performed at
least one read I/O operation. A scan that never started any read-ahead in
the first place shows neither line; an index scan does not begin
- prefetching until it has moved between several distinct table blocks, so
- a scan whose rows all come from one or two table blocks never does.
+ prefetching until it has switched from one table block to another several
+ times, so a scan that fetches only a few rows, or whose rows all come from
+ the same table block, never does.
</para>
<para>
diff --git a/src/backend/access/heap/heapam_indexscan.c b/src/backend/access/heap/heapam_indexscan.c
index 3489a749740..ba87122dad9 100644
--- a/src/backend/access/heap/heapam_indexscan.c
+++ b/src/backend/access/heap/heapam_indexscan.c
@@ -687,8 +687,10 @@ heapam_index_heap_fetch(IndexScanDesc scan, IndexScanHeapData *hscan,
hscan->xs_blkswitch_count++;
/*
- * Once enough distinct blocks are fetched, start prefetching (though
- * only if we haven't already).
+ * Once the scan has switched heap blocks enough times, start
+ * prefetching (though only if we haven't already). Note that this
+ * counts switches, not distinct blocks: a scan that alternates
+ * between two heap blocks gets there too.
*
* The counter can overflow. This is harmless because we only need it
* for this and for xs_visited_pages_limit accounting.
