Re: Documentation to upgrade logical replication cluster

On Fri, 9 Feb 2024 at 12:30, Peter Smith <smithpb2250(at)gmail(dot)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