Re: pg_upgrade and logical replication

On Wed, 1 Nov 2023 at 10:13, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Mon, Oct 30, 2023 at 03:05:09PM +0530, vignesh C wrote:
> > The patch was not applying because of recent commits. Here is a
> > rebased version of the patches.
>
> + * We don't want the launcher to run while upgrading because it may start
> + * apply workers which could start receiving changes from the publisher
> + * before the physical files are put in place, causing corruption on the
> + * new cluster upgrading to, so setting max_logical_replication_workers=0
> + * to disable launcher.
> */
> if (GET_MAJOR_VERSION(cluster->major_version) >= 1700)
> - appendPQExpBufferStr(&pgoptions, " -c max_slot_wal_keep_size=-1");
> + appendPQExpBufferStr(&pgoptions, " -c max_slot_wal_keep_size=-1 -c max_logical_replication_workers=0");
>
> At least that's consistent with the other side of the coin with
> publications. So 0001 looks basically OK seen from here.
>
> The indentation of 0002 seems off in a few places.

I fixed wherever possible for documentation and also ran pgindent and
pgperltidy.

> + <para>
> + Verify that all the subscription tables in the old subscriber are in
> + <literal>r</literal> (ready) state. Setup the
> + <link linkend="logical-replication-config-subscriber"> subscriber
> + configurations</link> in the new subscriber.
> [...]
> + <para>
> + There is a prerequisites that all the subscription tables should be in
> + <literal>r</literal> (ready) state for
> + <application>pg_upgrade</application> to be able to upgrade the
> + subscriber. If this is not met an error will be reported.
> + </para>
>
> This part is repeated.

Removed the duplicate contents.

> Globally, this documentation addition does not
> seem really helpful for the end-user as it describes the checks that
> are done during the upgrade. Shouldn't this part of the docs,
> similarly to the publication part, focus on providing a check list of
> actions to take to achieve a clean upgrade, with a list of commands
> and configurations required? The good part is that information about
> what's copied is provided (pg_subscription_rel and the origin status),
> still this could be improved.

I have slightly modified it now and also made it consistent with the
replication slot upgrade, but I was not sure if we need to add
anything more. Let me know if anything else needs to be added. I will
add it.

> + <para>
> + Enable the subscriptions by executing
> + <link linkend="sql-altersubscription"><command>ALTER SUBSCRIPTION ... ENABLE</command></link>.
> + </para>
>
> This is something users can act on, but how does this operation help
> with the upgrade? Should this happen for all the descriptions
> subscriptions? Or you mean that this is something that needs to be
> run after the upgrade?

The subscriptions will be upgraded in disabled mode. Users must enable
the subscriptions after the upgrade is completed. I have mentioned the
same to avoid confusion.

> + <para>
> + Create all the new tables that were created in the publication and
> + refresh the publication by executing
> + <link linkend="sql-altersubscription"><command>ALTER SUBSCRIPTION ... REFRESH PUBLICATION</command></link>.
> + </para>
>
> What does "new tables" refer to in this case? Are you referring to
> the case where new relations have been added on a publication node
> after an upgrade and need to be copied? Does one need to DISABLE the
> subscriptions on the subscriber node before running the upgrade, or is
> a REFRESH enough? The test only uses a REFRESH, so the docs and the
> code don't entirely agree with each other.

Yes, "new tables" refers to the new tables created in the publisher
when the upgrade is in progress. No need to disable the subscription
before upgrade, during upgrade the subscriptions will be copied in
disabled mode, they should be enabled after the upgrade. Mentioned all
these accordingly.

> + <para>
> + For upgradation of the subscriptions, all the subscription tables should be
> + in <literal>r</literal> (ready) state, or else the
> + <application>pg_upgrade</application> run will error.
> + </para>
>
> "Upgradation"?

I have removed this content since we have added this in the
prerequisite section now.

> +# Set tables to 'i' state
> +$old_sub->safe_psql(
> + 'postgres',
> + "UPDATE pg_subscription_rel
> + SET srsubstate = 'i' WHERE srsubstate = 'r'");
>
> I am not sure that doing catalog manipulation in the TAP test itself
> is a good idea, because this can finish by being unpredictible in the
> long-term for the test maintenance. I think that this portion of the
> test should just be removed. poll_query_until() or wait queries
> making sure that all the relations are in the state we want them to be
> before the beginning of the upgrade is enough in terms of test
> coverag, IMO.

Changed the scenario by using primary key failure.

> +$result = $new_sub->safe_psql('postgres',
> + "SELECT remote_lsn FROM pg_replication_origin_status");
>
> This assumes one row, but perhaps this had better do a match based on
> external_id and/or local_id?

Modified

The attached v11 version patch has the changes for the same.

Regards,
Vignesh