Re: [PoC] pg_upgrade: allow to upgrade publisher node - Mailing list pgsql-hackers

Hi Kuroda-san.

Here are some additional review comments for v35-0002  (and because we
overlapped, my v34-0002 review comments have not been addressed yet)

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

======
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>
                ^

~~~

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>temporary</structfield>
+       is <literal>false</literal>.
+      </para>
+     </listitem>

/there are no slots whose/there must be no slots where/

~~~

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.

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

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

~

6b.
+ os_info.libraries = (LibraryInfo *) pg_malloc(sizeof(LibraryInfo) *
(array_size));

Those extra parentheses around "(array_size)" seem overkill.

------
Kind Regards,
Peter Smith.
Fujitsu Australia