Re: [PATCH 06/12] space: space_vtab::build_secondary_key => build_index

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

On Tue, Apr 10, 2018 at 11:05:07AM +0300, Vladimir Davydov wrote:
> On Mon, Apr 09, 2018 at 11:39:25PM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [18/04/09 10:33]:
> > > -	space_build_secondary_key_xc(new_index_def->iid != 0 ?
> > > -				     alter->new_space : alter->old_space,
> > > -				     alter->new_space, new_index);
> > > +	space_build_index_xc(new_index_def->iid != 0 ?
> > > +			     alter->new_space : alter->old_space,
> > > +			     new_index, alter->new_space->format);
> > >  }
> > 
> > This is confusing, why do you ever need to pass the old space? 
> > A new index should always be built in the new space, no?
> 
> We always build an index *for* the new space, but the 'space' argument
> here denotes the space to use as the source for building the new index.
> If we are rebuilding the primary key, we have to pass the old space,
> because the new space is empty.
> 
> > 
> > >  	/**
> > > -	 * Called with the new empty secondary index.
> > > -	 * Fill the new index with data from the primary
> > > -	 * key of the space.
> > > +	 * Build a new index, primary or secondary, and fill it
> > > +	 * with tuples stored in the given space. The function is
> > > +	 * supposed to assure that all tuples conform to the new
> > > +	 * format.
> > >  	 */
> > > -	int (*build_secondary_key)(struct space *old_space,
> > > -				   struct space *new_space,
> > > -				   struct index *new_index);
> > > +	int (*build_index)(struct space *space, struct index *index,
> > > +			   struct tuple_format *format);
> > 
> > Not having parameter 'space' documented, and not having the
> > relationship between 'space' and 'format' documented (why can't we 
> > use space->format) doesn't provide more clarity either.
> 
> OK, I will update the comment.

Done. Please take a look.

(not on the branch, don't push)

From 6d89d2225c116eb86a5cd1735cb4e4537360edf1 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Date: Thu, 5 Apr 2018 13:59:16 +0300
Subject: [PATCH] space: space_vtab::build_secondary_key => build_index

The build_secondary_key method of space vtab is used not only for
building secondary indexes, but also for rebuilding primary indexes.
To avoid confusion, let's rename it to build_index and pass to it
the source space, the new index, and the new tuple format.

diff --git a/src/box/alter.cc b/src/box/alter.cc
index c4141145..11d367a3 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1206,8 +1206,8 @@ CreateIndex::alter(struct alter_space *alter)
 	struct index *new_index = space_index(alter->new_space,
 					      new_index_def->iid);
 	assert(new_index != NULL);
-	space_build_secondary_key_xc(alter->new_space,
-				     alter->new_space, new_index);
+	space_build_index_xc(alter->new_space, new_index,
+			     alter->new_space->format);
 }
 
 void
@@ -1279,9 +1279,9 @@ RebuildIndex::alter(struct alter_space *alter)
 	struct index *new_index = space_index(alter->new_space,
 					      new_index_def->iid);
 	assert(new_index != NULL);
-	space_build_secondary_key_xc(new_index_def->iid != 0 ?
-				     alter->new_space : alter->old_space,
-				     alter->new_space, new_index);
+	space_build_index_xc(new_index_def->iid != 0 ?
+			     alter->new_space : alter->old_space,
+			     new_index, alter->new_space->format);
 }
 
 void
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index b3e08e49..e68f7098 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -754,9 +754,8 @@ memtx_init_system_space(struct space *space)
 }
 
 static int
