On Wed, Aug 30, 2023 at 10:58 AM Peter Smith <smithpb2...@gmail.com> wrote: > > Here are some review comments for v28-0003. > > ====== > src/bin/pg_upgrade/check.c > > 1. check_and_dump_old_cluster > + /* > + * Logical replication slots can be migrated since PG17. See comments atop > + * get_old_cluster_logical_slot_infos(). > + */ > + if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700) > + check_old_cluster_for_valid_slots(live_check); > + > > IIUC we are preferring to use the <= 1600 style of version check > instead of >= 1700 where possible. >
Yeah, but in this case, following the nearby code style, I think it is okay to keep it as it is. > ~ > > 3b. > /Quick exit/Quick return/ > Hmm, either way should be okay. > ~ > > 4. > + prep_status("Checking for logical replication slots"); > > I felt that should add the word "valid" like: > "Checking for valid logical replication slots" > Agreed and fixed. > ~~~ > > 5. > + /* Check there are no logical replication slots with a 'lost' state. */ > + res = executeQueryOrDie(conn, > + "SELECT slot_name FROM pg_catalog.pg_replication_slots " > + "WHERE wal_status = 'lost' AND " > + "temporary IS FALSE;"); > > Since the SQL is checking if there *are* lost slots I felt it would be > more natural to reverse that comment. > > SUGGESTION > /* Check and reject if there are any logical replication slots with a > 'lost' state. */ > I changed the comments but differently. > ~~~ > > 6. > + /* > + * Do additional checks if a live check is not required. This requires > + * that confirmed_flush_lsn of all the slots is the same as the latest > + * checkpoint location, but it would be satisfied only when the server has > + * been shut down. > + */ > + if (!live_check) > > I think the comment can be rearranged slightly: > > SUGGESTION > 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. > Changed as per suggestion. > ====== > src/bin/pg_upgrade/controldata.c > > 7. > + /* > + * Read the latest checkpoint location if the cluster is PG17 > + * or later. This is used for upgrading logical replication > + * slots. > + */ > + if (GET_MAJOR_VERSION(cluster->major_version) >= 1700) > + { > > Fetching this "Latest checkpoint location:" value is only needed for > the check_old_cluster_for_valid_slots validation check, isn't it? But > AFAICT this code is common for both old_cluster and new_cluster. > > I am not sure what is best to do: > - Do only the minimal logic needed? > - Read the value redundantly even for new_cluster just to keep code simpler? > > Either way, maybe the comment should say something about this. > Added the comment. -- With Regards, Amit Kapila.