Dear Hou, Thank you for reviewing!
> +static void > +create_logical_replication_slots(void) > ... > + query = createPQExpBuffer(); > + escaped = createPQExpBuffer(); > + conn = connectToServer(&new_cluster, old_db->db_name); > > Since the connection here is not used anymore, so I think we can remove it. Per discussion [1], pg_upgrade must use connection again. So I kept it. > 2. > > +static void > +create_logical_replication_slots(void) > ... > + /* update new_cluster info again */ > + get_logical_slot_infos(&new_cluster); > +} > > Do we need to get new slots again after restoring ? I checked again and thought that it was not needed, removed. Similar function, create_new_objects(), was updated the information at the end. This was needed because the information was used to compare objects between old and new cluster, in transfer_all_new_tablespaces(). In terms of logical replication slots, however, such comparison was not done. No functions use updated information. > 3. > > + snprintf(query, sizeof(query), > + "SELECT slot_name, plugin, two_phase " > + "FROM pg_catalog.pg_replication_slots " > + "WHERE database = current_database() AND > temporary = false " > + "AND wal_status <> 'lost';"); > + > + res = executeQueryOrDie(conn, "%s", query); > + > > Instead of building the query in a new variable, can we directly put the SQL > in > executeQueryOrDie() > e.g. > executeQueryOrDie(conn, "SELECT slot_name, plugin, two_phase ..."); Right, fixed. > 4. > +int num_slots_on_old_cluster; > > Instead of a new global variable, would it be better to record this in the > cluster > info ? Per suggestion [2], the variable was removed. > 5. > > char sql_file_name[MAXPGPATH], > log_file_name[MAXPGPATH]; > + > DbInfo *old_db = &old_cluster.dbarr.dbs[dbnum]; > > There is an extra change here. Removed. > 6. > + for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++) > .. > + /* reap all children */ > + while (reap_child(true) == true) > + ; > + } > > Maybe we can move the "while (reap_child(true) == true)" out of the for() > loop ? Per discussion [1], I stopped to do in parallel. So this part was not needed anymore. The patch would be available in upcoming posts. [1]: https://www.postgresql.org/message-id/TYCPR01MB58701DAEE5E61B07AC84ADBBF51AA%40TYCPR01MB5870.jpnprd01.prod.outlook.com [2]: https://www.postgresql.org/message-id/TYAPR01MB5866691219B9CB280B709600F51BA%40TYAPR01MB5866.jpnprd01.prod.outlook.com Best Regards, Hayato Kuroda FUJITSU LIMITED
