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

Hi Kuroda-san. Here are some comments for patch v12-0001.

======
src/bin/pg_upgrade/check.c

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?

======
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.

~~~

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?

~~~

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.

------
[1] pg_upgrade logs -
https://www.postgresql.org/message-id/flat/CAHut%2BPuOB4bUwkYAjA_NkTrYaocKy6W3ZYK5Pin305R7mNSLgA%40mail.gmail.com
[2] Kuroda-san reply to my v11 review -
https://www.postgresql.org/message-id/TYAPR01MB5866BD618DEE62AF1836E612F5749%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Kind Regards,
Peter Smith.
Fujitsu Australia