From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f195.google.com (mail-lj1-f195.google.com [209.85.208.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 652F54696C3 for ; Sat, 28 Mar 2020 09:03:54 +0300 (MSK) Received: by mail-lj1-f195.google.com with SMTP id w1so12416740ljh.5 for ; Fri, 27 Mar 2020 23:03:54 -0700 (PDT) Date: Sat, 28 Mar 2020 09:03:52 +0300 From: Konstantin Osipov Message-ID: <20200328060352.GB23207@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v4 3/4] gc: rely on minimal vclock components instead of signatures List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Petrenko Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org * Serge Petrenko [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