Tarantool development patches archive
 help / color / mirror / Atom feed
* [tarantool-patches] [PATCH v2 0/2]  sql: check read access while executing SQL query
@ 2018-10-12 11:38 Kirill Yukhin
  2018-10-12 11:38 ` [tarantool-patches] [PATCH v2 1/2] Add space names cache Kirill Yukhin
  2018-10-12 11:38 ` [tarantool-patches] [PATCH v2 2/2] sql: check read access while executing SQL query Kirill Yukhin
  0 siblings, 2 replies; 5+ messages in thread
From: Kirill Yukhin @ 2018-10-12 11:38 UTC (permalink / raw)
  To: korablev; +Cc: tarantool-patches, Kirill Yukhin

This patchset intended to fix checking of read priviledges
while executing SQL queries. To do so, box interface for
fetching space pointer by name was replaced by requesting
(created in 1st patch) dedicated mapping of space name to
space pointer. This allows SQL machinery to read from system
spaces w/o any access checks. Actual check is performed by
IteratorOpen op-code.

https://github.com/tarantool/tarantool/commits/kyukhin/gh-2362-sql-read-access-check                                                                                                        
https://github.com/tarantool/tarantool/issues/2362

Kirill Yukhin (2):
  Add space names cache
  sql: check read access while executing SQL query

 src/box/alter.cc                              |  18 +--
 src/box/schema.cc                             | 123 +++++++++++++++---
 src/box/schema.h                              |  12 +-
 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 ++++++++++++++++
 .../sql/gh-2362-select-access-rights.test.lua |  42 ++++++
 13 files changed, 353 insertions(+), 140 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.19.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tarantool-patches] [PATCH v2 1/2] Add space names cache
  2018-10-12 11:38 [tarantool-patches] [PATCH v2 0/2] sql: check read access while executing SQL query Kirill Yukhin
