RE: [PoC] pg_upgrade: allow to upgrade publisher node

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