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