[PATCH v2 0/4] Check read access while executing SQL query

[PATCH v2 0/4] Check read access while executing SQL query

[Kirill Yukhin]

Since SQL front-end is not using box API,
no checkes for read access are performed by VDBE engine.
Fix this by introducing dedicated opcode which is executed
to perform read access check.
Note, that there's is no need to perform DML/DDL checkes as
they're performed by Tarantool's core.

Closes #2362
---
https://github.com/tarantool/tarantool/commits/kyukhin/gh-2362-sql-read-access-check
https://github.com/tarantool/tarantool/issues/2362
https://travis-ci.org/tarantool/tarantool/builds/445664874

Changes against v1:
  - Split addition of space_name->space_ptr mapping into 3 patches:
    a. Refactor cache replace/delete machinery.
    b. Introduce space_by_name() API.
    c. Use new API throughout SQL front-end.
  - Change argument order for space_cache_replace().
  - Move assert() into space_cache_replace(), make it return void.

Kirill Yukhin (4):
  schema: refactor space_cache API
  schema: add space names cache
  sql: use space_by_name in SQL
  sql: check read access while executing SQL query

 src/box/alter.cc                               |  26 +++---
 src/box/schema.cc                              | 108 ++++++++++++++++++------
 src/box/schema.h                               |  17 ++--
 src/box/sql/alter.c                            |  11 ++-
 src/box/sql/analyze.c                          |  44 ++++------
 src/box/sql/build.c                            |  57 +++++--------
 src/box/sql/delete.c                           |  26 +++---
 src/box/sql/insert.c                           |   7 +-
 src/box/sql/pragma.c                           |  32 +++----
 src/box/sql/vdbe.c                             |   5 ++
 test/box/stat.result                           |   6 +-
 test/sql/gh-2362-select-access-rights.result   | 110 +++++++++++++++++++++++++
 test/sql/gh-2362-select-access-rights.test.lua |  42 ++++++++++
 13 files changed, 326 insertions(+), 165 deletions(-)
 create mode 100644 test/sql/gh-2362-select-access-rights.result
 create mode 100644 test/sql/gh-2362-select-access-rights.test.lua

-- 
2.11.0

[PATCH v2 1/4] schema: refactor space_cache API

[Kirill Yukhin]

Remove function which deletes from cache, making replace
more general: it might be used for both insertions,
deletions and replaces. Also, put assert on equality
of space pointer found in cache to old one into
replace routine.
---
 src/box/alter.cc  | 26 ++++++++++----------------
 src/box/schema.cc | 52 +++++++++++++++++++++++++---------------------------
 src/box/schema.h  |  9 ++-------
 3 files changed, 37 insertions(+), 50 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index de3943c..79ff589 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -810,9 +810,7 @@ alter_space_rollback(struct trigger *trigger, void * /* event */)
 	 */
 	space_swap_triggers(alter->new_space, alter->old_space);
 	space_swap_fkeys(alter->new_space, alter->old_space);
-	struct space *new_space = space_cache_replace(alter->old_space);
-	assert(new_space == alter->new_space);
-	(void) new_space;
+	space_cache_replace(alter->new_space, alter->old_space);
 	alter_space_delete(alter);
 }
 
@@ -913,9 +911,7 @@ alter_space_do(struct txn *txn, struct alter_space *alter)
 	 * The new space is ready. Time to update the space
 	 * cache with it.
 	 */
-	struct space *old_space = space_cache_replace(alter->new_space);
-	(void) old_space;
-	assert(old_space == alter->old_space);
+	space_cache_replace(alter->old_space, alter->new_space);
 
 	/*
 	 * Install transaction commit/rollback triggers to either
@@ -1421,7 +1417,7 @@ on_drop_space_rollback(struct trigger *trigger, void *event)
 {
 	(void) event;
 	struct space *space = (struct space *)trigger->data;
-	space_cache_replace(space);
+	space_cache_replace(NULL, space);
 }
 
 /**
@@ -1447,9 +1443,7 @@ on_create_space_rollback(struct trigger *trigger, void *event)
 {
 	(void) event;
 	struct space *space = (struct space *)trigger->data;
-	struct space *cached = space_cache_delete(space_id(space));
-	(void) cached;
-	assert(cached == space);
+	space_cache_replace(space, NULL);
 	space_delete(space);
 }
 
@@ -1610,8 +1604,7 @@ on_drop_view_rollback(struct trigger *trigger, void *event)
  *
  * - space cache should be updated, and changes in the space
  *   cache should be reflected in Lua bindings
- *   (this is done in space_cache_replace() and
- *   space_cache_delete())
+ *   (this is done in space_cache_replace())
  *
  * - the space which is changed should be rebuilt according
  *   to the nature of the modification, i.e. indexes added/dropped,
@@ -1695,7 +1688,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 		 * cache right away to achieve linearisable
 		 * execution on a replica.
 		 */
-		(void) space_cache_replace(space);
+		(void) space_cache_replace(NULL, space);
 		/*
 		 * Do not forget to update schema_version right after
 		 * inserting the space to the space_cache, since no
@@ -1787,7 +1780,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 		 * cache right away to achieve linearisable
 		 * execution on a replica.
 		 */
-		struct space *space = space_cache_delete(space_id(old_space));
+		space_cache_replace(old_space, NULL);
 		/*
 		 * Do not forget to update schema_version right after
 		 * deleting the space from the space_cache, since no
@@ -1795,7 +1788,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 		 */
 		++schema_version;
 		struct trigger *on_commit =
-			txn_alter_trigger_new(on_drop_space_commit, space);
+			txn_alter_trigger_new(on_drop_space_commit, old_space);
 		txn_on_commit(txn, on_commit);
 		if (old_space->def->opts.is_view) {
 			struct Select *select =
@@ -1817,7 +1810,8 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 			select_guard.is_active = false;
 		}
 		struct trigger *on_rollback =
-			txn_alter_trigger_new(on_drop_space_rollback, space);
+			txn_alter_trigger_new(on_drop_space_rollback,
+					      old_space);
 		txn_on_rollback(txn, on_rollback);
 	} else { /* UPDATE, REPLACE */
 		assert(old_space != NULL && new_tuple != NULL);
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 2d2be26..08457a46 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -158,34 +158,33 @@ space_foreach(int (*func)(struct space *sp, void *udata), void *udata)
 	return 0;
 }
 
-/** Delete a space from the space cache and Lua. */
-struct space *
-space_cache_delete(uint32_t id)
-{
-	mh_int_t k = mh_i32ptr_find(spaces, id, NULL);
-	assert(k != mh_end(spaces));
-	struct space *space = (struct space *)mh_i32ptr_node(spaces, k)->val;
-	mh_i32ptr_del(spaces, k, NULL);
-	space_cache_version++;
-	return space;
-}
-
 /**
- * Update the space in the space cache and in Lua. Returns
- * the old space instance, if any, or NULL if it's a new space.
+ * Update the space in the space cache and in Lua.
  */
-struct space *
-space_cache_replace(struct space *space)
+void
+space_cache_replace(struct space *old_space, struct space *new_space)
 {
-	const struct mh_i32ptr_node_t node = { space_id(space), space };
-	struct mh_i32ptr_node_t old, *p_old = &old;
-	mh_int_t k = mh_i32ptr_put(spaces, &node, &p_old, NULL);
-	if (k == mh_end(spaces)) {
-		panic_syserror("Out of memory for the data "
-			       "dictionary cache.");
+	assert(new_space != NULL || old_space != NULL);
+	if (new_space != NULL) {
+		const struct mh_i32ptr_node_t node_p = { space_id(new_space),
+							 new_space };
+		struct mh_i32ptr_node_t old, *p_old = &old;
+		mh_int_t k = mh_i32ptr_put(spaces, &node_p, &p_old, NULL);
+		if (k == mh_end(spaces)) {
+			panic_syserror("Out of memory for the data "
+				       "dictionary cache.");
+		}
+		assert((struct space *)p_old->val == old_space);
+	} else {
+		mh_int_t k = mh_i32ptr_find(spaces, space_id(old_space), NULL);
+		assert(k != mh_end(spaces));
+		struct space *old_space_by_id =
+			(struct space *)mh_i32ptr_node(spaces, k)->val;
+		(void)old_space_by_id;
+		assert(old_space_by_id == old_space);
+		mh_i32ptr_del(spaces, k, NULL);
 	}
 	space_cache_version++;
-	return p_old ? (struct space *) p_old->val : NULL;
 }
 
 /** A wrapper around space_new() for data dictionary spaces. */
@@ -220,7 +219,7 @@ sc_space_new(uint32_t id, const char *name,
 	rlist_create(&key_list);
 	rlist_add_entry(&key_list, index_def, link);
 	struct space *space = space_new_xc(def, &key_list);
-	(void) space_cache_replace(space);
+	(void) space_cache_replace(NULL, space);
 	if (replace_trigger)
 		trigger_add(&space->on_replace, replace_trigger);
 	if (stmt_begin_trigger)
@@ -431,7 +430,7 @@ schema_init()
 		});
 		RLIST_HEAD(key_list);
 		struct space *space = space_new_xc(def, &key_list);
