On Wed, 10 Nov 2021 at 09:05, Dilip Kumar <[email protected]> wrote:
>
> On Tue, Nov 9, 2021 at 8:28 PM Rafia Sabih <[email protected]> wrote:
> >
> > On Tue, 2 Nov 2021 at 09:00, Dilip Kumar <[email protected]> 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
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index eb560955cd..4dfa33ffa9 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -839,7 +839,10 @@ CREATE VIEW pg_stat_activity AS
s.backend_xmin,
S.query_id,
S.query,
- S.backend_type
+ S.backend_type,
+ S.active_time,
+ S.idle_in_transaction_time,
+ S.idle_time
FROM pg_stat_get_activity(NULL) AS S
LEFT JOIN pg_database AS D ON (S.datid = D.oid)
LEFT JOIN pg_authid AS U ON (S.usesysid = U.oid);
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index b7d0fbaefd..3e0eb963b3 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -249,6 +249,7 @@ PgStat_Counter pgStatBlockWriteTime = 0;
static PgStat_Counter pgLastSessionReportTime = 0;
PgStat_Counter pgStatActiveTime = 0;
PgStat_Counter pgStatTransactionIdleTime = 0;
+PgStat_Counter pgStatTransactionIdleInTxnTime = 0;
SessionEndType pgStatSessionEndCause = DISCONNECT_NORMAL;
/* Record that's written to 2PC state file when pgstat state is persisted */
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index 7229598822..2d7d3b6dce 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -577,6 +577,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
*/
if ((beentry->st_state == STATE_RUNNING ||
beentry->st_state == STATE_FASTPATH ||
+ beentry->st_state == STATE_IDLE ||
beentry->st_state == STATE_IDLEINTRANSACTION ||
beentry->st_state == STATE_IDLEINTRANSACTION_ABORTED) &&
state != beentry->st_state)
@@ -590,9 +591,21 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
if (beentry->st_state == STATE_RUNNING ||
beentry->st_state == STATE_FASTPATH)
- pgstat_count_conn_active_time((PgStat_Counter) secs * 1000000 + usecs);
+ {
+ 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;
+ }
}
/*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index ff5aedc99c..471af0db6a 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -567,7 +567,7 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
Datum
pg_stat_get_activity(PG_FUNCTION_ARGS)
{
-#define PG_STAT_GET_ACTIVITY_COLS 30
+#define PG_STAT_GET_ACTIVITY_COLS 33
int num_backends = pgstat_fetch_stat_numbackends();
int curr_backend;
int pid = PG_ARGISNULL(0) ? -1 : PG_GETARG_INT32(0);
@@ -916,6 +916,10 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
nulls[29] = true;
else
values[29] = UInt64GetDatum(beentry->st_query_id);
+
+ values[30] = Int64GetDatum(beentry->st_active_time);
+ values[31] = Int64GetDatum(beentry->st_idle_in_transaction_time);
+ values[32] = Int64GetDatum(beentry->st_idle_time);
}
else
{
@@ -944,6 +948,9 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
nulls[27] = true;
nulls[28] = true;
nulls[29] = true;
+ nulls[30] = true;
+ nulls[31] = true;
+ nulls[32] = true;
}
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index d068d6532e..6f2b1a8dbd 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5348,9 +5348,9 @@
proname => 'pg_stat_get_activity', prorows => '100', proisstrict => 'f',
proretset => 't', provolatile => 's', proparallel => 'r',
prorettype => 'record', proargtypes => 'int4',
- proallargtypes => '{int4,oid,int4,oid,text,text,text,text,text,timestamptz,timestamptz,timestamptz,timestamptz,inet,text,int4,xid,xid,text,bool,text,text,int4,text,numeric,text,bool,text,bool,int4,int8}',
- proargmodes => '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
- proargnames => '{pid,datid,pid,usesysid,application_name,state,query,wait_event_type,wait_event,xact_start,query_start,backend_start,state_change,client_addr,client_hostname,client_port,backend_xid,backend_xmin,backend_type,ssl,sslversion,sslcipher,sslbits,ssl_client_dn,ssl_client_serial,ssl_issuer_dn,gss_auth,gss_princ,gss_enc,leader_pid,query_id}',
+ proallargtypes => '{int4,oid,int4,oid,text,text,text,text,text,timestamptz,timestamptz,timestamptz,timestamptz,inet,text,int4,xid,xid,text,bool,text,text,int4,text,numeric,text,bool,text,bool,int4,int8,int4,int4},int4',
+ proargmodes => '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
+ proargnames => '{pid,datid,pid,usesysid,application_name,state,query,wait_event_type,wait_event,xact_start,query_start,backend_start,state_change,client_addr,client_hostname,client_port,backend_xid,backend_xmin,backend_type,ssl,sslversion,sslcipher,sslbits,ssl_client_dn,ssl_client_serial,ssl_issuer_dn,gss_auth,gss_princ,gss_enc,leader_pid,query_id,active_time,idle_in_transaction_time,idle_time}',
prosrc => 'pg_stat_get_activity' },
{ oid => '3318',
descr => 'statistics: information about progress of backends running maintenance command',
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index bcd3588ea2..916764a02a 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -985,6 +985,7 @@ extern PgStat_Counter pgStatBlockWriteTime;
*/
extern PgStat_Counter pgStatActiveTime;
extern PgStat_Counter pgStatTransactionIdleTime;
+extern PgStat_Counter pgStatTransactionIdleInTxnTime;
/*
@@ -1092,6 +1093,8 @@ extern void pgstat_initstats(Relation rel);
(pgStatActiveTime += (n))
#define pgstat_count_conn_txn_idle_time(n) \
(pgStatTransactionIdleTime += (n))
+#define pgstat_count_conn_txn_idle_in_txn_time(n) \
+ (pgStatTransactionIdleInTxnTime += (n))
extern void pgstat_count_heap_insert(Relation rel, PgStat_Counter n);
extern void pgstat_count_heap_update(Relation rel, bool hot);
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index 8042b817df..063262e6ae 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -168,6 +168,10 @@ typedef struct PgBackendStatus
/* query identifier, optionally computed using post_parse_analyze_hook */
uint64 st_query_id;
+
+ int64 st_active_time;
+ int64 st_idle_in_transaction_time;
+ int64 st_idle_time;
} PgBackendStatus;
