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

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 1ecaba0d5749f36b2459ed4605a166bb7286052d..4bec40aa2879597059deebaada85394c7a14b63e 100644 (file)
--- 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
index b56259c69207a7772a32597530a4168cffd6d65a..a2a4bb6f2d5ceb881cf541e8eb015240dd1b3ba8 100644 (file)
--- 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
index 9244a2a7b75c7eb71a36fadf7f87a42ff203619e..ae5389ec96794173340838e59a6ef8a64c91a931 100644 (file)
--- 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);