On Tue, May 9, 2023 at 17:44 PM Hayato Kuroda (Fujitsu) <kuroda.hay...@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
