Date: Thu, 14 Mar 2019 17:26:11 -0700
From: Shawn Debnath <sdn@amazon.com>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
CC: <thomas.munro@gmail.com>, <pgsql-hackers@postgresql.org>
Subject: Re: Introduce timeout capability for ConditionVariableSleep
Message-ID: <20190315002611.GA1119@f01898859afd.ant.amazon.com>
References: <eeb06007ccfe46e399df6af18bfcd15a@EX13D05UWC002.ant.amazon.com>
 <CA+hUKGJDt3yopu1K1xPW5NtbLq-0n6fbXxZcaOpqD0P0-CruyA@mail.gmail.com>
 <20190313005342.GA8301@f01898859afd.ant.amazon.com>
 <20190313.172415.156658364.horiguchi.kyotaro@lab.ntt.co.jp>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="C7zPtVaVf+AK4Oqc"
Content-Disposition: inline
In-Reply-To: <20190313.172415.156658364.horiguchi.kyotaro@lab.ntt.co.jp>
User-Agent: Mutt/1.11.3 (2019-02-01)
Precedence: Bulk
Precedence: bulk

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

Thank you reviewing. Comments inline.

On Wed, Mar 13, 2019 at 05:24:15PM +0900, Kyotaro HORIGUCHI wrote:
> > 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.
> 
> I think so, too. And actually the duplicate timeout calculation
> doesn't seem good. We could eliminate the duplicate by allowing
> WaitEventSetWait to exit by unwanted events, something like the
> attached.

After thinking about this more, I see WaitEventSetWait()'s contract as 
wait for an event or timeout if no events are received in that time 
frame. Although ConditionVariableTimedSleep() is also using the same 
word, I believe the semantics are different. The timeout period in 
ConditionVariableTimedSleep() is intended to limit the time we wait 
until removal from the wait queue. Whereas, in the case of 
WaitEventSetWait, the timeout period is intended to limit the time the 
caller waits till the first event.

Hence, I believe the code is correct as is and we shouldn't change the 
contract for WaitEventSetWait.

> > 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.
> 
> Maybe because it is not a kind of WaitEvent, so it naturally
> cannot be a part of occurred_events.

Hmm, I don't agree with that completely. One could argue that the 
backend is waiting for any event in order to be woken up, including a 
timeout event.

> # By the way, you can obtain a short hash of a commit by git
> #  rev-parse --short <full hash>.

Good to know! :-) Luckily git is smart enough to still match it to the 
correct commit.

> > Updated v2 patch attached.
> 
> I'd like to comment on the patch.
> 
> + * 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.
> 
> Maybe this could be more simpler, that like:
> 
> * ConditionVariableTimedSleep - allows us to specify timeout
> * 
> * If timeout = =1, block until the condition is satisfied.
> * 
> * Returns -1 when timeout expires, otherwise returns 0.
> * 
> * See ConditionVariableSleep() for general behavior and usage.

Agree. Changed to:

  * Wait for the given condition variable to be signaled or till timeout.
  *
  * Returns -1 when timeout expires, otherwise returns 0.
  *
  * See ConditionVariableSleep() for general usage.

> +ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,
> 
> Counldn't the two-state return value be a boolean?

I wanted to leave the option open to use the positive integers for other 
purposes but you are correct, bool suffices for now. If needed, we can 
change it in the future.

> +	int			ret = 0;
> 
> As a general coding convention, we are not to give such a generic
> name for a variable with such a long life if avoidable. In the
> case the 'ret' could be 'timeout_fired' or something and it would
> be far verbose.
> 
> 
> +		if (rc == 0 && timeout >= 0)
> 
> WaitEventSetWait returns 0 only in the case of timeout
> expiration, so the second term is useless.  Just setting ret to
> -1 and break seems to me almost the same with "goto".  The reason
> why the existing ConditionVariableSleep uses do {} while(done) is
> that it is straightforwad. Timeout added everal exit point in the
> loop so it's make the loop rather complex by going around with
> the variable. Whole the loop could be in the following more flat
> shape.
> 
>    while (true)
>    {
>       CHECK_FOR_INTERRUPTS();
>       rc = WaitEventSetWait();
>       ResetLatch();
> 
>       /* timeout expired, return */
>       if (rc == 0) return -1;
>       SpinLockAcquire();
>       if (!proclist...)
>       {
>          done = true;
>       }
>       SpinLockRelease();
> 
>       /* condition satisfied, return */
>       if (done) return 0;
> 
>       /* if we're here, we should wait for the remaining time */
>       INSTR_TIME_SET_CURRENT()
>       ...
>   }

Agree. The timeout did complicate the logic for a single variable to
track the exit condition. Adopted the approach above.

> +			Assert(ret == 0);
> 
> I don't see a point in the assertion so much.

Being overly verbose. Removed.

-- 
Shawn Debnath
Amazon Web Services (AWS)

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

From cca6894d4b070ffca2922e333cdc9bbf03c0d3c2 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 | 63 ++++++++++++++++++++++++---
 src/include/storage/condition_variable.h      |  2 +
 2 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.c
index 58b7b51472..57a8098d90 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"
@@ -122,8 +123,23 @@ ConditionVariablePrepareToSleep(ConditionVariable *cv)
 void
 ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
 {
-	WaitEvent	event;
-	bool		done = false;
+	(void) ConditionVariableTimedSleep(cv, -1 /* no timeout */, wait_event_info);
+}
+
+/*
+ * Wait for the given condition variable to be signaled or till timeout.
+ *
+ * Returns true when timeout expires, otherwise returns false.
+ *
+ * See ConditionVariableSleep() for general usage.
+ */
+bool
+ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,
+							uint32 wait_event_info)
+{
+	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,23 +159,42 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
 	if (cv_sleep_target != cv)
 	{
 		ConditionVariablePrepareToSleep(cv);
-		return;
+		return false;
 	}
 
-	do
+	/*
+	 * 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;
+	}
+
+	while (true)
+	{
+		WaitEvent	event;
+		bool		done = false;
+		int 		rc;
+
 		CHECK_FOR_INTERRUPTS();
 
 		/*
 		 * 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)
+			return true;
+
 		/*
 		 * If this process has been taken out of the wait list, then we know
 		 * that it has been signaled by ConditionVariableSignal (or
@@ -182,7 +217,23 @@ ConditionVariableSleep(ConditionVariable *cv, uint32 wait_event_info)
 			proclist_push_tail(&cv->wakeup, MyProc->pgprocno, cvWaitLink);
 		}
 		SpinLockRelease(&cv->mutex);
-	} while (!done);
+
+		/* We were signaled, so return */
+		if (done)
+			return false;
+
+		/* If we're not done, update rem_timeout for next iteration */
+		if (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);
+
+			/* Have we crossed the timeout threshold? */
+			if (rem_timeout <= 0)
+				return true;
+		}
+	}
 }
 
 /*
diff --git a/src/include/storage/condition_variable.h b/src/include/storage/condition_variable.h
index 2a0249392c..ee06e051ce 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 bool ConditionVariableTimedSleep(ConditionVariable *cv, long timeout,
+										uint32 wait_event_info);
 extern void ConditionVariableCancelSleep(void);
 
 /*
-- 
2.16.5


--C7zPtVaVf+AK4Oqc--