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