Re: Add connection active, idle time to pg_stat_activity

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.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com