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

Dear Amit,

> 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.

I also think that pg_log_warning is not good.

> >
> > 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.

Removed. Based on the decision, your new comment 1 is not needed anymore.

> > 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.

Added.

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

Used the word 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?

Reverted the change. Originally it was needed to pass the Assert()
in the free_db_and_rel_infos(), but it was removed per above.

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

They did a same thing. I used pg_malloc_array() macro to keep the code
within 80 columns.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED