* [PATCH v2 1/4] schema: refactor space_cache API
2018-10-25 8:17 [PATCH v2 0/4] Check read access while executing SQL query Kirill Yukhin
@ 2018-10-25 8:17 ` Kirill Yukhin
2018-10-25 14:48 ` Vladimir Davydov
2018-10-25 8:17 ` [PATCH v2 2/4] schema: add space names cache Kirill Yukhin
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Kirill Yukhin @ 2018-10-25 8:17 UTC (permalink / raw)
To: vdavydov.dev; +Cc: tarantool-patches, 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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 2/4] schema: add space names cache
2018-10-25 8:17 [PATCH v2 0/4] Check read access while executing SQL query Kirill Yukhin
2018-10-25 8:17 ` [PATCH v2 1/4] schema: refactor space_cache API Kirill Yukhin
@ 2018-10-25 8:17 ` Kirill Yukhin
2018-10-26 20:55 ` Vladimir Davydov
2018-10-25 8:17 ` [PATCH v2 3/4] sql: use space_by_name in SQL Kirill Yukhin
2018-10-25 8:17 ` [PATCH v2 4/4] sql: check read access while executing SQL query Kirill Yukhin
3 siblings, 1 reply; 12+ messages in thread
From: Kirill Yukhin @ 2018-10-25 8:17 UTC (permalink / raw)
To: vdavydov.dev; +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.
---
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 3/4] sql: use space_by_name in SQL
2018-10-25 8:17 [PATCH v2 0/4] Check read access while executing SQL query Kirill Yukhin
2018-10-25 8:17 ` [PATCH v2 1/4] schema: refactor space_cache API Kirill Yukhin
2018-10-25 8:17 ` [PATCH v2 2/4] schema: add space names cache Kirill Yukhin
@ 2018-10-25 8:17 ` Kirill Yukhin
2018-10-26 21:00 ` Vladimir Davydov
2018-10-25 8:17 ` [PATCH v2 4/4] sql: check read access while executing SQL query Kirill Yukhin
3 siblings, 1 reply; 12+ messages in thread
From: Kirill Yukhin @ 2018-10-25 8:17 UTC (permalink / raw)
To: vdavydov.dev; +Cc: tarantool-patches, 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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 4/4] sql: check read access while executing SQL query
2018-10-25 8:17 [PATCH v2 0/4] Check read access while executing SQL query Kirill Yukhin
` (2 preceding siblings ...)
2018-10-25 8:17 ` [PATCH v2 3/4] sql: use space_by_name in SQL Kirill Yukhin
@ 2018-10-25 8:17 ` Kirill Yukhin
2018-10-26 21:04 ` Vladimir Davydov
3 siblings, 1 reply; 12+ messages in thread
From: Kirill Yukhin @ 2018-10-25 8:17 UTC (permalink / raw)
To: vdavydov.dev; +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 +++++++++++++++++++++++++
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
^ permalink raw reply [flat|nested] 12+ messages in thread