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

Dear Peter,

Thank you for reviewing!

>
> ======
> src/bin/pg_upgrade/controldata.c
>
> 1. get_control_data
>
> + if (GET_MAJOR_VERSION(cluster->major_version) >= 1700)
> + {
> + bool have_error = false;
> +
> + p = strchr(p, ':');
> +
> + if (p == NULL || strlen(p) <= 1)
> + pg_fatal("%d: controldata retrieval problem", __LINE__);
> +
> + p++; /* remove ':' char */
> +
> + p = strpbrk(p, "01234567890ABCDEF");
> +
> + if (p == NULL || strlen(p) <= 1)
> + pg_fatal("%d: controldata retrieval problem", __LINE__);
> +
> + cluster->controldata.chkpnt_latest =
> + strtoLSN(p, &have_error);
>
> 1a.
> The declaration assignment of 'have_error' is redundant because it
> gets overwritten before it is checked anyhow.
>
> ~
>
> 1b.
> IMO that first check logic should also be shifted to be *inside* the
> strtoLSN and it would just return have_error true. This eliminates
> having 2x pg_fatal that have the same purpose.
>
> ~~~
>
> 2. strtoLSN
>
> +/*
> + * Convert String to XLogRecPtr.
> + *
> + * This function is ported from pg_lsn_in_internal(). The function cannot be
> + * called from client binaries.
> + */
> +XLogRecPtr
> +strtoLSN(const char *str, bool *have_error)
>
> SUGGESTION (comment wording)
> This function is ported from pg_lsn_in_internal() which cannot be
> called from client binaries.

These changes are reverted.

> src/bin/pg_upgrade/function.c
>
> 3. struct plugin_list
>
> +typedef struct plugin_list
> +{
> + int dbnum;
> + char *plugin;
> + struct plugin_list *next;
> +} plugin_list;
>
> I found that name confusing. IMO should be like 'plugin_list_elem'.
>
> e.g. it gets too strange in subsequent code:
> + plugin_list *newentry = (plugin_list *) pg_malloc(sizeof(plugin_list));
>
> ~~~
>
> 4. is_plugin_unique
>
> +/* Has the given plugin already been listed? */
> +static bool
> +is_plugin_unique(plugin_list_head *listhead, const char *plugin)
> +{
> + plugin_list *point;
> +
> + /* Quick return if the head is NULL */
> + if (listhead == NULL)
> + return true;
> +
> + /* Seek the plugin list */
> + for (point = listhead->head; point; point = point->next)
> + {
> + if (strcmp(point->plugin, plugin) == 0)
> + return false;
> + }
> +
> + return true;
> +}
>
> What's the meaning of the name 'point'? Maybe something generic like
> 'cur' or similar is better?
>
> ~~~
>
> 5. get_output_plugins
>
> +/*
> + * Load the list of unique output plugins.
> + *
> + * XXX: Currently, we extract the list of unique output plugins, but this may
> + * be overkill. The list is used for two purposes - 1) to allocate the minimal
> + * memory for the library list and 2) to skip storing duplicated plugin names.
> + * However, the consumer check_loadable_libraries() can avoid double checks
> for
> + * the same library. The above means that we can arrange output plugins without
> + * considering their uniqueness, so that we can remove this function.
> + */
> +static plugin_list_head *
> +get_output_plugins(void)
> +{
> + plugin_list_head *head = NULL;
> + int dbnum;
> +
> + /* Quick return if there are no logical slots to be migrated. */
> + if (count_old_cluster_logical_slots() == 0)
> + return NULL;
> +
> + for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
> + {
> + LogicalSlotInfoArr *slot_arr = &old_cluster.dbarr.dbs[dbnum].slot_arr;
> + int slotnum;
> +
> + for (slotnum = 0; slotnum < slot_arr->nslots; slotnum++)
> + {
> + LogicalSlotInfo *slot = &slot_arr->slots[slotnum];
> +
> + /* Add to the list if the plugin has not been listed yet */
> + if (is_plugin_unique(head, slot->plugin))
> + add_plugin_list_item(&head, dbnum, slot->plugin);
> + }
> + }
> +
> + return head;
> +}
>
> About the XXX. Yeah, since the uniqueness seems checked later anyway
> all this extra code seems overkill. Instead of all the extra code you
> just need a comment to mention how it will be sorted and checked
> later.
>
> But even if you prefer to keep it, I thought those 2 functions
> 'is_plugin_unique()' and 'add_plugin_list_item()' could have been
> combined to just have 'add_plugin_list_unique_item()'. Since order
> does not matter, such a function would just add items to the end of
> the list (after finding uniqueness) instead of to the head.

Based on suggestions from you and Hou[1], I withdrew to check their uniqueness.
So these functions and structures are removed.

> 6. get_loadable_libraries
>
> FirstNormalObjectId);
> +
> totaltups += PQntuples(ress[dbnum]);
> ~
>
> The extra blank line in the existing code is not needed in this patch.

