Improve management of statement timeouts.

Commit f8e5f156b 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. be42015fc), 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

1 files changed, 32 insertions, 17 deletions

diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
index b56259c6920..a2a4bb6f2d5 100644
--- 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,16 +617,27 @@ 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
  *
  * If reset_indicator is true, reset the indicator when returning true.