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
