On Tuesday, September 5, 2023 3:35 PM Kuroda, Hayato/黒田 隼人 <kuroda.hay...@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
0001-use-simple-ptr-list_topup_patch
Description: 0001-use-simple-ptr-list_topup_patch
