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

On Thursday, September 7, 2023 8:24 PM Kuroda, Hayato/黒田 隼人 <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Peter,
>
> Thank you for reviewing! PSA new version.

Thanks for updating the patches !

Here are some comments:

1.

bool reap_child(bool wait_for_child);
+
+XLogRecPtr strtoLSN(const char *str, bool *have_error);

This function has be removed.

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)

3.
- create_script_for_old_cluster_deletion(&deletion_script_file_name);
-

Is there a reason for reordering this function ? Sorry If I missed some
previous discussions.

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 ?

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 ?

6.

+ bool is_lost; /* Is the slot in 'lost'? */
+} LogicalSlotInfo;

Would it be better to use 'invalidated', as the same is used in error message
of ReportSlotInvalidation() and logicaldecoding.sgml.

7.
+ for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
+ {
...
+ if (script)
+ {
+ fclose(script);
+
+ pg_log(PG_REPORT, "fatal");
+ pg_fatal("The source cluster contains one or more problematic logical replication slots.\n"

I think we should do this pg_fatal out of the for() loop, otherwise we cannot
collect all the problematic slots.

Best Regards,
Hou zj