Dear Peter, Thank you for reviewing! PSA new version.
> 1. check_new_cluster
>
> + if (user_opts.include_logical_slots)
> + {
> + get_logical_slot_infos(&new_cluster);
> + check_for_parameter_settings(&new_cluster);
> + }
> +
> check_new_cluster_is_empty();
> ~
>
> The code is OK, but maybe your reply/explanation (see [2] #2) saying
> get_logical_slot_infos() needs to be called before
> check_new_cluster_is_empty() would be good to have in a comment here?
Indeed, added.
> src/bin/pg_upgrade/info.c
>
> 2. get_logical_slot_infos
>
> + if (ntups)
> + slotinfos = (LogicalSlotInfo *) pg_malloc(sizeof(LogicalSlotInfo) * ntups);
> + else
> + {
> + slotinfos = NULL;
> + goto cleanup;
> + }
> +
> + i_slotname = PQfnumber(res, "slot_name");
> + i_plugin = PQfnumber(res, "plugin");
> + i_twophase = PQfnumber(res, "two_phase");
> +
> + for (slotnum = 0; slotnum < ntups; slotnum++)
> + {
> + LogicalSlotInfo *curr = &slotinfos[num_slots++];
> +
> + curr->slotname = pg_strdup(PQgetvalue(res, slotnum, i_slotname));
> + curr->plugin = pg_strdup(PQgetvalue(res, slotnum, i_plugin));
> + curr->two_phase = (strcmp(PQgetvalue(res, slotnum, i_twophase), "t") == 0);
> + }
> +
> +cleanup:
> + PQfinish(conn);
>
> IMO the goto/label coding is not warranted here - a simple if/else can
> do the same thing.
Yeah, I could simplify by if-statement. Additionally, some definitions of
variables
are moved to the code block.
> 3. free_db_and_rel_infos, free_logical_slot_infos
>
> static void
> free_db_and_rel_infos(DbInfoArr *db_arr)
> {
> int dbnum;
>
> for (dbnum = 0; dbnum < db_arr->ndbs; dbnum++)
> {
> free_rel_infos(&db_arr->dbs[dbnum].rel_arr);
> pg_free(db_arr->dbs[dbnum].db_name);
> }
> pg_free(db_arr->dbs);
> db_arr->dbs = NULL;
> db_arr->ndbs = 0;
> }
>
> ~
>
> In v12 now you removed the free_logical_slot_infos(). But isn't it
> better to still call free_logical_slot_infos() from the above
> free_db_and_rel_infos() still so the slot memory
> (dbinfo->slot_arr.slots) won't stay lying around?
The free_db_and_rel_infos() is called at restore phase, and slot_arr has
malloc'd
members only when logical slots are defined on new_cluster. In this case the
FATAL
error is occured in the checking phase, so there is no possibility to reach
restore
phase.
> 4. get_logical_slot_infos, print_slot_infos
>
> In another thread [1] I am posting some minor patch changes to the
> VERBOSE logging (changes to double-quotes and commas etc.). Please
> keep a watch on that thread because if gets pushed then this one will
> be impacted. e.g. your logging here ought also to include the same
> suggested double quotes.
I thought it would be pushed soon, so the suggestion was included.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
v13-0001-pg_upgrade-Add-include-logical-replication-slots.patch
Description: v13-0001-pg_upgrade-Add-include-logical-replication-slots.patch
v13-0002-Always-persist-to-disk-logical-slots-during-a-sh.patch
Description: v13-0002-Always-persist-to-disk-logical-slots-during-a-sh.patch
v13-0003-pg_upgrade-Add-check-function-for-include-logica.patch
Description: v13-0003-pg_upgrade-Add-check-function-for-include-logica.patch
v13-0004-Change-the-method-used-to-check-logical-replicat.patch
Description: v13-0004-Change-the-method-used-to-check-logical-replicat.patch
