Re: [Tarantool-patches] [PATCH v4 1/3] gc/xlog: delay xlog cleanup until relays are subscribed

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Wed, Mar 24, 2021 at 11:10:11PM +0100, Vladislav Shpilevoy wrote:
> > +	if (gc.is_paused) {
> > +		double start_time = fiber_clock();
> > +		while (!fiber_is_cancelled()) {
> > +			double deadline = start_time + gc.wal_cleanup_delay;
> > +			double timeout = gc.wal_cleanup_delay;
> 
> 2. You didn't fix it really. Just that now the timeout is taken
> fresh from the config still making it possible to go beyond the
> deadline.
> 
> The fact that fiber_clock() < deadline does not mean that
> fiber_clock() + timeout is also < deadline. You need to calculate
> the timeout properly and try to cover it with a test.

Here is an update on top. I see a few red tests on our github actions
but still trying to figure out what is happening, since I've been
passing the tests on my local instance
---
diff --git a/src/box/box.cc b/src/box/box.cc
index a269f7357..ab298d223 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -774,10 +774,18 @@ box_check_wal_queue_max_size(void)
 static double
 box_check_wal_cleanup_delay(void)
 {
+	const double MAX_TIMEOUT = TIMEOUT_INFINITY;
+	const double MIN_TIMEOUT = 0.001;
+
 	double value = cfg_getd("wal_cleanup_delay");
-	if (value < 0) {
+	if (value < 0 || (value != 0 && value < MIN_TIMEOUT) ||
+	    value > MAX_TIMEOUT) {
+		char message[64];
+		snprintf(message, sizeof(message),
+			 "the value must be 0 or in range [%g; %g]",
+			 MIN_TIMEOUT, TIMEOUT_INFINITY);
 		diag_set(ClientError, ER_CFG, "wal_cleanup_delay",
-			 "the value must be >= 0");
+			 message);
 		return -1;
 	}
 
@@ -1488,8 +1496,8 @@ box_set_wal_cleanup_delay(void)
 		return -1;
 	/*
 	 * Anonymous replicas do not require
-	 * delay the cleanup procedure since they
-	 * are read only.
+	 * delay since they can't be a source
+	 * of replication.
 	 */
 	if (replication_anon)
 		delay = 0;
diff --git a/src/box/gc.c b/src/box/gc.c
index 8dbcbcede..10f899923 100644
--- a/src/box/gc.c
+++ b/src/box/gc.c
@@ -251,12 +251,9 @@ gc_cleanup_fiber_f(va_list ap)
 	 */
 	if (gc.is_paused) {
 		double start_time = fiber_clock();
+		double timeout = gc.wal_cleanup_delay;
 		while (!fiber_is_cancelled()) {
-			double deadline = start_time + gc.wal_cleanup_delay;
-			double timeout = gc.wal_cleanup_delay;
-
-			if (fiber_clock() >= deadline ||
-			    fiber_yield_timeout(timeout)) {
+			if (fiber_yield_timeout(timeout)) {
 				say_info("wal/engine cleanup is resumed "
 					 "due to timeout expiration");
 				gc.is_paused = false;
@@ -272,6 +269,19 @@ gc_cleanup_fiber_f(va_list ap)
 				say_info("wal/engine cleanup is resumed");
 				break;
 			}
+
+			/*
+			 * Woken up to update the timeout.
+			 */
+			double elapsed = fiber_clock() - start_time;
+			if (elapsed >= gc.wal_cleanup_delay) {
+				say_info("wal/engine cleanup is resumed "
+					 "due to timeout manual update");
+				gc.is_paused = false;
+				gc.delay_ref = 0;
+				break;
+			}
+			timeout = gc.wal_cleanup_delay - elapsed;
 		}
 	}