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