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
