Dear Wang, Thank you for reviewing! PSA new version.
> For patches 0001
>
> 1. The latest patch set fails to apply because the new commit (0245f8d) in
> HEAD.
I didn't notice that. Thanks, fixed.
> 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.
As you said, this attribute is not needed. This is a garbage of previous
efforts.
Removed.
> 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.
You meant to say that we should not change handlings for physical case, right?
> For patch 0003
>
> 4. In the function check_for_parameter_settings
> ```
> + /* --include-logical-replication-slots can be used since PG 16. */
> + 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)
Done. I grepped with GET_MAJOR_VERSION, and confirmed they were fixed.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
v15-0001-pg_upgrade-Add-include-logical-replication-slots.patch
Description: v15-0001-pg_upgrade-Add-include-logical-replication-slots.patch
v15-0002-Always-persist-to-disk-logical-slots-during-a-sh.patch
Description: v15-0002-Always-persist-to-disk-logical-slots-during-a-sh.patch
v15-0003-pg_upgrade-Add-check-function-for-include-logica.patch
Description: v15-0003-pg_upgrade-Add-check-function-for-include-logica.patch
v15-0004-Change-the-method-used-to-check-logical-replicat.patch
Description: v15-0004-Change-the-method-used-to-check-logical-replicat.patch
