[PATCH 0/2] Extend backup API to backup any checkpoint

[PATCH 0/2] Extend backup API to backup any checkpoint

[Vladimir Davydov]

Currently box.backup.start() returns files corresponding to the last
checkpoint. This patch extends it so that it is now possible to backup
any of stored checkpoints.

https://github.com/tarantool/tarantool/issues/3410
https://github.com/tarantool/tarantool/commits/gh-3410-allow-to-backup-any-checkpoint

Vladimir Davydov (2):
  engine: constify vclock argument
  box: allow to specify the checkpoint to backup

 src/box/box.cc           |  22 +++--
 src/box/box.h            |   9 +-
 src/box/engine.c         |   6 +-
 src/box/engine.h         |  16 ++--
 src/box/lua/init.c       |   8 +-
 src/box/memtx_engine.c   |  10 ++-
 src/box/sysview_engine.c |  10 ++-
 src/box/vinyl.c          |  10 ++-
 src/box/vy_log.c         |   2 +-
 src/box/vy_log.h         |   2 +-
 test/box/backup.result   | 230 ++++++++++++++++++++++++++++++++---------------
 test/box/backup.test.lua | 150 ++++++++++++++++++-------------
 12 files changed, 306 insertions(+), 169 deletions(-)

-- 
2.11.0

[PATCH 1/2] engine: constify vclock argument

[Vladimir Davydov]

None of engine_wait_checkpoint, engine_commit_checkpoint, engine_join,
engine_backup needs to modify the vclock argument.
---
 src/box/engine.c         |  6 +++---
 src/box/engine.h         | 16 ++++++++--------
 src/box/memtx_engine.c   | 10 ++++++----
 src/box/sysview_engine.c | 10 ++++++----
 src/box/vinyl.c          | 10 ++++++----
 src/box/vy_log.c         |  2 +-
 src/box/vy_log.h         |  2 +-
 7 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/src/box/engine.c b/src/box/engine.c
index b3e3fe1d..82293fd1 100644
--- a/src/box/engine.c
+++ b/src/box/engine.c
@@ -126,7 +126,7 @@ engine_begin_checkpoint(void)
 }
 
 int
