RE: [PoC] pg_upgrade: allow to upgrade publisher node

Dear Amit,

Thank you for giving comments! PSA new version patch set.

> 1.
> + <link linkend="sql-altersubscription"><command>ALTER
> SUBSCRIPTION ... DISABLE</command></link>.
> + After the upgrade is complete, execute the
> + <command>ALTER SUBSCRIPTION ... CONNECTION</command>
> command to update the
> + connection string, and then re-enable the subscription.
>
> Why does one need to update the connection string?

I wrote like that because the old and new port number can be different. But you
are partially right - it is not always needed. Updated to clarify that.

> 2.
> + /*
> + * Checking for logical slots must be done before
> + * check_new_cluster_is_empty() because the slot_arr attribute of the
> + * new_cluster will be checked in that function.
> + */
> + if (count_logical_slots(&old_cluster))
> + {
> + get_logical_slot_infos(&new_cluster, false);
> + check_for_logical_replication_slots(&new_cluster);
> + }
> +
> check_new_cluster_is_empty();
>
> Can't we simplify this checking by simply querying
> pg_replication_slots for any usable slot something similar to what we
> are doing in check_for_prepared_transactions()? We can add this check
> in the function check_for_logical_replication_slots().

Some checks were included to check_for_logical_replication_slots(), and
get_logical_slot_infos() for new_cluster was removed as you said.

But get_logical_slot_infos() cannot be removed completely, because the old
cluster has already been shut down when the new cluster is checked. We must
store the information of old cluster on the memory.

Note that the existence of slots are now checked in any cases because such slots
could not be used after the upgrade.

check_new_cluster_is_empty() is no longer checks logical slots, so all changes for
this function was reverted.

> Also, do we
> need a count function, or instead can we have a simple function like
> is_logical_slot_present() where we return even if there is one slot
>

I think this is still needed, because max_replication_slots and the number
of existing replication slots must be compared.

Of course we can add another simple function like
is_logical_slot_present_on_old_cluster() and use in main(), but not sure defining
some similar functions are good.

> Apart from this, (a) I have made a few changes (changed comments) in
> patch 0001 as shared in the email [1]; (b) some modifications in the
> docs as you can see in the attached. Please include those changes in
> the next version if you think they are okay.

I checked and your modification seems nice.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED