Date: Tue, 12 Mar 2019 17:53:43 -0700
From: Shawn Debnath <sdn@amazon.com>
To: Thomas Munro <thomas.munro@gmail.com>
CC: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Introduce timeout capability for ConditionVariableSleep
Message-ID: <20190313005342.GA8301@f01898859afd.ant.amazon.com>
References: <eeb06007ccfe46e399df6af18bfcd15a@EX13D05UWC002.ant.amazon.com>
 <CA+hUKGJDt3yopu1K1xPW5NtbLq-0n6fbXxZcaOpqD0P0-CruyA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="mxv5cy4qt+RJ9ypb"
Content-Disposition: inline
In-Reply-To: 
 <CA+hUKGJDt3yopu1K1xPW5NtbLq-0n6fbXxZcaOpqD0P0-CruyA@mail.gmail.com>
User-Agent: Mutt/1.11.3 (2019-02-01)
Precedence: Bulk
Precedence: bulk

--mxv5cy4qt+RJ9ypb
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline

Hi Thomas,

Thanks for reviewing!

On Wed, Mar 13, 2019 at 12:40:57PM +1300, Thomas Munro wrote:
> Can we just refer to the other function's documentation for this?  I
> don't want two copies of this blurb (and this copy-paste already
> failed to include "Timed" in the example function name).

Hah - point well taken. Fixed.

