Message-ID: <7599fe17eabde4e5b1256af63c05a72023132de7.camel@moonset.ru>
Subject: Re: Add connection active, idle time to pg_stat_activity
From: Andrei Zubkov <zubkov@moonset.ru>
To: Sergey Dudoladov <sergey.dudoladov@gmail.com>, PostgreSQL Hackers
	 <pgsql-hackers@lists.postgresql.org>
Date: Wed, 25 Oct 2023 16:12:42 +0300
In-Reply-To: <CAA8Fd-pk89q=spKJ1vDeO2GPwEQme5Ub34v7B8XrpCE_sEOtZA@mail.gmail.com>
References: <20220201.135516.23011068590309830.horikyota.ntt@gmail.com>
	 <CAA8Fd-pUwWz=2fu3cSzB1iH5f7zQYpeHNmV0-sqK8mxPnq2nnQ@mail.gmail.com>
	 <20220322001335.x76b2htvnih2eofk@alap3.anarazel.de>
	 <CAA8Fd-q+2zpQngKdLk=vu+NVDuJp=5kts-3mJdo-swoAo0kcfw@mail.gmail.com>
	 <d449de03-fa88-0638-c279-65335b995f13@amazon.com>
	 <CAA8Fd-oS8WHe5n6bWZWW07+65dk4p+njCRCV0hMDYPVc-9c0pQ@mail.gmail.com>
	 <a8ec82fc03dcac57998cdb31a9979cc3@oss.nttdata.com>
	 <CAA8Fd-pMc6rwGZUpesi2sfPaP-q85qwt06cSESTRDiAsNyuaLQ@mail.gmail.com>
	 <20221109015614.evw5egu7fansxj37@awork3.anarazel.de>
	 <CAKFQuwYeprTF-sxd-4uDKH9CmNecL-N=6wz4TM-X0noXAg9Z8A@mail.gmail.com>
	 <20221109023727.enmes3ab6gvr3zsq@awork3.anarazel.de>
	 <CAKFQuwZ920iekwcwWP7WUHd8LRi2H6H_E1dZEmrHG68364PaFQ@mail.gmail.com>
	 <CAA8Fd-rY1hDjrCFaXOaOz9h70SCKX5_BY4jhUfJO+tZ8iRD3Ug@mail.gmail.com>
	 <CAAhFRxibrZkLmJnH30T8dTK4JtQXCbmnquU9QvBg0vS18iu8wg@mail.gmail.com>
	 <CAA8Fd-pk89q=spKJ1vDeO2GPwEQme5Ub34v7B8XrpCE_sEOtZA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4-2 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/7599fe17eabde4e5b1256af63c05a72023132de7.camel%40moonset.ru>
Precedence: bulk

Hi Sergey,

I've done a review of this patch. I found the patch idea very useful,
thank you for the patch. I've noted something observing this patch:
1. Patch can't be applied on the current master. My review is based on
   application of this patch over ac68323a878
2. Being applied over ac68323a878 patch works as expected.
3. Field names seems quite long to me (and they should be uniformly
   named with the same statistics in other views. For example
   "running" term is called "active" in pg_stat_database)
4. Meaningless spaces at the end of line:
   - backend_status.c:586
   - monitoring.sgml:5857
5. Patch adds

     usecs_diff =3D secs * 1000000 + usecs;

   at backend_status.c:pgstat_report_activity() to optimize
   calculations. But

     pgstat_count_conn_active_time((PgStat_Counter) secs * 1000000 +
usecs);
   and =20
     pgstat_count_conn_txn_idle_time((PgStat_Counter) secs * 1000000 +
usecs);

   are left in place after that.
6. I'm not sure that I can understand the comment
     /* Keep statistics for pg_stat_database intact */
   at backend_status.c:600 correctly. Can you please explain it a
   little?
7. Tests seems incomplete. It looks like we can check increments in
   all fields playing with transactions in tests.

Also, I have a thought about other possible improvements fitting to
this patch.

The view pg_stat_session is really needed in Postgres but I think it
should have much more statistics. I mean all resource statistics
related to sessions. Every backend has instrumentation that tracks
resource consumption. Data of this instrumentation goes to the
cumulative statistics system and is used in monitoring extensions
(like pg_stat_statements). I think pg_stat_session view is able to add
one more dimension of monitoring - a dimension of sessions. In my
opinion this view should provide resource consumption statistics of
current sessions in two cumulative sets of statistics - since backend
start and since transaction start.  Such view will be really useful in
monitoring of long running sessions and transactions providing
resource consumption information besides timing statistics.

regards, Andrei Zubkov
Postgres Professional