Improve management of statement timeouts.

tglsfdc · tglsfdc · commit 22f6f2c1ccb5 · 2019-10-25T11:41:16.000-04:00
Commit f8e5f15 added private state in postgres.c to track whether a statement timeout is running. This seems like bad design to me; timeout.c's private state should be the single source of truth about that. We already fixed one bug associated with failure to keep those states in sync (cf. be42015), and I've got little faith that we won't find more in future. So get rid of postgres.c's local variable by exposing a way to ask timeout.c whether a timeout is running. (Obviously, such an inquiry is subject to race conditions, but it seems fine for the purpose at hand.) To make get_timeout_active() as cheap as possible, add a flag in the per-timeout struct showing whether that timeout is active. This allows some small savings elsewhere in timeout.c, mainly elimination of unnecessary searches of the active_timeouts array. While at it, fix enable_statement_timeout to not call disable_timeout when statement_timeout is 0 and the timeout is not running. This avoids a useless deschedule-and-reschedule-timeouts cycle, which represents a significant savings (at least one kernel call) when there is any other active timeout. Right now, there usually isn't, but there are proposals around to change that. Discussion: https://postgr.es/m/16035-456e6e69ebfd4374@postgresql.org
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
@@ -145,11 +145,6 @@ static bool DoingCommandRead = false;
 static bool doing_extended_query_message = false;
 static bool ignore_till_sync = false;
 
-/*
- * Flag to keep track of whether statement timeout timer is active.
- */
-static bool stmt_timeout_active = false;
-
 /*
  * If an unnamed prepared statement exists, it's stored here.
  * We keep it separate from the hashtable kept by commands/prepare.c
@@ -4029,7 +4024,6 @@ PostgresMain(int argc, char *argv[],
 		 */
 		disable_all_timeouts(false);
 		QueryCancelPending = false; /* second to avoid race condition */
-		stmt_timeout_active = false;
 
 		/* Not reading from the client anymore. */
 		DoingCommandRead = false;
@@ -4711,14 +4705,14 @@ enable_statement_timeout(void)
 
 	if (StatementTimeout > 0)
 	{
-		if (!stmt_timeout_active)
-		{
+		if (!get_timeout_active(STATEMENT_TIMEOUT))
 			enable_timeout_after(STATEMENT_TIMEOUT, StatementTimeout);
-			stmt_timeout_active = true;
-		}
 	}
 	else
-		disable_timeout(STATEMENT_TIMEOUT, false);
+	{
+		if (get_timeout_active(STATEMENT_TIMEOUT))
+			disable_timeout(STATEMENT_TIMEOUT, false);
+	}
 }
 
 /*
@@ -4727,10 +4721,6 @@ enable_statement_timeout(void)
 static void
 disable_statement_timeout(void)
 {
-	if (stmt_timeout_active)
-	{
+	if (get_timeout_active(STATEMENT_TIMEOUT))
 		disable_timeout(STATEMENT_TIMEOUT, false);
-
-		stmt_timeout_active = false;
-	}
 }
diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
@@ -27,7 +27,8 @@ typedef struct timeout_params
 {
 	TimeoutId	index;			/* identifier of timeout reason */
 
-	/* volatile because it may be changed from the signal handler */
+	/* volatile because these may be changed from the signal handler */
+	volatile bool active;		/* true if timeout is in active_timeouts[] */
 	volatile bool indicator;	/* true if timeout has occurred */
 
 	/* callback function for timeout, or NULL if timeout not registered */
@@ -105,6 +106,9 @@ insert_timeout(TimeoutId id, int index)
 		elog(FATAL, "timeout index %d out of range 0..%d", index,
 			 num_active_timeouts);
 
+	Assert(!all_timeouts[id].active);
+	all_timeouts[id].active = true;
+
 	for (i = num_active_timeouts - 1; i >= index; i--)
 		active_timeouts[i + 1] = active_timeouts[i];
 
@@ -125,6 +129,9 @@ remove_timeout_index(int index)
 		elog(FATAL, "timeout index %d out of range 0..%d", index,
 			 num_active_timeouts - 1);
 