@ 2018-10-12 11:38 ` Kirill Yukhin
  2018-10-15 10:08   ` Vladimir Davydov
  2018-10-12 11:38 ` [tarantool-patches] [PATCH v2 2/2] sql: check read access while executing SQL query Kirill Yukhin
  1 sibling, 1 reply; 5+ messages in thread
From: Kirill Yukhin @ 2018-10-12 11:38 UTC (permalink / raw)
  To: korablev; +Cc: tarantool-patches, 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.
Update SQL sources thoroughly.
Remove function which deletes from cache, making replace
more general: it might be used for both insertions,
deletions and replaces.
---
 src/box/alter.cc      |  18 ++++---
 src/box/schema.cc     | 123 ++++++++++++++++++++++++++++++++++++------
 src/box/schema.h      |  12 ++++-
 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 +--
 10 files changed, 196 insertions(+), 140 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index de3943c..8f84196 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -810,7 +810,8 @@ 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);
+	struct space *new_space = space_cache_replace(alter->old_space,
+						      alter->new_space);
 	assert(new_space == alter->new_space);
 	(void) new_space;
 	alter_space_delete(alter);
@@ -913,7 +914,8 @@ 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);
+	struct space *old_space = space_cache_replace(alter->new_space,
+						      alter->old_space);
 	(void) old_space;
 	assert(old_space == alter->old_space);
 
@@ -1421,7 +1423,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(space, NULL);
 }
 
 /**
@@ -1447,7 +1449,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));
+	struct space *cached = space_cache_delete(space);
 	(void) cached;
 	assert(cached == space);
 	space_delete(space);
@@ -1610,8 +1612,8 @@ 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(),
+ *   space_cache_delete() and space_name_cache_delete())
  *
  * - the space which is changed should be rebuilt according
  *   to the nature of the modification, i.e. indexes added/dropped,
@@ -1695,7 +1697,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(space, NULL);
 		/*
 		 * Do not forget to update schema_version right after
 		 * inserting the space to the space_cache, since no
@@ -1787,7 +1789,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));
+		struct space *space = space_cache_delete(old_space);
 		/*
 		 * Do not forget to update schema_version right after
 		 * deleting the space from the space_cache, since no
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 2d2be26..f109b34 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()
@@ -160,14 +172,26 @@ space_foreach(int (*func)(struct space *sp, void *udata), void *udata)
 
 /** Delete a space from the space cache and Lua. */
 struct space *
-space_cache_delete(uint32_t id)
+space_cache_delete(struct space *space)
 {
-	mh_int_t k = mh_i32ptr_find(spaces, id, NULL);
+	mh_int_t k = mh_i32ptr_find(spaces, space_id(space), NULL);
 	assert(k != mh_end(spaces));
-	struct space *space = (struct space *)mh_i32ptr_node(spaces, k)->val;
+	struct space *space_by_id = (struct space *)mh_i32ptr_node(spaces,
+								   k)->val;
+	assert(space_by_id == space);
 	mh_i32ptr_del(spaces, k, NULL);
+
+	const char *name = space_name(space);
+	k = mh_strnptr_find_inp(spaces_by_name, name, strlen(name));
+	assert(k != mh_end(spaces_by_name));
+	struct space *space_by_n = (struct space *)mh_strnptr_node(spaces_by_name,
+								   k)->val;
+	assert(space_by_n == space_by_id);
+	(void)space_by_id;
+	mh_strnptr_del(spaces_by_name, k, NULL);
+
 	space_cache_version++;
-	return space;
+	return space_by_n;
 }
 
 /**
@@ -175,17 +199,83 @@ space_cache_delete(uint32_t id)
  * the old space instance, if any, or NULL if it's a new space.
  */
 struct space *
-space_cache_replace(struct space *space)
+space_cache_replace(struct space *new_space, struct space *old_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);
+	struct space *old_space_by_n = NULL;
+	struct space *old_space_by_id = 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: Sincce 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;
+			space_cache_version++;
+		}
+		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.");
+		}
+		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;
+		}
+	} else {
+		mh_int_t k = mh_i32ptr_find(spaces, space_id(old_space), NULL);
+		assert(k != mh_end(spaces));
+		old_space_by_id = (struct space *)mh_i32ptr_node(spaces, k)->val;
+		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++;
-	return p_old ? (struct space *) p_old->val : NULL;
+	return old_space_by_n;
+}
+
+/** Delete a space from the space name cache. */
+struct space *
+space_name_cache_delete(const char *name)
+{
+	mh_int_t k = mh_strnptr_find_inp(spaces_by_name, name, strlen(name));
+	assert(k != mh_end(spaces_by_name));
+	struct space *space = (struct space *)mh_strnptr_node(spaces_by_name,
+							      k)->val;
+	mh_strnptr_del(spaces_by_name, k, NULL);
+	return space;
 }
 
 /** A wrapper around space_new() for data dictionary spaces. */
@@ -220,7 +310,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(space, NULL);
 	if (replace_trigger)
 		trigger_add(&space->on_replace, replace_trigger);
 	if (stmt_begin_trigger)
@@ -303,6 +393,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();
@@ -431,7 +522,7 @@ schema_init()
 		});
 		RLIST_HEAD(key_list);
 		struct space *space = space_new_xc(def, &key_list);
-		space_cache_replace(space);
+		space_cache_replace(space, NULL);
 		init_system_space(space);
 		trigger_run_xc(&on_alter_space, space);
 	}
@@ -447,10 +538,12 @@ schema_free(void)
 
 		struct space *space = (struct space *)
 				mh_i32ptr_node(spaces, i)->val;
-		space_cache_delete(space_id(space));
+		space_cache_delete(space);
+		space_name_cache_delete(space_name(space));
 		space_delete(space);
 	}
 	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 264c16b..c54f5a8 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();
 
@@ -137,11 +145,11 @@ space_cache_find_xc(uint32_t id)
  * Returns the old space, if any.
  */
 struct space *
-space_cache_replace(struct space *space);
+space_cache_replace(struct space *new_space, struct space *old_space);
 
 /** Delete a space from the space cache. */
 struct space *
-space_cache_delete(uint32_t id);
+space_cache_delete(struct space *space);
 
 void
 schema_init();
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..ad068a5 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.19.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tarantool-patches] [PATCH v2 2/2] sql: check read access while executing SQL query
  2018-10-12 11:38 [tarantool-patches] [PATCH v2 0/2] sql: check read access while executing SQL query Kirill Yukhin
  2018-10-12 11:38 ` [tarantool-patches] [PATCH v2 1/2] Add space names cache Kirill Yukhin
@ 2018-10-12 11:38 ` Kirill Yukhin
  1 sibling, 0 replies; 5+ messages in thread
From: Kirill Yukhin @ 2018-10-12 11:38 UTC (permalink / raw)
  To: korablev; +Cc: tarantool-patches, 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 ++++++++++++++++++
 .../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.19.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [tarantool-patches] [PATCH v2 1/2] Add space names cache
  2018-10-12 11:38 ` [tarantool-patches] [PATCH v2 1/2] Add space names cache Kirill Yukhin
@ 2018-10-15 10:08   ` Vladimir Davydov
  2018-10-24 13:34     ` Kirill Yukhin
  0 siblings, 1 reply; 5+ messages in thread
From: Vladimir Davydov @ 2018-10-15 10:08 UTC (permalink / raw)
  To: Kirill Yukhin; +Cc: korablev, tarantool-patches

