Re: [PoC] pg_upgrade: allow to upgrade publisher node - Mailing list pgsql-hackers

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