Re: [Tarantool-patches] [PATCH v4 3/4] gc: rely on minimal vclock components instead of signatures

[Konstantin Osipov <kostja.osipov@gmail.com>]

* Serge Petrenko <sergepetrenko@tarantool.org> [20/03/27 18:08]:
> +	struct vclock min_vclock;
> +	struct gc_consumer *consumer = gc_tree_first(&gc.consumers);

The code would be easier to follow if the entire vclock api used
would be ignore0:

> +	/*
> +	 * Vclock of the oldest WAL row to keep is a by-component
> +	 * minimum of all consumer vclocks and the oldest
> +	 * checkpoint vclock. This ensures that all rows needed by
> +	 * at least one consumer are kept.
> +	 */
> +	vclock_copy(&min_vclock, &checkpoint->vclock);

E.g. use vclock_copy_ignore0 here

> +	while (consumer != NULL) {
> +		/*
> +		 * Consumers will never need rows signed
> +		 * with a zero instance id (local rows).
> +		 */
> +		vclock_min_ignore0(&min_vclock, &consumer->vclock);
> +		consumer = gc_tree_next(&gc.consumers, consumer);
> +	}
> +
> +	if (vclock_sum(&min_vclock) > vclock_sum(&gc.vclock)) {

Please use vclock_sum_ignore0
> +		vclock_copy(&gc.vclock, &min_vclock);

Please use vclock_copy_ignore0

The goal is to switch as many places as possible to ignore0 api,
then see the few cases left where we don't, and flip around: 
rename ignore0 api to the default naming scheme, and no-ignore0 to
vlock_copy_local(), vclock_compare_local() vclock_copy_local().

This doesn't have to be part of your patch set, but 
would be nice to get to.

Ideally ignore0 vclock should be a distinct data type
with an explicit conversion to and from non-ignore0 vclock
and no implicit assignment (using vclock_copy already more
or less ensures that).

Other than that you seem to be on track with the patch.

-- 
Konstantin Osipov, Moscow, Russia