Re: Documentation to upgrade logical replication cluster

On Fri, 9 Feb 2024 at 12:30, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for patch v7-0001.
>
> ======
> doc/src/sgml/glossary.sgml
>
> 1.
> +  <glossentry id="glossary-logical-replication-cluster">
> +   <glossterm>Logical replication cluster</glossterm>
> +   <glossdef>
> +    <para>
> +     A set of publisher and subscriber instance with publisher instance
> +     replicating changes to the subscriber instance.
> +    </para>
> +   </glossdef>
> +  </glossentry>
>
> 1a.
> /instance with/instances with/

Modified

> ~~~
>
> 1b.
> The description then made me want to look up the glossary definition
> of a "publisher instance" and "subscriber instance", but then I was
> quite surprised that even "Publisher" and "Subscriber" terms are not
> described in the glossary. Should this patch add those, or should we
> start another thread for adding them?

 I felt it is better to start a new thread for this

> ======
> doc/src/sgml/logical-replication.sgml
>
> 2.
> +  <para>
> +   Migration of logical replication clusters is possible only when all the
> +   members of the old logical replication clusters are version 17.0 or later.
> +  </para>
>
> Here is where "logical replication clusters" is mentioned. Shouldn't
> this first reference be linked to that new to the glossary entry --
> e.g. <glossterm linkend="...">logical replication clusters</glossterm>

Modified

> ~~~
>
> 3.
> +   <para>
> +    Following are the prerequisites for <application>pg_upgrade</application>
> +    to be able to upgrade the logical slots. If these are not met an error
> +    will be reported.
> +   </para>
>
> SUGGESTION
> The following prerequisites are required for ...
>
> ~~~
>
> 4.
> +     <para>
> +      All slots on the old cluster must be usable, i.e., there are no slots
> +      whose
> +      <link
linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>conflict_reason</structfield>
> +      is not <literal>NULL</literal>.
> +     </para>
>
> The double-negative is too tricky "no slots whose ... not NULL", needs
> rewording. Maybe it is better to instead use an example as the next
> bullet point does.

The other way is to mention "all slots should have conflic_reason is
NULL", but in this case i feel checking for records is not NULL is
better. So I have kept the wording the same and added an example to
avoid any confusion.

> ~~~
>
> 5.
> +
> +   <para>
> +    Following are the prerequisites for
> <application>pg_upgrade</application> to
> +    be able to upgrade the subscriptions. If these are not met an error
> +    will be reported.
> +   </para>
>
> SUGGESTION
> The following prerequisites are required for ...

Modified

> ======
> doc/src/sgml/ref/pgupgrade.sgml
>
> 6.
> +   <note>
> +    <para>
> +     The steps to upgrade logical replication clusters are not covered here;
> +     refer to <xref linkend="logical-replication-upgrade"/> for details.
> +    </para>
> +   </note>
>
> Maybe here too there should be a link to the glossary term "logical
> replication clusters".

Modified

Thanks for the comments, the attached v8 version patch has the changes
for the patch.

Regards,
Vignesh