RE: [PoC] pg_upgrade: allow to upgrade publisher node - Mailing list pgsql-hackers

From Wei Wang (Fujitsu)
Subject RE: [PoC] pg_upgrade: allow to upgrade publisher node
Date
Msg-id OS3PR01MB6275F61633DA00ECF5AF578D9E439@OS3PR01MB6275.jpnprd01.prod.outlook.com
Whole thread Raw
In response to RE: [PoC] pg_upgrade: allow to upgrade publisher node  ("Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>)
Responses RE: [PoC] pg_upgrade: allow to upgrade publisher node
List pgsql-hackers
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

pgsql-hackers by date:

Previous
From: Nathan Bossart
Date:
Subject: Re: createuser --memeber and PG 16
Next
From: Thomas Munro
Date:
Subject: Re: LLVM 16 (opaque pointers)