Thanks so much for your review!
On Mon, Mar 3, 2025 at 12:08 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
> > +bool
> > +check_log_connections(char **newval, void **extra, GucSource source)
> > +{
> >
> > This function is pretty close to check_debug_io_direct() for example and its
> > overall content, memory management, looks good to me.
>
> I guess that check_log_connections() is implemented to use SplitGUCList()
> because check_debug_io_direct() does too. However, according to the comment for
> SplitGUCList() — "This is used to split the value of a GUC_LIST_QUOTE GUC variable",
> it seems more appropriate to use SplitIdentifierString() instead,
> like other options such as log_destination, since log_connections is not
> a GUC_LIST_QUOTE GUC.
Ah, this was a very good catch. It seems like actually debug_io_direct
should use that too. Now I'm not using the string list method anymore,
but thanks for catching this.
> +#log_connections = ''
>
> It would also be helpful to include all valid values in a comment
> within postgresql.conf.sample, making it easier for users to configure.
I haven't updated postgresql.conf.sample in v9 proposed here [1]. But
now that log_connections has three options, do you think I should
specify all three? It used to only specify off, but probably because
it was obvious that "on" was the opposite of "off".
- Melanie
[1] https://www.postgresql.org/message-id/CAAKRu_aoerKAOYKkc7-JGq2bixrYTbViK_7EeLNhFUGoT_omFw%40mail.gmail.com
