Re: Log connection establishment timings

Hi,

On Thu, Mar 06, 2025 at 06:16:07PM -0500, Melanie Plageman wrote:
> Attached v12 does this (uses timestamps instead of instr_time).

Thanks for the new version!

> I've done some assorted cleanup. Most notably:
>
> I removed LOG_CONNECTION_NONE because it could lead to wrong results
> to have a bitmask with a flag value that is 0 (it could be set at the
> same time other flags are set, so you could never use it correctly).

Oh right, and starting with LOG_CONNECTION_NONE/OFF = (1 << 0) does not
make that much sense...

A couple of comments regarding 0002:

=== 01

Given that conn_timing.ready_for_use is only used here:

+ if (!reported_first_ready_for_query &&
+ (log_connections & LOG_CONNECTION_READY_FOR_USE) &&
+ IsConnectionBackend(MyBackendType))
+ {
+ uint64 total_duration,
+ fork_duration,
+ auth_duration;
+
+ conn_timing.ready_for_use = GetCurrentTimestamp();
+
+ total_duration =
+ TimestampDifferenceMicroseconds(conn_timing.socket_create,
+ conn_timing.ready_for_use);

I wonder if ready_for_use needs to be part of ConnectionTiming after all.
I mean we could just:

"
total_duration = TimestampDifferenceMicroseconds(conn_timing.socket_create, GetCurrentTimestamp())
"

That said, having ready_for_use part of ConnectionTiming could be
useful if we decide to create a SQL API on top of this struct though. So,
that's probably fine and better as it is.

And if we keep it part of ConnectionTiming, then:

=== 02

+ if (!reported_first_ready_for_query &&
+ (log_connections & LOG_CONNECTION_READY_FOR_USE) &&
+ IsConnectionBackend(MyBackendType))

What about getting rid of reported_first_ready_for_query and check if
conn_timing.ready_for_use is zero instead?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com