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

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for the patch!

All the CI jobs are red. Please, try to fix it.

See 2 comments below.

> diff --git a/src/box/box.cc b/src/box/box.cc
> index cc59564e1..a269f7357 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1465,6 +1480,23 @@ box_set_wal_queue_max_size(void)
>  	return 0;
>  }
>  
> +int
> +box_set_wal_cleanup_delay(void)
> +{
> +	double delay = box_check_wal_cleanup_delay();
> +	if (delay < 0)
> +		return -1;
> +	/*
> +	 * Anonymous replicas do not require
> +	 * delay the cleanup procedure since they
> +	 * are read only.

1. It is not related to being read-only. Besides, they can write
to local spaces at least.

I think it is because anyway they can't be a source of replication.
There is no anybody to keep the files for.

> +	 */
> +	if (replication_anon)
> +		delay = 0;
> +	gc_set_wal_cleanup_delay(delay);
> +	return 0;
> +}
> diff --git a/src/box/gc.c b/src/box/gc.c
> index 9af4ef958..8dbcbcede 100644
> --- a/src/box/gc.c
> +++ b/src/box/gc.c> @@ -238,6 +243,41 @@ static int
>  gc_cleanup_fiber_f(va_list ap)
>  {
>  	(void)ap;
> +
> +	/*
> +	 * Stage 1 (optional): in case if we're booting
> +	 * up with cleanup disabled lets do wait in a
> +	 * separate cycle to minimize branching on stage 2.
> +	 */
> +	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.

> +
> +			if (fiber_clock() >= deadline ||
> +			    fiber_yield_timeout(timeout)) {
> +				say_info("wal/engine cleanup is resumed "
> +					 "due to timeout expiration");
> +				gc.is_paused = false;
> +				gc.delay_ref = 0;
> +				break;
> +			}
> +
> +			/*
> +			 * If a last reference is dropped
> +			 * we can exit out early.
> +			 */
> +			if (!gc.is_paused) {
> +				say_info("wal/engine cleanup is resumed");
> +				break;
> +			}
> +		}
> +	}