agora inbox for [email protected]  

help / color / mirror / Atom feed

From: Andres Freund <[email protected]>
Subject: [PATCH v2 2/4] WIP: pgstat: reduce timer overhead.
Date: Fri, 17 Jun 2022 12:48:34 -0700

While at it, also update assertion in pgstat_report_stat() to be more precise.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/backend/tcop/postgres.c         | 47 ++++++++++++++++++-----------
 src/backend/utils/activity/pgstat.c |  2 +-
 2 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8b6b5bbaaab..a3aa9063dc9 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);
 	}
@@ -4051,7 +4054,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);
@@ -4428,13 +4430,30 @@ 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 */
+					disable_timeout(IDLE_STATS_UPDATE_TIMEOUT, false);
 				}
 
 				set_ps_display("idle");
@@ -4470,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.
@@ -4488,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 ad5cf6fb915..826008be8b2 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -570,7 +570,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)
-- 
2.35.1.677.gabf474a5dd


--qvt45kjxs4ewr5po
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0003-WIP-expression-eval-fix-EEOP_HASHED_SCALARARRAYOP.patch"

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected]
  Subject: Re: [PATCH v2 2/4] WIP: pgstat: reduce timer overhead.
  In-Reply-To: <no-message-id-128077@localhost>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox