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

On Fri, Sep 8, 2023 at 2:12 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> 2.
>
> +       if (nslots_on_new)
> +       {
> +               if (nslots_on_new == 1)
> +                       pg_fatal("New cluster must not have logical replication slots but found a slot.");
> +               else
> +                       pg_fatal("New cluster must not have logical replication slots but found %d slots.",
> +                                        nslots_on_new);
>
> We could try ngettext() here:
>                 pg_log_warning(ngettext("New cluster must not have logical replication slots but found %d slot.",
>                                                                 "New cluster must not have logical replication slots but found %d slots",
>                                                                 nslots_on_new)
>

Will using pg_log_warning suffice for the purpose of exiting the
upgrade process? I don't think the intention here is to continue after
finding such a case.

>
> 4.
>
> @@ -610,6 +724,12 @@ free_db_and_rel_infos(DbInfoArr *db_arr)
>         {
>                 free_rel_infos(&db_arr->dbs[dbnum].rel_arr);
>                 pg_free(db_arr->dbs[dbnum].db_name);
> +
> +               /*
> +                * Logical replication slots must not exist on the new cluster before
> +                * create_logical_replication_slots().
> +                */
> +               Assert(db_arr->dbs[dbnum].slot_arr.nslots == 0);
>
>
> I think the assert is not necessary, as the patch will check the new cluster's
> slots in another function. Besides, this function is not only used for new
> cluster, but the comment only mentioned the new cluster which seems a bit
> inconsistent. So, how about removing it ?
>

Yeah, I also find it odd.

> 5.
>                          (cluster == &new_cluster) ?
> -                        " -c synchronous_commit=off -c fsync=off -c full_page_writes=off" : "",
> +                        " -c synchronous_commit=off -c fsync=off -c full_page_writes=off" :
> +                        " -c max_slot_wal_keep_size=-1",
>
> I think we need to set max_slot_wal_keep_size on new cluster as well, otherwise
> it's possible that the new created slots get invalidated during upgrade, what
> do you think ?
>

I also think that would be better.

> 6.
>
> +       bool            is_lost;                /* Is the slot in 'lost'? */
> +} LogicalSlotInfo;
>
> Would it be better to use 'invalidated',
>

Or how about simply 'invalid'?

A few other points:
1.
  ntups = PQntuples(res);
- dbinfos = (DbInfo *) pg_malloc(sizeof(DbInfo) * ntups);
+ dbinfos = (DbInfo *) pg_malloc0(sizeof(DbInfo) * ntups);

Can we write a comment to say why we need zero memory here?

 2. Why get_old_cluster_logical_slot_infos() need to use
pg_malloc_array whereas for similar stuff get_rel_infos() use
pg_malloc()?

-- 
With Regards,
Amit Kapila.