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
v32-0001-Persist-logical-slots-to-disk-during-a-shutdown-.patch
Description: v32-0001-Persist-logical-slots-to-disk-during-a-shutdown-.patch
v32-0002-pg_upgrade-Allow-to-replicate-logical-replicatio.patch
Description: v32-0002-pg_upgrade-Allow-to-replicate-logical-replicatio.patch