Removed.

> 7. get_loadable_libraries
>
> int rowno;
> + plugin_list *point;
>
> ~
>
> Same as a prior comment #4. What's the meaning of the name 'point'?

The variable was removed.

> 8. get_loadable_libraries
> +
> + /*
> + * If the old cluster has logical replication slots, plugins used by
> + * them must be also stored. It must be done only once, so do it at
> + * dbnum == 0 case.
> + */
> + if (output_plugins == NULL)
> + continue;
> +
> + if (dbnum != 0)
> + continue;
>
> This logic seems misplaced. If this "must be done only once" then why
> is it within the db loop in the first place? Shouldn't this be done
> seperately outside the loop?

The logic was removed.

> src/bin/pg_upgrade/info.c
>
> 9.
> +/*
> + * Helper function for get_old_cluster_logical_slot_infos()
> + */
> +static WALAvailability
> +GetWALAvailabilityByString(const char *str)
>
> Should this be forward declared like the other static functions are?

The function was removed.

> 10. get_old_cluster_logical_slot_infos
>
> + for (slotnum = 0; slotnum < num_slots; slotnum++)
> + {
> + LogicalSlotInfo *curr = &slotinfos[slotnum];
> + bool have_error = false;
>
> Here seems an unnecessary assignment to 'have_error' because it will
> always be assigned again before it is checked.

The variable was removed.

> 11. get_old_cluster_logical_slot_infos
>
> + curr->confirmed_flush = strtoLSN(
> + PQgetvalue(res,
> + slotnum,
> + i_confirmed_flush),
> + &have_error);
> + curr->wal_status = GetWALAvailabilityByString(
> + PQgetvalue(res,
> + slotnum,
> + i_wal_status));
>
> Can this excessive wrapping be improved? Maybe new vars are needed.

The part was removed.

> 12.
> +static void
> +print_slot_infos(LogicalSlotInfoArr *slot_arr)
> +{
> + int slotnum;
> +
> + for (slotnum = 0; slotnum < slot_arr->nslots; slotnum++)
> + {
> + LogicalSlotInfo *slot_info = &slot_arr->slots[slotnum];
> +
> + if (slotnum == 0)
> + pg_log(PG_VERBOSE, "Logical replication slots within the database:");
> +
> + pg_log(PG_VERBOSE, "slotname: \"%s\", plugin: \"%s\", two_phase: %d",
> + slot_info->slotname,
> + slot_info->plugin,
> + slot_info->two_phase);
> + }
> +}
>
> This seems an odd way to output the heading. Isn't it better to put
> this outside the loop?
>
> SUGGESTION
> if (slot_arr->nslots > 0)
> pg_log(PG_VERBOSE, "Logical replication slots within the database:");

Fixed.

> src/bin/pg_upgrade/pg_upgrade.c
>
> 13.
> +/*
> + * setup_new_cluster()
> + *
> + * Starts a new cluster for updating the wal_level in the control fine, then
> + * does final setups. Logical slots are also created here.
> + */
> +static void
> +setup_new_cluster(void)
>
> typo
>
> /control fine/control file/

Fixed.

[1]: https://www.postgresql.org/message-id/OS0PR01MB57165A8F24BEFF5F4CCBBE5994EFA%40OS0PR01MB5716.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED