Re: Log connection establishment timings

On Wed, Mar 5, 2025 at 5:36 AM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Looking at the enum array:
>
> "
> static const struct config_enum_entry log_connections_options[] = {
> {"off", 0, false},
> {"on", LOG_CONNECTION_BASIC, false},
> {"true", LOG_CONNECTION_BASIC, true},
> {"false", 0, true},
> {"yes", LOG_CONNECTION_BASIC, true},
> {"no", 0, true},
> {"timings", LOG_CONNECTION_TIMINGS | LOG_CONNECTION_BASIC, false},
> {"1", LOG_CONNECTION_BASIC, true},
> {"0", 0, true},
> {NULL, 0, false}
> };
> "
> and at parse_bool_with_len(), then it looks like it used to work with
> log_connections set to things like "y, ye, yes, t, tr, tru, true, f, fa, fal,
> fals, false, n and no" but now we want the full words.

Yes, this is a bit unfortunate, but I don't think we can accept t, f,
fa, etc. There is precedent for changing a boolean GUC to an enum --
synchronous_commit was like this.

> I wonder if we should go so deep in the backward compatibility though. Maybe
> that's worth to do if simple enough? (not sure how complicated it would be).

Yea, I don't think it makes sense to do more than the ones included in
that array.

> + ereport(LOG,
> + errmsg("connection establishment times (ms): total: %ld, fork: %ld, authentication: %ld",
> + (long int) INSTR_TIME_GET_MILLISEC(total_duration),
> + (long int) INSTR_TIME_GET_MILLISEC(conn_timing.fork_duration),
> + (long int) INSTR_TIME_GET_MILLISEC(conn_timing.auth_duration)));
>
> I wonder if doing:
>
> + errmsg("connection establishment times (ms): total: %.3f, fork: %.3f, authentication: %.3f",
> + (double) INSTR_TIME_GET_NANOSEC(total_duration) / 1000000.0,
> + (double) INSTR_TIME_GET_NANOSEC(conn_timing.fork_duration) / 1000000.0,
> + (double) INSTR_TIME_GET_NANOSEC(conn_timing.auth_duration) / 1000000.0));
>
> wouln't be better. For example, on my machine I'd get:
>
> connection establishment times (ms): total: 13.537, fork: 0.619, authentication: 0.267
>
> instead of:
>
> connection establishment times (ms): total: 13, fork: 0, authentication: 0

Good idea. I've done this in my branch which I will post as patches later.

This morning, Andres reminded me off-list that we actually can still
support unquoted off and on values (as well as other single word
tokens like true, 1, etc) even if we transitioned to a string/list GUC
type. He also pointed out that tab-completion would be basically
useless for log_connections since it is not allowed to be set once the
connection is already established.

As such, I wonder if my PGC_SET idea is not worth the effort and I
should revise the earlier patch version which specified the stages but
allow for on, off, true, yes, 1 for backwards compatibility and not
include disconnections (so we don't remove the log_disconnections GUC
this release).

That would allow
log_connections = on
log_connections = off
as well as
log_connections = 'received, authenticated'
and even
log_connections = received

- Melanie