Re: Issue with logical replication slot during switchover

Hi,
Here the version 1 of the patch with the modifications.
I do a git diff --check until there are no more errors.
In a next version of the patch, I think I have make change to call
ReplicationSlotCreate() function with the value of the flag
allow_overwrite be consistent and modify the
interface ReplicationSlotCreate to display the attribute allow_overwrite.

Best regards,

Fabrice

On Mon, Sep 1, 2025 at 9:42 AM Fabrice Chapuis <fabrice636861(at)gmail(dot)com>
wrote:

> Hi,
> I will make de modifications based on the remarks you mentioned.
>
> Regards,
>
> Fabrice
>
> On Mon, Sep 1, 2025 at 5:45 AM shveta malik <shveta(dot)malik(at)gmail(dot)com>
> wrote:
>
>> On Sat, Aug 30, 2025 at 11:43 AM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com>
>> wrote:
>> >
>> > Hi Fabrice,
>> >
>> > Thanks for providing the patch. I reviewed your patch and have
>> > following comment:
>> >
>> > 1. I think we should add a commit message in the patch. It would help
>> > to give an understanding of the patch.
>> >
>> > 2. I tried applying patch on HEAD, it generates following warnings:
>> > Applying: fix failover slot issue when doing a switchover
>> > .git/rebase-apply/patch:31: trailing whitespace.
>> > bool allow_overwrite = false;
>> > .git/rebase-apply/patch:45: trailing whitespace.
>> > bool synced;
>> > .git/rebase-apply/patch:55: trailing whitespace.
>> >
>> > .git/rebase-apply/patch:56: trailing whitespace.
>> > if (!synced){
>> > .git/rebase-apply/patch:57: trailing whitespace.
>> > /*
>> > warning: squelched 16 whitespace errors
>> > warning: 21 lines add whitespace errors.
>>
>> 'git diff --check' can be used before 'git commit' to get info of
>> above issues before creating a patch.
>>
>> > 3. I also tried to build the patch on HEAD and it is giving the
>> following error:
>> > ubuntu(at)ubuntu-Virtual-Machine:~/Project/pg/postgres$
>> > ./../../install-clean.sh pg_30_8 > warn.log
>> > launcher.c: In function ‘CreateConflictDetectionSlot’:
>> > launcher.c:1457:9: error: too few arguments to function
>> ‘ReplicationSlotCreate’
>> > 1457 | ReplicationSlotCreate(CONFLICT_DETECTION_SLOT, false,
>> > RS_PERSISTENT, false,
>> > | ^~~~~~~~~~~~~~~~~~~~~
>> > In file included from launcher.c:35:
>> > ../../../../src/include/replication/slot.h:307:13: note: declared here
>> > 307 | extern void ReplicationSlotCreate(const char *name, bool
>> db_specific,
>> > | ^~~~~~~~~~~~~~~~~~~~~
>> > make[4]: *** [<builtin>: launcher.o] Error 1
>> > make[3]: *** [../../../src/backend/common.mk:37: logical-recursive]
>> Error 2
>> > make[2]: *** [common.mk:37: replication-recursive] Error 2
>> > make[1]: *** [Makefile:42: install-backend-recurse] Error 2
>> > make: *** [GNUmakefile:11: install-src-recurse] Error 2
>> >
>> > 4. I have some general comments regarding formatting of the patch.
>> > + // Both local and remote slot have the same name
>> >
>> > We use following format for single line comments:
>> > /* comment text */
>> > and for multi line comments we use following format:
>> > /*
>> > * comment text begins here
>> > * and continues here
>> > */
>> >
>> > 5. We should use proper indentation here:
>> > + elog(LOG, "Logical replication slot %s created with option
>> > allow_overwrite to %s",
>> > + NameStr(slot->data.name),
>> > + slot->data.allow_overwrite ? "true" : "false");
>> >
>>
>> src/tools/pgindent <file name>
>> can be used to do indentation before creating a patch.
>>
>> > 6.
>> > + if (!synced){
>> > + /*
>> > + * Check if we need to overwrite an existing
>> > + * logical slot
>> > + */
>> > We should start the parentheses from the next line.
>> > Also, indentation for comment is not correct here.
>> >
>> > 7.
>> > + if (allow_overwrite){
>> > + /*
>> > + * Get rid of a replication slot that is no
>> > + *longer wanted
>> > + */
>> > Similar comment as above.
>> >
>> > Please refer [1] [2] for proper formatting of the patch.
>> > [1]: https://www.postgresql.org/docs/devel/source-format.html
>> > [2]: https://wiki.postgresql.org/wiki/Creating_Clean_Patches
>> >
>> > Thanks,
>> > Shlok Kyal
>>
>