Re: [PATCH 08/13] gc: keep track of available checkpoints

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Fri, Oct 05, 2018 at 12:59:32AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [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.