-engine_commit_checkpoint(struct vclock *vclock)
+engine_commit_checkpoint(const struct vclock *vclock)
 {
 	struct engine *engine;
 	engine_foreach(engine) {
@@ -159,7 +159,7 @@ engine_collect_garbage(int64_t lsn)
 }
 
 int
-engine_backup(struct vclock *vclock, engine_backup_cb cb, void *cb_arg)
+engine_backup(const struct vclock *vclock, engine_backup_cb cb, void *cb_arg)
 {
 	struct engine *engine;
 	engine_foreach(engine) {
@@ -170,7 +170,7 @@ engine_backup(struct vclock *vclock, engine_backup_cb cb, void *cb_arg)
 }
 
 int
-engine_join(struct vclock *vclock, struct xstream *stream)
+engine_join(const struct vclock *vclock, struct xstream *stream)
 {
 	struct engine *engine;
 	engine_foreach(engine) {
diff --git a/src/box/engine.h b/src/box/engine.h
index 436bb967..825f059e 100644
--- a/src/box/engine.h
+++ b/src/box/engine.h
@@ -76,7 +76,7 @@ struct engine_vtab {
 	/**
 	 * Write statements stored in checkpoint @vclock to @stream.
 	 */
-	int (*join)(struct engine *engine, struct vclock *vclock,
+	int (*join)(struct engine *engine, const struct vclock *vclock,
 		    struct xstream *stream);
 	/**
 	 * Begin a new single or multi-statement transaction.
@@ -144,12 +144,12 @@ struct engine_vtab {
 	/**
 	 * Wait for a checkpoint to complete.
 	 */
-	int (*wait_checkpoint)(struct engine *, struct vclock *);
+	int (*wait_checkpoint)(struct engine *, const struct vclock *);
 	/**
 	 * All engines prepared their checkpoints,
 	 * fix up the changes.
 	 */
-	void (*commit_checkpoint)(struct engine *, struct vclock *);
+	void (*commit_checkpoint)(struct engine *, const struct vclock *);
 	/**
 	 * An error in one of the engines, abort checkpoint.
 	 */
@@ -172,7 +172,7 @@ struct engine_vtab {
 	 * that needs to be backed up in order to restore from the
 	 * checkpoint @vclock.
 	 */
-	int (*backup)(struct engine *engine, struct vclock *vclock,
+	int (*backup)(struct engine *engine, const struct vclock *vclock,
 		      engine_backup_cb cb, void *cb_arg);
 	/**
 	 * Accumulate engine memory statistics.
@@ -310,7 +310,7 @@ engine_end_recovery(void);
  * (called on the master).
  */
 int
-engine_join(struct vclock *vclock, struct xstream *stream);
+engine_join(const struct vclock *vclock, struct xstream *stream);
 
 int
 engine_begin_checkpoint(void);
@@ -319,7 +319,7 @@ engine_begin_checkpoint(void);
  * Create a checkpoint.
  */
 int
-engine_commit_checkpoint(struct vclock *vclock);
+engine_commit_checkpoint(const struct vclock *vclock);
 
 void
 engine_abort_checkpoint(void);
@@ -328,7 +328,7 @@ int
 engine_collect_garbage(int64_t lsn);
 
 int
-engine_backup(struct vclock *vclock, engine_backup_cb cb, void *cb_arg);
+engine_backup(const struct vclock *vclock, engine_backup_cb cb, void *cb_arg);
 
 void
 engine_memory_stat(struct engine_memory_stat *stat);
@@ -415,7 +415,7 @@ engine_end_recovery_xc(void)
 }
 
 static inline void
-engine_join_xc(struct vclock *vclock, struct xstream *stream)
+engine_join_xc(const struct vclock *vclock, struct xstream *stream)
 {
 	if (engine_join(vclock, stream) != 0)
 		diag_raise();
diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index 1d3d4943..0e5eb42a 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -677,7 +677,8 @@ memtx_engine_begin_checkpoint(struct engine *engine)
 }
 
 static int
-memtx_engine_wait_checkpoint(struct engine *engine, struct vclock *vclock)
+memtx_engine_wait_checkpoint(struct engine *engine,
+			     const struct vclock *vclock)
 {
 	struct memtx_engine *memtx = (struct memtx_engine *)engine;
 
@@ -708,7 +709,8 @@ memtx_engine_wait_checkpoint(struct engine *engine, struct vclock *vclock)
 }
 
 static void
-memtx_engine_commit_checkpoint(struct engine *engine, struct vclock *vclock)
+memtx_engine_commit_checkpoint(struct engine *engine,
+			       const struct vclock *vclock)
 {
 	(void) vclock;
 	struct memtx_engine *memtx = (struct memtx_engine *)engine;
@@ -793,7 +795,7 @@ memtx_engine_collect_garbage(struct engine *engine, int64_t lsn)
 }
 
 static int
-memtx_engine_backup(struct engine *engine, struct vclock *vclock,
+memtx_engine_backup(struct engine *engine, const struct vclock *vclock,
 		    engine_backup_cb cb, void *cb_arg)
 {
 	struct memtx_engine *memtx = (struct memtx_engine *)engine;
@@ -854,7 +856,7 @@ memtx_initial_join_f(va_list ap)
 }
 
 static int
-memtx_engine_join(struct engine *engine, struct vclock *vclock,
+memtx_engine_join(struct engine *engine, const struct vclock *vclock,
 		  struct xstream *stream)
 {
 	struct memtx_engine *memtx = (struct memtx_engine *)engine;
diff --git a/src/box/sysview_engine.c b/src/box/sysview_engine.c
index 5aea87e5..762bf93e 100644
--- a/src/box/sysview_engine.c
+++ b/src/box/sysview_engine.c
@@ -286,7 +286,7 @@ sysview_engine_end_recovery(struct engine *engine)
 }
 
 static int
-sysview_engine_join(struct engine *engine, struct vclock *vclock,
+sysview_engine_join(struct engine *engine, const struct vclock *vclock,
 		    struct xstream *stream)
 {
 	(void)engine;
@@ -303,7 +303,8 @@ sysview_engine_begin_checkpoint(struct engine *engine)
 }
 
 static int
-sysview_engine_wait_checkpoint(struct engine *engine, struct vclock *vclock)
+sysview_engine_wait_checkpoint(struct engine *engine,
+			       const struct vclock *vclock)
 {
 	(void)engine;
 	(void)vclock;
@@ -311,7 +312,8 @@ sysview_engine_wait_checkpoint(struct engine *engine, struct vclock *vclock)
 }
 
 static void
-sysview_engine_commit_checkpoint(struct engine *engine, struct vclock *vclock)
+sysview_engine_commit_checkpoint(struct engine *engine,
+				 const struct vclock *vclock)
 {
 	(void)engine;
 	(void)vclock;
@@ -332,7 +334,7 @@ sysview_engine_collect_garbage(struct engine *engine, int64_t lsn)
 }
 
 static int
-sysview_engine_backup(struct engine *engine, struct vclock *vclock,
+sysview_engine_backup(struct engine *engine, const struct vclock *vclock,
 		      engine_backup_cb cb, void *cb_arg)
 {
 	(void)engine;
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index f0d26874..fd0e9ca6 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -2809,7 +2809,8 @@ vinyl_engine_begin_checkpoint(struct engine *engine)
 }
 
 static int
-vinyl_engine_wait_checkpoint(struct engine *engine, struct vclock *vclock)
+vinyl_engine_wait_checkpoint(struct engine *engine,
+			     const struct vclock *vclock)
 {
 	struct vy_env *env = vy_env(engine);
 	assert(env->status == VINYL_ONLINE);
@@ -2821,7 +2822,8 @@ vinyl_engine_wait_checkpoint(struct engine *engine, struct vclock *vclock)
 }
 
 static void
-vinyl_engine_commit_checkpoint(struct engine *engine, struct vclock *vclock)
+vinyl_engine_commit_checkpoint(struct engine *engine,
+			       const struct vclock *vclock)
 {
 	(void)vclock;
 	struct vy_env *env = vy_env(engine);
@@ -3174,7 +3176,7 @@ vy_join_f(va_list ap)
 }
 
 static int
-vinyl_engine_join(struct engine *engine, struct vclock *vclock,
+vinyl_engine_join(struct engine *engine, const struct vclock *vclock,
 		  struct xstream *stream)
 {
 	struct vy_env *env = vy_env(engine);
@@ -3415,7 +3417,7 @@ vinyl_engine_collect_garbage(struct engine *engine, int64_t lsn)
 /* {{{ Backup */
 
 static int
-vinyl_engine_backup(struct engine *engine, struct vclock *vclock,
+vinyl_engine_backup(struct engine *engine, const struct vclock *vclock,
 		    engine_backup_cb cb, void *cb_arg)
 {
 	struct vy_env *env = vy_env(engine);
diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index bdf9eee3..ab74c9b0 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -1023,7 +1023,7 @@ vy_log_collect_garbage(int64_t signature)
 }
 
 const char *
-vy_log_backup_path(struct vclock *vclock)
+vy_log_backup_path(const struct vclock *vclock)
 {
 	/*
 	 * Use the previous log file, because the current one
diff --git a/src/box/vy_log.h b/src/box/vy_log.h
index d77cb298..3c1482be 100644
--- a/src/box/vy_log.h
+++ b/src/box/vy_log.h
@@ -405,7 +405,7 @@ vy_log_collect_garbage(int64_t signature);
  * in order to recover to checkpoint @vclock.
  */
 const char *
-vy_log_backup_path(struct vclock *vclock);
+vy_log_backup_path(const struct vclock *vclock);
 
 /** Allocate a unique ID for a vinyl object. */
 int64_t
-- 
2.11.0

[PATCH 2/2] box: allow to specify the checkpoint to backup

[Vladimir Davydov]

Even though we store last box.cfg.checkpoint_count checkpoints, we can
only restore to the last one, because the backup API only allows to
backup the last checkpoint. This patch adds an optional argument to
box.backup.start() which specifies the index of the checkpoint to
backup. If it is omitted or is 0, box.backup.start() will return files
corresponding to the last checkpoint. If it is 1, it will back the
previous checkpoint, and so on.

Closes #3410
---
 src/box/box.cc           |  22 +++--
 src/box/box.h            |   9 +-
 src/box/lua/init.c       |   8 +-
 test/box/backup.result   | 230 ++++++++++++++++++++++++++++++++---------------
 test/box/backup.test.lua | 150 ++++++++++++++++++-------------
 5 files changed, 275 insertions(+), 144 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index c728a4c5..011c9732 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2009,21 +2009,27 @@ end:
 }
 
 int
-box_backup_start(box_backup_cb cb, void *cb_arg)
+box_backup_start(int checkpoint_idx, box_backup_cb cb, void *cb_arg)
 {
+	assert(checkpoint_idx >= 0);
 	if (backup_gc != NULL) {
 		diag_set(ClientError, ER_BACKUP_IN_PROGRESS);
 		return -1;
 	}
-	struct vclock vclock;
-	if (checkpoint_last(&vclock) < 0) {
-		diag_set(ClientError, ER_MISSING_SNAPSHOT);
-		return -1;
-	}
-	backup_gc = gc_consumer_register("backup", vclock_sum(&vclock));
+	const struct vclock *vclock;
+	struct checkpoint_iterator it;
+	checkpoint_iterator_init(&it);
+	do {
+		vclock = checkpoint_iterator_prev(&it);
+		if (vclock == NULL) {
+			diag_set(ClientError, ER_MISSING_SNAPSHOT);
+			return -1;
+		}
+	} while (checkpoint_idx-- > 0);
+	backup_gc = gc_consumer_register("backup", vclock_sum(vclock));
 	if (backup_gc == NULL)
 		return -1;
-	int rc = engine_backup(&vclock, cb, cb_arg);
+	int rc = engine_backup(vclock, cb, cb_arg);
 	if (rc != 0) {
 		gc_consumer_unregister(backup_gc);
 		backup_gc = NULL;
diff --git a/src/box/box.h b/src/box/box.h
index d1a227e3..794f9893 100644
--- a/src/box/box.h
+++ b/src/box/box.h
@@ -122,12 +122,17 @@ typedef int (*box_backup_cb)(const char *path, void *arg);
 
 /**
  * Start a backup. This function calls @cb for each file that
- * needs to be backed up to recover from the last checkpoint.
+ * needs to be backed up to recover from the specified checkpoint.
+ *
+ * The checkpoint is given by @checkpoint_idx. If @checkpoint_idx
+ * is 0, the last checkpoint will be backed up; if it is 1, next
+ * to last, and so on.
+ *
  * The caller is supposed to call box_backup_stop() after he's
  * done copying the files.
  */
 int
-box_backup_start(box_backup_cb cb, void *cb_arg);
+box_backup_start(int checkpoint_idx, box_backup_cb cb, void *cb_arg);
 
 /**
  * Finish backup started with box_backup_start().
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index d4b5788f..af92abef 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -185,11 +185,17 @@ lbox_backup_cb(const char *path, void *cb_arg)
 static int
 lbox_backup_start(struct lua_State *L)
 {
+	int checkpoint_idx = 0;
+	if (lua_gettop(L) > 0) {
+		checkpoint_idx = luaL_checkint(L, 1);
+		if (checkpoint_idx < 0)
+			return luaL_error(L, "invalid checkpoint index");
+	}
 	lua_newtable(L);
 	struct lbox_backup_arg arg = {
 		.L = L,
 	};
-	if (box_backup_start(lbox_backup_cb, &arg) != 0)
+	if (box_backup_start(checkpoint_idx, lbox_backup_cb, &arg) != 0)
 		return luaT_error(L);
 	return 1;
 }
diff --git a/test/box/backup.result b/test/box/backup.result
index 4b97b6c0..f2286b6b 100644
--- a/test/box/backup.result
+++ b/test/box/backup.result
@@ -10,48 +10,79 @@ test_run = require('test_run').new()
 test_run:cleanup_cluster()
 ---
 ...
--- Make sure that garbage collection is disabled
--- while backup is in progress.
-default_checkpoint_count = box.cfg.checkpoint_count
+-- Basic argument check.
+box.backup.start('test')
 ---
+- error: 'bad argument #1 to ''?'' (number expected, got string)'
 ...
-box.cfg{checkpoint_count = 1}
+box.backup.start(-1)
+---
+- error: invalid checkpoint index
+...
+default_checkpoint_count = box.cfg.checkpoint_count
 ---
 ...
 ENGINES = {'memtx', 'vinyl'}
 ---
 ...
--- Directories where files can be stored,
--- from longest to shortest.
-CFG_DIRS = {box.cfg.wal_dir, box.cfg.memtx_dir, box.cfg.vinyl_dir}
+BACKUP_DIR = fio.pathjoin(fio.cwd(), 'backup')
 ---
 ...
-table.sort(CFG_DIRS, function(a, b) return #a > #b end)
+-- Make test spaces.
+for _, engine in ipairs(ENGINES) do box.schema.space.create(engine, {engine=engine}):create_index('pk') end
 ---
 ...
--- Create and populate tables. Make a snapshot to backup.
 _ = test_run:cmd("setopt delimiter ';'")
 ---
 ...
-for _, engine in ipairs(ENGINES) do
-    s = box.schema.space.create(engine, {engine=engine})
-    _ = s:create_index('pk')
-    for i=1,3 do s:insert{i, engine..i} end
-end
-box.snapshot()
+-- Function that updates content of test spaces.
+function do_update(files)
+    for _, engine in ipairs(ENGINES) do
+        local s = box.space[engine]
+        for i = 1, 3 do
+            s:upsert({i, engine .. i, 1}, {{'+', 3, 1}})
+        end
+    end
+end;
+---
+...
+-- Function that copies files returned by box.backup.start to BACKUP_DIR.
+function do_backup(files)
+    os.execute(string.format('rm -rf %s', BACKUP_DIR))
+    log.info(string.format('save backup to %s', BACKUP_DIR))
+    for _, path in ipairs(files) do
+        local dir
+        local suffix = string.gsub(path, '.*%.', '')
+        if suffix == 'xlog' then
+            dir = box.cfg.wal_dir
+        elseif suffix == 'snap' then
+            dir = box.cfg.memtx_dir
+        elseif suffix == 'vylog' or suffix == 'run' or suffix == 'index' then
+            dir = box.cfg.vinyl_dir
+        end
+        assert(dir ~= nil)
+        local rel_path = string.sub(path, string.len(dir) + 2)
+        local dest_dir = fio.pathjoin(BACKUP_DIR, fio.dirname(rel_path))
+        log.info(string.format('copy %s', rel_path))
+        os.execute(string.format('mkdir -p %s && cp %s %s', dest_dir, path, dest_dir))
+    end
+end;
+---
+...
 _ = test_run:cmd("setopt delimiter ''");
 ---
 ...
--- Add more data, but don't make a snapshot.
--- These data won't make it to the backup.
-_ = test_run:cmd("setopt delimiter ';'")
+-- Make a checkpoint to backup.
+do_update()
 ---
 ...
-for _, engine in ipairs(ENGINES) do
-    s = box.space[engine]
-    for i=1,3 do s:insert{i*10} end
-end
-_ = test_run:cmd("setopt delimiter ''");
+box.snapshot()
+---
+- ok
+...
+-- Add more data, but don't make a checkpoint.
+-- These data won't make it to the backup.
+do_update()
 ---
 ...
 -- Start backup.
@@ -62,95 +93,152 @@ box.backup.start() -- error: backup is already in progress
 ---
 - error: Backup is already in progress
 ...
--- Make sure new snapshots are not included into an ongoing backups.
-_ = test_run:cmd("setopt delimiter ';'")
----
-...
 -- Even though checkpoint_count is set to 1, this must not trigger
 -- garbage collection, because the checkpoint is pinned by backup.
-for _, engine in ipairs(ENGINES) do
-    s = box.space[engine]
-    for i=1,3 do s:insert{i*100} end
-end
+box.cfg{checkpoint_count = 1}
+---
+...
 box.snapshot()
-_ = test_run:cmd("setopt delimiter ''");
 ---
+- ok
 ...
--- Prepare backup directory
-backup_dir = fio.pathjoin(fio.cwd(), 'backup')
+-- Copy files.
+do_backup(files)
 ---
 ...
-_ = os.execute(string.format('rm -rf %s', backup_dir))
+box.backup.stop()
 ---
 ...
-log.info(string.format('save backup to %s', backup_dir))
+-- Previous checkpoint must have been removed by garbage collection
+-- as soon as box.backup.stop() was called.
+files = box.backup.start(1) -- error: checkpoint not found
 ---
+- error: Can't find snapshot
 ...
--- Copy files to the backup directory
-_ = test_run:cmd("setopt delimiter ';'")
+-- Check that we can restore from the backup we've just made.
+_ = test_run:cmd(string.format("create server copy1 with script='box/backup_test.lua', workdir='%s'", BACKUP_DIR))
 ---
 ...
-for _, path in ipairs(files) do
-    suffix = string.gsub(path, '.*%.', '')
-    if suffix == 'xlog' then
-        dir = box.cfg.wal_dir
-    elseif suffix == 'snap' then
-        dir = box.cfg.memtx_dir
-    elseif suffix == 'vylog' or suffix == 'run' or suffix == 'index' then
-        dir = box.cfg.vinyl_dir
-    end
-    assert(dir ~= nil)
-    rel_path = string.sub(path, string.len(dir) + 2)
-    dest_dir = fio.pathjoin(backup_dir, fio.dirname(rel_path))
-    log.info(string.format('copy %s', rel_path))
-    os.execute(string.format('mkdir -p %s && cp %s %s', dest_dir, path, dest_dir))
-end
-_ = test_run:cmd("setopt delimiter ''");
+_ = test_run:cmd("start server copy1")
+---
+...
+_ = test_run:cmd('switch copy1')
+---
+...
+box.space.memtx:select()
+---
+- - [1, 'memtx1', 1]
+  - [2, 'memtx2', 1]
+  - [3, 'memtx3', 1]
+...
+box.space.vinyl:select()
+---
+- - [1, 'vinyl1', 1]
+  - [2, 'vinyl2', 1]
+  - [3, 'vinyl3', 1]
+...
+_ = test_run:cmd('switch default')
+---
+...
+_ = test_run:cmd("stop server copy1")
+---
+...
+_ = test_run:cmd("cleanup server copy1")
+---
+...
+-- Make another checkpoint. Do not delete the previous one.
+box.cfg{checkpoint_count = 2}
+---
+...
+do_update()
+---
+...
+box.snapshot()
+---
+- ok
+...
+-- Backup and restore the previous checkpoint.
+files = box.backup.start(1)
+---
+...
+do_backup(files)
 ---
 ...
 box.backup.stop()
 ---
 ...
--- Check that we can restore from the backup.
-_ = test_run:cmd(string.format("create server copy with script='box/backup_test.lua', workdir='%s'", backup_dir))
+_ = test_run:cmd(string.format("create server copy2 with script='box/backup_test.lua', workdir='%s'", BACKUP_DIR))
 ---
 ...
-_ = test_run:cmd("start server copy")
+_ = test_run:cmd("start server copy2")
 ---
 ...
-_ = test_run:cmd('switch copy')
+_ = test_run:cmd('switch copy2')
 ---
 ...
-box.space['memtx']:select()
+box.space.memtx:select()
 ---
-- - [1, 'memtx1']
-  - [2, 'memtx2']
-  - [3, 'memtx3']
+- - [1, 'memtx1', 2]
+  - [2, 'memtx2', 2]
+  - [3, 'memtx3', 2]
 ...
-box.space['vinyl']:select()
+box.space.vinyl:select()
 ---
-- - [1, 'vinyl1']
-  - [2, 'vinyl2']
-  - [3, 'vinyl3']
+- - [1, 'vinyl1', 2]
+  - [2, 'vinyl2', 2]
+  - [3, 'vinyl3', 2]
 ...
 _ = test_run:cmd('switch default')
 ---
 ...
-_ = test_run:cmd("stop server copy")
+_ = test_run:cmd("stop server copy2")
+---
+...
+_ = test_run:cmd("cleanup server copy2")
 ---
 ...
-_ = test_run:cmd("cleanup server copy")
+-- Backup and restore the last checkpoint. Pass its index explicitly.
+files = box.backup.start(0)
 ---
 ...
--- Check that backup still works.
-_ = box.backup.start()
+do_backup(files)
 ---
 ...
 box.backup.stop()
 ---
 ...
+_ = test_run:cmd(string.format("create server copy3 with script='box/backup_test.lua', workdir='%s'", BACKUP_DIR))
+---
+...
+_ = test_run:cmd("start server copy3")
+---
+...
+_ = test_run:cmd('switch copy3')
+---
+...
+box.space.memtx:select()
+---
+- - [1, 'memtx1', 3]
+  - [2, 'memtx2', 3]
+  - [3, 'memtx3', 3]
+...
+box.space.vinyl:select()
+---
+- - [1, 'vinyl1', 3]
+  - [2, 'vinyl2', 3]
+  - [3, 'vinyl3', 3]
+...
+_ = test_run:cmd('switch default')
+---
+...
+_ = test_run:cmd("stop server copy3")
+---
+...
+_ = test_run:cmd("cleanup server copy3")
+---
+...
 -- Cleanup.
-_ =  os.execute(string.format('rm -rf %s', backup_dir))
+_ =  os.execute(string.format('rm -rf %s', BACKUP_DIR))
 ---
 ...
 for _, engine in ipairs(ENGINES) do box.space[engine]:drop() end
diff --git a/test/box/backup.test.lua b/test/box/backup.test.lua
index fe02f5a1..46703d54 100644
--- a/test/box/backup.test.lua
+++ b/test/box/backup.test.lua
@@ -4,95 +4,121 @@ test_run = require('test_run').new()
 
 test_run:cleanup_cluster()
 
--- Make sure that garbage collection is disabled
--- while backup is in progress.
+-- Basic argument check.
+box.backup.start('test')
+box.backup.start(-1)
+
 default_checkpoint_count = box.cfg.checkpoint_count
-box.cfg{checkpoint_count = 1}
 
 ENGINES = {'memtx', 'vinyl'}
+BACKUP_DIR = fio.pathjoin(fio.cwd(), 'backup')
 
--- Directories where files can be stored,
--- from longest to shortest.
-CFG_DIRS = {box.cfg.wal_dir, box.cfg.memtx_dir, box.cfg.vinyl_dir}
-table.sort(CFG_DIRS, function(a, b) return #a > #b end)
+-- Make test spaces.
+for _, engine in ipairs(ENGINES) do box.schema.space.create(engine, {engine=engine}):create_index('pk') end
 
--- Create and populate tables. Make a snapshot to backup.
 _ = test_run:cmd("setopt delimiter ';'")
-for _, engine in ipairs(ENGINES) do
-    s = box.schema.space.create(engine, {engine=engine})
-    _ = s:create_index('pk')
-    for i=1,3 do s:insert{i, engine..i} end
-end
-box.snapshot()
+-- Function that updates content of test spaces.
+function do_update(files)
+    for _, engine in ipairs(ENGINES) do
+        local s = box.space[engine]
+        for i = 1, 3 do
+            s:upsert({i, engine .. i, 1}, {{'+', 3, 1}})
+        end
+    end
+end;
+-- Function that copies files returned by box.backup.start to BACKUP_DIR.
+function do_backup(files)
+    os.execute(string.format('rm -rf %s', BACKUP_DIR))
+    log.info(string.format('save backup to %s', BACKUP_DIR))
+    for _, path in ipairs(files) do
+        local dir
+        local suffix = string.gsub(path, '.*%.', '')
+        if suffix == 'xlog' then
+            dir = box.cfg.wal_dir
+        elseif suffix == 'snap' then
+            dir = box.cfg.memtx_dir
+        elseif suffix == 'vylog' or suffix == 'run' or suffix == 'index' then
+            dir = box.cfg.vinyl_dir
+        end
+        assert(dir ~= nil)
+        local rel_path = string.sub(path, string.len(dir) + 2)
+        local dest_dir = fio.pathjoin(BACKUP_DIR, fio.dirname(rel_path))
+        log.info(string.format('copy %s', rel_path))
+        os.execute(string.format('mkdir -p %s && cp %s %s', dest_dir, path, dest_dir))
+    end
+end;
 _ = test_run:cmd("setopt delimiter ''");
 
--- Add more data, but don't make a snapshot.
+-- Make a checkpoint to backup.
+do_update()
+box.snapshot()
+
+-- Add more data, but don't make a checkpoint.
 -- These data won't make it to the backup.
-_ = test_run:cmd("setopt delimiter ';'")
-for _, engine in ipairs(ENGINES) do
-    s = box.space[engine]
-    for i=1,3 do s:insert{i*10} end
-end
-_ = test_run:cmd("setopt delimiter ''");
+do_update()
 
 -- Start backup.
 files = box.backup.start()
-
 box.backup.start() -- error: backup is already in progress
 
--- Make sure new snapshots are not included into an ongoing backups.
-_ = test_run:cmd("setopt delimiter ';'")
-for _, engine in ipairs(ENGINES) do
-    s = box.space[engine]
-    for i=1,3 do s:insert{i*100} end
-end
 -- Even though checkpoint_count is set to 1, this must not trigger
 -- garbage collection, because the checkpoint is pinned by backup.
+box.cfg{checkpoint_count = 1}
 box.snapshot()
-_ = test_run:cmd("setopt delimiter ''");
 
--- Prepare backup directory
-backup_dir = fio.pathjoin(fio.cwd(), 'backup')
-_ = os.execute(string.format('rm -rf %s', backup_dir))
-log.info(string.format('save backup to %s', backup_dir))
+-- Copy files.
+do_backup(files)
+box.backup.stop()
 
--- Copy files to the backup directory
-_ = test_run:cmd("setopt delimiter ';'")
-for _, path in ipairs(files) do
-    suffix = string.gsub(path, '.*%.', '')
-    if suffix == 'xlog' then
-        dir = box.cfg.wal_dir
-    elseif suffix == 'snap' then
-        dir = box.cfg.memtx_dir
-    elseif suffix == 'vylog' or suffix == 'run' or suffix == 'index' then
-        dir = box.cfg.vinyl_dir
-    end
-    assert(dir ~= nil)
-    rel_path = string.sub(path, string.len(dir) + 2)
-    dest_dir = fio.pathjoin(backup_dir, fio.dirname(rel_path))
-    log.info(string.format('copy %s', rel_path))
-    os.execute(string.format('mkdir -p %s && cp %s %s', dest_dir, path, dest_dir))
-end
-_ = test_run:cmd("setopt delimiter ''");
+-- Previous checkpoint must have been removed by garbage collection
+-- as soon as box.backup.stop() was called.
+files = box.backup.start(1) -- error: checkpoint not found
 
+-- Check that we can restore from the backup we've just made.
+_ = test_run:cmd(string.format("create server copy1 with script='box/backup_test.lua', workdir='%s'", BACKUP_DIR))
+_ = test_run:cmd("start server copy1")
+_ = test_run:cmd('switch copy1')
+box.space.memtx:select()
+box.space.vinyl:select()
+_ = test_run:cmd('switch default')
+_ = test_run:cmd("stop server copy1")
+_ = test_run:cmd("cleanup server copy1")
+
+-- Make another checkpoint. Do not delete the previous one.
+box.cfg{checkpoint_count = 2}
+do_update()
+box.snapshot()
+
+-- Backup and restore the previous checkpoint.
+files = box.backup.start(1)
+do_backup(files)
 box.backup.stop()
 
--- Check that we can restore from the backup.
-_ = test_run:cmd(string.format("create server copy with script='box/backup_test.lua', workdir='%s'", backup_dir))
-_ = test_run:cmd("start server copy")
-_ = test_run:cmd('switch copy')
-box.space['memtx']:select()
-box.space['vinyl']:select()
+_ = test_run:cmd(string.format("create server copy2 with script='box/backup_test.lua', workdir='%s'", BACKUP_DIR))
+_ = test_run:cmd("start server copy2")
+_ = test_run:cmd('switch copy2')
+box.space.memtx:select()
+box.space.vinyl:select()
 _ = test_run:cmd('switch default')
-_ = test_run:cmd("stop server copy")
-_ = test_run:cmd("cleanup server copy")
+_ = test_run:cmd("stop server copy2")
+_ = test_run:cmd("cleanup server copy2")
 
--- Check that backup still works.
-_ = box.backup.start()
+-- Backup and restore the last checkpoint. Pass its index explicitly.
+files = box.backup.start(0)
+do_backup(files)
 box.backup.stop()
 
+_ = test_run:cmd(string.format("create server copy3 with script='box/backup_test.lua', workdir='%s'", BACKUP_DIR))
+_ = test_run:cmd("start server copy3")
+_ = test_run:cmd('switch copy3')
+box.space.memtx:select()
+box.space.vinyl:select()
+_ = test_run:cmd('switch default')
+_ = test_run:cmd("stop server copy3")
+_ = test_run:cmd("cleanup server copy3")
+
 -- Cleanup.
-_ =  os.execute(string.format('rm -rf %s', backup_dir))
+_ =  os.execute(string.format('rm -rf %s', BACKUP_DIR))
 for _, engine in ipairs(ENGINES) do box.space[engine]:drop() end
 
 box.cfg{checkpoint_count = default_checkpoint_count}
-- 
2.11.0

Re: [PATCH 1/2] engine: constify vclock argument

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [18/05/30 11:45]:
> None of engine_wait_checkpoint, engine_commit_checkpoint, engine_join,
> engine_backup needs to modify the vclock argument.

Pushed.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [PATCH 2/2] box: allow to specify the checkpoint to backup

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [18/05/30 11:45]:
> Even though we store last box.cfg.checkpoint_count checkpoints, we can
> only restore to the last one, because the backup API only allows to
> backup the last checkpoint. This patch adds an optional argument to
> box.backup.start() which specifies the index of the checkpoint to
> backup. If it is omitted or is 0, box.backup.start() will return files
> corresponding to the last checkpoint. If it is 1, it will back the
> previous checkpoint, and so on.
> 
> Closes #3410

Please open a documentation request.

The test case fails on my system, most likely once again because I
use symlinks.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [PATCH 2/2] box: allow to specify the checkpoint to backup

[Vladimir Davydov]

On Wed, May 30, 2018 at 10:08:05PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [18/05/30 11:45]:
> > Even though we store last box.cfg.checkpoint_count checkpoints, we can
> > only restore to the last one, because the backup API only allows to
> > backup the last checkpoint. This patch adds an optional argument to
> > box.backup.start() which specifies the index of the checkpoint to
> > backup. If it is omitted or is 0, box.backup.start() will return files
> > corresponding to the last checkpoint. If it is 1, it will back the
> > previous checkpoint, and so on.
> > 
> > Closes #3410
> 
> Please open a documentation request.
> 
> The test case fails on my system, most likely once again because I
> use symlinks.

Looks like this is actually a bug in garbage collection, which
was introduced by my commit 35db70fac55c ("vinyl: remove runs not
referenced by any checkpoint immediately"). I opened a ticket:

https://github.com/tarantool/tarantool/issues/3437

It shouldn't be difficult to fix. I guess we should assign it to 1.9.