From 056cc366fafa8110f5761a7383ae1cd903dd728c Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Fri, 17 Jun 2022 12:48:34 -0700 Subject: [PATCH] pgstat: reduce timer overhead by leaving timer running. Previously the timer was enabled whenever there were any pending stats after executing a statement, just to then be disabled again when not idle anymore. That lead to an increase in GetCurrentTimestamp() calls from within timeout.c compared to 14. To avoid that increase, leave the timer enabled until stats are reported, rather than until idle. The timer is only disabled once the pending stats have been reported. For me this fixes the increase in GetCurrentTimestamp() calls, there now are fewer calls in 15 than in 14, in the previously slowed down workload. While at it, also update assertion in pgstat_report_stat() to be more precise. Author: Andres Freund Discussion: https://postgr.es/m/20220616233130.rparivafipt6doj3@alap3.anarazel.de Backpatch: 15- --- src/backend/tcop/postgres.c | 48 +++++++++++++++++++---------- src/backend/utils/activity/pgstat.c | 2 +- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 495cbf2006..5ab91c2c58 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -3371,10 +3371,13 @@ ProcessInterrupts(void) IdleSessionTimeoutPending = false; } - if (IdleStatsUpdateTimeoutPending) + /* + * If there are pending stats updates and we currently are truly idle + * (matching the conditions in PostgresMain(), report stats now. + */ + if (IdleStatsUpdateTimeoutPending && + DoingCommandRead && !IsTransactionOrTransactionBlock()) { - /* timer should have been disarmed */ - Assert(!IsTransactionBlock()); IdleStatsUpdateTimeoutPending = false; pgstat_report_stat(true); } @@ -4050,7 +4053,6 @@ PostgresMain(const char *dbname, const char *username) volatile bool send_ready_for_query = true; bool idle_in_transaction_timeout_enabled = false; bool idle_session_timeout_enabled = false; - bool idle_stats_update_timeout_enabled = false; AssertArg(dbname != NULL); AssertArg(username != NULL); @@ -4427,13 +4429,31 @@ PostgresMain(const char *dbname, const char *username) if (notifyInterruptPending) ProcessNotifyInterrupt(false); - /* Start the idle-stats-update timer */ + /* + * Check if we need to report stats. If pgstat_report_stat() + * decides it's too soon to flush out pending stats / lock + * contention prevented reporting, it'll tell us when we + * should try to report stats again (so that stats updates + * aren't unduly delayed if the connection goes idle for a + * long time). We only enable the timeout if we don't already + * have a timeout in progress, because we don't disable the + * timeout below. enable_timeout_after() needs to determine + * the current timestamp, which can have a negative + * performance impact. That's OK because pgstat_report_stat() + * won't have us wake up sooner than a prior call. + */ stats_timeout = pgstat_report_stat(false); if (stats_timeout > 0) { - idle_stats_update_timeout_enabled = true; - enable_timeout_after(IDLE_STATS_UPDATE_TIMEOUT, - stats_timeout); + if (!get_timeout_active(IDLE_STATS_UPDATE_TIMEOUT)) + enable_timeout_after(IDLE_STATS_UPDATE_TIMEOUT, + stats_timeout); + } + else + { + /* all stats flushed, no need for the timeout */ + if (get_timeout_active(IDLE_STATS_UPDATE_TIMEOUT)) + disable_timeout(IDLE_STATS_UPDATE_TIMEOUT, false); } set_ps_display("idle"); @@ -4469,10 +4489,9 @@ PostgresMain(const char *dbname, const char *username) firstchar = ReadCommand(&input_message); /* - * (4) turn off the idle-in-transaction, idle-session and - * idle-stats-update timeouts if active. We do this before step (5) - * so that any last-moment timeout is certain to be detected in step - * (5). + * (4) turn off the idle-in-transaction and idle-session timeouts if + * active. We do this before step (5) so that any last-moment timeout + * is certain to be detected in step (5). * * At most one of these timeouts will be active, so there's no need to * worry about combining the timeout.c calls into one. @@ -4487,11 +4506,6 @@ PostgresMain(const char *dbname, const char *username) disable_timeout(IDLE_SESSION_TIMEOUT, false); idle_session_timeout_enabled = false; } - if (idle_stats_update_timeout_enabled) - { - disable_timeout(IDLE_STATS_UPDATE_TIMEOUT, false); - idle_stats_update_timeout_enabled = false; - } /* * (5) disable async signal conditions again. diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index 0d9d09c492..88e5dd1b2b 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -571,7 +571,7 @@ pgstat_report_stat(bool force) bool nowait; pgstat_assert_is_up(); - Assert(!IsTransactionBlock()); + Assert(!IsTransactionOrTransactionBlock()); /* "absorb" the forced flush even if there's nothing to flush */ if (pgStatForceNextFlush)