> One difference compared to pthread_cond_timedwait() is that pthread
> uses an absolute time and here you use a relative time (as we do in
> WaitEventSet API).  The first question is which makes a better API,
> and the second is what the semantics of a relative timeout should be:
> start again every time we get a spurious wake-up, or track time
> already waited?  Let's see...
> 
> -        (void) WaitEventSetWait(cv_wait_event_set, -1, &event, 1,
> +        ret = WaitEventSetWait(cv_wait_event_set, timeout, &event, 1,
>                                  wait_event_info);
> 
> Here you're restarting the timeout clock every time through the loop
> without adjustment, and I think that's not a good choice: spurious
> wake-ups cause bonus waiting.

Agree. In my testing WaitEventSetWait did the work for calculating the 
right timeout remaining. It's a bummer that we have to repeat the same 
pattern at the ConditionVariableTimedSleep() but I guess anyone who 
loops in such cases will have to adjust their values accordingly.

BTW, I am curious why Andres in 98a64d0bd71 didn't just create an 
artificial event with WL_TIMEOUT and return that from 
WaitEventSetWait(). Would have made it cleaner than checking checking 
return values for -1.

Updated v2 patch attached.

-- 
Shawn Debnath
Amazon Web Services (AWS)

--mxv5cy4qt+RJ9ypb
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: attachment;
	filename="0001-Introduce-timeout-capability-for-ConditionVariableSleep-v2.patch"

From 878462c4e05217a6c13107811a676948bb7f2a65 Mon Sep 17 00:00:00 2001
From: Shawn Debnath <sdn@amazon.com>
Date: Tue, 12 Mar 2019 23:21:41 +0000
Subject: [PATCH] Introduce timeout capability for ConditionVariableSleep

This patch introduces ConditionVariableTimedSleep to enable timing out
of waiting for a condition variable to get signalled.
---
 src/backend/storage/lmgr/condition_variable.c | 66 ++++++++++++++++++++++++++-
 src/include/storage/condition_variable.h      |  2 +
 2 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.c
index 58b7b51472..3afb726131 100644
--- a/src/backend/storage/lmgr/condition_variable.c
+++ b/src/backend/storage/lmgr/condition_variable.c
@@ -19,6 +19,7 @@
 #include "postgres.h"
 
 #include "miscadmin.h"
+#include "portability/instr_time.h"
 #include "storage/condition_variable.h"
 #include "storage/ipc.h"
 #include "storage/proc.h"
@@ -121,9 +122,31 @@ ConditionVariablePrepareToSleep(ConditionVariable *cv)
  */
 void
 ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
+{
+	(void) ConditionVariableTimedSleep(cv, -1 /* no timeout */, wait_event_info);
+}
+
+/*
+ * Wait for the given condition variable to be signaled or till timeout.
+ *
+ * In the event of a timeout, we simply return and the caller
+ * calls ConditionVariableCancelSleep to remove themselves from the
+ * wait queue. See ConditionVariableSleep() for notes on how to correctly check
+ * for the exit condition.
+ *
+ * Returns 0, or -1 if timed out.
+ */
+int
+ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,
+							uint32 wait_event_info)
 {
 	WaitEvent	event;
 	bool		done = false;
+	int			rc;
+	int			ret = 0;
+	long		rem_timeout = -1;
+	instr_time	start_time;
+	instr_time	cur_time;
 
 	/*
 	 * If the caller didn't prepare to sleep explicitly, then do so now and
@@ -143,7 +166,18 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
 	if (cv_sleep_target != cv)
 	{
 		ConditionVariablePrepareToSleep(cv);
-		return;
+		return ret;
+	}
+
+	/*
+	 * Track the current time so that we can calculate the remaining timeout
+	 * if we are woken up spuriously.
+	 */
+	if (timeout >= 0)
+	{
+		INSTR_TIME_SET_CURRENT(start_time);
+		Assert(timeout >= 0 && timeout <= INT_MAX);
+		rem_timeout = timeout;
 	}
 
 	do
@@ -154,12 +188,19 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
 		 * Wait for latch to be set.  (If we're awakened for some other
 		 * reason, the code below will cope anyway.)
 		 */
-		(void) WaitEventSetWait(cv_wait_event_set, -1, &event, 1,
+		rc = WaitEventSetWait(cv_wait_event_set, rem_timeout, &event, 1,
 								wait_event_info);
 
 		/* Reset latch before examining the state of the wait list. */
 		ResetLatch(MyLatch);
 
+		/* Timed out */
+		if (rc == 0 && timeout >= 0)
+		{
+			ret = -1;	/* timeout */
+			break;
+		}
+
 		/*
 		 * If this process has been taken out of the wait list, then we know
 		 * that it has been signaled by ConditionVariableSignal (or
@@ -179,10 +220,31 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
 		if (!proclist_contains(&cv->wakeup, MyProc->pgprocno, cvWaitLink))
 		{
 			done = true;
+			Assert(ret == 0);
 			proclist_push_tail(&cv->wakeup, MyProc->pgprocno, cvWaitLink);
 		}
 		SpinLockRelease(&cv->mutex);
+
+		/* If we're not done, update rem_timeout for next iteration */
+		if (!done && timeout >= 0)
+		{
+			INSTR_TIME_SET_CURRENT(cur_time);
+			INSTR_TIME_SUBTRACT(cur_time, start_time);
+			rem_timeout = timeout - (long) INSTR_TIME_GET_MILLISEC(cur_time);
+
+			/*
+			 * Check if we have reached the timeout threshold after calculating
+			 * the remaining timeout value.
+			 */
+			if (rem_timeout <= 0)
+			{
+				ret = -1;	/* timeout */
+				break;
+			}
+		}
 	} while (!done);
+
+	return ret;
 }
 
 /*
diff --git a/src/include/storage/condition_variable.h b/src/include/storage/condition_variable.h
index 2a0249392c..f62164e483 100644
--- a/src/include/storage/condition_variable.h
+++ b/src/include/storage/condition_variable.h
@@ -43,6 +43,8 @@ extern void ConditionVariableInit(ConditionVariable *cv);
  * the condition variable.
  */
 extern void ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info);
+extern int ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,
+										uint32 wait_event_info);
 extern void ConditionVariableCancelSleep(void);
 
 /*
-- 
2.16.5


--mxv5cy4qt+RJ9ypb--