Add idle active time patch by sdudoladov · Pull Request #4 · sdudoladov/postgres

sdudoladov · 2022-01-20T09:23:57Z

replacement of #2

I have reformatted the PR to have v5 and v6 as separate commits and reverted earlier merge of v5 on the master branch to ease reviews.
I did not find any explicit discussion on why idle_time was introduced and indeed got the identical values in the following query:

select idle_time * interval '1 millisecond' as tracked_idle_time, now() - backend_start - interval '1 millisecond' * (active_time + idle_in_transaction_time) as computed_idle_time  from pg_stat_activity where pid = your_pid  ;

so I've removed the idle time tracking.

float8 and double are effectively synonyms so I left the types of st_total_active_time and st_total_transaction_idle_time as is. In fact for example DoubleGetDatum is not even defined.
bugfix relative to the previous pr: already available counters in PG aka pgStatActiveTime and pgStatidleInTransactionTime are no longer used to track per-backend values, see comments for explanation.
fixed whitespace issues in pgstat.h

CyberDem0n

Last, but not least, code is not formatted according to Postgres standards. Please make yourself familiar with https://wiki.postgresql.org/wiki/Developer_FAQ#What.27s_the_formatting_style_used_in_PostgreSQL_source_code.3F and https://wiki.postgresql.org/wiki/Configuring_vim_for_postgres_development

CyberDem0n · 2022-01-20T10:13:27Z

+	float8		st_total_active_time;;
+	float8 		st_total_transaction_idle_time;


I believe we should use PgStat_Counter for that (which is internally int64).

CyberDem0n · 2022-01-20T10:23:16Z

+			beentry->st_total_active_time += ((double) secs * 1000000 + usecs) / 1000.0; // convert to msec
+		}
+		else if (beentry->st_state ==  STATE_IDLEINTRANSACTION ||
+				 beentry->st_state == STATE_IDLEINTRANSACTION_ABORTED)
+		{
 			pgstat_count_conn_txn_idle_time((PgStat_Counter) secs * 1000000 + usecs);
+			beentry->st_total_transaction_idle_time += ((double) secs * 1000000 + usecs) / 1000.0; // convert to msec


If st_total_active_time and st_total_transaction_idle_time are PgStat_Counter types, calculation becomes a simple assignment:

beentry->st_total_active_time = pgStatActiveTime; beentry->st_total_transaction_idle_time = pgStatTransactionIdleTime;

And the conversion from usec to msec should happen in the pg_stat_get_activity() function https://github.com/sdudoladov/postgres/pull/4/files#diff-55e2a3d715341559d0d9c960cd2bc697595435c7e15fe59b35f4b36f279ca360R920-R921.

It will help to save a few CPU cycles.

pg_stat_get_db_idle_in_transaction_time() and pg_stat_get_db_active_time() are using exactly this approach.

sdudoladov

@CyberDem0n What is the status of pgident ? Is it still used ? When I run it on the source tree I see tens of violations

sdudoladov · 2022-01-21T12:55:12Z

 }
+
+Datum
+pg_stat_get_time_in_msec(PG_FUNCTION_ARGS)


This function saves CPU cycles because it does fp division only on data display and not on every update of the backend status, right ?

is there a need to split this function into two pg_stat_get_backend_active_time and pg_stat_get_backend_idle_in_transaction_time ? Those would be completely identical except the names.

Why have you created this function in the first place?
I only talked about modifying https://github.com/sdudoladov/postgres/pull/4/files#diff-55e2a3d715341559d0d9c960cd2bc697595435c7e15fe59b35f4b36f279ca360R920-R921

If you do something similar to how pg_stat_database view work - that doesn't make sense.

If you do something similar to how pg_stat_database view work

yes

that doesn't make sense.

true, reverted to simple fp division. I was too focused on understanding
pg_stat_databse and mechanics of catalog initialization.

sdudoladov · 2022-01-21T12:57:41Z

