On Thursday, September 7, 2023 8:24 PM Kuroda, Hayato/黒田 隼人 <kuroda.hay...@fujitsu.com> wrote: > > Dear Peter, > > Thank you for reviewing! PSA new version.
Thanks for updating the patches ! Here are some comments: 1. bool reap_child(bool wait_for_child); + +XLogRecPtr strtoLSN(const char *str, bool *have_error); This function has be removed. 2. + if (nslots_on_new) + { + if (nslots_on_new == 1) + pg_fatal("New cluster must not have logical replication slots but found a slot."); + else + pg_fatal("New cluster must not have logical replication slots but found %d slots.", + nslots_on_new); We could try ngettext() here: pg_log_warning(ngettext("New cluster must not have logical replication slots but found %d slot.", "New cluster must not have logical replication slots but found %d slots", nslots_on_new) 3. - create_script_for_old_cluster_deletion(&deletion_script_file_name); - Is there a reason for reordering this function ? Sorry If I missed some previous discussions. 4. @@ -610,6 +724,12 @@ free_db_and_rel_infos(DbInfoArr *db_arr) { free_rel_infos(&db_arr->dbs[dbnum].rel_arr); pg_free(db_arr->dbs[dbnum].db_name); + + /* + * Logical replication slots must not exist on the new cluster before + * create_logical_replication_slots(). + */ + Assert(db_arr->dbs[dbnum].slot_arr.nslots == 0); I think the assert is not necessary, as the patch will check the new cluster's slots in another function. Besides, this function is not only used for new cluster, but the comment only mentioned the new cluster which seems a bit inconsistent. So, how about removing it ? 5. (cluster == &new_cluster) ? - " -c synchronous_commit=off -c fsync=off -c full_page_writes=off" : "", + " -c synchronous_commit=off -c fsync=off -c full_page_writes=off" : + " -c max_slot_wal_keep_size=-1", I think we need to set max_slot_wal_keep_size on new cluster as well, otherwise it's possible that the new created slots get invalidated during upgrade, what do you think ? 6. + bool is_lost; /* Is the slot in 'lost'? */ +} LogicalSlotInfo; Would it be better to use 'invalidated', as the same is used in error message of ReportSlotInvalidation() and logicaldecoding.sgml. 7. + for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++) + { ... + if (script) + { + fclose(script); + + pg_log(PG_REPORT, "fatal"); + pg_fatal("The source cluster contains one or more problematic logical replication slots.\n" I think we should do this pg_fatal out of the for() loop, otherwise we cannot collect all the problematic slots. Best Regards, Hou zj