Re: [PATCH] Connection time for \conninfo

=?UTF-8?Q?Rodrigo_Ram=C3=ADrez_Norambuena?= <decipher.hk@gmail.com> writes:
> On Tue, Sep 3, 2019 at 11:06 PM Michael Paquier <michael@paquier.xyz> wrote:
>> You can do basically the same thing by looking at
>> pg_stat_activity.backend_start and compare it with the clock
>> timestamp.  Doing it at SQL level the way you want has also the
>> advantage to offer you a modular format output.

> What are you say seams little trick to what I propose in this patch
> because you'll need filter what is your connection, and if the
> connection is through  the connection pooler could be more.
> The main goal this is only getting information from the client side
> (psql) as frontend.

A couple of thoughts on this ---

* Michael's right that it's *possible* to get the session start time in
SQL, but having to find one's session's entry in pg_stat_activity is
pretty awkward.  I wonder if we should invent pg_backend_start_time()
analogous to pg_backend_pid().  Most of the other columns in
pg_stat_activity either already have similar functions (e.g. CURRENT_USER)
or don't seem especially useful to be able to retrieve for one's own
session, but this one maybe is.  That's somewhat orthogonal to the
proposed patch but we should probably be considering it in the same
discussion.

* It's not quite clear what the use-case is for displaying this in
\conninfo.  That's easy for humans to look at, but I don't really
understand why a human would care, and it's just about useless for
any programmatic application.

* I wonder why the proposal is to display duration of connection rather
than start time.  I don't especially like the idea that \conninfo
would change every time you look at it, so I'd tend to lean to the
latter, but maybe there are other arguments for one or the other.

* The patch appears to be tracking time measured at the client side,
which is going to be just enough different from the server's idea of
the session start time to be confusing/annoying.  Do we really want
to do it that way?

* There are other definitional questions like what happens when you
reconnect (either due to intentional \connect or momentary connection
loss), or what should happen when you have no connection at all thanks
to a non-recoverable connection loss.  I suppose the patch's code
provides some answers but I'm not sure that it's consistent or well
thought out.  (Commit aef362385 provides some precedent you should look
at in this regard, plus I think this patch has to be rebased over it
anyway.)

BTW, the result type of time(2) is not "int", so this is just wrong:

+    int connected; /* unixtime for connected init time */

This field name is pretty poorly chosen as well; it sounds like
it's a boolean "am I connected" state, and there's certainly no
hint that it's a time value.

            regards, tom lane