On 2/1/23 11:59, Sergey Dudoladov wrote:
> Justin, thank you for the fast review. The new version is attached.
This is looking very good. One bigger comment:
> + myextra = (int *) guc_malloc(ERROR, sizeof(int));
> + *myextra = newlogconnect;
If I've understood Tom correctly in [1], both of these guc_mallocs
should be using a loglevel less than ERROR, to avoid forcing a
postmaster exit when out of memory. (I used WARNING in that thread
instead, which seemed to be acceptable.)
And a couple of nitpicks:
> + Causes the specified stages of each connection attempt to the server to be logged. Example:
<literal>authorized,disconnected</literal>.
Long line; should be rewrapped.
> + else {
> + GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE);
> + GUC_check_errmsg("invalid value '%s'", stage);
> + GUC_check_errdetail("Valid values to use in the list are 'all', 'received', 'authenticate
> + " If 'all' is present, it must be the only value in the list.");
I think the errmsg here should reuse the standard message format
invalid value for parameter "%s": "%s"
both for consistency and ease of translation.
Thanks!
--Jacob
[1] https://www.postgresql.org/message-id/2012342.1658356951%40sss.pgh.pa.us
