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