On Fri, Oct 12, 2018 at 02:38:18PM +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.
> Update SQL sources thoroughly.
> Remove function which deletes from cache, making replace
> more general: it might be used for both insertions,
> deletions and replaces.

Overall, I like the reasoning behind the patch, but IMO before doing
this we need to cleanup the mess we have with schema object lookup
functions.

See, we have <class>_by_name and <class>_by_id which look up an object
of the corresponding class by the given name or id. Some of them set
diag if the object isn't found, others not. I find it ugly. I think that
either all or none of them should set diag.

Next, we have <class>_cache_find and <class>_find, which typically work
as wrappers around <class>_by_id setting diag if the object isn't found.
Sometimes (space_cache_find) they also cache the last returned object in
a static variable. If we make <class>_by_id and <class>_by_name set
diag, we won't be needing most of them. I think that we should only
leave space_cache_find. May be, it's worth renaming it to make its
purpose more obvious.

Please consider doing this cleanup first in a separate patch. It should
be trivial. This should be done on top of 1.10 IMO.

Regarding the patch itself.

 - space_cache_replace should take the old space as the first argument
   and the new space as the second argument, not vice versa. This would
   be consistent with other 'replace' functions we have in Tarantool.

 - Why would you need space_cache_delete and space_name_cache_delete if
   you could use space_cache_replace instead. I mean,

   space_cache_replace(space, NULL); /* deletes the space */

 - I guess that space_cache_replace shouldn't return anything.

 - I think the change of space_cache_replace API should be done in a
   separate patch.

 - IMO you should replace box_space_id_by_name with space_by_name in a
   separate patch.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [tarantool-patches] [PATCH v2 1/2] Add space names cache
  2018-10-15 10:08   ` Vladimir Davydov
@ 2018-10-24 13:34     ` Kirill Yukhin
  0 siblings, 0 replies; 5+ messages in thread
From: Kirill Yukhin @ 2018-10-24 13:34 UTC (permalink / raw)
  To: Vladimir Davydov; +Cc: korablev, tarantool-patches

On 15 Oct 13:08, Vladimir Davydov wrote:
> On Fri, Oct 12, 2018 at 02:38:18PM +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.
> > Update SQL sources thoroughly.
> > Remove function which deletes from cache, making replace
> > more general: it might be used for both insertions,
> > deletions and replaces.
> 
> Overall, I like the reasoning behind the patch, but IMO before doing
> this we need to cleanup the mess we have with schema object lookup
> functions.
> 
> See, we have <class>_by_name and <class>_by_id which look up an object
> of the corresponding class by the given name or id. Some of them set
> diag if the object isn't found, others not. I find it ugly. I think that
> either all or none of them should set diag.
> 
> Next, we have <class>_cache_find and <class>_find, which typically work
> as wrappers around <class>_by_id setting diag if the object isn't found.
> Sometimes (space_cache_find) they also cache the last returned object in
> a static variable. If we make <class>_by_id and <class>_by_name set
> diag, we won't be needing most of them. I think that we should only
> leave space_cache_find. May be, it's worth renaming it to make its
> purpose more obvious.
> 
> Please consider doing this cleanup first in a separate patch. It should
> be trivial. This should be done on top of 1.10 IMO.
We've discussed that verbally and came to the conclusion that its not
about time to do so. Need to evict EH first.


> Regarding the patch itself.
> 
>  - space_cache_replace should take the old space as the first argument
>    and the new space as the second argument, not vice versa. This would
>    be consistent with other 'replace' functions we have in Tarantool.

Done.

>  - Why would you need space_cache_delete and space_name_cache_delete if
>    you could use space_cache_replace instead. I mean,
> 
>    space_cache_replace(space, NULL); /* deletes the space */

Done.

>  - I guess that space_cache_replace shouldn't return anything.

Done.

>  - I think the change of space_cache_replace API should be done in a
>    separate patch.

Done.

>  - IMO you should replace box_space_id_by_name with space_by_name in a
>    separate patch.

Done.

I'll re-send updated 4-patch patchset as v2.

--
Regards, Kirill Yukhin

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-10-24 13:34 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-12 11:38 [tarantool-patches] [PATCH v2 0/2] sql: check read access while executing SQL query Kirill Yukhin
2018-10-12 11:38 ` [tarantool-patches] [PATCH v2 1/2] Add space names cache Kirill Yukhin
2018-10-15 10:08   ` Vladimir Davydov
2018-10-24 13:34     ` Kirill Yukhin
2018-10-12 11:38 ` [tarantool-patches] [PATCH v2 2/2] sql: check read access while executing SQL query Kirill Yukhin

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