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.ma...@gmail.com> wrote:
> On Sat, Aug 30, 2025 at 11:43 AM Shlok Kyal <shlok.kyal....@gmail.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@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
>
