Re: Log connection establishment timings

On Thu, Feb 27, 2025 at 1:50 AM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Agree. IIUC, I think that Fujii-san's idea was to extend log_connections with
> a new option "timing" (i.e move it from ConfigureNamesBool to say
> ConfigureNamesEnum with say on, off and timing?). I think that's a good idea.
>
> I just did a quick check and changing a GUC from ConfigureNamesBool to
> ConfigureNamesEnum is something that has already been done in the past (see
> 240067b3b0f and ffd37740ee6 for example).
>
> In my previous up-thead message, I did not mean to suggest to add a new GUC,
> just saying that when new "timing" is measured then users have the choice to
> enable or disable it.

I was just talking to Andres off-list and he mentioned that the volume
of log_connections messages added in recent releases can really be a
problem for users. He said ideally we would emit one message which
consolidated these (and make sure we did so for failed connections too
detailing the successfully completed stages).

However, since that is a bigger project (with more refactoring, etc),
he suggested that we change log_connections to a GUC_LIST
(ConfigureNamesString) with options like "none", "received,
"authenticated", "authorized", "all".

Then we could add one like "established" for the final message and
timings my patch set adds. I think the overhead of an additional log
message being emitted probably outweighs the overhead of taking those
additional timings.

String GUCs are a lot more work than enum GUCs, so I was thinking if
there is a way to do it as an enum.

I think we want the user to be able to specify a list of all the log
messages they want included, not just have each one include the
previous ones. So, then it probably has to be a list right? There is
no good design that would fit as an enum.

> > I've added INSTR_TIME_GET_DURATION_SINCE(start_time). Which I like
> > because it seems generally useful.
>
> Great idea! Could probably be used in other places but I did not check and
> it's outside the scope of this thread anyway.
>
> > It does not however cut down on LOC, so I'm somewhat on the fence.
>
> I think that's somehow also around code maintenance (not only LOC), say for example
> if we want to add more "child_type" in the check (no need to remember to update both
> locations).

I didn't include checking the child_type in that function since it is
unrelated to instr_time, so it sadly wouldn't help with that. We could
macro-ize the child_type check were we to add another child_type.

- Melanie