Re: pg_upgrade instructions involving "rsync --size-only" might lead to standby corruption?

Hi,

On Mon, Jul 10, 2023 at 02:37:24PM -0700, Nikolay Samokhvalov wrote:
> On Mon, Jul 10, 2023 at 2:02 PM Michael Banck <mbanck(at)gmx(dot)net> wrote:
> > > +++ b/doc/src/sgml/ref/pgupgrade.sgml
> > > @@ -380,22 +380,28 @@ NET STOP postgresql-&majorversion;
> > > </para>
> > >
> > > <para>
> > > - Streaming replication and log-shipping standby servers can
> > > + Streaming replication and log-shipping standby servers must
> > > remain running until a later step.
> > > </para>
> > > </step>
> > >
> > > - <step>
> > > + <step id="pgupgrade-step-prepare-standbys">
> > >
> > > <para>
> > > - If you are upgrading standby servers using methods outlined in
> > section <xref
> > > - linkend="pgupgrade-step-replicas"/>, verify that the old standby
> > > - servers are caught up by running
> > <application>pg_controldata</application>
> > > - against the old primary and standby clusters. Verify that the
> > > - <quote>Latest checkpoint location</quote> values match in all
> > clusters.
> > > - (There will be a mismatch if old standby servers were shut down
> > > - before the old primary or if the old standby servers are still
> > running.)
> > > + If you are upgrading standby servers using methods outlined in
> > > + <xref linkend="pgupgrade-step-replicas"/>,
> >
> > You dropped the "section" before the xref, I think that should be kept
> > around.
>
> Seems to be a problem in discussing source code that looks quite different
> than the final result.
>
> In the result – the docs – we currently have "section Step 9", looking
> weird. I still think it's good to remove it. We also have "in Step 17
> below" (without the extra word "section") in a different place on the same
> page.

Ok.

> > > + ensure that they were
> > running when
> > > + you shut down the primaries in the previous step, so all the
> > latest changes
> >
> > You talk of primaries in plural here, that is a bit weird for PostgreSQL
> > documentation.
>
> The same docs already discuss two primaries ("8. Stop both primaries"), but
> I agree it might look confusing if you read only a part of the doc, jumping
> into middle of it, like I do all the time when using the docs in "check the
> reference" style.

[...]

> > I think this should be something like "ensure both that the primary is
> > shut down and that the standbys have received all the changes".
>
> Well, we have two primary servers – old and new. I tried to clarify it in
> the new version.

Yeah sorry about that, I think I should have first have coffee and/or
slept over this review before sending it.

Maybe one reason why I was confused is beause I consider a "primary"
more like a full server/VM, not necessarily a database instance (though
one could of course have a primary/seconday pair on the same host).
Possibly "primary instances" or something might be clearer, but I think

I should re-read the whole section first before commenting further.

Michael