Re: Add connection active, idle time to pg_stat_activity

From: Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>
To: Dilip Kumar <dilipbalaut(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Add connection active, idle time to pg_stat_activity
Date: 2021-11-10 08:17:22
Message-ID: CA+FpmFc2PVg=deZ5DY2zY3jekoqEo-F84c0Xcf0D3MJf1D+3zA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, 10 Nov 2021 at 09:05, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Tue, Nov 9, 2021 at 8:28 PM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com> wrote:
> >
> > On Tue, 2 Nov 2021 at 09:00, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
>
> > > About the patch, IIUC earlier all the idle time was accumulated in the
> > > "pgStatTransactionIdleTime" counter, now with your patch you have
> > > introduced one more counter which specifically tracks the
> > > STATE_IDLEINTRANSACTION state. But my concern is that the
> > > STATE_IDLEINTRANSACTION_ABORTED is still computed under STATE_IDLE and
> > > that looks odd to me. Either STATE_IDLEINTRANSACTION_ABORTED should
> > > be accumulated in the "pgStatTransactionIdleInTxnTime" counter or
> > > there should be a separate counter for that. But after your patch we
> > > can not accumulate this in the "pgStatTransactionIdleTime" counter.
> > >
> > As per your comments I have added it in pgStatTransactionIdleInTxnTime.
> > Please let me know if there are any further comments.
>
> I have a few comments,
>
> nulls[29] = true;
> + values[30] = true;
> + values[31] = true;
> + values[32] = true;
>
> This looks wrong, this should be nulls[] = true not values[]=true.
>
> if ((beentry->st_state == STATE_RUNNING ||
> beentry->st_state == STATE_FASTPATH ||
> beentry->st_state == STATE_IDLEINTRANSACTION ||
> beentry->st_state == STATE_IDLEINTRANSACTION_ABORTED) &&
> state != beentry->st_state)
> {
> if (beentry->st_state == STATE_RUNNING ||
> beentry->st_state == STATE_FASTPATH)
> {
> pgstat_count_conn_active_time((PgStat_Counter) secs * 1000000 + usecs);
> beentry->st_active_time = pgStatActiveTime;
> }
> else if (beentry->st_state == STATE_IDLEINTRANSACTION ||
> beentry->st_state == STATE_IDLEINTRANSACTION_ABORTED)
> {
> pgstat_count_conn_txn_idle_in_txn_time((PgStat_Counter) secs *
> 1000000 + usecs);
> beentry->st_idle_in_transaction_time = pgStatTransactionIdleInTxnTime;
> }
> else
> {
> pgstat_count_conn_txn_idle_time((PgStat_Counter) secs * 1000000 + usecs);
> beentry->st_idle_time = pgStatTransactionIdleTime;
> }
>
> It seems that in beentry->st_idle_time, you want to compute the
> STATE_IDLE, but that state is not handled in the outer "if", that
> means whenever it comes out of the
> STATE_IDLE, it will not enter inside this if check. You can run and
> test, I am sure that with this patch the "idle_time" will always
> remain 0.
>
Thank you Dilip for your time on this.
And yes you are right in both your observations.
Please find the attached patch for the updated version.

--
Regards,
Rafia Sabih

Attachment Content-Type Size
v3_add_idle_active_time.patch text/x-patch 6.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dinesh Chemuduru 2021-11-10 08:28:58 Re: [PROPOSAL] new diagnostic items for the dynamic sql
Previous Message Kyotaro Horiguchi 2021-11-10 08:14:11 Re: standby recovery fails (tablespace related) (tentative patch and discussion)