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

On Tuesday, September 5, 2023 3:35 PM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com> wrote:
> 
> Dear Hou-san,
> 
> > Based on this, it’s possible that the slots we get each time when
> > checking wal_status are different, because they may get changed in between
> these checks.
> > This may not cause serious problems for now, because we will either
> > copy all the slots including ones invalidated when upgrading or we
> > report ERROR. But I feel it's better to get consistent result each
> > time we check the slots to close the possibility for problems in the
> > future. So, I feel we could centralize the check for wal_status and
> > slots fetch, so that even if some slots status changed after that, it won't have
> a risk to affect our check. What do you think ?
> 
> Thank you for giving the suggestion! I agreed that to centralize checks, and I
> had already started to modify. Here is the updated patch.
> 
> In this patch all slot infos are extracted in the
> get_old_cluster_logical_slot_infos(),
> upcoming functions uses them. Based on the change, two attributes
> confirmed_flush and wal_status were added in LogicalSlotInfo.
> 
> IIUC we cannot use strcut List in the client codes, so structures and related
> functions are added in the function.c. These are used for extracting unique
> plugins, but it may be overkill because check_loadable_libraries() handle
> duplicated entries. If we can ignore duplicated entries, these functions can be
> removed.
> 
> Also, for simplifying codes, only a first-met invalidated slot is output in the
> check_old_cluster_for_valid_slots(). Warning messages int the function were
> removed. I think it may be enough because check_new_cluster_is_empty() do
> similar thing. Please tell me if it should be reverted...

Thank for updating the patch ! here are few comments.

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.

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 ?


3.

+			curr->confirmed_flush = strtoLSN(
+											 PQgetvalue(res,
+														slotnum,
+														i_confirmed_flush),
+											 &have_error);

The indention looks a bit unusual.

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.

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.

Best Regards,
Hou zj