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