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
