Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: kostja@tarantool.org
Cc: tarantool-patches@freelists.org
Subject: [PATCH 10/12] vinyl: remove superfluous ddl checks
Date: Sat,  7 Apr 2018 16:38:07 +0300	[thread overview]
Message-ID: <a1d42a76d6e9a79fcd77f548904c75bf86478217.1523105106.git.vdavydov.dev@gmail.com> (raw)
In-Reply-To: <cover.1523105106.git.vdavydov.dev@gmail.com>
In-Reply-To: <cover.1523105106.git.vdavydov.dev@gmail.com>

There's no need to iterate over all indexes in vinyl_space_prepare_alter
and check if any of them is going to be rebuilt - alter_space_do already
does that for us. All we need to do is raise an error if the build_index
callback is invoked for a non-empty space after recovery is complete.
The tuple format check is pointless too, as it is already done by
CheckSpaceFormat AlterSpaceOp. Remove them.
---
 src/box/vinyl.c       | 73 ++++++++++++++++-----------------------------------
 test/box/alter.result |  4 +--
 test/vinyl/ddl.result | 26 +++++++++---------
 test/vinyl/gh.result  |  2 +-
 4 files changed, 38 insertions(+), 67 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 0c475af4..47804ecc 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -976,58 +976,12 @@ vinyl_init_system_space(struct space *space)
 static int
 vinyl_space_prepare_alter(struct space *old_space, struct space *new_space)
 {
+	(void)new_space;
 	struct vy_env *env = vy_env(old_space->engine);
 
 	if (vinyl_check_wal(env, "DDL") != 0)
 		return -1;
-	/*
-	 * The space with no indexes can contain no rows.
-	 * Allow alter.
-	 */
-	if (old_space->index_count == 0)
-		return 0;
-	struct vy_lsm *pk = vy_lsm(old_space->index[0]);
-	/*
-	 * During WAL recovery, the space may be not empty. But we
-	 * open existing indexes, not creating new ones. Allow
-	 * alter.
-	 */
-	if (env->status != VINYL_ONLINE)
-		return 0;
-	/* The space is empty. Allow alter. */
-	if (pk->stat.disk.count.rows == 0 &&
-	    pk->stat.memory.count.rows == 0)
-		return 0;
-	if (old_space->index_count == new_space->index_count) {
-		/* Check index_defs to be unchanged. */
-		for (uint32_t i = 0; i < old_space->index_count; ++i) {
-			struct index_def *old_def, *new_def;
-			old_def = space_index_def(old_space, i);
-			new_def = space_index_def(new_space, i);
-			/*
-			 * We do not support a full rebuild in
-			 * vinyl yet.
-			 */
-			if (index_def_change_requires_rebuild(old_def,
-							      new_def)) {
-				diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
-					 "changing the definition of "
-					 "a non-empty index");
-				return -1;
-			}
-		}
-	}
-	if (old_space->index_count < new_space->index_count) {
-		diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
-			 "adding an index to a non-empty space");
-		return -1;
-	}
-	if (! tuple_format1_can_store_format2_tuples(new_space->format,
-						     old_space->format)) {
-		diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
-			 "changing space format of a non-empty space");
-		return -1;
-	}
+
 	return 0;
 }
 
@@ -1036,7 +990,6 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
 {
 	(void)format;
 	struct vy_env *env = vy_env(space->engine);
-	/* @sa vy_prepare_alter_space for checks below. */
 	if (space->index_count == 0)
 		return 0;
 	struct vy_lsm *pk = vy_lsm(space->index[0]);
@@ -1045,7 +998,7 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
 	if (pk->stat.disk.count.rows == 0 && pk->stat.memory.count.rows == 0)
 		return 0;
 	diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
-		 "adding new fields to a non-empty space");
+		 "changing format of a non-empty space");
 	return -1;
 }
 
