Re: WIP: WAL prefetch (another approach) - Mailing list pgsql-hackers

On Wed, Apr 8, 2020 at 11:27 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Wed, Apr 8, 2020 at 12:52 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> > * he gave some feedback on the read_local_xlog_page() modifications: I
> > probably need to reconsider the change to logical.c that passes NULL
> > instead of cxt to the read_page callback; and the switch statement in
> > read_local_xlog_page() probably should have a case for the preexisting
> > mode
>
> So... logical.c wants to give its LogicalDecodingContext to any
> XLogPageReadCB you give it, via "private_data"; that is, it really
> only accepts XLogPageReadCB implementations that understand that (or
> ignore it).  What I want to do is give every XLogPageReadCB the chance
> to have its own state that it is control of (to receive settings
> specific to the implementation, or whatever), that you supply along
> with it.  We can't do both kinds of things with private_data, so I
> have added a second member read_page_data to XLogReaderState.  If you
> pass in read_local_xlog_page as read_page, then you can optionally
> install a pointer to XLogReadLocalOptions as reader->read_page_data,
> to activate the new behaviours I added for prefetching purposes.
>
> While working on that, I realised the readahead XLogReader was
> breaking a rule expressed in XLogReadDetermineTimeLine().  Timelines
> are really confusing and there were probably several subtle or not to
> subtle bugs there.  So I added an option to skip all of that logic,
> and just say "I command you to read only from TLI X".  It reads the
> same TLI as recovery is reading, until it hits the end of readable
> data and that causes prefetching to shut down.  Then the main recovery
> loop resets the prefetching module when it sees a TLI switch, so then
> it starts up again.  This seems to work reliably, but I've obviously
> had limited time to test.  Does this scheme sound sane?
>
> I think this is basically committable (though of course I wish I had
> more time to test and review).  Ugh.  Feature freeze in half an hour.

Ok, so the following parts of this work have been committed:

b09ff536:  Simplify the effective_io_concurrency setting.
fc34b0d9:  Introduce a maintenance_io_concurrency setting.
3985b600:  Support PrefetchBuffer() in recovery.
d140f2f3:  Rationalize GetWalRcv{Write,Flush}RecPtr().

However, I didn't want to push the main patch into the tree at
(literally) the last minute after doing such much work on it in the
last few days, without more review from recovery code experts and some
independent testing.  Judging by the comments made in this thread and
elsewhere, I think the feature is in demand so I hope there is a way
we could get it into 13 in the next couple of days, but I totally
accept the release management team's prerogative on that.