Re: pg_rewind docs correction

On Tue, Apr 28, 2020 at 12:31 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Mon, Mar 09, 2020 at 09:26:17AM -0400, James Coleman wrote:
> >> - <filename>pg_stat_tmp/</filename>, and
> >> - <filename>pg_subtrans/</filename> are omitted from the data copied
> >> - from the source cluster. Any file or directory beginning with
> >> - <filename>pgsql_tmp</filename> is omitted, as well as are
> >> + <filename>pg_stat_tmp/</filename>, and
> >> <filename>pg_subtrans/</filename>
> >> + are omitted from the data copied from the source cluster. The files
> >>
> >> This is just reorganizing an existing list, why?
> >>
> >
> > The grammar seemed a bit awkward to me, so while I was already reworking
> > this paragraph I tried to clean that up a bit.
>
> Thanks for the new patch, and sorry for the delay.
>
> Okay, I saw what you were coming at here, with one sentence for
> directories, and one for files.
>
> > Still ongoing, correct? I guess I mentally think of them as being only one
> > month, but I guess that's not actually true. Regardless I'm not sure what
> > policy is for patches that have been in flight in hackers for a while but
> > just missed being added to the CF app.
>
> This is a documentation patch, so improving this part of the docs now
> is fine by me, particularly as this is an improvement. Here are more
> notes from me:
> - I have removed the "As with a base backup" at the beginning of the
> second paragraph you modified. The first paragraph modified already
> references a base backup, so one reference is enough IMO.
> - WAL replay does not happen from the WAL position where WAL diverged,
> but from the last checkpoint before WAL diverged.
> - Did some tweaks about the new part for configuration files, as it
> may actually not be necessary to update the configuration for recovery
> to complete (depending on the settings of the source, the target may
> just require the creation of a standby.signal file in its data
> directory particularly with a common archive location for multiple
> clusters).
> - Some word-smithing in the step-by-step description.
>
> Is the updated version fine for you?

In your revised patched the follow paragraph:

+ <para>
+ As <application>pg_rewind</application> copies configuration files
+ entirely from the source, it may be required to correct the configuration
+ used for recovery before restarting the target server, especially the
+ the target is reintroduced as a standby of the source. If you restart
+ the server after the rewind operation has finished but without configuring
+ recovery, the target may again diverge from the primary.
+ </para>

I think is missing a word. Instead of "especially the the target"
should be "especially if the target".

In this block:

+ Create a <filename>backup_label</filename> file to begin WAL replay at
+ the checkpoint created at failover and configure the
+ <filename>pg_control</filename> file with a minimum consistency LSN
+ defined as the result of <literal>pg_current_wal_insert_lsn()</literal>
+ when rewinding from a live source and using the last checkpoint LSN
+ when rewinding from a stopped source.
+ </para>

Perhaps change "and using the last checkpoint LSN" to "or the last
checkpoint LSN". Alternatively you could make the grammar parallel by
changing to "and defined as the last checkpoint LSN", but that seems
wordy, and the "defined as [item or item]" is already a good grammar
construction.

Other than those two small things, your proposed revision looks good to me.

Thanks,
James