@@ -1095,6 +1048,24 @@ vinyl_space_build_index(struct space *space, struct index *index,
 			struct tuple_format *format)
 {
 	(void)format;
+
+	struct vy_env *env = vy_env(space->engine);
+	struct vy_lsm *pk = vy_lsm(space->index[0]);
+
+	/*
+	 * During local recovery we are loading existing indexes
+	 * from disk, not building new ones.
+	 */
+	if (env->status != VINYL_INITIAL_RECOVERY_LOCAL &&
+	    env->status != VINYL_FINAL_RECOVERY_LOCAL) {
+		if (pk->stat.disk.count.rows != 0 ||
+		    pk->stat.memory.count.rows != 0) {
+			diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
+				 "building an index for a non-empty space");
+			return -1;
+		}
+	}
+
 	/*
 	 * Unlike Memtx, Vinyl does not need building of a secondary index.
 	 * This is true because of two things:
@@ -1114,7 +1085,7 @@ vinyl_space_build_index(struct space *space, struct index *index,
 		return -1;
 
 	/* Set pointer to the primary key for the new index. */
-	vy_lsm_update_pk(vy_lsm(index), vy_lsm(space_index(space, 0)));
+	vy_lsm_update_pk(vy_lsm(index), pk);
 	return 0;
 }
 
diff --git a/test/box/alter.result b/test/box/alter.result
index 5e3cbf63..945b0cfd 100644
--- a/test/box/alter.result
+++ b/test/box/alter.result
@@ -1244,7 +1244,7 @@ t[5] = 1
 ...
 box.space._space:replace(t)
 ---
-- error: Vinyl does not support changing space format of a non-empty space
+- error: Vinyl does not support changing format of a non-empty space
 ...
 s:drop()
 ---
@@ -1558,7 +1558,7 @@ format[2] = {name = 'field2', type = 'unsigned'}
 ...
 s:format(format)
 ---
-- error: Vinyl does not support changing space format of a non-empty space
+- error: Vinyl does not support changing format of a non-empty space
 ...
 s:drop()
 ---
diff --git a/test/vinyl/ddl.result b/test/vinyl/ddl.result
index 456977e5..4ea94d36 100644
--- a/test/vinyl/ddl.result
+++ b/test/vinyl/ddl.result
@@ -83,11 +83,11 @@ space:insert({1})
 -- fail because of wrong tuple format {1}, but need {1, ...}
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ---
-- error: Vinyl does not support adding an index to a non-empty space
+- error: Vinyl does not support building an index for a non-empty space
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support building an index for a non-empty space
 ...
 #box.space._index:select({space.id})
 ---
@@ -112,11 +112,11 @@ space:insert({1, 2})
 ...
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ---
-- error: Vinyl does not support adding an index to a non-empty space
+- error: Vinyl does not support building an index for a non-empty space
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support building an index for a non-empty space
 ...
 #box.space._index:select({space.id})
 ---
@@ -141,11 +141,11 @@ space:insert({1, 2})
 ...
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ---
-- error: Vinyl does not support adding an index to a non-empty space
+- error: Vinyl does not support building an index for a non-empty space
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support building an index for a non-empty space
 ...
 #box.space._index:select({space.id})
 ---
@@ -161,11 +161,11 @@ space:delete({1})
 -- must fail because vy_mems have data
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ---
-- error: Vinyl does not support adding an index to a non-empty space
+- error: Vinyl does not support building an index for a non-empty space
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support building an index for a non-empty space
 ...
 box.snapshot()
 ---
@@ -232,11 +232,11 @@ while space.index.primary:info().run_count ~= 2 do fiber.sleep(0.01) end
 -- must fail because vy_runs have data
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ---
-- error: Vinyl does not support adding an index to a non-empty space
+- error: Vinyl does not support building an index for a non-empty space
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support building an index for a non-empty space
 ...
 -- After compaction the REPLACE + DELETE + DELETE = nothing, so
 -- the space is now empty and can be altered.
@@ -291,7 +291,7 @@ space:auto_increment{3}
 ...
 box.space._index:replace{space.id, 0, 'pk', 'tree', {unique=true}, {{0, 'unsigned'}, {1, 'unsigned'}}}
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support building an index for a non-empty space
 ...
 space:select{}
 ---
@@ -774,7 +774,7 @@ format[2].is_nullable = false
 ...
 space:format(format)
 ---
