Dear Amit, Thank you for reviewing!
> Few comments: > ============== > 1. > + <link > linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>c > onfirmed_flush_lsn</structfield> > + of all slots on the old cluster must be the same as the latest > + checkpoint location. > > We can add something like: "This ensures that all the data has been > replicated before the upgrade." to make it clear why this test is > important. Added. > 2. Move the wal_level related restriction before max_replication_slots. > > 3. > + /* Is the slot still usable? */ > + if (slot->invalid) > + { > + if (script == NULL && > + (script = fopen_priv(output_path, "w")) == NULL) > + pg_fatal("could not open file \"%s\": %s", > + output_path, strerror(errno)); > + > + fprintf(script, > + "slotname :%s\tproblem: The slot is unusable\n", > + slot->slotname); > + } > + > + /* > + * Do additional checks to ensure that confirmed_flush LSN of all > + * the slots is the same as the latest checkpoint location. > + * > + * Note: This can be satisfied only when the old cluster has been > + * shut down, so we skip this for live checks. > + */ > + if (!live_check && !slot->caught_up) > > Isn't it better to continue for the next slot once we find that slot > is invalid instead of checking other conditions? Right, fixed. > 4. > + > + fprintf(script, > + "slotname :%s\tproblem: The slot is unusable\n", > + slot->slotname); > > Let's keep it as one string and change the message to: "The slot > "\"%s\" is invalid" Changed. > + fprintf(script, > + "slotname :%s\tproblem: The slot has not consumed WALs yet\n", > + slot->slotname); > + } > > On a similar line, we can change this to: "The slot "\"%s\" has not > consumed the WAL yet" Changed. > 5. > + snprintf(output_path, sizeof(output_path), "%s/%s", > + log_opts.basedir, > + "problematic_logical_relication_slots.txt"); > > I think we can name this file as "invalid_logical_replication_slots" > or simply "logical_replication_slots" The latter one seems too general for me, "invalid_..." was chosen. > 6. > + pg_fatal("The source cluster contains one or more problematic > logical replication slots.\n" > + "The needed workaround depends on the problem.\n" > + "1) If the problem is \"The slot is unusable,\" You can drop such > replication slots.\n" > + "2) If the problem is \"The slot has not consumed WALs yet,\" you > can consume all remaining WALs.\n" > + "Then, you can restart the upgrade.\n" > + "A list of problematic logical replication slots is in the file:\n" > + " %s", output_path); > > This doesn't match the similar existing comments. So, let's change it > to something like: > > "Your installation contains invalid logical replication slots. These > slots can't be copied so this cluster cannot currently be upgraded. > Consider either removing such slots or consuming the pending WAL if > any and then restart the upgrade. A list of invalid logical > replication slots is in the file:" Basically changed to your suggestion, but slightly reworded based on what Grammarly said. > Apart from the above, I have edited a few other comments in the patch. > See attached. Thanks for attaching! Included. Best Regards, Hayato Kuroda FUJITSU LIMITED