From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | 'Amit Kapila' <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | Peter Smith <smithpb2250(at)gmail(dot)com>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Bruce Momjian <bruce(at)momjian(dot)us>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
Subject: | RE: [PoC] pg_upgrade: allow to upgrade publisher node |
Date: | 2023-09-08 13:03:41 |
Message-ID: | TYAPR01MB5866A1DDE26216C7A2D41CBEF5EDA@TYAPR01MB5866.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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].
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
From | Date | Subject | |
---|---|---|---|
Next Message | Hayato Kuroda (Fujitsu) | 2023-09-08 13:05:55 | RE: [PoC] pg_upgrade: allow to upgrade publisher node |
Previous Message | Hayato Kuroda (Fujitsu) | 2023-09-08 13:01:10 | RE: [PoC] pg_upgrade: allow to upgrade publisher node |