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

Dear Amit,

> On Fri, Sep 8, 2023 at 2:12 PM Zhijie Hou (Fujitsu)
> <houzj(dot)fnst(at)fujitsu(dot)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