RE: Synchronizing slots from primary to standby - Mailing list pgsql-hackers

From Zhijie Hou (Fujitsu)
Subject RE: Synchronizing slots from primary to standby
Date
Msg-id OS0PR01MB5716C0B9B93CE2F51C51C90D945F2@OS0PR01MB5716.jpnprd01.prod.outlook.com
Whole thread Raw
In response to Re: Synchronizing slots from primary to standby  (Bertrand Drouvot <bertranddrouvot.pg@gmail.com>)
Responses RE: Synchronizing slots from primary to standby
List pgsql-hackers
On Wednesday, February 28, 2024 7:36 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
>
> On Wed, Feb 28, 2024 at 02:23:27AM +0000, Zhijie Hou (Fujitsu) wrote:
> > Attach the V100 patch set which addressed above comments.
>
> A few random comments:

Thanks for the comments!

>
> 1 ===
>
> +       if (!ok)
> +       {
> +               GUC_check_errdetail("List syntax is invalid.");
> +       }
>
> What about to get rid of the brackets here?

I personally prefer the current style.

>
> 2 ===
>
> +
> +       /*
> +        * If the replication slots' data have been initialized, verify if the
> +        * specified slots exist and are logical slots.
> +        */
>
> remove the empty line above the comment?

I feel it would be clean to have an empty line before the comments.

>
> 3 ===
>
> +check_standby_slot_names(char **newval, void **extra, GucSource source)
> +{
> +       if ((*newval)[0] == '\0')
> +               return true;
>
> I think "**newval == '\0'" is easier to read but that's a matter of taste and
> check_synchronous_standby_names() is already using the same so it's a nit.

I don't have a strong opinion on this, so will change if others also feel so.

>
> 4 ===
>
> Regarding the test, what about adding one to test the "new" behavior
> discussed up-thread? (logical replication will wait if slot mentioned in
> standby_slot_names is dropped and/or does not exist when the engine starts?)

Will think about this.

Best Regards,
Hou zj



pgsql-hackers by date:

Previous
From: jian he
Date:
Subject: Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445)
Next
From: Michael Paquier
Date:
Subject: Re: Add checkpoint/redo LSNs to recovery errors.