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

Dear Peter,

Thank you for reviewing!

=====
> Commit message
>
> 1.
> Note that the pg_resetwal command would remove WAL files, which are required
> as
> restart_lsn. If WALs required by logical replication slots are removed, they are
> unusable. Therefore, during the upgrade, slot restoration is done
> after the final
> pg_resetwal command. The workflow ensures that required WALs are remained.
>
> ~
>
> SUGGESTION (minor wording and /required as/required for/ and
> /remained/retained/)
> Note that the pg_resetwal command would remove WAL files, which are
> required for restart_lsn. If WALs required by logical replication
> slots are removed, the slots are unusable. Therefore, during the
> upgrade, slot restoration is done after the final pg_resetwal command.
> The workflow ensures that required WALs are retained.

Fixed.

> doc/src/sgml/ref/pgupgrade.sgml
>
> 2.
> The SGML is mal-formed so I am unable to build PG DOCS. Please try
> building the docs before posting the patch.
>
> ref/pgupgrade.sgml:446: parser error : Opening and ending tag
> mismatch: itemizedlist line 410 and listitem
> </listitem>
> ^

Fixed. Sorry for noise.

> 3.
> + <listitem>
> + <para>
> + The new cluster must not have permanent logical replication slots, i.e.,
> + there are no slots whose
> + <link
> linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>t
> emporary</structfield>
> + is <literal>false</literal>.
> + </para>
> + </listitem>
>
> /there are no slots whose/there must be no slots where/

Fixed.

> 4.
> or take a file system backup as the standbys are still synchronized
> - with the primary.) Replication slots are not copied and must
> - be recreated.
> + with the primary.) Only logical slots on the primary are migrated to the
> + new standby, and other slots on the old standby must be recreated as
> + they are not copied.
> </para>
>
> Mixing the terms "migrated" and "copied" seems to complicate this.
> Does the following suggestion work better instead?
>
> SUGGESTION (??)
> Only logical slots on the primary are migrated to the new standby. Any
> other slots present on the old standby must be recreated.

Hmm, I preferred to use "copied". How do you think?

> src/backend/replication/slot.c
>
> 5. InvalidatePossiblyObsoleteSlot
>
> + /*
> + * The logical replication slots shouldn't be invalidated as
> + * max_slot_wal_keep_size is set to -1 during the upgrade.
> + */
> + if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
> + elog(ERROR, "Replication slots must not be invalidated during the upgrade.");
> +
>
> I felt the comment could have another sentence like "The following is
> just a sanity check."

Added.

> src/bin/pg_upgrade/function.c
>
> 6. get_loadable_libraries
>
> + array_size = totaltups + count_old_cluster_logical_slots();
> + os_info.libraries = (LibraryInfo *) pg_malloc(sizeof(LibraryInfo) *
> (array_size));
> totaltups = 0;
>
> 6a.
> Maybe something like 'n_libinfos' would be a more meaningful name than
> 'array_size'?

Fixed.

> 6b.
> + os_info.libraries = (LibraryInfo *) pg_malloc(sizeof(LibraryInfo) *
> (array_size));
>
> Those extra parentheses around "(array_size)" seem overkill.

Removed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED