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

Dear Amit,

Thank you for reviewing!

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

Changed. I used the word 'lost' in check_old_cluster_for_valid_slots() because of
the line, so changed them accordingly.

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

I preferred to include explicitly... but fixed.

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

Moved.

> 4.
> @@ -81,7 +84,11 @@ get_loadable_libraries(void)
> {
> ...
> + totaltups++;
> + }
> +
>   }
> 
> Spurious new line in the above code.

Removed.

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

Changed.

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

Changed.

> 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

Replaced.

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

Changed.

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

Reworded.

You can see the new version in [1].

[1]:
https://www.postgresql.org/message-id/TYAPR01MB5866AB60B4CF404419D9373DF5EDA%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED