Re: Changing WAL Header to reduce contention during ReserveXLogInsertLocation()

On Wed, Mar 28, 2018 at 08:50:21PM -0400, Robert Haas wrote:
> On Tue, Mar 27, 2018 at 11:53 PM, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com> wrote:
>> Yeah, we should not do that. The patch surely does not intend to replay any
>> more WAL than what we do today. The idea is to just use a different
>> mechanism to find the prior checkpoint. But we should surely find the latest
>> prior checkpoint. In the rare scenario that Tom showed, we should just throw
>> an error and fix the patch if it's not doing that already.
>
> It's not clear to me that there is any reasonable fix short of giving
> up on this approach altogether. But I might be missing something.

One way to get things done right is to scan recursively segments instead
of records. Not impossible but this introduces more code churns in
pg_rewind's findLastCheckpoint which is now dead-simple as it just need
to look at things backwards. This way you can know as well what is the
real last checkpoint as you know as well what's the point where WAL has
forked. So for multiple checkpoints on the same segment, the lookup
just needs to happen up to the point where WAL has forked, while keeping
track of the last checkpoint record found.

If we would want to keep compatibility with backward searches, it may be
nice to keep a mostly-compatible API layer in xlogreader.c. Now I have
honestly not thought about that much. And there may be other tools
which rely on being able to do backward searches. Breaking
compatibility for them could cause wraith because that would mean less
availability when running a rewind-like operation.

This thread makes me wonder as well if we are overlooking other
things...
--
Michael