On Thu, Sep 7, 2023 at 5:54 PM Hayato Kuroda (Fujitsu)
<kuroda.hay...@fujitsu.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.