+		{
 			pgstat_count_conn_active_time((PgStat_Counter) secs * 1000000 + usecs);
-		else
+			beentry->st_total_active_time += (secs * 1000000 + usecs);


using simple assignemnt

beentry->st_total_active_time = pgStatActiveTime; beentry->st_total_transaction_idle_time = pgStatTransactionIdleTime;

here won't work, see the comment before the if code section

I see, pgstat_send_tabstat() resets them after sending...
Ok, then at least we can introduce a new variable in order to store the result of secs * 1000000 + usecs calculation.

sdudoladov · 2022-02-01T10:46:42Z

credit for diffs that form the core of v8: Kyotaro Horiguchi
see mailing list

In a similar effort to f736e18 and 110d817, fixup various usages of string functions where a more appropriate function is available and more fit for purpose. These changes include: 1. Use cstring_to_text_with_len() instead of cstring_to_text() when working with a StringInfoData and the length can easily be obtained. 2. Use appendStringInfoString() instead of appendStringInfo() when no formatting is required. 3. Use pstrdup(...) instead of psprintf("%s", ...) 4. Use pstrdup(...) instead of psprintf(...) (with no formatting) 5. Use appendPQExpBufferChar() instead of appendPQExpBufferStr() when the length of the string being appended is 1. 6. appendStringInfoChar() instead of appendStringInfo() when no formatting is required and string is 1 char long. 7. Use appendPQExpBufferStr(b, .) instead of appendPQExpBuffer(b, "%s", .) 8. Don't use pstrdup when it's fine to just point to the string constant. I (David) did find other cases of #8 but opted to use #4 instead as I wasn't certain enough that applying #8 was ok (e.g in hba.c) Author: Ranier Vilela, David Rowley Discussion: https://postgr.es/m/CAApHDvo2j2+RJBGhNtUz6BxabWWh2Jx16wMUMWKUjv70Ver1vg@mail.gmail.com

There've been a few complaints that it can be overly difficult to figure out why the planner picked a Memoize plan. To help address that, here we adjust the EXPLAIN output to display the following additional details: 1) The estimated number of cache entries that can be stored at once 2) The estimated number of unique lookup keys that we expect to see 3) The number of lookups we expect 4) The estimated hit ratio Technically #4 can be calculated using #1, #2 and #3, but it's not a particularly obvious calculation, so we opt to display it explicitly. The original patch by Lukas Fittl only displayed the hit ratio, but there was a fear that might lead to more questions about how that was calculated. The idea with displaying all 4 is to be transparent which may allow queries to be tuned more easily. For example, if #2 isn't correct then maybe extended statistics or a manual n_distinct estimate can be used to help fix poor plan choices. Author: Ilia Evdokimov <ilya.evdokimov@tantorlabs.com> Author: Lukas Fittl <lukas@fittl.com> Reviewed-by: David Rowley <dgrowleyml@gmail.com> Reviewed-by: Andrei Lepikhov <lepihov@gmail.com> Reviewed-by: Robert Haas <robertmhaas@gmail.com> Discussion: https://postgr.es/m/CAP53Pky29GWAVVk3oBgKBDqhND0BRBN6yTPeguV_qSivFL5N_g%40mail.gmail.com

apply v5 of the patch

1b873e3

sdudoladov mentioned this pull request Jan 20, 2022

apply v6 of the patch #2

Closed

8 tasks

sdudoladov force-pushed the add-idle-active-time-patch branch from 6295ac8 to 6b514fb Compare January 20, 2022 09:48

CyberDem0n reviewed Jan 20, 2022

View reviewed changes

sdudoladov force-pushed the add-idle-active-time-patch branch from 6b514fb to 60ec5d4 Compare January 21, 2022 12:44

sdudoladov commented Jan 21, 2022

View reviewed changes

sdudoladov force-pushed the add-idle-active-time-patch branch 3 times, most recently from d4ff7d7 to ac8fa4b Compare January 26, 2022 13:47

apply v6 of the patch

999e782

sdudoladov force-pushed the add-idle-active-time-patch branch 3 times, most recently from bd63917 to 0ed1bc3 Compare January 31, 2022 13:56

apply v7

146dacb

sdudoladov force-pushed the add-idle-active-time-patch branch from 0ed1bc3 to 146dacb Compare January 31, 2022 13:58

sdudoladov force-pushed the add-idle-active-time-patch branch 2 times, most recently from 73ef7d4 to db9ccfb Compare February 1, 2022 12:49

apply v8

a1d7858

sdudoladov force-pushed the add-idle-active-time-patch branch from db9ccfb to a1d7858 Compare February 1, 2022 13:13

sdudoladov mentioned this pull request Apr 20, 2022

Prepare v10 of the patch #5

Closed

sdudoladov closed this Apr 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add idle active time patch#4

Add idle active time patch#4
sdudoladov wants to merge 4 commits intomasterfrom
add-idle-active-time-patch

sdudoladov commented Jan 20, 2022 •

edited

Loading

Uh oh!

CyberDem0n left a comment

Uh oh!

CyberDem0n Jan 20, 2022

Uh oh!

CyberDem0n Jan 20, 2022

Uh oh!

sdudoladov left a comment

Uh oh!

Uh oh!

sdudoladov Jan 21, 2022

Uh oh!

CyberDem0n Jan 21, 2022

Uh oh!

sdudoladov Jan 24, 2022 •

edited

Loading

Uh oh!

sdudoladov Jan 21, 2022

Uh oh!

CyberDem0n Jan 21, 2022

Uh oh!

sdudoladov commented Feb 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		float8 st_total_active_time;;
		float8 st_total_transaction_idle_time;

Conversation

sdudoladov commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyberDem0n left a comment

Choose a reason for hiding this comment

Uh oh!

CyberDem0n Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

CyberDem0n Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

sdudoladov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sdudoladov Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

CyberDem0n Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

sdudoladov Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdudoladov Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

CyberDem0n Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

sdudoladov commented Feb 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sdudoladov commented Jan 20, 2022 •

edited

Loading

sdudoladov Jan 24, 2022 •

edited

Loading