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

On Tues, May 16, 2023 at 14:15 PM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com> wrote:
> Dear Peter,
> 
> Thanks for reviewing! PSA new version patchset.

Thanks for updating the patch set.

Here are some comments:
===
For patches 0001

1. The latest patch set fails to apply because the new commit (0245f8d) in HEAD.

~~~

2. In file pg_dump.h.
```
+/*
+ * The LogicalReplicationSlotInfo struct is used to represent replication
+ * slots.
+ *
+ * XXX: add more attributes if needed
+ */
+typedef struct _LogicalReplicationSlotInfo
+{
+	DumpableObject dobj;
+	char	   *plugin;
+	char	   *slottype;
+	bool		twophase;
+} LogicalReplicationSlotInfo;
```

Do we need the structure member "slottype"? It seems we do not use "slottype"
because we only dump logical replication slot.

===
For patch 0002

3. In the function SaveSlotToPath
```
-	/* and don't do anything if there's nothing to write */
-	if (!was_dirty)
+	/*
+	 * and don't do anything if there's nothing to write, unless it's this is
+	 * called for a logical slot during a shutdown checkpoint, as we want to
+	 * persist the confirmed_flush_lsn in that case, even if that's the only
+	 * modification.
+	 */
+	if (!was_dirty && !is_shutdown && !SlotIsLogical(slot))
```
It seems that the code isn't consistent with our expectation.
If this is called for a physical slot during a shutdown checkpoint and there's
nothing to write, I think it will also persist physical slots to disk.

===
For patch 0003

4. In the function check_for_parameter_settings
```
+	/* --include-logical-replication-slots can be used since PG16. */
+	if (GET_MAJOR_VERSION(new_cluster->major_version < 1600))
+		return;
```
It seems that there is a slight mistake (the input of GET_MAJOR_VERSION) in the
if-condition:
GET_MAJOR_VERSION(new_cluster->major_version < 1600)
->
GET_MAJOR_VERSION(new_cluster->major_version) <= 1500

Please also check the similar if-conditions in the below two functions
check_for_confirmed_flush_lsn (in 0003 patch)
check_are_logical_slots_active (in 0004 patch)

Regards,
Wang wei