Be more predictable about reporting "lock timeout" vs "statement timeout".

If both timeout indicators are set when we arrive at ProcessInterrupts,
we've historically just reported "lock timeout".  However, some buildfarm
members have been observed to fail isolationtester's timeouts test by
reporting "lock timeout" when the statement timeout was expected to fire
first.  The cause seems to be that the process is allowed to sleep longer
than expected (probably due to heavy machine load) so that the lock
timeout happens before we reach the point of reporting the error, and
then this arbitrary tiebreak rule does the wrong thing.  We can improve
matters by comparing the scheduled timeout times to decide which error
to report.

I had originally proposed greatly reducing the 1-second window between
the two timeouts in the test cases.  On reflection that is a bad idea,
at least for the case where the lock timeout is expected to fire first,
because that would assume that it takes negligible time to get from
statement start to the beginning of the lock wait.  Thus, this patch
doesn't completely remove the risk of test failures on slow machines.
Empirically, however, the case this handles is the one we are seeing
in the buildfarm.  The explanation may be that the other case requires
the scheduler to take the CPU away from a busy process, whereas the
case fixed here only requires the scheduler to not give the CPU back
right away to a process that has been woken from a multi-second sleep
(and, perhaps, has been swapped out meanwhile).

Back-patch to 9.3 where the isolationtester timeouts test was added.

Discussion: <8693.1464314819@sss.pgh.pa.us>

1 files changed, 15 insertions, 1 deletions

diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
index 3b3f220e6e6..7171a7c59ce 100644
--- a/src/backend/utils/misc/timeout.c
+++ b/src/backend/utils/misc/timeout.c
@@ -34,7 +34,7 @@ typedef struct timeout_params
 	timeout_handler_proc timeout_handler;
 
 	TimestampTz start_time;		/* time that timeout was last activated */
-	TimestampTz fin_time;		/* if active, time it is due to fire */
+	TimestampTz fin_time;		/* time it is, or was last, due to fire */
 } timeout_params;
 
 /*
@@ -654,3 +654,17 @@ get_timeout_start_time(TimeoutId id)
 {
 	return all_timeouts[id].start_time;
 }
+
+/*
+ * Return the time when the timeout is, or most recently was, due to fire
+ *
+ * Note: will return 0 if timeout has never been activated in this process.
+ * However, we do *not* reset the fin_time when a timeout occurs, so as
+ * not to create a race condition if SIGALRM fires just as some code is
+ * about to fetch the value.
+ */
+TimestampTz
+get_timeout_finish_time(TimeoutId id)
+{
+	return all_timeouts[id].fin_time;
+}