-		space_cache_replace(space);
+		space_cache_replace(NULL, space);
 		init_system_space(space);
 		trigger_run_xc(&on_alter_space, space);
 	}
@@ -447,8 +446,7 @@ schema_free(void)
 
 		struct space *space = (struct space *)
 				mh_i32ptr_node(spaces, i)->val;
-		space_cache_delete(space_id(space));
-		space_delete(space);
+		space_cache_replace(space, NULL);
 	}
 	mh_i32ptr_delete(spaces);
 	while (mh_size(funcs) > 0) {
diff --git a/src/box/schema.h b/src/box/schema.h
index 264c16b..05f32de 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -134,14 +134,9 @@ space_cache_find_xc(uint32_t id)
 /**
  * Update contents of the space cache.  Typically the new space is
  * an altered version of the original space.
- * Returns the old space, if any.
  */
-struct space *
-space_cache_replace(struct space *space);
-
-/** Delete a space from the space cache. */
-struct space *
-space_cache_delete(uint32_t id);
+void
+space_cache_replace(struct space *old_space, struct space *new_space);
 
 void
 schema_init();
-- 
2.11.0

[PATCH v2 2/4] schema: add space names cache

[Kirill Yukhin]

Since SQL is heavily using name -> space mapping,
introduce (instead of scanning _space space) dedicated
cache, which maps space name to space pointer.
---
 src/box/schema.cc | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 src/box/schema.h  |  8 ++++++++
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/src/box/schema.cc b/src/box/schema.cc
index 08457a46..b931fdf 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -55,6 +55,7 @@
 
 /** All existing spaces. */
 static struct mh_i32ptr_t *spaces;
+static struct mh_strnptr_t *spaces_by_name;
 static struct mh_i32ptr_t *funcs;
 static struct mh_strnptr_t *funcs_by_name;
 static struct mh_i32ptr_t *sequences;
@@ -95,6 +96,17 @@ space_by_id(uint32_t id)
 	return (struct space *) mh_i32ptr_node(spaces, space)->val;
 }
 
+/** Return space by its name */
+struct space *
+space_by_name(const char *name)
+{
+	mh_int_t space = mh_strnptr_find_inp(spaces_by_name, name,
+					     strlen(name));
+	if (space == mh_end(spaces_by_name))
+		return NULL;
+	return (struct space *) mh_strnptr_node(spaces_by_name, space)->val;
+}
+
 /** Return current schema version */
 uint32_t
 box_schema_version()
@@ -165,7 +177,25 @@ void
 space_cache_replace(struct space *old_space, struct space *new_space)
 {
 	assert(new_space != NULL || old_space != NULL);
+	struct space *old_space_by_n = NULL;
 	if (new_space != NULL) {
+		/*
+		 * If replacing is performed and space name was
+		 * changed, then need to explicitly remove old
+		 * entry from spaces cache.
+		 * NB: Since space_id never changed - no need
+		 * to do so for space_id cache.
+		 */
+		if (old_space != NULL && strcmp(space_name(old_space),
+						new_space->def->name) != 0) {
+			const char *name = space_name(old_space);
+			mh_int_t k = mh_strnptr_find_inp(spaces_by_name, name,
+							 strlen(name));
+			assert(k != mh_end(spaces_by_name));
+			mh_strnptr_del(spaces_by_name, k, NULL);
+			old_space_by_n = (struct space *)mh_strnptr_node(spaces_by_name,
+									 k)->val;
+		}
 		const struct mh_i32ptr_node_t node_p = { space_id(new_space),
 							 new_space };
 		struct mh_i32ptr_node_t old, *p_old = &old;
@@ -174,7 +204,23 @@ space_cache_replace(struct space *old_space, struct space *new_space)
 			panic_syserror("Out of memory for the data "
 				       "dictionary cache.");
 		}
-		assert((struct space *)p_old->val == old_space);
+		const char *name = space_name(new_space);
+		uint32_t name_len = strlen(name);
+		uint32_t hash = mh_strn_hash(name, name_len);
+		const struct mh_strnptr_node_t node_s = { name, name_len, hash,
+							  new_space };
+		struct mh_strnptr_node_t old_s, *p_old_s = &old_s;
+		k = mh_strnptr_put(spaces_by_name, &node_s, &p_old_s, NULL);
+		if (k == mh_end(spaces_by_name)) {
+			panic_syserror("Out of memory for the data "
+				       "dictionary cache.");
+		}
+		assert(old_space_by_n == NULL || p_old_s == NULL);
+		if(old_space_by_n == NULL && p_old != NULL) {
+			assert(p_old->val == p_old_s->val);
+			old_space_by_n = (struct space *)p_old->val;
+		}
+		assert((struct space*)old_space_by_n == old_space);
 	} else {
 		mh_int_t k = mh_i32ptr_find(spaces, space_id(old_space), NULL);
 		assert(k != mh_end(spaces));
@@ -183,6 +229,14 @@ space_cache_replace(struct space *old_space, struct space *new_space)
 		(void)old_space_by_id;
 		assert(old_space_by_id == old_space);
 		mh_i32ptr_del(spaces, k, NULL);
+		const char *name = space_name(old_space);
+		k = mh_strnptr_find_inp(spaces_by_name, name, strlen(name));
+		assert(k != mh_end(spaces_by_name));
+		old_space_by_n = (struct space *)mh_strnptr_node(spaces_by_name,
+								 k)->val;
+		assert(old_space_by_n == old_space_by_id);
+		(void)old_space_by_id;
+		mh_strnptr_del(spaces_by_name, k, NULL);
 	}
 	space_cache_version++;
 }
@@ -302,6 +356,7 @@ schema_init()
 
 	/* Initialize the space cache. */
 	spaces = mh_i32ptr_new();
+	spaces_by_name = mh_strnptr_new();
 	funcs = mh_i32ptr_new();
 	funcs_by_name = mh_strnptr_new();
 	sequences = mh_i32ptr_new();
@@ -449,6 +504,7 @@ schema_free(void)
 		space_cache_replace(space, NULL);
 	}
 	mh_i32ptr_delete(spaces);
