Re: Issue with logical replication slot during switchover - Mailing list pgsql-hackers

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