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

On Thu, Sep 7, 2023 at 5:54 PM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Thank you for reviewing! PSA new version.
>

Few comments:
=============
1.
<para>
+ 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>.
+ </para>

Shall we refer to conflicting flag here instead of wal_status?

2.
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -9,6 +9,7 @@

#include "postgres_fe.h"

+#include "access/xlogdefs.h"

This include doesn't seem to be required as we already include this
file via pg_upgrade.h.

3.
+ res = executeQueryOrDie(conn, "SHOW wal_level;");
+
+ if (PQntuples(res) != 1)
+ pg_fatal("could not determine wal_level.");
+
+ wal_level = PQgetvalue(res, 0, 0);
+
+ if (strcmp(wal_level, "logical") != 0)
+ pg_fatal("wal_level must be \"logical\", but is set to \"%s\"",
+ wal_level);

wal_level should be checked before the number of slots required.

4.
@@ -81,7 +84,11 @@ get_loadable_libraries(void)
{
...
+ totaltups++;
+ }
+
}

Spurious new line in the above code.

5.
- os_info.libraries = (LibraryInfo *) pg_malloc(totaltups *
sizeof(LibraryInfo));
+ /*
+ * Allocate memory for extensions and logical replication output plugins.
+ */
+ os_info.libraries = pg_malloc_array(LibraryInfo,

We haven't referred to extensions previously in this function, so how
about changing the comment to: "Allocate memory for required libraries
and logical replication output plugins."?

6.
+ /*
+ * If we are reading the old_cluster, gets infos for logical
+ * replication slots.
+ */

How about changing the comment to: "Retrieve the logical replication
slots infos for the old cluster."?

7.
+ /*
+ * The temporary slots are expressly ignored while checking because such
+ * slots cannot exist after the upgrade. During the upgrade, clusters are
+ * started and stopped several times causing any temporary slots to be
+ * removed.
+ */

/expressly/explicitly

8.
+/*
+ * count_old_cluster_logical_slots()
+ *
+ * Sum up and return the number of logical replication slots for all databases.

I think it would be better to just say: "Returns the number of logical
replication slots for all databases."

9.
+ * Note: This must be done after doing the pg_resetwal command because
+ * pg_resetwal would remove required WALs.
+ */
+ if (count_old_cluster_logical_slots())
+ create_logical_replication_slots();

We can slightly change the Note to: "This must be done after executing
pg_resetwal command in the caller because pg_resetwal would remove
required WALs."

--
With Regards,
Amit Kapila.