From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Fri, 5 Oct 2018 11:50:44 +0300 From: Vladimir Davydov Subject: Re: [PATCH 08/13] gc: keep track of available checkpoints Message-ID: <20181005085044.fxmcdakpyevqyfxf@esperanza> References: <20181004215932.GH22855@chai> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181004215932.GH22855@chai> To: Konstantin Osipov Cc: tarantool-patches@freelists.org List-ID: On Fri, Oct 05, 2018 at 12:59:32AM +0300, Konstantin Osipov wrote: > * Vladimir Davydov [18/10/05 00:11]: > > Currently, the checkpoint iterator is in fact a wrapper around > > memtx_engine::snap_dir while the garbage collector knows nothing about > > checkpoints. This feels like encapsulation violation. Let's keep track > > of all available checkpoints right in the garbage collector instead > > and export gc_ API to iterate over checkpoints. > > > > +void > > +gc_add_checkpoint(const struct vclock *vclock) > > +{ > > + struct gc_checkpoint *last_checkpoint = gc_last_checkpoint(); > > + if (last_checkpoint != NULL && > > + vclock_sum(&last_checkpoint->vclock) == vclock_sum(vclock)) { > > + /* > > + * No new checkpoint was actually created. > > + * Rerun the garbage collector to delete old > > + * files in case box.cfg.checkpoint_count > > + * was changed. > > + */ > > + gc_run(); > > You only compare with the last checkpoint, not all existing > checkpoints. When do you need this? When do you add an existing checkpoint > and know it could be a duplicate of the last one? Is it initial > recovery? Why do you need to deal with it this way? Please explain. There could be no changes (xlogs) after the last checkpoint, in which case box.snapshot() will still call engine methods and try to add a new checkpoint to gc. Currently, it's up to engines and gc to handle this case. Why can't we detect this in box.snapshot() and bail out without calling engine/gc methods? Because memtx needs to touch the snapshot in in this case, see commit c10874f4b ("gh-2045: Update snapshot timestamp if snapshot already exists"). Why? I guess this is needed for checkpoint daemon. We could introduce another engine callback for this though, something like engine_touch_checkpoint.