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