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

Dear Peter,

Thanks for reviewing! PSA new version patch set.
Note again that 0001 patch was replaced to new one[1], but you do not have to
discuss that - it should be done in forked thread.

>
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>.
>

Fixed.

>
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.
>

Fixed.

>
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);
>

You are right, the new_cluster always has the same version as pg_upgrade.
Removed.

>
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.
>

First of all, I will not fix that in this thread, it should be done in another
place. I do not want to expand the thread anymore. Personally, it seemed that
totaltups was just reused as index for the array.

>
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.
>

Upgrading logical slots with verifications requires that they surely saved to
disk while shutting down (0001 patch). Currently we do not have a plan to
backpatch it, so I think the checking must be needed. Instead, I added
descriptions in the doc and code comments.

>
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.
>

As I said above, this is needed.

[1]: https://www.postgresql.org/message-id/CALDaNm0VrAt24e2FxbOX6eJQ-G_tZ0gVpsFBjzQM99NxG0hZfg%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED