On 08/19/2014 11:07 AM, Michael Paquier wrote:
> Patch 1 looks good. The main difference with the v1 submitted a couple of
> days back is that the global variable approach is replaced with additional
> arguments for the LSN position and record pointer in XLogReplayBuffer. I
> have as well run a couple of tests with the page comparison tool, done some
> tests based on installcheck-world with a slave replaying WAL behind, and
> found no issues with it.
Thanks!
> Perhaps we could consider pushing it to facilitate the next work? Even if
> the second patch is dropped it is still a win IMO to have backup block
> replay managed within a single function (being it named XLogReplayBuffer in
> latest patch), and having it return a status flag.
Yeah, that was my plan.
Regarding the name, the following have been suggested so far:
XLogReplayBuffer
XLogRestoreBuffer
XLogRecoverBuffer
XLogReadBufferForReplay
ReadBufferForXLogReplay
One more idea:
XLogRedoBuffer (would be like three first options above, but would match
that we call the functions that call this "redo" functions)
I think XLogReadBufferForReplay is the most descriptive. Andres and
Alvaro both suggested it - independently I believe - so that seems to
come out naturally. But maybe make it XLogReadBufferForRedo, since we
call the redo functions redo functions and not replay functions.
Yet another option is to just call it XLogReadBuffer, and rename the
existing XLogReadBuffer to something else. With the 2nd patch, there are
only a few callers of XLogReadBuffer left. But is it too deceitful if
XLogReadBuffer doesn't merely read the page, but also sometimes replaces
it with a full-page image? Maybe it's OK..
Barring objections or better ideas, I'm leaning towards
XLogReadBufferForRedo.
- Heikki
