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

On Wed, Sep 6, 2023 at 11:01 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Tue, Sep 5, 2023 at 7:34 PM Hayato Kuroda (Fujitsu)
> <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> >
> > Also, for simplifying codes, only a first-met invalidated slot is output in the
> > check_old_cluster_for_valid_slots(). Warning messages int the function were
> > removed. I think it may be enough because check_new_cluster_is_empty() do
> > similar thing. Please tell me if it should be reverted...
> >
>
> Another possible idea is to show all the WARNINGS but only when in verbose mode.
>

I think it would be better to write problematic slots in the script
file like we are doing in the function
check_for_composite_data_type_usage()->check_for_data_types_usage()
and give a message suggesting what the user can do as we are doing in
check_for_composite_data_type_usage(). That will be helpful for the
user to take necessary action.

A few other comments:
=================
1.
@@ -189,6 +199,8 @@ check_new_cluster(void)
{
get_db_and_rel_infos(&new_cluster);

+ check_new_cluster_logical_replication_slots();
+
check_new_cluster_is_empty();

check_loadable_libraries();

Why check_new_cluster_logical_replication_slots is done before
check_new_cluster_is_empty? At least check_new_cluster_is_empty()
would be much quicker to return an error if any. I think if we don't
have a specific reason to position this new check, we can do it at the
end after check_for_new_tablespace_dir() to avoid breaking the order
of existing checks.

2. Shall we rename get_db_and_rel_infos() to
get_db_rel_and_slot_infos() or something like that as that function
now fetches the slot information as well?

--
With Regards,
Amit Kapila.