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

Dear Hou,

Thank you for giving comments! PSA new version.
0001 is updated based on the forked thread.

> 
> 1.
> 
> +	res = executeQueryOrDie(conn, "SHOW wal_level;");
> +	wal_level = PQgetvalue(res, 0, 0);
> 
> +	res = executeQueryOrDie(conn, "SHOW wal_level;");
> +	wal_level = PQgetvalue(res, 0, 0);
> 
> I think it would be better to do a sanity check using PQntuples() before
> calling PQgetvalue() in above places.

Added.

> 2.
> 
> +/*
> + * Helper function for get_old_cluster_logical_slot_infos()
> + */
> +static WALAvailability
> +GetWALAvailabilityByString(const char *str)
> +{
> +	WALAvailability status = WALAVAIL_INVALID_LSN;
> +
> +	if (strcmp(str, "reserved") == 0)
> +		status = WALAVAIL_RESERVED;
> 
> Not a comment, but I am wondering if we could use conflicting field to do this
> check, so that we could avoid the new conversion function and structure
> movement. What do you think ?

I checked pg_get_replication_slots() and agreed that pg_replication_slots.conflicting
indicates whether the slot is usable or not. I can use the attribute instead of porting
WALAvailability. Fixed.

> 3.
> 
> +			curr->confirmed_flush = strtoLSN(
> +
> 		 PQgetvalue(res,
> +
> 					slotnum,
> +
> 					i_confirmed_flush),
> +
> 		 &have_error);
> 
> The indention looks a bit unusual.

The part is not needed anymore.

> 4.
> +	 * XXX: As mentioned in comments atop get_output_plugins(), we may
> not
> +	 * have to consider the uniqueness of entries. If so, we can use
> +	 * count_old_cluster_logical_slots() instead of plugin_list_length().
> +	 */
> 
> I think check_loadable_libraries() will avoid loading the same library, so it
> seems fine to skip duplicating the plugins and we can save some codes.
> 
> ----
> 		/* Did the library name change?  Probe it. */
> 		if (libnum == 0 || strcmp(lib, os_info.libraries[libnum -
> 1].name) != 0)
> ----
> 
> But if we think duplicating them would be better, I feel we could use the
> SimpleStringList to store and duplicate the plugin name. get_output_plugins can
> return an array of the stringlist, each stringlist includes the plugins names
> in one db. I shared a rough POC patch to show how it works, the intention is to
> avoid introducing our new plugin list API.

Actually I do not like the style neither. Peter also said that we can skip checking the
uniqueness, so removed.

> 5.
> 
> +	os_info.libraries = (LibraryInfo *) pg_malloc(
> +
> 			  (totaltups + plugin_list_length(output_plugins)) *
> sizeof(LibraryInfo));
> 
> If we think this looks too long, maybe using pg_malloc_array can help.
>

I checked whole of the patch and used these shorten macros if the line exceeded
80 columns.

Also, I found a cfbot failure [1] but I could not find any reasons.
I will keep investigating more about it.

[1]: https://cirrus-ci.com/task/4634769732927488

Best Regards,
Hayato Kuroda
FUJITSU LIMITED