Re: Log connection establishment timings - Mailing list pgsql-hackers

Hi,

On Thu, Mar 06, 2025 at 11:41:03AM -0500, Melanie Plageman wrote:
> 
> I still need to do some more manual testing and validation.
> 
> On Thu, Mar 6, 2025 at 9:56 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> 
> > +       /* If an empty string was passed, we're done. */
> > +       if (list_length(elemlist) == 0)
> > +               return 0;
> > +
> < -- snip -->
> > what about storing the list_length(elemlist) at the start to avoid the multiple
> > list_length(elemlist) calls?
> 
> Since it would only be called twice

Oh right, I was focusing on the top loop but missed the "return" if the length is
 > 1.

> and it has the length stored in
> the List struct, I prefer it as is -- without an extra local variable.

Yeah, me too, sorry for the noise.

> How do you find spelling mistakes in patches usually?

Given the fact that I'm not a native english speaker I would say: By luck ;-)
I just try do one pass during the review just focusing on those.

> I tried `git
> show | aspell list` -- but of course that includes lots of variables
> and such that aren't actually spelling mistakes.

Trying to "automate" is actually a good idea!

> Well, I actually didn't want to call it "timings" because it implies
> that we only measure the timings when it is specified. But we actually
> get the timings unconditionally for client backends and wal sender.

> Don't you think it is more confusing for the user if we call it
> timings and users think if they don't include that timings aren't
> measured?

That's a good question. I think the expectations are set in the log_connections
GUC documentation. It says "Causes the specified stages of each connection attempt
to the server to be logged". So for me that would be like: log yes or no the
timings.

Of course to be logged those need to be measured and one could expect the timings
not to be measured if timings is not set.

But at the end, what we're "really" interested in this thread, given its $SUBJECT,
is to actually log the timings. 

So yeah, from my point of view I think that it would be better if the option
name highlights the fact that it is about timing (and not only in its description
as with ready_for_query). One option to address your concern could be to re-word
the doc a bit saying (logs timings that are measured independently of the GUC value,
something like this).

That said If you feel that it's too confusing to use "timings" then I'm fine too.

> Also, I thought changing the message output to say "ready for query"
> somewhere in it makes it more clear when that message is actually
> emitted in a connection's lifetime. What do you think?

I agree if we keep ready_for_query as the option name.

Regards,

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