Re: Log connection establishment timings

On 2025/03/04 1:14, Bertrand Drouvot wrote:
> === 2
>
> +/*
> + * Validate the input for the log_connections GUC.
> + */
> +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.

> "
> + else if (pg_strcasecmp(item, "all") == 0)
> + {
> + GUC_check_errdetail("Must specify \"all\" alone with no additional options, whitespace, or characters.");
> + goto log_connections_error;
> + }
> "
>
> but yeah that probably makes more sense that way.

Wouldn't this restriction be a bit confusing for users? If there is no particular
reason for it, wouldn’t it be simpler and clearer to allow "all" with additional
options, whitespace, or characters?

> === 5
>
> +typedef enum ConnectionLogOption
> +{
> + LOG_CONNECTION_RECEIVED = (1 << 0),
> + LOG_CONNECTION_AUTHENTICATED = (1 << 1),
> + LOG_CONNECTION_AUTHORIZED = (1 << 2),
> + LOG_CONNECTION_DISCONNECTED = (1 << 3),
> +} ConnectionLogOption;
>
> I wonder if it would make sense to add LOG_CONNECTION_ALL here
> (LOG_CONNECTION_RECEIVED | LOG_CONNECTION_AUTHENTICATED ..)

+1

Here are some review comments from me:

+ <literal>''</literal> which causes no connection logging messages to be
+ emitted.
<snip>
+ May be set to <literal>''</literal> to disable connection logging,

Wouldn't it be clearer to describe this as "the empty string <literal>''</literal>"
like other GUC options, so users immediately understand what it represents?

+ {"log_connections", PGC_SU_BACKEND, LOGGING_WHAT,
+ gettext_noop("Logs information about events during connection establishment."),
+ NULL,
+ GUC_LIST_INPUT

Like log_destination, wouldn’t it be better to explicitly list
all valid values in the long description?

+#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.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION