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

Thanks for the updated patches.

Here are some review comments for the patch v24-0002

======
doc/src/sgml/ref/pgupgrade.sgml

1.
+ <listitem>
+ <para>
+ All slots on the old cluster must be usable, i.e., there are no
slots
+ whose <structfield>wal_status</structfield> is
<literal>lost</literal> (see
+ <xref linkend="view-pg-replication-slots"/>).
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ <structfield>confirmed_flush_lsn</structfield> (see <xref
linkend="view-pg-replication-slots"/>)
+ of all slots on the old cluster must be the same as the latest
+ checkpoint location.
+ </para>
+ </listitem>

It might be more tidy to change the way those links (e.g. "See section
54.19") are presented:

1a.
SUGGESTION
All slots on the old cluster must be usable, i.e., there are no slots whose
<link
linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>wal_status</structfield>
is <literal>lost</literal>.

~

1b.
SUGGESTION
<link
linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>confirmed_flush_lsn</structfield>
of all slots on the old cluster must be the same as the latest checkpoint
location.

======
src/bin/pg_upgrade/check.c

2.
+ /* Logical replication slots can be migrated since PG17. */
+ if (GET_MAJOR_VERSION(new_cluster.major_version) >= 1700)
+ check_new_cluster_logical_replication_slots();
+

Does it even make sense to check the new_cluster version? IIUC pg_upgrade
*always* updates to the current PG version, which must be 1700 by
definition, because this only is a PG17 patch, right?

For example, see check_cluster_versions() function where it does this check:

/* Only current PG version is supported as a target */
if (GET_MAJOR_VERSION(new_cluster.major_version) !=
GET_MAJOR_VERSION(PG_VERSION_NUM))
pg_fatal("This utility can only upgrade to PostgreSQL version %s.",
PG_MAJORVERSION);

======
src/bin/pg_upgrade/function.c

3.
os_info.libraries = (LibraryInfo *) pg_malloc(totaltups *
sizeof(LibraryInfo));
totaltups = 0;

for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
{
PGresult *res = ress[dbnum];
int ntups;
int rowno;

ntups = PQntuples(res);
for (rowno = 0; rowno < ntups; rowno++)
{
char *lib = PQgetvalue(res, rowno, 0);

os_info.libraries[totaltups].name = pg_strdup(lib);
os_info.libraries[totaltups].dbnum = dbnum;

totaltups++;
}
PQclear(res);
}

~

Although this was not introduced by your patch, I do not understand why the
'totaltups' variable gets reset to zero and then re-incremented in these
loops.

In other words, how is it possible for the end result of 'totaltups' to be
any different from what was already calculated earlier in this function?

IMO totaltups = 0; and totaltups++; is just redundant code.

======
src/bin/pg_upgrade/info.c

4. get_logical_slot_infos

+/*
+ * get_logical_slot_infos()
+ *
+ * Higher level routine to generate LogicalSlotInfoArr for all databases.
+ */
+void
+get_logical_slot_infos(ClusterInfo *cluster)
+{
+ int dbnum;
+
+ /* Logical slots can be migrated since PG17. */
+ if (GET_MAJOR_VERSION(cluster->major_version) <= 1600)
+ return;

It is no longer clear to me what is the purpose of these version checks.

As mentioned in comment #2 above, I don't think we need to check the
new_cluster >= 1700, because this patch is for PG17 by definition.

OTOH, I also don't recognise the reason why there has to be a PG17
restriction on the 'old_cluster' version. Such a restriction seems to
cripple the usefulness of this patch (eg. cannot even upgrade slots from
PG16 to PG17), and there is no explanation given for it. If there is some
valid incompatibility reason why only PG17 old_cluster slots can be
upgraded then it ought to be described in detail and probably also
mentioned in the PG DOCS.

~~~

5. count_logical_slots

+/*
+ * count_logical_slots()
+ *
+ * Sum up and return the number of logical replication slots for all
databases.
+ */
+int
+count_logical_slots(ClusterInfo *cluster)
+{
+ int dbnum;
+ int slot_count = 0;
+
+ /* Quick exit if the version is prior to PG17. */
+ if (GET_MAJOR_VERSION(cluster->major_version) <= 1600)
+ return 0;
+
+ for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ slot_count += cluster->dbarr.dbs[dbnum].slot_arr.nslots;
+
+ return slot_count;
+}

Same as the previous comment #4. I had doubts about the intent/need for
this cluster version checking.

------
Kind Regards,
Peter Smith.
Fujitsu Australia