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