-memtx_space_build_secondary_key(struct space *old_space,
-				struct space *new_space,
-				struct index *new_index)
+memtx_space_build_index(struct space *src_space, struct index *new_index,
+			struct tuple_format *new_format)
 {
 	/**
 	 * If it's a secondary key, and we're not building them
@@ -764,17 +763,17 @@ memtx_space_build_secondary_key(struct space *old_space,
 	 */
 	if (new_index->def->iid != 0) {
 		struct memtx_space *memtx_space;
-		memtx_space = (struct memtx_space *)new_space;
+		memtx_space = (struct memtx_space *)src_space;
 		if (!(memtx_space->replace == memtx_space_replace_all_keys))
 			return 0;
 	}
-	struct index *pk = index_find(old_space, 0);
+	struct index *pk = index_find(src_space, 0);
 	if (pk == NULL)
 		return -1;
 
-	struct errinj *inj = errinj(ERRINJ_BUILD_SECONDARY, ERRINJ_INT);
+	struct errinj *inj = errinj(ERRINJ_BUILD_INDEX, ERRINJ_INT);
 	if (inj != NULL && inj->iparam == (int)new_index->def->iid) {
-		diag_set(ClientError, ER_INJECTION, "buildSecondaryKey");
+		diag_set(ClientError, ER_INJECTION, "build index");
 		return -1;
 	}
 
@@ -798,7 +797,7 @@ memtx_space_build_secondary_key(struct space *old_space,
 		 * Check that the tuple is OK according to the
 		 * new format.
 		 */
-		rc = tuple_validate(new_space->format, tuple);
+		rc = tuple_validate(new_format, tuple);
 		if (rc != 0)
 			break;
 		/*
@@ -851,7 +850,7 @@ static const struct space_vtab memtx_space_vtab = {
 	/* .add_primary_key = */ memtx_space_add_primary_key,
 	/* .drop_primary_key = */ memtx_space_drop_primary_key,
 	/* .check_format  = */ memtx_space_check_format,
-	/* .build_secondary_key = */ memtx_space_build_secondary_key,
+	/* .build_index = */ memtx_space_build_index,
 	/* .swap_index = */ generic_space_swap_index,
 	/* .prepare_alter = */ memtx_space_prepare_alter,
 };
diff --git a/src/box/space.h b/src/box/space.h
index 758d575e..804c7fb7 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -48,6 +48,7 @@ struct txn;
 struct request;
 struct port;
 struct tuple;
+struct tuple_format;
 
 struct space_vtab {
 	/** Free a space instance. */
@@ -96,13 +97,19 @@ struct space_vtab {
 	int (*check_format)(struct space *new_space,
 			    struct space *old_space);
 	/**
-	 * Called with the new empty secondary index.
-	 * Fill the new index with data from the primary
-	 * key of the space.
+	 * Build a new index, primary or secondary, and fill it
+	 * with tuples stored in the given space. The function is
+	 * supposed to assure that all tuples conform to the new
+	 * format.
+	 *
+	 * @param src_space   space to use as build source
+	 * @param new_index   index to build
+	 * @param new_format  format for validating tuples
+	 * @retval  0         success
+	 * @retval -1         build failed
 	 */
-	int (*build_secondary_key)(struct space *old_space,
-				   struct space *new_space,
-				   struct index *new_index);
+	int (*build_index)(struct space *src_space, struct index *new_index,
+			   struct tuple_format *new_format);
 	/**
 	 * Exchange two index objects in two spaces. Used
 	 * to update a space with a newly built index, while
@@ -329,12 +336,10 @@ space_drop_primary_key(struct space *space)
 }
 
 static inline int
-space_build_secondary_key(struct space *old_space,
-			  struct space *new_space, struct index *new_index)
+space_build_index(struct space *src_space, struct index *new_index,
+		  struct tuple_format *new_format)
 {
-	assert(old_space->vtab == new_space->vtab);
-	return new_space->vtab->build_secondary_key(old_space,
-						    new_space, new_index);
+	return src_space->vtab->build_index(src_space, new_index, new_format);
 }
 
 static inline void
@@ -480,10 +485,10 @@ space_check_format_xc(struct space *new_space, struct space *old_space)
 }
 
 static inline void
-space_build_secondary_key_xc(struct space *old_space,
-			     struct space *new_space, struct index *new_index)
+space_build_index_xc(struct space *src_space, struct index *new_index,
+		     struct tuple_format *new_format)
 {
-	if (space_build_secondary_key(old_space, new_space, new_index) != 0)
+	if (space_build_index(src_space, new_index, new_format) != 0)
 		diag_raise();
 }
 
diff --git a/src/box/sysview_engine.c b/src/box/sysview_engine.c
index d28430aa..47409d23 100644
--- a/src/box/sysview_engine.c
+++ b/src/box/sysview_engine.c
@@ -136,13 +136,12 @@ sysview_space_drop_primary_key(struct space *space)
 }
 
 static int
-sysview_space_build_secondary_key(struct space *old_space,
-				  struct space *new_space,
-				  struct index *new_index)
+sysview_space_build_index(struct space *src_space, struct index *new_index,
+			  struct tuple_format *new_format)
 {
-	(void)old_space;
-	(void)new_space;
+	(void)src_space;
 	(void)new_index;
+	(void)new_format;
 	return 0;
 }
 
@@ -177,7 +176,7 @@ static const struct space_vtab sysview_space_vtab = {
 	/* .add_primary_key = */ sysview_space_add_primary_key,
 	/* .drop_primary_key = */ sysview_space_drop_primary_key,
 	/* .check_format = */ sysview_space_check_format,
-	/* .build_secondary_key = */ sysview_space_build_secondary_key,
+	/* .build_index = */ sysview_space_build_index,
 	/* .swap_index = */ generic_space_swap_index,
 	/* .prepare_alter = */ sysview_space_prepare_alter,
 };
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index e809f6a8..abc30fe8 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1094,11 +1094,10 @@ vinyl_space_drop_primary_key(struct space *space)
 }
 
 static int
-vinyl_space_build_secondary_key(struct space *old_space,
-				struct space *new_space,
-				struct index *new_index)
+vinyl_space_build_index(struct space *src_space, struct index *new_index,
+			struct tuple_format *new_format)
 {
-	(void)old_space;
+	(void)new_format;
 	/*
 	 * Unlike Memtx, Vinyl does not need building of a secondary index.
 	 * This is true because of two things:
@@ -1113,17 +1112,12 @@ vinyl_space_build_secondary_key(struct space *old_space,
 	 * by itself during WAL recovery.
 	 * III. Vinyl is online. The space is definitely empty and there's
 	 * nothing to build.
-	 *
-	 * When we start to implement alter of non-empty vinyl spaces, it
-	 *  seems that we should call here:
-	 *   Engine::buildSecondaryKey(old_space, new_space, new_index_arg);
-	 *  but aware of three cases mentioned above.
 	 */
 	if (vinyl_index_open(new_index) != 0)
 		return -1;
 
 	/* Set pointer to the primary key for the new index. */
-	vy_lsm_update_pk(vy_lsm(new_index), vy_lsm(space_index(new_space, 0)));
+	vy_lsm_update_pk(vy_lsm(new_index), vy_lsm(space_index(src_space, 0)));
 	return 0;
 }
 
@@ -3931,7 +3925,7 @@ static const struct space_vtab vinyl_space_vtab = {
 	/* .add_primary_key = */ vinyl_space_add_primary_key,
 	/* .drop_primary_key = */ vinyl_space_drop_primary_key,
 	/* .check_format = */ vinyl_space_check_format,
-	/* .build_secondary_key = */ vinyl_space_build_secondary_key,
+	/* .build_index = */ vinyl_space_build_index,
 	/* .swap_index = */ vinyl_space_swap_index,
 	/* .prepare_alter = */ vinyl_space_prepare_alter,
 };
diff --git a/src/errinj.h b/src/errinj.h
index ab5988cd..2f9d2408 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -102,7 +102,7 @@ struct errinj {
 	_(ERRINJ_XLOG_READ, ERRINJ_INT, {.iparam = -1}) \
 	_(ERRINJ_VYRUN_INDEX_GARBAGE, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_VYRUN_DATA_READ, ERRINJ_BOOL, {.bparam = false}) \
-	_(ERRINJ_BUILD_SECONDARY, ERRINJ_INT, {.iparam = -1}) \
+	_(ERRINJ_BUILD_INDEX, ERRINJ_INT, {.iparam = -1}) \
 	_(ERRINJ_VY_POINT_ITER_WAIT, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_RELAY_EXIT_DELAY, ERRINJ_DOUBLE, {.dparam = 0}) \
 	_(ERRINJ_VY_DELAY_PK_LOOKUP, ERRINJ_BOOL, {.bparam = false}) \
diff --git a/test/box/errinj.result b/test/box/errinj.result
index 92ae7235..958f82dd 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -56,6 +56,8 @@ errinj.info()
     state: false
   ERRINJ_VY_RUN_WRITE:
     state: false
+  ERRINJ_BUILD_INDEX:
+    state: -1
   ERRINJ_RELAY_FINAL_SLEEP:
     state: false
   ERRINJ_VY_RUN_DISCARD:
@@ -64,30 +66,28 @@ errinj.info()
     state: false
   ERRINJ_LOG_ROTATE:
     state: false
-  ERRINJ_VY_POINT_ITER_WAIT:
-    state: false
   ERRINJ_RELAY_EXIT_DELAY:
     state: 0
   ERRINJ_IPROTO_TX_DELAY:
     state: false
-  ERRINJ_TUPLE_FIELD:
+  ERRINJ_VY_POINT_ITER_WAIT:
     state: false
-  ERRINJ_INDEX_ALLOC:
+  ERRINJ_TUPLE_FIELD:
     state: false
   ERRINJ_XLOG_GARBAGE:
     state: false
-  ERRINJ_VY_RUN_WRITE_TIMEOUT:
-    state: 0
+  ERRINJ_INDEX_ALLOC:
+    state: false
   ERRINJ_RELAY_TIMEOUT:
     state: 0
   ERRINJ_TESTING:
     state: false
-  ERRINJ_VY_LOG_FLUSH:
-    state: false
+  ERRINJ_VY_RUN_WRITE_TIMEOUT:
+    state: 0
   ERRINJ_VY_SQUASH_TIMEOUT:
     state: 0
-  ERRINJ_BUILD_SECONDARY:
-    state: -1
+  ERRINJ_VY_LOG_FLUSH:
+    state: false
   ERRINJ_VY_INDEX_DUMP:
     state: -1
 ...
@@ -949,13 +949,13 @@ i3:select{}
   - [2, 3, 1, 2, 10, 10]
   - [1, 4, 3, 4, 10, 10]
 ...
-box.error.injection.set('ERRINJ_BUILD_SECONDARY', i2.id)
+box.error.injection.set('ERRINJ_BUILD_INDEX', i2.id)
 ---
 - ok
 ...
 i1:alter{parts = {3, "unsigned"}}
 ---
-- error: Error injection 'buildSecondaryKey'
+- error: Error injection 'build index'
 ...
 _ = collectgarbage('collect')
 ---
@@ -981,13 +981,13 @@ i3:select{}
   - [2, 3, 1, 2, 10, 10]
   - [1, 4, 3, 4, 10, 10]
 ...
-box.error.injection.set('ERRINJ_BUILD_SECONDARY', i3.id)
+box.error.injection.set('ERRINJ_BUILD_INDEX', i3.id)
 ---
 - ok
 ...
 i1:alter{parts = {4, "unsigned"}}
 ---
-- error: Error injection 'buildSecondaryKey'
+- error: Error injection 'build index'
 ...
 _ = collectgarbage('collect')
 ---
@@ -1013,7 +1013,7 @@ i3:select{}
   - [2, 3, 1, 2, 10, 10]
   - [1, 4, 3, 4, 10, 10]
 ...
-box.error.injection.set('ERRINJ_BUILD_SECONDARY', -1)
+box.error.injection.set('ERRINJ_BUILD_INDEX', -1)
 ---
 - ok
 ...
@@ -1037,14 +1037,14 @@ s:replace{1, 1}
 ---
 - [1, 1]
 ...
-box.error.injection.set('ERRINJ_BUILD_SECONDARY', sk.id)
+box.error.injection.set('ERRINJ_BUILD_INDEX', sk.id)
 ---
 - ok
 ...
 sk:alter({parts = {2, 'number'}})
 ---
 ...
-box.error.injection.set('ERRINJ_BUILD_SECONDARY', -1)
+box.error.injection.set('ERRINJ_BUILD_INDEX', -1)
 ---
 - ok
 ...
diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
index 7dea2769..a3dde845 100644
--- a/test/box/errinj.test.lua
+++ b/test/box/errinj.test.lua
@@ -311,7 +311,7 @@ i1:select{}
 i2:select{}
 i3:select{}
 
-box.error.injection.set('ERRINJ_BUILD_SECONDARY', i2.id)
+box.error.injection.set('ERRINJ_BUILD_INDEX', i2.id)
 
 i1:alter{parts = {3, "unsigned"}}
 
@@ -320,7 +320,7 @@ i1:select{}
 i2:select{}
 i3:select{}
 
-box.error.injection.set('ERRINJ_BUILD_SECONDARY', i3.id)
+box.error.injection.set('ERRINJ_BUILD_INDEX', i3.id)
 
 i1:alter{parts = {4, "unsigned"}}
 
@@ -329,7 +329,7 @@ i1:select{}
 i2:select{}
 i3:select{}
 
-box.error.injection.set('ERRINJ_BUILD_SECONDARY', -1)
+box.error.injection.set('ERRINJ_BUILD_INDEX', -1)
 
 s:drop()
 
@@ -341,9 +341,9 @@ s = box.schema.space.create('test')
 pk = s:create_index('pk')
 sk = s:create_index('sk', {parts = {2, 'unsigned'}})
 s:replace{1, 1}
-box.error.injection.set('ERRINJ_BUILD_SECONDARY', sk.id)
+box.error.injection.set('ERRINJ_BUILD_INDEX', sk.id)
 sk:alter({parts = {2, 'number'}})
-box.error.injection.set('ERRINJ_BUILD_SECONDARY', -1)
+box.error.injection.set('ERRINJ_BUILD_INDEX', -1)
 s:drop()
 
 --