+	Assert(active_timeouts[index]->active);
+	active_timeouts[index]->active = false;
+
 	for (i = index + 1; i < num_active_timeouts; i++)
 		active_timeouts[i - 1] = active_timeouts[i];
 
@@ -147,9 +154,8 @@ enable_timeout(TimeoutId id, TimestampTz now, TimestampTz fin_time)
 	 * If this timeout was already active, momentarily disable it.  We
 	 * interpret the call as a directive to reschedule the timeout.
 	 */
-	i = find_active_timeout(id);
-	if (i >= 0)
-		remove_timeout_index(i);
+	if (all_timeouts[id].active)
+		remove_timeout_index(find_active_timeout(id));
 
 	/*
 	 * Find out the index where to insert the new timeout.  We sort by
@@ -349,6 +355,7 @@ InitializeTimeouts(void)
 	for (i = 0; i < MAX_TIMEOUTS; i++)
 	{
 		all_timeouts[i].index = i;
+		all_timeouts[i].active = false;
 		all_timeouts[i].indicator = false;
 		all_timeouts[i].timeout_handler = NULL;
 		all_timeouts[i].start_time = 0;
@@ -524,8 +531,6 @@ enable_timeouts(const EnableTimeoutParams *timeouts, int count)
 void
 disable_timeout(TimeoutId id, bool keep_indicator)
 {
-	int			i;
-
 	/* Assert request is sane */
 	Assert(all_timeouts_initialized);
 	Assert(all_timeouts[id].timeout_handler != NULL);
@@ -534,9 +539,8 @@ disable_timeout(TimeoutId id, bool keep_indicator)
 	disable_alarm();
 
 	/* Find the timeout and remove it from the active list. */
-	i = find_active_timeout(id);
-	if (i >= 0)
-		remove_timeout_index(i);
+	if (all_timeouts[id].active)
+		remove_timeout_index(find_active_timeout(id));
 
 	/* Mark it inactive, whether it was active or not. */
 	if (!keep_indicator)
@@ -571,13 +575,11 @@ disable_timeouts(const DisableTimeoutParams *timeouts, int count)
 	for (i = 0; i < count; i++)
 	{
 		TimeoutId	id = timeouts[i].id;
-		int			idx;
 
 		Assert(all_timeouts[id].timeout_handler != NULL);
 
-		idx = find_active_timeout(id);
-		if (idx >= 0)
-			remove_timeout_index(idx);
+		if (all_timeouts[id].active)
+			remove_timeout_index(find_active_timeout(id));
 
 		if (!timeouts[i].keep_indicator)
 			all_timeouts[id].indicator = false;
@@ -595,6 +597,8 @@ disable_timeouts(const DisableTimeoutParams *timeouts, int count)
 void
 disable_all_timeouts(bool keep_indicators)
 {
+	int			i;
+
 	disable_alarm();
 
 	/*
@@ -613,15 +617,26 @@ disable_all_timeouts(bool keep_indicators)
 
 	num_active_timeouts = 0;
 
-	if (!keep_indicators)
+	for (i = 0; i < MAX_TIMEOUTS; i++)
 	{
-		int			i;
-
-		for (i = 0; i < MAX_TIMEOUTS; i++)
+		all_timeouts[i].active = false;
+		if (!keep_indicators)
 			all_timeouts[i].indicator = false;
 	}
 }
 
+/*
+ * Return true if the timeout is active (enabled and not yet fired)
+ *
+ * This is, of course, subject to race conditions, as the timeout could fire
+ * immediately after we look.
+ */
+bool
+get_timeout_active(TimeoutId id)
+{
+	return all_timeouts[id].active;
+}
+
 /*
  * Return the timeout's I've-been-fired indicator
  *
diff --git a/src/include/utils/timeout.h b/src/include/utils/timeout.h
@@ -80,6 +80,7 @@ extern void disable_timeouts(const DisableTimeoutParams *timeouts, int count);
 extern void disable_all_timeouts(bool keep_indicators);
 
 /* accessors */
+extern bool get_timeout_active(TimeoutId id);
 extern bool get_timeout_indicator(TimeoutId id, bool reset_indicator);
 extern TimestampTz get_timeout_start_time(TimeoutId id);
 extern TimestampTz get_timeout_finish_time(TimeoutId id);
