On Fri, Feb 8, 2019 at 5:18 AM Amit Khandekar <amitdkhan...@gmail.com> wrote: > In the attached v1 patch, the prefetch_distance is calculated as > effective_io_concurrency + 10. Also it has some cosmetic changes.
I did a little brief review of this patch and noticed the following things. +} PrefetchState; That name seems too generic. +/* + * An arbitrary way to come up with a pre-fetch distance that grows with io + * concurrency, but is at least 10 and not more than the max effective io + * concurrency. + */ This comment is kinda useless, because it only tells us what the code does (which is obvious anyway) and not why it does that. Saying that your formula is arbitrary may not be the best way to attract support for it. + for (i = prefetch_state->next_item; i < nitems && count < prefetch_count; i++) It looks strange to me that next_item is stored in prefetch_state and nitems is passed around as an argument. Is there some reason why it's like that? + /* prefetch a fixed number of pages beforehand. */ Not accurate -- the number of pages we prefetch isn't fixed. It depends on effective_io_concurrency. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company