-- error: Vinyl does not support changing space format of a non-empty space
+- error: Vinyl does not support changing format of a non-empty space
 ...
 space:drop()
 ---
@@ -832,7 +832,7 @@ s.index.secondary.unique
 ...
 s.index.secondary:alter{unique = true} -- error
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support building an index for a non-empty space
 ...
 s.index.secondary.unique
 ---
diff --git a/test/vinyl/gh.result b/test/vinyl/gh.result
index 9c72acc8..47695acb 100644
--- a/test/vinyl/gh.result
+++ b/test/vinyl/gh.result
@@ -144,7 +144,7 @@ s:insert{5, 5}
 ...
 s.index.primary:alter({parts={2,'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support building an index for a non-empty space
 ...
 s:drop()
 ---
-- 
2.11.0

  parent reply	other threads:[~2018-04-07 13:38 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-07 13:37 [PATCH 00/12] vinyl: allow to modify format of non-empty spaces Vladimir Davydov
2018-04-07 13:37 ` [PATCH 01/12] alter: introduce CheckSpaceFormat AlterSpaceOp for validating format Vladimir Davydov
2018-04-09 20:25   ` Konstantin Osipov
2018-04-07 13:37 ` [PATCH 02/12] alter: fold ModifySpaceFormat into ModifySpace Vladimir Davydov
2018-04-09 20:26   ` Konstantin Osipov
2018-04-07 13:38 ` [PATCH 03/12] alter: move dictionary update from ModifySpace::alter_def to alter Vladimir Davydov
2018-04-09 20:32   ` Konstantin Osipov
2018-04-10  7:53     ` Vladimir Davydov
2018-04-10 11:45     ` Vladimir Davydov
2018-04-07 13:38 ` [PATCH 04/12] alter: use space_index instead of index_find where appropriate Vladimir Davydov
2018-04-09 20:34   ` Konstantin Osipov
2018-04-07 13:38 ` [PATCH 05/12] alter: allocate triggers before the point of no return Vladimir Davydov
2018-04-09 20:36   ` Konstantin Osipov
2018-04-10  7:57     ` Vladimir Davydov
2018-04-10 11:54       ` Vladimir Davydov
2018-04-07 13:38 ` [PATCH 06/12] space: space_vtab::build_secondary_key => build_index Vladimir Davydov
2018-04-09 20:39   ` Konstantin Osipov
2018-04-10  8:05     ` Vladimir Davydov
2018-04-10 12:14       ` Vladimir Davydov
2018-04-07 13:38 ` [PATCH 07/12] space: pass new format instead of new space to space_vtab::check_format Vladimir Davydov
2018-04-09 20:40   ` Konstantin Osipov
2018-04-07 13:38 ` [PATCH 08/12] alter: introduce preparation phase Vladimir Davydov
2018-04-09 20:46   ` [tarantool-patches] " Konstantin Osipov
2018-04-10  8:31     ` Vladimir Davydov
2018-04-10  8:46       ` Konstantin Osipov
2018-04-07 13:38 ` [PATCH 09/12] alter: zap space_def_check_compatibility Vladimir Davydov
2018-04-09 20:49   ` Konstantin Osipov
2018-04-07 13:38 ` Vladimir Davydov [this message]
2018-04-09 20:49   ` [PATCH 10/12] vinyl: remove superfluous ddl checks Konstantin Osipov
2018-04-07 13:38 ` [PATCH 11/12] vinyl: force index rebuild if indexed field type is narrowed Vladimir Davydov
2018-04-09 20:51   ` Konstantin Osipov
2018-04-07 13:38 ` [PATCH 12/12] vinyl: allow to modify format of non-empty spaces Vladimir Davydov
2018-04-09  8:24   ` Vladimir Davydov
2018-04-09 20:55   ` Konstantin Osipov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a1d42a76d6e9a79fcd77f548904c75bf86478217.1523105106.git.vdavydov.dev@gmail.com \
    --to=vdavydov.dev@gmail.com \
    --cc=kostja@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [PATCH 10/12] vinyl: remove superfluous ddl checks' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox