[PATCH 06/12] space: space_vtab::build_secondary_key => build_index
Vladimir Davydov
vdavydov.dev at gmail.com
Tue Apr 10 15:14:09 MSK 2018
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 at 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 at 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()
--
More information about the Tarantool-patches
mailing list