+	mh_strnptr_delete(spaces_by_name);
 	while (mh_size(funcs) > 0) {
 		mh_int_t i = mh_first(funcs);
 
diff --git a/src/box/schema.h b/src/box/schema.h
index 05f32de..89866a8 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -58,6 +58,14 @@ extern struct latch schema_lock;
 struct space *
 space_by_id(uint32_t id);
 
+/**
+ * Try to look up a space by space name in the space name cache.
+ *
+ * @return NULL if space not found, otherwise space object.
+ */
+struct space *
+space_by_name(const char *name);
+
 uint32_t
 box_schema_version();
 
-- 
2.11.0

[PATCH v2 3/4] sql: use space_by_name in SQL

[Kirill Yukhin]

Since hash, which maps space name to space
pointer was introduced in previous patch, use
it in SQL front-end as it is heavily needed.
---
 src/box/sql/alter.c   | 11 +++++-----
 src/box/sql/analyze.c | 44 +++++++++++++++------------------------
 src/box/sql/build.c   | 57 ++++++++++++++++++---------------------------------
 src/box/sql/delete.c  | 26 ++++++++++-------------
 src/box/sql/insert.c  |  7 ++-----
 src/box/sql/pragma.c  | 32 ++++++++++-------------------
 test/box/stat.result  |  6 +++---
 7 files changed, 68 insertions(+), 115 deletions(-)

diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 3d72e31..7c28437 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -47,18 +47,17 @@ sql_alter_table_rename(struct Parse *parse, struct SrcList *src_tab,
 	if (new_name == NULL)
 		goto exit_rename_table;
 	/* Check that new name isn't occupied by another table. */
-	uint32_t space_id = box_space_id_by_name(new_name, strlen(new_name));
-	if (space_id != BOX_ID_NIL) {
+	struct space *space = space_by_name(new_name);
+	if (space != NULL) {
 		diag_set(ClientError, ER_SPACE_EXISTS, new_name);
 		goto tnt_error;
 	}
 	const char *tbl_name = src_tab->a[0].zName;
-	space_id = box_space_id_by_name(tbl_name, strlen(tbl_name));
-	if (space_id == BOX_ID_NIL) {
+	space = space_by_name(tbl_name);
+	if (space == NULL) {
 		diag_set(ClientError, ER_NO_SUCH_SPACE, tbl_name);
 		goto tnt_error;
 	}
-	struct space *space = space_by_id(space_id);
 	assert(space != NULL);
 	if (space->def->opts.is_view) {
 		diag_set(ClientError, ER_ALTER_SPACE, tbl_name,
@@ -68,7 +67,7 @@ sql_alter_table_rename(struct Parse *parse, struct SrcList *src_tab,
 	sql_set_multi_write(parse, false);
 	/* Drop and reload the internal table schema. */
 	struct Vdbe *v = sqlite3GetVdbe(parse);
-	sqlite3VdbeAddOp4(v, OP_RenameTable, space_id, 0, 0, new_name,
+	sqlite3VdbeAddOp4(v, OP_RenameTable, space->def->id, 0, 0, new_name,
 			  P4_DYNAMIC);
 exit_rename_table:
 	sqlite3SrcListDelete(db, src_tab);
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 674d53d..ad068a54 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1114,16 +1114,15 @@ sqlite3Analyze(Parse * pParse, Token * pName)
 		/* Form 2:  Analyze table named */
 		char *z = sqlite3NameFromToken(db, pName);
 		if (z != NULL) {
-			uint32_t space_id = box_space_id_by_name(z, strlen(z));
-			if (space_id != BOX_ID_NIL) {
-				struct space *sp = space_by_id(space_id);
-				assert(sp != NULL);
-				if (sp->def->opts.is_view) {
+			struct space *space = space_by_name(z);
+			if (space != NULL) {
+				assert(space != NULL);
+				if (space->def->opts.is_view) {
 					sqlite3ErrorMsg(pParse, "VIEW isn't "\
 							"allowed to be "\
 							"analyzed");
 				} else {
-					vdbe_emit_analyze_table(pParse, sp);
+					vdbe_emit_analyze_table(pParse, space);
 				}
 			} else {
 				diag_set(ClientError, ER_NO_SUCH_SPACE, z);
@@ -1224,13 +1223,12 @@ analysis_loader(void *data, int argc, char **argv, char **unused)
 	struct analysis_index_info *info = (struct analysis_index_info *) data;
 	assert(info->stats != NULL);
 	struct index_stat *stat = &info->stats[info->index_count++];
-	uint32_t space_id = box_space_id_by_name(argv[0], strlen(argv[0]));
-	if (space_id == BOX_ID_NIL)
+	struct space *space = space_by_name(argv[0]);
+	if (space == NULL)
 		return -1;
-	struct space *space = space_by_id(space_id);
-	assert(space != NULL);
 	struct index *index;
-	uint32_t iid = box_index_id_by_name(space_id, argv[1], strlen(argv[1]));
+	uint32_t iid = box_index_id_by_name(space->def->id, argv[1],
+					    strlen(argv[1]));
 	/*
 	 * Convention is if index's name matches with space's
 	 * one, then it is primary index.
@@ -1395,13 +1393,10 @@ load_stat_from_space(struct sqlite3 *db, const char *sql_select_prepare,
 		if (index_name == NULL)
 			continue;
 		uint32_t sample_count = sqlite3_column_int(stmt, 2);
-		uint32_t space_id = box_space_id_by_name(space_name,
-							 strlen(space_name));
-		assert(space_id != BOX_ID_NIL);
-		struct space *space = space_by_id(space_id);
+		struct space *space = space_by_name(space_name);
 		assert(space != NULL);
 		struct index *index;
-		uint32_t iid = box_index_id_by_name(space_id, index_name,
+		uint32_t iid = box_index_id_by_name(space->def->id, index_name,
 						    strlen(index_name));
 		if (sqlite3_stricmp(space_name, index_name) == 0 &&
 		    iid == BOX_ID_NIL)
@@ -1466,13 +1461,10 @@ load_stat_from_space(struct sqlite3 *db, const char *sql_select_prepare,
 		const char *index_name = (char *)sqlite3_column_text(stmt, 1);
 		if (index_name == NULL)
 			continue;
-		uint32_t space_id = box_space_id_by_name(space_name,
-							 strlen(space_name));
-		assert(space_id != BOX_ID_NIL);
-		struct space *space = space_by_id(space_id);
-		assert(space != NULL);
+		struct space *space = space_by_name(space_name);
+		assert (space != NULL);
 		struct index *index;
-		uint32_t iid = box_index_id_by_name(space_id, index_name,
+		uint32_t iid = box_index_id_by_name(space->def->id, index_name,
 						    strlen(index_name));
 		if (iid != BOX_ID_NIL) {
 			index = space_index(space, iid);
@@ -1550,14 +1542,10 @@ load_stat_to_index(struct sqlite3 *db, const char *sql_select_load,
 		const char *index_name = (char *)sqlite3_column_text(stmt, 1);
 		if (index_name == NULL)
 			continue;
-		uint32_t space_id = box_space_id_by_name(space_name,
-							 strlen(space_name));
-		if (space_id == BOX_ID_NIL)
-			return -1;
-		struct space *space = space_by_id(space_id);
+		struct space *space = space_by_name(space_name);
 		assert(space != NULL);
 		struct index *index;
-		uint32_t iid = box_index_id_by_name(space_id, index_name,
+		uint32_t iid = box_index_id_by_name(space->def->id, index_name,
 						    strlen(index_name));
 		if (iid != BOX_ID_NIL) {
 			index = space_index(space, iid);
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a806fb4..c5a527d 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -329,8 +329,8 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
 	if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK)
 		goto cleanup;
 
-	uint32_t space_id = box_space_id_by_name(zName, strlen(zName));
-	if (space_id != BOX_ID_NIL) {
+	struct space *space = space_by_name(zName);
+	if (space != NULL) {
 		if (!noErr) {
 			sqlite3ErrorMsg(pParse, "table %s already exists",
 					zName);
@@ -1863,9 +1863,8 @@ sql_drop_table(struct Parse *parse_context, struct SrcList *table_name_list,
 	assert(parse_context->nErr == 0);
 	assert(table_name_list->nSrc == 1);
 	const char *space_name = table_name_list->a[0].zName;
-	uint32_t space_id = box_space_id_by_name(space_name,
-						 strlen(space_name));
-	if (space_id == BOX_ID_NIL) {
+	struct space *space = space_by_name(space_name);
+	if (space == NULL) {
 		if (!is_view && !if_exists)
 			sqlite3ErrorMsg(parse_context, "no such table: %s",
 					space_name);
@@ -1874,8 +1873,6 @@ sql_drop_table(struct Parse *parse_context, struct SrcList *table_name_list,
 					space_name);
 		goto exit_drop_table;
 	}
-	struct space *space = space_by_id(space_id);
-	assert(space != NULL);
 	/*
 	 * Ensure DROP TABLE is not used on a view,
 	 * and DROP VIEW is not used on a table.
@@ -1990,17 +1987,13 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
 	}
 	assert(!is_alter || (child != NULL && child->nSrc == 1));
 	struct space *child_space = NULL;
-	uint32_t child_id = 0;
 	if (is_alter) {
 		const char *child_name = child->a[0].zName;
-		child_id = box_space_id_by_name(child_name,
-						strlen(child_name));
-		if (child_id == BOX_ID_NIL) {
+		child_space = space_by_name(child_name);
+		if (child_space == NULL) {
 			diag_set(ClientError, ER_NO_SUCH_SPACE, child_name);
 			goto tnt_error;
 		}
-		child_space = space_by_id(child_id);
-		assert(child_space != NULL);
 	} else {
 		struct fkey_parse *fk = region_alloc(&parse_context->region,
 						     sizeof(*fk));
@@ -2016,8 +2009,7 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
 	parent_name = sqlite3NameFromToken(db, parent);
 	if (parent_name == NULL)
 		goto exit_create_fk;
-	uint32_t parent_id = box_space_id_by_name(parent_name,
-						  strlen(parent_name));
+	struct space *parent_space = space_by_name(parent_name);
 	/*
 	 * Within ALTER TABLE ADD CONSTRAINT FK also can be
 	 * self-referenced, but in this case parent (which is
@@ -2025,9 +2017,7 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
 	 */
 	is_self_referenced = !is_alter &&
 			     strcmp(parent_name, new_tab->def->name) == 0;
-	struct space *parent_space;
-	if (parent_id == BOX_ID_NIL) {
-		parent_space = NULL;
+	if (parent_space == NULL) {
 		if (is_self_referenced) {
 			struct fkey_parse *fk =
 				rlist_first_entry(&parse_context->new_fkey,
@@ -2039,8 +2029,6 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
 			goto tnt_error;
 		}
 	} else {
-		parent_space = space_by_id(parent_id);
-		assert(parent_space != NULL);
 		if (parent_space->def->opts.is_view) {
 			sqlite3ErrorMsg(parse_context,
 					"referenced table can't be view");
@@ -2092,8 +2080,8 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
 		goto tnt_error;
 	}
 	fk->field_count = child_cols_count;
-	fk->child_id = child_id;
-	fk->parent_id = parent_id;
+	fk->child_id = child_space != NULL ? child_space->def->id : 0;
+	fk->parent_id = parent_space != NULL ? parent_space->def->id : 0;
 	fk->is_deferred = is_deferred;
 	fk->match = (enum fkey_match) ((actions >> 16) & 0xff);
 	fk->on_update = (enum fkey_action) ((actions >> 8) & 0xff);
@@ -2184,9 +2172,8 @@ sql_drop_foreign_key(struct Parse *parse_context, struct SrcList *table,
 {
 	assert(table != NULL && table->nSrc == 1);
 	const char *table_name = table->a[0].zName;
-	uint32_t child_id = box_space_id_by_name(table_name,
-						 strlen(table_name));
-	if (child_id == BOX_ID_NIL) {
+	struct space *child = space_by_name(table_name);
+	if (child == NULL) {
 		diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
 		parse_context->rc = SQL_TARANTOOL_ERROR;
 		parse_context->nErr++;
@@ -2195,7 +2182,8 @@ sql_drop_foreign_key(struct Parse *parse_context, struct SrcList *table,
 	char *constraint_name = sqlite3NameFromToken(parse_context->db,
 						     constraint);
 	if (constraint_name != NULL)
-		vdbe_emit_fkey_drop(parse_context, constraint_name, child_id);
+		vdbe_emit_fkey_drop(parse_context, constraint_name,
+				    child->def->id);
 }
 
 /*
@@ -2415,8 +2403,8 @@ sql_create_index(struct Parse *parse, struct Token *token,
 	if (tbl_name != NULL) {
 		assert(token != NULL && token->z != NULL);
 		const char *name = tbl_name->a[0].zName;
-		uint32_t space_id = box_space_id_by_name(name, strlen(name));
-		if (space_id == BOX_ID_NIL) {
+		space = space_by_name(name);
+		if (space == NULL) {
 			if (! if_not_exist) {
 				diag_set(ClientError, ER_NO_SUCH_SPACE, name);
 				parse->rc = SQL_TARANTOOL_ERROR;
@@ -2424,8 +2412,6 @@ sql_create_index(struct Parse *parse, struct Token *token,
 			}
 			goto exit_create_index;
 		}
-		space = space_by_id(space_id);
-		assert(space != NULL);
 		def = space->def;
 	} else {
 		if (parse->pNewTable == NULL)
@@ -2738,16 +2724,15 @@ sql_drop_index(struct Parse *parse_context, struct SrcList *index_name_list,
 	sqlite3VdbeCountChanges(v);
 	assert(index_name_list->nSrc == 1);
 	assert(table_token->n > 0);
-	uint32_t space_id = box_space_id_by_name(table_name,
-						 strlen(table_name));
-	if (space_id == BOX_ID_NIL) {
+	struct space *space = space_by_name(table_name);
+	if (space == NULL) {
 		if (!if_exists)
 			sqlite3ErrorMsg(parse_context, "no such space: %s",
 					table_name);
 		goto exit_drop_index;
 	}
 	const char *index_name = index_name_list->a[0].zName;
-	uint32_t index_id = box_index_id_by_name(space_id, index_name,
+	uint32_t index_id = box_index_id_by_name(space->def->id, index_name,
 						 strlen(index_name));
 	if (index_id == BOX_ID_NIL) {
 		if (!if_exists)
@@ -2755,8 +2740,6 @@ sql_drop_index(struct Parse *parse_context, struct SrcList *index_name_list,
 					table_name, index_name);
 		goto exit_drop_index;
 	}
-	struct space *space = space_by_id(space_id);
-	assert(space != NULL);
 	struct index *index = space_index(space, index_id);
 	assert(index != NULL);
 	/*
@@ -2779,7 +2762,7 @@ sql_drop_index(struct Parse *parse_context, struct SrcList *index_name_list,
 	sql_clear_stat_spaces(parse_context, table_name, index->def->name);
 	int record_reg = ++parse_context->nMem;
 	int space_id_reg = ++parse_context->nMem;
-	sqlite3VdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
+	sqlite3VdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
 	sqlite3VdbeAddOp2(v, OP_Integer, index_id, space_id_reg + 1);
 	sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
 	sqlite3VdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 8f6c76f..72e0575 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -36,15 +36,14 @@
 #include "tarantoolInt.h"
 
 struct Table *
-sql_lookup_table(struct Parse *parse, struct SrcList_item *tbl_name)
+sql_lookup_table(struct Parse *parse, struct SrcList_item *src_name)
 {
-	assert(tbl_name != NULL);
-	assert(tbl_name->pTab == NULL);
-	uint32_t space_id = box_space_id_by_name(tbl_name->zName,
-						 strlen(tbl_name->zName));
-	struct space *space = space_by_id(space_id);
-	if (space_id == BOX_ID_NIL || space == NULL) {
-		sqlite3ErrorMsg(parse, "no such table: %s", tbl_name->zName);
+	assert(src_name != NULL);
+	assert(src_name->pTab == NULL);
+	const char *name = src_name->zName;
+	struct space *space = space_by_name(name);
+	if (space == NULL) {
+		sqlite3ErrorMsg(parse, "no such table: %s", name);
 		return NULL;
 	}
 	assert(space != NULL);
@@ -61,8 +60,8 @@ sql_lookup_table(struct Parse *parse, struct SrcList_item *tbl_name)
 	table->def = space->def;
 	table->space = space;
 	table->nTabRef = 1;
-	tbl_name->pTab = table;
-	if (sqlite3IndexedByLookup(parse, tbl_name) != 0)
+	src_name->pTab = table;
+	if (sqlite3IndexedByLookup(parse, src_name) != 0)
 		table = NULL;
 	return table;
 }
@@ -98,14 +97,11 @@ sql_table_truncate(struct Parse *parse, struct SrcList *tab_list)
 		goto cleanup;
 
 	const char *tab_name = tab_list->a->zName;
-	uint32_t space_id = box_space_id_by_name(tab_name, strlen(tab_name));
-	if (space_id == BOX_ID_NIL) {
+	struct space *space = space_by_name(tab_name);
+	if (space == NULL) {
 		diag_set(ClientError, ER_NO_SUCH_SPACE, tab_name);
 		goto tarantool_error;
 	}
-	struct space *space = space_cache_find(space_id);
-	assert(space != NULL);
-
 	if (! rlist_empty(&space->parent_fkey)) {
 		const char *err_msg =
 			tt_sprintf("can not truncate space '%s' because other "
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 5862917..eef27bc 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1197,13 +1197,10 @@ xferOptimization(Parse * pParse,	/* Parser context */
 	 * we have to check the semantics.
 	 */
 	pItem = pSelect->pSrc->a;
-	uint32_t src_id = box_space_id_by_name(pItem->zName,
-					       strlen(pItem->zName));
+	struct space *src = space_by_name(pItem->zName);
 	/* FROM clause does not contain a real table. */
-	if (src_id == BOX_ID_NIL)
+	if (src == NULL)
 		return 0;
-	struct space *src = space_by_id(src_id);
-	assert(src != NULL);
 	/* Src and dest may not be the same table. */
 	if (src->def->id == dest->def->id)
 		return 0;
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 2e007f9..9ece82a 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -257,11 +257,9 @@ sql_pragma_table_info(struct Parse *parse, const char *tbl_name)
 {
 	if (tbl_name == NULL)
 		return;
-	uint32_t space_id = box_space_id_by_name(tbl_name, strlen(tbl_name));
-	if (space_id == BOX_ID_NIL)
+	struct space *space = space_by_name(tbl_name);
+	if (space == NULL)
 		return;
-	struct space *space = space_by_id(space_id);
-	assert(space != NULL);
 	struct index *pk = space_index(space, 0);
 	parse->nMem = 6;
 	if (space->def->opts.is_view)
@@ -336,14 +334,12 @@ sql_pragma_index_info(struct Parse *parse, const PragmaName *pragma,
 {
 	if (idx_name == NULL || tbl_name == NULL)
 		return;
-	uint32_t space_id = box_space_id_by_name(tbl_name, strlen(tbl_name));
-	if (space_id == BOX_ID_NIL)
+	struct space *space = space_by_name(tbl_name);
+	if (space == NULL)
 		return;
-	struct space *space = space_by_id(space_id);
-	assert(space != NULL);
 	if (space->def->opts.sql == NULL)
 		return;
-	uint32_t iid = box_index_id_by_name(space_id, idx_name,
+	uint32_t iid = box_index_id_by_name(space->def->id, idx_name,
 					     strlen(idx_name));
 	if (iid == BOX_ID_NIL)
 		return;
@@ -390,11 +386,9 @@ sql_pragma_index_list(struct Parse *parse, const char *tbl_name)
 {
 	if (tbl_name == NULL)
 		return;
-	uint32_t space_id = box_space_id_by_name(tbl_name, strlen(tbl_name));
-	if (space_id == BOX_ID_NIL)
+	struct space *space = space_by_name(tbl_name);
+	if (space == NULL)
 		return;
-	struct space *space = space_by_id(space_id);
-	assert(space != NULL);
 	parse->nMem = 5;
 	struct Vdbe *v = sqlite3GetVdbe(parse);
 	for (uint32_t i = 0; i < space->index_count; ++i) {
@@ -516,15 +510,13 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
 
 	case PragTyp_COLLATION_LIST:{
 		int i = 0;
-		uint32_t space_id;
-		space_id = box_space_id_by_name("_collation",
-						(uint32_t) strlen("_collation"));
+		struct space *space = space_by_name("_collation");
 		char key_buf[16]; /* 16 is enough to encode 0 len array */
 		char *key_end = key_buf;
 		key_end = mp_encode_array(key_end, 0);
 		box_tuple_t *tuple;
 		box_iterator_t* iter;
-		iter = box_index_iterator(space_id, 0,ITER_ALL, key_buf, key_end);
+		iter = box_index_iterator(space->def->id, 0,ITER_ALL, key_buf, key_end);
 		int rc = box_iterator_next(iter, &tuple);
 		(void) rc;
 		assert(rc == 0);
@@ -544,11 +536,9 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
 	case PragTyp_FOREIGN_KEY_LIST:{
 		if (zRight == NULL)
 			break;
-		uint32_t space_id = box_space_id_by_name(zRight,
-							 strlen(zRight));
-		if (space_id == BOX_ID_NIL)
+		struct space *space = space_by_name(zRight);
+		if (space == NULL)
 			break;
-		struct space *space = space_by_id(space_id);
 		int i = 0;
 		pParse->nMem = 8;
 		struct fkey *fkey;
diff --git a/test/box/stat.result b/test/box/stat.result
index 60ba88f..af1607d 100644
--- a/test/box/stat.result
+++ b/test/box/stat.result
@@ -24,7 +24,7 @@ box.stat.REPLACE.total
 ...
 box.stat.SELECT.total
 ---
-- 2
+- 1
 ...
 box.stat.ERROR.total
 ---
@@ -59,7 +59,7 @@ box.stat.REPLACE.total
 ...
 box.stat.SELECT.total
 ---
-- 5
+- 4
 ...
 -- check exceptions
 space:get('Impossible value')
@@ -118,7 +118,7 @@ box.stat.REPLACE.total
 ...
 box.stat.SELECT.total
 ---
-- 2
+- 1
 ...
 box.stat.ERROR.total
 ---
-- 
2.11.0

[PATCH v2 4/4] sql: check read access while executing SQL query

[Kirill Yukhin]

Since SQL front-end is not using box API,
no checkes for read access are performed by VDBE engine.
Add check to IteratorOpen op-code to make sure that read
privilege exists for given space.
Note, that there's is no need to perform DML/DDL checkes as
they're performed by Tarantool's core.

@TarantoolBot document
Title: Document behaviour of SQL in presence of
read access restrictions. Need to clarify, that
if there's no read access to the space, then not
only SELECT statements will fail, but also those DML
which implies reading from spaces indirectly, e.g.:
  UPDATE t1 SET a=2 WHERE b=3;

Closes #2362
---
 src/box/sql/vdbe.c                             |   5 ++
 test/sql/gh-2362-select-access-rights.result   | 110 +++++++++++++++++++++++++
 test/sql/gh-2362-select-access-rights.test.lua |  42 ++++++++++
 3 files changed, 157 insertions(+)
 create mode 100644 test/sql/gh-2362-select-access-rights.result
 create mode 100644 test/sql/gh-2362-select-access-rights.test.lua

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7c1015c..10b58b4 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3099,6 +3099,11 @@ case OP_IteratorOpen:
 	else
 		space = aMem[pOp->p3].u.p;
 	assert(space != NULL);
+	if (access_check_space(space, PRIV_R) != 0) {
+		rc = SQL_TARANTOOL_ERROR;
+		goto abort_due_to_error;
+	}
+
 	struct index *index = space_index(space, pOp->p2);
 	assert(index != NULL);
 	assert(pOp->p1 >= 0);
diff --git a/test/sql/gh-2362-select-access-rights.result b/test/sql/gh-2362-select-access-rights.result
new file mode 100644
index 0000000..b42ee36
--- /dev/null
+++ b/test/sql/gh-2362-select-access-rights.result
@@ -0,0 +1,110 @@
+test_run = require('test_run').new()
+---
+...
+engine = test_run:get_cfg('engine')
+---
+...
+nb = require('net.box')
+---
+...
+box.sql.execute("PRAGMA sql_default_engine='"..engine.."'")
+---
+...
+box.sql.execute("CREATE TABLE t1 (s1 INT PRIMARY KEY, s2 INT UNIQUE);")
+---
+...
+box.sql.execute("CREATE TABLE t2 (s1 INT PRIMARY KEY);")
+---
+...
+box.sql.execute("INSERT INTO t1 VALUES (1, 1);")
+---
+...
+box.sql.execute("INSERT INTO t2 VALUES (1);")
+---
+...
+box.schema.user.grant('guest','read', 'space', 'T1')
+---
+...
+c = nb.connect(box.cfg.listen)
+---
+...
+c:execute("SELECT * FROM t1;")
+---
+- metadata:
+  - name: S1
+  - name: S2
+  rows:
+  - [1, 1]
+...
+box.schema.user.revoke('guest','read', 'space', 'T1')
+---
+...
+c = nb.connect(box.cfg.listen)
+---
+...
+c:execute("SELECT * FROM t1;")
+---
+- error: 'Failed to execute SQL statement: Read access to space ''T1'' is denied for
+    user ''guest'''
+...
+box.schema.user.grant('guest','read', 'space', 'T2')
+---
+...
+c = nb.connect(box.cfg.listen)
+---
+...
+c:execute('SELECT * FROM t1, t2 WHERE t1.s1 = t2.s1')
+---
+- error: 'Failed to execute SQL statement: Read access to space ''T1'' is denied for
+    user ''guest'''
+...
+box.sql.execute("CREATE VIEW v AS SELECT * FROM t1")
+---
+...
+box.schema.user.grant('guest','read', 'space', 'V')
+---
+...
+v = nb.connect(box.cfg.listen)
+---
+...
+c:execute('SELECT * FROM v')
+---
+- error: 'Failed to execute SQL statement: Read access to space ''T1'' is denied for
+    user ''guest'''
+...
+box.sql.execute('CREATE TABLE t3 (s1 INT PRIMARY KEY, fk INT, FOREIGN KEY (fk) REFERENCES t1(s2))')
+---
+...
+box.schema.user.grant('guest','read','space', 'T3')
+---
+...
+v = nb.connect(box.cfg.listen)
+---
+...
+c:execute('INSERT INTO t3 VALUES (1, 1)')
+---
+- error: 'Failed to execute SQL statement: Read access to space ''T1'' is denied for
+    user ''guest'''
+...
+-- Cleanup
+box.schema.user.revoke('guest','read','space', 'V')
+---
+...
+box.schema.user.revoke('guest','read','space', 'T2')
+---
+...
+box.schema.user.revoke('guest','read','space', 'T3')
+---
+...
+box.sql.execute('DROP VIEW v')
+---
+...
+box.sql.execute('DROP TABLE t3')
+---
+...
+box.sql.execute('DROP TABLE t2')
+---
+...
+box.sql.execute("DROP TABLE t1")
+---
+...
diff --git a/test/sql/gh-2362-select-access-rights.test.lua b/test/sql/gh-2362-select-access-rights.test.lua
new file mode 100644
index 0000000..9c50e19
--- /dev/null
+++ b/test/sql/gh-2362-select-access-rights.test.lua
@@ -0,0 +1,42 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+nb = require('net.box')
+
+box.sql.execute("PRAGMA sql_default_engine='"..engine.."'")
+box.sql.execute("CREATE TABLE t1 (s1 INT PRIMARY KEY, s2 INT UNIQUE);")
+box.sql.execute("CREATE TABLE t2 (s1 INT PRIMARY KEY);")
+box.sql.execute("INSERT INTO t1 VALUES (1, 1);")
+box.sql.execute("INSERT INTO t2 VALUES (1);")
+
+box.schema.user.grant('guest','read', 'space', 'T1')
+c = nb.connect(box.cfg.listen)
+c:execute("SELECT * FROM t1;")
+
+box.schema.user.revoke('guest','read', 'space', 'T1')
+c = nb.connect(box.cfg.listen)
+c:execute("SELECT * FROM t1;")
+
+box.schema.user.grant('guest','read', 'space', 'T2')
+c = nb.connect(box.cfg.listen)
+c:execute('SELECT * FROM t1, t2 WHERE t1.s1 = t2.s1')
+
+box.sql.execute("CREATE VIEW v AS SELECT * FROM t1")
+
+box.schema.user.grant('guest','read', 'space', 'V')
+v = nb.connect(box.cfg.listen)
+c:execute('SELECT * FROM v')
+
+box.sql.execute('CREATE TABLE t3 (s1 INT PRIMARY KEY, fk INT, FOREIGN KEY (fk) REFERENCES t1(s2))')
+box.schema.user.grant('guest','read','space', 'T3')
+v = nb.connect(box.cfg.listen)
+c:execute('INSERT INTO t3 VALUES (1, 1)')
+
+-- Cleanup
+box.schema.user.revoke('guest','read','space', 'V')
+box.schema.user.revoke('guest','read','space', 'T2')
+box.schema.user.revoke('guest','read','space', 'T3')
+
+box.sql.execute('DROP VIEW v')
+box.sql.execute('DROP TABLE t3')
+box.sql.execute('DROP TABLE t2')
+box.sql.execute("DROP TABLE t1")
-- 
2.11.0

Re: [PATCH v2 1/4] schema: refactor space_cache API

[Vladimir Davydov]

On Thu, Oct 25, 2018 at 11:17:09AM +0300, Kirill Yukhin wrote:
> Remove function which deletes from cache, making replace
> more general: it might be used for both insertions,
> deletions and replaces. Also, put assert on equality
> of space pointer found in cache to old one into
> replace routine.

I guess this refactoring should be pushed to 1.10-features, right?

> ---
>  src/box/alter.cc  | 26 ++++++++++----------------
>  src/box/schema.cc | 52 +++++++++++++++++++++++++---------------------------
>  src/box/schema.h  |  9 ++-------
>  3 files changed, 37 insertions(+), 50 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index de3943c..79ff589 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1610,8 +1604,7 @@ on_drop_view_rollback(struct trigger *trigger, void *event)
>   *
>   * - space cache should be updated, and changes in the space
>   *   cache should be reflected in Lua bindings
> - *   (this is done in space_cache_replace() and
> - *   space_cache_delete())
> + *   (this is done in space_cache_replace())

This comment is obsolete. Lua bindings are updated by an on_alter_space
trigger callback. Please fix.

>   *
>   * - the space which is changed should be rebuilt according
>   *   to the nature of the modification, i.e. indexes added/dropped,
> @@ -1695,7 +1688,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
>  		 * cache right away to achieve linearisable
>  		 * execution on a replica.
>  		 */
> -		(void) space_cache_replace(space);
> +		(void) space_cache_replace(NULL, space);

(void) is not needed here anymore.

>  		/*
>  		 * Do not forget to update schema_version right after
>  		 * inserting the space to the space_cache, since no
> @@ -447,8 +446,7 @@ schema_free(void)
>  
>  		struct space *space = (struct space *)
>  				mh_i32ptr_node(spaces, i)->val;
> -		space_cache_delete(space_id(space));
> -		space_delete(space);
> +		space_cache_replace(space, NULL);

space_cache_replace(), just like its predecessor space_cache_delete(),
doesn't delete the space so space_delete() must remain.

>  	}
>  	mh_i32ptr_delete(spaces);
>  	while (mh_size(funcs) > 0) {
> diff --git a/src/box/schema.h b/src/box/schema.h
> index 264c16b..05f32de 100644
> --- a/src/box/schema.h
> +++ b/src/box/schema.h
> @@ -134,14 +134,9 @@ space_cache_find_xc(uint32_t id)
>  /**
>   * Update contents of the space cache.  Typically the new space is
>   * an altered version of the original space.
> - * Returns the old space, if any.

What's the old and new spaces are for? Please improve the comment.

>   */
> -struct space *
> -space_cache_replace(struct space *space);
> -
> -/** Delete a space from the space cache. */
> -struct space *
> -space_cache_delete(uint32_t id);
> +void
> +space_cache_replace(struct space *old_space, struct space *new_space);
>  
>  void
>  schema_init();

Re: [PATCH v2 1/4] schema: refactor space_cache API

[Kirill Yukhin]

Hello,

Incremental patch in the bottom. My answers are inlines.

On 25 Oct 17:48, Vladimir Davydov wrote:
> On Thu, Oct 25, 2018 at 11:17:09AM +0300, Kirill Yukhin wrote:
> > Remove function which deletes from cache, making replace
> > more general: it might be used for both insertions,
> > deletions and replaces. Also, put assert on equality
> > of space pointer found in cache to old one into
> > replace routine.
> 
> I guess this refactoring should be pushed to 1.10-features, right?

It's up to you.

> > ---
> >  src/box/alter.cc  | 26 ++++++++++----------------
> >  src/box/schema.cc | 52 +++++++++++++++++++++++++---------------------------
> >  src/box/schema.h  |  9 ++-------
> >  3 files changed, 37 insertions(+), 50 deletions(-)
> > 
> > diff --git a/src/box/alter.cc b/src/box/alter.cc
> > index de3943c..79ff589 100644
> > --- a/src/box/alter.cc
> > +++ b/src/box/alter.cc
> > @@ -1610,8 +1604,7 @@ on_drop_view_rollback(struct trigger *trigger, void *event)
> >   *
> >   * - space cache should be updated, and changes in the space
> >   *   cache should be reflected in Lua bindings
> > - *   (this is done in space_cache_replace() and
> > - *   space_cache_delete())
> > + *   (this is done in space_cache_replace())
> 
> This comment is obsolete. Lua bindings are updated by an on_alter_space
> trigger callback. Please fix.

Removed Lua mention.

> >   *
> >   * - the space which is changed should be rebuilt according
> >   *   to the nature of the modification, i.e. indexes added/dropped,
> > @@ -1695,7 +1688,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
> >  		 * cache right away to achieve linearisable
> >  		 * execution on a replica.
> >  		 */
> > -		(void) space_cache_replace(space);
> > +		(void) space_cache_replace(NULL, space);
> 
> (void) is not needed here anymore.

Done.

> >  		/*
> >  		 * Do not forget to update schema_version right after
> >  		 * inserting the space to the space_cache, since no
> > @@ -447,8 +446,7 @@ schema_free(void)
> >  
> >  		struct space *space = (struct space *)
> >  				mh_i32ptr_node(spaces, i)->val;
> > -		space_cache_delete(space_id(space));
> > -		space_delete(space);
> > +		space_cache_replace(space, NULL);
> 
> space_cache_replace(), just like its predecessor space_cache_delete(),
> doesn't delete the space so space_delete() must remain.

Reverted.

> >  	}
> >  	mh_i32ptr_delete(spaces);
> >  	while (mh_size(funcs) > 0) {
> > diff --git a/src/box/schema.h b/src/box/schema.h
> > index 264c16b..05f32de 100644
> > --- a/src/box/schema.h
> > +++ b/src/box/schema.h
> > @@ -134,14 +134,9 @@ space_cache_find_xc(uint32_t id)
> >  /**
> >   * Update contents of the space cache.  Typically the new space is
> >   * an altered version of the original space.
> > - * Returns the old space, if any.
> 
> What's the old and new spaces are for? Please improve the comment.
Done.

> >   */
> > -struct space *
> > -space_cache_replace(struct space *space);
> > -
> > -/** Delete a space from the space cache. */
> > -struct space *
> > -space_cache_delete(uint32_t id);
> > +void
> > +space_cache_replace(struct space *old_space, struct space *new_space);
> >  
> >  void
> >  schema_init();

--
Regards, Kirill Yukhin

index 79ff589..986d4da 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1602,9 +1602,7 @@ on_drop_view_rollback(struct trigger *trigger, void *event)
  * Generally, whenever a data dictionary change occurs
  * 2 things should be done:
  *
- * - space cache should be updated, and changes in the space
- *   cache should be reflected in Lua bindings
- *   (this is done in space_cache_replace())
+ * - space cache should be updated
  *
  * - the space which is changed should be rebuilt according
  *   to the nature of the modification, i.e. indexes added/dropped,
@@ -1688,7 +1686,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
                 * cache right away to achieve linearisable
                 * execution on a replica.
                 */
-          (void) space_cache_replace(NULL, space);
+               space_cache_replace(NULL, space);
                /*
                 * Do not forget to update schema_version right after
                 * inserting the space to the space_cache, since no
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 08457a4..87a5c41 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -446,6 +446,7 @@ schema_free(void)

                struct space *space = (struct space *)
                                mh_i32ptr_node(spaces, i)->val;
+               space_delete(space);
                space_cache_replace(space, NULL);
        }
        mh_i32ptr_delete(spaces);
diff --git a/src/box/schema.h b/src/box/schema.h
index 05f32de..66ab3bc 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -132,8 +132,8 @@ space_cache_find_xc(uint32_t id)
 }

 /**
- * Update contents of the space cache.  Typically the new space is
- * an altered version of the original space.
+ * Update contents of the space caches with new space pointer.
+ * Old space pointer must exist in the caches.
  */
 void
 space_cache_replace(struct space *old_space, struct space *new_space);

Re: [PATCH v2 1/4] schema: refactor space_cache API

[Vladimir Davydov]

Pushed to 1.10-features

Re: [PATCH v2 2/4] schema: add space names cache

[Vladimir Davydov]

On Thu, Oct 25, 2018 at 11:17:10AM +0300, Kirill Yukhin wrote:
> Since SQL is heavily using name -> space mapping,
> introduce (instead of scanning _space space) dedicated
> cache, which maps space name to space pointer.
> ---
>  src/box/schema.cc | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  src/box/schema.h  |  8 ++++++++
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/schema.cc b/src/box/schema.cc
> index 08457a46..b931fdf 100644
> --- a/src/box/schema.cc
> +++ b/src/box/schema.cc
> @@ -55,6 +55,7 @@
>  
>  /** All existing spaces. */
>  static struct mh_i32ptr_t *spaces;
> +static struct mh_strnptr_t *spaces_by_name;
>  static struct mh_i32ptr_t *funcs;
>  static struct mh_strnptr_t *funcs_by_name;
>  static struct mh_i32ptr_t *sequences;
> @@ -95,6 +96,17 @@ space_by_id(uint32_t id)
>  	return (struct space *) mh_i32ptr_node(spaces, space)->val;
>  }
>  
> +/** Return space by its name */
> +struct space *
> +space_by_name(const char *name)
> +{
> +	mh_int_t space = mh_strnptr_find_inp(spaces_by_name, name,
> +					     strlen(name));
> +	if (space == mh_end(spaces_by_name))
> +		return NULL;
> +	return (struct space *) mh_strnptr_node(spaces_by_name, space)->val;
> +}
> +
>  /** Return current schema version */
>  uint32_t
>  box_schema_version()
> @@ -165,7 +177,25 @@ void
>  space_cache_replace(struct space *old_space, struct space *new_space)
>  {
>  	assert(new_space != NULL || old_space != NULL);
> +	struct space *old_space_by_n = NULL;
>  	if (new_space != NULL) {
> +		/*
> +		 * If replacing is performed and space name was
> +		 * changed, then need to explicitly remove old
> +		 * entry from spaces cache.
> +		 * NB: Since space_id never changed - no need
> +		 * to do so for space_id cache.
> +		 */
> +		if (old_space != NULL && strcmp(space_name(old_space),
> +						new_space->def->name) != 0) {
> +			const char *name = space_name(old_space);
> +			mh_int_t k = mh_strnptr_find_inp(spaces_by_name, name,
> +							 strlen(name));
> +			assert(k != mh_end(spaces_by_name));
> +			mh_strnptr_del(spaces_by_name, k, NULL);
> +			old_space_by_n = (struct space *)mh_strnptr_node(spaces_by_name,
> +									 k)->val;
> +		}
>  		const struct mh_i32ptr_node_t node_p = { space_id(new_space),
>  							 new_space };
>  		struct mh_i32ptr_node_t old, *p_old = &old;
> @@ -174,7 +204,23 @@ space_cache_replace(struct space *old_space, struct space *new_space)
>  			panic_syserror("Out of memory for the data "
>  				       "dictionary cache.");
>  		}
> -		assert((struct space *)p_old->val == old_space);
> +		const char *name = space_name(new_space);
> +		uint32_t name_len = strlen(name);
> +		uint32_t hash = mh_strn_hash(name, name_len);
> +		const struct mh_strnptr_node_t node_s = { name, name_len, hash,
> +							  new_space };
> +		struct mh_strnptr_node_t old_s, *p_old_s = &old_s;
> +		k = mh_strnptr_put(spaces_by_name, &node_s, &p_old_s, NULL);
> +		if (k == mh_end(spaces_by_name)) {
> +			panic_syserror("Out of memory for the data "
> +				       "dictionary cache.");
> +		}
> +		assert(old_space_by_n == NULL || p_old_s == NULL);
> +		if(old_space_by_n == NULL && p_old != NULL) {
                                             ^^^^^
Apparently, it should be p_old_s here (with _s), but when I tried to fix
that, sql/view started to fail on the assertion checking old_space_by_n
below. Turned out there was a bug in on_replace_dd_space (see the patch
below for more details). I fixed it and then pushed this patch (after
some code cleanup) to 2.1. I wonder if it really was a typo or you
simply didn't want to dig deeper...

> +			assert(p_old->val == p_old_s->val);
> +			old_space_by_n = (struct space *)p_old->val;
> +		}
> +		assert((struct space*)old_space_by_n == old_space);
>  	} else {
>  		mh_int_t k = mh_i32ptr_find(spaces, space_id(old_space), NULL);
>  		assert(k != mh_end(spaces));

From d792fd2664cdbf87237253f96c96d94e833e822c Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Date: Fri, 26 Oct 2018 23:10:50 +0300
Subject: [PATCH] alter: install space commit/rollback triggers before
 preparing sql view

sql_compile_view() may fail, in which case the space will never be
deleted from (in case of space creation) or inserted back into (in case
of space drop) the space cache, because commit/rollback triggers, which
are supposed to do the job, are only installed after preparing a view.
Fix this by installing triggers before sql_compile_view().

No need to write a test as without this commit sql/view test will
crash after applying the next commit (the one that introduces space
name cache).

Fixes commit dc358cb01428 ("sql: rework VIEW internals").

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 986d4daf..a6bb5a0f 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1702,6 +1702,12 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 		 * so it's safe to simply drop the space on
 		 * rollback.
 		 */
+		struct trigger *on_commit =
+			txn_alter_trigger_new(on_create_space_commit, space);
+		txn_on_commit(txn, on_commit);
+		struct trigger *on_rollback =
+			txn_alter_trigger_new(on_create_space_rollback, space);
+		txn_on_rollback(txn, on_rollback);
 		if (def->opts.is_view) {
 			struct Select *select = sql_view_compile(sql_get(),
 								 def->opts.sql);
@@ -1732,12 +1738,6 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 			txn_on_rollback(txn, on_rollback_view);
 			select_guard.is_active = false;
 		}
-		struct trigger *on_commit =
-			txn_alter_trigger_new(on_create_space_commit, space);
-		txn_on_commit(txn, on_commit);
-		struct trigger *on_rollback =
-			txn_alter_trigger_new(on_create_space_rollback, space);
-		txn_on_rollback(txn, on_rollback);
 	} else if (new_tuple == NULL) { /* DELETE */
 		access_check_ddl(old_space->def->name, old_space->def->id,
 				 old_space->def->uid, SC_SPACE, PRIV_D, true);
@@ -1788,6 +1788,9 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 		struct trigger *on_commit =
 			txn_alter_trigger_new(on_drop_space_commit, old_space);
 		txn_on_commit(txn, on_commit);
+		struct trigger *on_rollback =
+			txn_alter_trigger_new(on_drop_space_rollback, old_space);
+		txn_on_rollback(txn, on_rollback);
 		if (old_space->def->opts.is_view) {
 			struct Select *select =
 				sql_view_compile(sql_get(),
@@ -1807,10 +1810,6 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 			txn_on_rollback(txn, on_rollback_view);
 			select_guard.is_active = false;
 		}
-		struct trigger *on_rollback =
-			txn_alter_trigger_new(on_drop_space_rollback,
-					      old_space);
-		txn_on_rollback(txn, on_rollback);
 	} else { /* UPDATE, REPLACE */
 		assert(old_space != NULL && new_tuple != NULL);
 		struct space_def *def =

Re: [PATCH v2 3/4] sql: use space_by_name in SQL

[Vladimir Davydov]

On Thu, Oct 25, 2018 at 11:17:11AM +0300, Kirill Yukhin wrote:
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index 3d72e31..7c28437 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -47,18 +47,17 @@ sql_alter_table_rename(struct Parse *parse, struct SrcList *src_tab,
>  	if (new_name == NULL)
>  		goto exit_rename_table;
>  	/* Check that new name isn't occupied by another table. */
> -	uint32_t space_id = box_space_id_by_name(new_name, strlen(new_name));
> -	if (space_id != BOX_ID_NIL) {
> +	struct space *space = space_by_name(new_name);
> +	if (space != NULL) {
>  		diag_set(ClientError, ER_SPACE_EXISTS, new_name);
>  		goto tnt_error;
>  	}
>  	const char *tbl_name = src_tab->a[0].zName;
> -	space_id = box_space_id_by_name(tbl_name, strlen(tbl_name));
> -	if (space_id == BOX_ID_NIL) {
> +	space = space_by_name(tbl_name);
> +	if (space == NULL) {
>  		diag_set(ClientError, ER_NO_SUCH_SPACE, tbl_name);
>  		goto tnt_error;
>  	}
> -	struct space *space = space_by_id(space_id);
>  	assert(space != NULL);

Pointless assertion.

>  	if (space->def->opts.is_view) {
>  		diag_set(ClientError, ER_ALTER_SPACE, tbl_name,
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index 674d53d..ad068a54 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -1114,16 +1114,15 @@ sqlite3Analyze(Parse * pParse, Token * pName)
>  		/* Form 2:  Analyze table named */
>  		char *z = sqlite3NameFromToken(db, pName);
>  		if (z != NULL) {
> -			uint32_t space_id = box_space_id_by_name(z, strlen(z));
> -			if (space_id != BOX_ID_NIL) {
> -				struct space *sp = space_by_id(space_id);
> -				assert(sp != NULL);
> -				if (sp->def->opts.is_view) {
> +			struct space *space = space_by_name(z);
> +			if (space != NULL) {
> +				assert(space != NULL);

Another one.

> +				if (space->def->opts.is_view) {
>  					sqlite3ErrorMsg(pParse, "VIEW isn't "\
>  							"allowed to be "\
>  							"analyzed");
>  				} else {
> -					vdbe_emit_analyze_table(pParse, sp);
> +					vdbe_emit_analyze_table(pParse, space);
>  				}
>  			} else {
>  				diag_set(ClientError, ER_NO_SUCH_SPACE, z);
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index 8f6c76f..72e0575 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -36,15 +36,14 @@
>  #include "tarantoolInt.h"
>  
>  struct Table *
> -sql_lookup_table(struct Parse *parse, struct SrcList_item *tbl_name)
> +sql_lookup_table(struct Parse *parse, struct SrcList_item *src_name)
>  {
> -	assert(tbl_name != NULL);
> -	assert(tbl_name->pTab == NULL);
> -	uint32_t space_id = box_space_id_by_name(tbl_name->zName,
> -						 strlen(tbl_name->zName));
> -	struct space *space = space_by_id(space_id);
> -	if (space_id == BOX_ID_NIL || space == NULL) {
> -		sqlite3ErrorMsg(parse, "no such table: %s", tbl_name->zName);
> +	assert(src_name != NULL);
> +	assert(src_name->pTab == NULL);
> +	const char *name = src_name->zName;
> +	struct space *space = space_by_name(name);
> +	if (space == NULL) {
> +		sqlite3ErrorMsg(parse, "no such table: %s", name);

I hate it when one tries to squeeze some unrelated "cleanup", like
this variable renaming, in a patch that doesn't really need it.
This complicates review and clutters the history. I reverted this
part, removed useless assertions, and pushed the patch to 2.1.

>  		return NULL;
>  	}
>  	assert(space != NULL);

Re: [PATCH v2 4/4] sql: check read access while executing SQL query

[Vladimir Davydov]

On Thu, Oct 25, 2018 at 11:17:12AM +0300, Kirill Yukhin wrote:
> Since SQL front-end is not using box API,
> no checkes for read access are performed by VDBE engine.
> Add check to IteratorOpen op-code to make sure that read
> privilege exists for given space.
> Note, that there's is no need to perform DML/DDL checkes as
> they're performed by Tarantool's core.
> 
> @TarantoolBot document
> Title: Document behaviour of SQL in presence of
> read access restrictions. Need to clarify, that

This line is like 50 characters long. Why? The limit for commit messages
is 72. Doesn't emacs now that? Vim does.

> if there's no read access to the space, then not
> only SELECT statements will fail, but also those DML
> which implies reading from spaces indirectly, e.g.:
>   UPDATE t1 SET a=2 WHERE b=3;
> 
> Closes #2362
> ---
>  src/box/sql/vdbe.c                             |   5 ++
>  test/sql/gh-2362-select-access-rights.result   | 110 +++++++++++++++++++++++++
>  test/sql/gh-2362-select-access-rights.test.lua |  42 ++++++++++
>  3 files changed, 157 insertions(+)
>  create mode 100644 test/sql/gh-2362-select-access-rights.result
>  create mode 100644 test/sql/gh-2362-select-access-rights.test.lua

Pushed to 2.1

[tarantool-patches] Re: [PATCH v2 2/4] schema: add space names cache

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [18/10/29 20:25]:
> >  		const struct mh_i32ptr_node_t node_p = { space_id(new_space),
> >  							 new_space };
> >  		struct mh_i32ptr_node_t old, *p_old = &old;

In some places we use pold, in other p_old, in others old_p, and
in all of these places p means "pointer".

Let's agree once and for all that it's p_old, p_space, p_name, etc.


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