Re: automatic analyze: readahead - add "IO read time" log message - Mailing list pgsql-hackers

On 13/01/2021 23:17, Stephen Frost wrote:
> Would be great to get a review / comments from others as to if there's
> any concerns.  I'll admit that it seems reasonably straight-forward to
> me, but hey, I wrote most of it, so that's not really a fair
> assessment... ;)

Look good overall. A few minor comments:

The patch consists of two part: add stats to the log for auto-analyze, 
and implement prefetching. They seem like independent features, consider 
splitting into two patches.

It's a bit weird that you get more stats in the log for 
autovacuum/autoanalyze than you get with VACUUM/ANALYZE VERBOSE. Not 
really this patch's fault though.

This conflicts with the patch at 
https://commitfest.postgresql.org/31/2907/, to refactor the table AM 
analyze API. That's OK, it's straightforward to resolve regardless of 
which patch is committed first.

>     /* Outer loop over blocks to sample */
>     while (BlockSampler_HasMore(&bs))
>     {
> #ifdef USE_PREFETCH
>         BlockNumber prefetch_targblock = InvalidBlockNumber;
> #endif
>         BlockNumber targblock = BlockSampler_Next(&bs);
> 
> #ifdef USE_PREFETCH
> 
>         /*
>          * Make sure that every time the main BlockSampler is moved forward
>          * that our prefetch BlockSampler also gets moved forward, so that we
>          * always stay out ahead.
>          */
>         if (BlockSampler_HasMore(&prefetch_bs))
>             prefetch_targblock = BlockSampler_Next(&prefetch_bs);
> #endif
> 
>         vacuum_delay_point();
> 
>         if (!table_scan_analyze_next_block(scan, targblock, vac_strategy))
>             continue;
> 
> #ifdef USE_PREFETCH
> 
>         /*
>          * When pre-fetching, after we get a block, tell the kernel about the
>          * next one we will want, if there's any left.
>          */
>         if (effective_io_concurrency && prefetch_targblock != InvalidBlockNumber)
>             PrefetchBuffer(scan->rs_rd, MAIN_FORKNUM, prefetch_targblock);
> #endif
> 
>         while (table_scan_analyze_next_tuple(scan, OldestXmin, &liverows, &deadrows, slot))
>         {
>             ...
>         }
> 
>         pgstat_progress_update_param(PROGRESS_ANALYZE_BLOCKS_DONE,
>                                      ++blksdone);
>     }

If effective_io_concurrency == 0, this calls 
BlockSampler_Next(&prefetch_bs) anyway, which is a waste of cycles.

If table_scan_analyze_next_block() returns false, we skip the 
PrefetchBuffer() call. That seem wrong.

Is there any potential harm from calling PrefetchBuffer() on a page that 
table_scan_analyze_next_block() later deems as unsuitable for smapling 
and returns false? That's theoretical at the moment, because 
heapam_scan_analyze_next_block() always returns true. (The tableam 
ANALYZE API refactor patch will make this moot, as it moves this logic 
into the tableam's implementation, so the implementation can do whatever 
make sense for the particular AM.)

- Heikki