On Tue, May 9, 2023 at 17:44 PM Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> wrote:
> Thank you for reviewing! PSA new version.
Thanks for your patches.
Here are some comments for 0001 patch:
1. In the function getLogicalReplicationSlots
```
+ /*
+ * Note: Currently we do not have any options to include/exclude slots
+ * in dumping, so all the slots must be selected.
+ */
+ slotinfo[i].dobj.dump = DUMP_COMPONENT_ALL;
```
I think currently we are only dumping the definition of logical replication
slots. It seems better to set it as DUMP_COMPONENT_DEFINITION here.
2. In the function dumpLogicalReplicationSlot
```
+ ArchiveEntry(fout, slotinfo->dobj.catId, slotinfo->dobj.dumpId,
+ ARCHIVE_OPTS(.tag = slotname,
+ .description = "REPLICATION SLOT",
+ .section = SECTION_POST_DATA,
+ .createStmt = query->data));
```
I think if we do not set the member dropStmt in macro ARCHIVE_OPTS here, when we
specifying the option "--logical-replication-slots-only" and option "-c/--clean"
together, the "-c/--clean" will not work.
I think that we could use the function pg_drop_replication_slot to set this
member. Then, in the main function in the pg_dump.c file, we should add a check
to prevent specifying option "--logical-replication-slots-only" and
option "--if-exists" together.
Or, we could simply add a check to prevent specifying option
"--logical-replication-slots-only" and option "-c/--clean" together.
What do you think?
Regards,
Wang wei
