On 3/8/21 8:42 PM, Stephen Frost wrote: > Greetings, > > * Tomas Vondra (tomas.von...@enterprisedb.com) wrote: >> On 2/10/21 11:10 PM, Stephen Frost wrote: >>> * Heikki Linnakangas (hlinn...@iki.fi) wrote: >>>> On 05/02/2021 23:22, Stephen Frost wrote: >>>>> Unless there's anything else on this, I'll commit these sometime next >>>>> week. >>>> >>>> One more thing: Instead of using 'effective_io_concurrency' GUC directly, >>>> should call get_tablespace_maintenance_io_concurrency(). >>> >>> Ah, yeah, of course. >>> >>> Updated patch attached. >> >> A couple minor comments: >> >> 1) I think the patch should be split into two parts, one adding the >> track_io_timing, one adding the prefetching. > > This was already done.. >
Not sure what you mean by "done"? I see the patch still does both changes related to track_io_timing and prefetching. >> 2) I haven't tried but I'm pretty sure there'll be a compiler warning >> about 'prefetch_maximum' being unused without USE_PREFETCH defined. > > Ah, that part is likely true, moved down into the #ifdef block to > address that, which also is good since it should avoid mistakenly using > it outside of the #ifdef's later on by mistake too. > OK >> 3) Is there a way to reduce the amount of #ifdef in acquire_sample_rows? > > Perhaps.. > >> This makes the code rather hard to read, IMHO. It seems to me we can >> move the code around a bit and merge some of the #ifdef blocks - see the >> attached patch. Most of this is fairly trivial, with the exception of >> moving PrefetchBuffer before table_scan_analyze_next_block - AFAIK this >> does not materially change the behavior, but perhaps I'm wrong. > > but I don't particularly like doing the prefetch right before we run > vacuum_delay_point() and potentially sleep. > Why? Is that just a matter of personal preference (fair enough) or is there a reason why that would be wrong/harmful? I think e.g. prefetch_targblock could be moved to the next #ifdef, which will eliminate the one-line ifdef. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
