From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4E9FF46970E for ; Sat, 21 Dec 2019 20:59:13 +0300 (MSK) References: <71b3e0f017780fe6efcf2d1ccb5e4d46f0d666cd.1576743850.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: <1fcb44f4-9de4-1360-7dd1-a503d33071ce@tarantool.org> Date: Sat, 21 Dec 2019 18:59:10 +0100 MIME-Version: 1.0 In-Reply-To: <71b3e0f017780fe6efcf2d1ccb5e4d46f0d666cd.1576743850.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 3/3] box: add SQL settings to _session_settings List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org Thanks for the patch! I've pushed my review fixes on top of this commit. See it below and on the branch. If you agree, then squash. Otherwise lets discuss. In my commit you can find inlined explanations about some changes. ================================================================================ commit 0cb8d1f6a337a1eb912ada031a6393bc22616e2e Author: Vladislav Shpilevoy Date: Sat Dec 21 18:08:06 2019 +0100 Review fixes 3/3 diff --git a/src/box/errcode.h b/src/box/errcode.h index c660b1c70..11894fccc 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -258,6 +258,7 @@ struct errcode_record { /*203 */_(ER_BOOTSTRAP_READONLY, "Trying to bootstrap a local read-only instance as master") \ /*204 */_(ER_SQL_FUNC_WRONG_RET_COUNT, "SQL expects exactly one argument returned from %s, got %d")\ /*205 */_(ER_FUNC_INVALID_RETURN_TYPE, "Function '%s' returned value of invalid type: expected %s got %s") \ + /*206 */_(ER_SESSION_SETTING_INVALID_VALUE, "Session setting %s expected a value of type %s") \ ================================================================================ I've decided that ER_FIELD_TYPE, used before, does not really fit here. Because it produces a not user-friendly message, and because formally speaking in the space format we said that type of the field is 'any'. So it should have accepted everything. Via that new error I've tried to explain to a user the problem a bit deeper. ================================================================================ /* * !IMPORTANT! Please follow instructions at start of the file diff --git a/src/box/session_settings.h b/src/box/session_settings.h index 8166f17f8..25490a7e3 100644 --- a/src/box/session_settings.h +++ b/src/box/session_settings.h @@ -30,9 +30,19 @@ * SUCH DAMAGE. */ -enum session_setting_module { - SESSION_SETTING_MODULE_SQL, - SESSION_SETTING_MODULE_max +/** + * Session has settings. Settings belong to different subsystems, + * such as SQL. Each subsystem registers here its session setting + * type and a set of settings with getter and setter functions. + * The self-registration of modules allows session setting code + * not to depend on all the subsystems. + * + * The types should be ordered in alphabetical order, because the + * type list is used by setting iterators. + */ +enum session_setting_type { + SESSION_SETTING_SQL, + session_setting_type_MAX, }; struct session_setting_module { diff --git a/src/box/sql.c b/src/box/sql.c index f1df55571..cc826177b 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -64,9 +64,14 @@ static const uint32_t default_sql_flags = SQL_ShortColNames | SQL_AutoIndex | SQL_RecTriggers; +extern void +sql_session_settings_init(); + void sql_init() { + sql_session_settings_init(); + default_flags |= default_sql_flags; current_session()->sql_flags |= default_sql_flags; diff --git a/src/box/sql.h b/src/box/sql.h index 3f86568a6..0fa52fc0b 100644 --- a/src/box/sql.h +++ b/src/box/sql.h @@ -33,7 +33,6 @@ #include #include -#include "iterator_type.h" #if defined(__cplusplus) extern "C" { @@ -71,7 +70,6 @@ struct Select; struct Table; struct sql_trigger; struct space_def; -struct tuple_format; /** * Perform parsing of provided expression. This is done by @@ -422,51 +420,6 @@ void vdbe_field_ref_prepare_tuple(struct vdbe_field_ref *field_ref, struct tuple *tuple); - -/** - * Return number of SQL session settings. - * - * @retval Number of SQL session settings. - */ -int -sql_session_settings_count(); - -/** - * Matches given key with SQL settings names and returns tuple - * that contains name and value of the setting. - * - * @param format The format of result tuple. - * @param id The ID to start search setting from. - * @param key The key to find the setting. - * @param type Type of iterator to match key with settings name. - * @param end_id The ID of returned setting. - * @param result Result tuple. - * - * @retval 0 on success. - * @retval -1 on error. - */ -int -sql_session_setting_get(struct tuple_format *format, int id, const char *key, - enum iterator_type type, int *end_id, - struct tuple **result); - -/** - * Set new value to SQL session setting. Value given in MsgPack - * format. Returns tuple that contains name and new value of the - * setting. - * - * @param format Format of result tuple. - * @param id ID of SQL setting to set new value. - * @param value MsgPack contains value to set. - * @param result Result tuple. - * - * @retval 0 on success. - * @retval -1 on error. - */ -int -sql_session_setting_set(struct tuple_format *format, int id, const char *value, - struct tuple **result); - #if defined(__cplusplus) } /* extern "C" { */ #endif diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 995a32e8d..a6e3752fc 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -46,7 +46,6 @@ #include #include "sqlInt.h" #include "vdbeInt.h" -#include "box/tuple.h" #include "tarantoolInt.h" #include "box/box.h" #include "box/ck_constraint.h" @@ -57,6 +56,7 @@ #include "box/schema.h" #include "box/tuple_format.h" #include "box/coll_id_cache.h" +#include "box/session_settings.h" void sql_finish_coding(struct Parse *parse_context) @@ -3272,177 +3272,119 @@ enum { SQL_SESSION_SETTING_VDBE_TRACE, SQL_SESSION_SETTING_WHERE_TRACE, #endif - SQL_SESSION_SETTING_max, + sql_session_setting_MAX, }; -int -sql_session_settings_count() -{ - return SQL_SESSION_SETTING_max; -} +static const char *sql_session_setting_strs[sql_session_setting_MAX] = { + "sql_default_engine", + "sql_defer_foreign_keys", + "sql_full_column_names", +#ifndef NDEBUG + "sql_parser_trace", +#endif + "sql_recursive_triggers", + "sql_reverse_unordered_selects", +#ifndef NDEBUG + "sql_select_trace", + "sql_trace", + "sql_vdbe_addoptrace", + "sql_vdbe_debug", + "sql_vdbe_eqp", + "sql_vdbe_listing", + "sql_vdbe_trace", + "sql_where_trace", +#endif +}; /** * A local structure that allows to establish a connection between - * the name of the parameter, its field type and mask, if it have - * one. + * parameter and its field type and mask, if it has one. */ struct sql_option_metadata { - const char *name; uint32_t field_type; uint32_t mask; }; /** - * Variable that contains names of the SQL session options, their - * field types and mask if they have one or 0 if don't have. + * Variable that contains SQL session option field types and masks + * if they have one or 0 if don't have. * * It is IMPORTANT that these options sorted by name. */ static struct sql_option_metadata sql_session_opts[] = { /** SQL_SESSION_SETTING_DEFAULT_ENGINE */ - {"sql_default_engine", FIELD_TYPE_STRING, 0}, + {FIELD_TYPE_STRING, 0}, /** SQL_SESSION_SETTING_DEFER_FOREIGN_KEYS */ - {"sql_defer_foreign_keys", FIELD_TYPE_BOOLEAN, SQL_DeferFKs}, + {FIELD_TYPE_BOOLEAN, SQL_DeferFKs}, /** SQL_SESSION_SETTING_FULL_COLUMN_NAMES */ - {"sql_full_column_names", FIELD_TYPE_BOOLEAN, SQL_FullColNames}, + {FIELD_TYPE_BOOLEAN, SQL_FullColNames}, #ifndef NDEBUG /** SQL_SESSION_SETTING_PARSER_TRACE */ - {"sql_parser_trace", FIELD_TYPE_BOOLEAN, PARSER_TRACE_FLAG}, + {FIELD_TYPE_BOOLEAN, PARSER_TRACE_FLAG}, #endif /** SQL_SESSION_SETTING_RECURSIVE_TRIGGERS */ - {"sql_recursive_triggers", FIELD_TYPE_BOOLEAN, SQL_RecTriggers}, + {FIELD_TYPE_BOOLEAN, SQL_RecTriggers}, /** SQL_SESSION_SETTING_REVERSE_UNORDERED_SELECTS */ - {"sql_reverse_unordered_selects", FIELD_TYPE_BOOLEAN, SQL_ReverseOrder}, + {FIELD_TYPE_BOOLEAN, SQL_ReverseOrder}, #ifndef NDEBUG /** SQL_SESSION_SETTING_SELECT_TRACE */ - {"sql_select_trace", FIELD_TYPE_BOOLEAN, SQL_SelectTrace}, + {FIELD_TYPE_BOOLEAN, SQL_SelectTrace}, /** SQL_SESSION_SETTING_TRACE */ - {"sql_trace", FIELD_TYPE_BOOLEAN, SQL_SqlTrace}, + {FIELD_TYPE_BOOLEAN, SQL_SqlTrace}, /** SQL_SESSION_SETTING_VDBE_ADDOPTRACE */ - {"sql_vdbe_addoptrace", FIELD_TYPE_BOOLEAN, SQL_VdbeAddopTrace}, + {FIELD_TYPE_BOOLEAN, SQL_VdbeAddopTrace}, /** SQL_SESSION_SETTING_VDBE_DEBUG */ - {"sql_vdbe_debug", FIELD_TYPE_BOOLEAN, + {FIELD_TYPE_BOOLEAN, SQL_SqlTrace | SQL_VdbeListing | SQL_VdbeTrace}, /** SQL_SESSION_SETTING_VDBE_EQP */ - {"sql_vdbe_eqp", FIELD_TYPE_BOOLEAN, SQL_VdbeEQP}, + {FIELD_TYPE_BOOLEAN, SQL_VdbeEQP}, /** SQL_SESSION_SETTING_VDBE_LISTING */ - {"sql_vdbe_listing", FIELD_TYPE_BOOLEAN, SQL_VdbeListing}, + {FIELD_TYPE_BOOLEAN, SQL_VdbeListing}, /** SQL_SESSION_SETTING_VDBE_TRACE */ - {"sql_vdbe_trace", FIELD_TYPE_BOOLEAN, SQL_VdbeTrace}, + {FIELD_TYPE_BOOLEAN, SQL_VdbeTrace}, /** SQL_SESSION_SETTING_WHERE_TRACE */ - {"sql_where_trace", FIELD_TYPE_BOOLEAN, SQL_WhereTrace}, + {FIELD_TYPE_BOOLEAN, SQL_WhereTrace}, #endif }; -/** - * Return SQL setting ID that matches the given key and iterator - * type. Search begins from next_id. - * - * @param key Key to match settings name to. - * @param next_id ID to start search from. - * @param type Type of iterator. - * - * @retval setting ID. - */ -static int -sql_session_setting_id_by_name(const char *key, int next_id, - enum iterator_type type) -{ - int id = next_id; - if (key == NULL) - return id; - if (iterator_type_is_reverse(type)) { - for (; id >= 0; --id) { - int compare = strcmp(sql_session_opts[id].name, key); - if (compare == 0 && (type == ITER_REQ || - type == ITER_LE)) - break; - if (compare < 0 && (type == ITER_LT || type == ITER_LE)) - break; - } - } else { - for (; id < SQL_SESSION_SETTING_max; ++id) { - int compare = strcmp(sql_session_opts[id].name, key); - if (compare == 0 && (type == ITER_EQ || - type == ITER_GE || - type == ITER_ALL)) - break; - if (compare > 0 && (type == ITER_GT || - type == ITER_GE || - type == ITER_ALL)) - break; - } - } - return id; -} - -/** - * Return a tuple with the specified format, which contains the - * name and value of the SQL setting with the specified ID. - * - * @param format Format of tuple to return. - * @param id ID of tuple to return. - * @param result[out] Returned tuple. - * - * @retval 0 on success. - * @retval -1 on error. - */ -static int -sql_session_setting_tuple(struct tuple_format *format, int id, - struct tuple **result) +static void +sql_session_setting_get(int id, const char **mp_pair, const char **mp_pair_end) { - if (id < 0 || id >= SQL_SESSION_SETTING_max) { - *result = NULL; - return 0; - } + assert(id >= 0 && id < sql_session_setting_MAX); struct session *session = current_session(); uint32_t flags = session->sql_flags; - uint32_t mask = sql_session_opts[id].mask; - const char *engine = NULL; - /* Tuple format contains two fields - name and value. */ - uint32_t column_count = format->min_field_count; - assert(column_count == 2); - size_t size = mp_sizeof_array(column_count) + - mp_sizeof_str(strlen(sql_session_opts[id].name)); + struct sql_option_metadata *opt = &sql_session_opts[id]; + uint32_t mask = opt->mask; + const char *name = sql_session_setting_strs[id]; + size_t name_len = strlen(name); + size_t engine_len; + const char *engine; + size_t size = mp_sizeof_array(2) + mp_sizeof_str(name_len); /* * Currently, SQL session settings are of a boolean or * string type. */ - bool is_bool = sql_session_opts[id].field_type == FIELD_TYPE_BOOLEAN; + bool is_bool = opt->field_type == FIELD_TYPE_BOOLEAN; if (is_bool) { size += mp_sizeof_bool(true); } else { assert(id == SQL_SESSION_SETTING_DEFAULT_ENGINE); engine = sql_storage_engine_strs[session->sql_default_engine]; - size += mp_sizeof_str(strlen(engine)); + engine_len = strlen(engine); + size += mp_sizeof_str(engine_len); } - char *pos_ret = static_alloc(size); - assert(pos_ret != NULL); - char *pos = mp_encode_array(pos_ret, column_count); - pos = mp_encode_str(pos, sql_session_opts[id].name, - strlen(sql_session_opts[id].name)); + char *pos = static_alloc(size); + assert(pos != NULL); + char *pos_end = mp_encode_array(pos, 2); + pos_end = mp_encode_str(pos_end, name, name_len); if (is_bool) - pos = mp_encode_bool(pos, (flags & mask) == mask); + pos_end = mp_encode_bool(pos_end, (flags & mask) == mask); else - pos = mp_encode_str(pos, engine, strlen(engine)); - struct tuple *tuple = tuple_new(format, pos_ret, pos_ret + size); - if (tuple == NULL) - return -1; - *result = tuple; - return 0; -} - -int -sql_session_setting_get(struct tuple_format *format, int id, const char *key, - enum iterator_type type, int *end_id, - struct tuple **result) -{ - int new_id = sql_session_setting_id_by_name(key, id, type); - if (end_id != NULL) - *end_id = new_id; - return sql_session_setting_tuple(format, new_id, result); + pos_end = mp_encode_str(pos_end, engine, engine_len); + *mp_pair = pos; + *mp_pair_end = pos_end; } static int @@ -3481,37 +3423,40 @@ sql_set_string_option(int id, const char *value) return 0; } -bool -is_value_type_correct(char mp_type, enum field_type field_type) -{ - if (mp_typeof(mp_type) == MP_BOOL && field_type == FIELD_TYPE_BOOLEAN) - return true; - if (mp_typeof(mp_type) == MP_STR && field_type == FIELD_TYPE_STRING) - return true; - return false; +static int +sql_session_setting_set(int id, const char *mp_value) +{ + assert(id >= 0 && id < sql_session_setting_MAX); + enum mp_type mtype = mp_typeof(*mp_value); + enum field_type stype = sql_session_opts[id].field_type; + uint32_t len; + const char *tmp; + switch(stype) { + case FIELD_TYPE_BOOLEAN: + if (mtype != MP_BOOL) + break; + return sql_set_boolean_option(id, mp_decode_bool(&mp_value)); + case FIELD_TYPE_STRING: + if (mtype != MP_STR) + break; + tmp = mp_decode_str(&mp_value, &len); + tmp = tt_cstr(tmp, len); + return sql_set_string_option(id, tmp); + default: + unreachable(); + } + diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE, + sql_session_setting_strs[id], field_type_strs[stype]); + return -1; } -int -sql_session_setting_set(struct tuple_format *format, int id, const char *value, - struct tuple **result) -{ - if (id < 0 || id >= SQL_SESSION_SETTING_max) - return 0; - if (!is_value_type_correct(*value, sql_session_opts[id].field_type)) { - diag_set(ClientError, ER_FIELD_TYPE, "2", - field_type_strs[sql_session_opts[id].field_type]); - return -1; - } - if (sql_session_opts[id].field_type == FIELD_TYPE_BOOLEAN) { - if (sql_set_boolean_option(id, mp_decode_bool(&value)) != 0) - return -1; - } else { - assert(sql_session_opts[id].field_type == FIELD_TYPE_STRING); - uint32_t len; - const char *tmp = mp_decode_str(&value, &len); - const char *decoded_value = tt_cstr(tmp, len); - if (sql_set_string_option(id, decoded_value) != 0) - return -1; - } - return sql_session_setting_tuple(format, id, result); +void +sql_session_settings_init() +{ + struct session_setting_module *module = + &session_setting_modules[SESSION_SETTING_SQL]; + module->settings = sql_session_setting_strs; + module->setting_count = sql_session_setting_MAX; + module->get = sql_session_setting_get; + module->set = sql_session_setting_set; } diff --git a/src/box/sql/main.c b/src/box/sql/main.c index 826cd1c90..0b20f2132 100644 --- a/src/box/sql/main.c +++ b/src/box/sql/main.c @@ -39,7 +39,6 @@ #include "vdbeInt.h" #include "version.h" #include "box/session.h" -#include "box/session_settings.h" /* * If the following global variable points to a string which is the @@ -399,13 +398,6 @@ sql_init_db(sql **out_db) /* Enable the lookaside-malloc subsystem */ setupLookaside(db, 0, LOOKASIDE_SLOT_SIZE, LOOKASIDE_SLOT_NUMBER); - struct session_settings_modules *sql_settings = - &session_settings_modules[SESSION_SETTING_MODULE_SQL]; - sql_settings->id = SESSION_SETTING_MODULE_SQL; - sql_settings->size = sql_session_settings_count(); - sql_settings->get = sql_session_setting_get; - sql_settings->set = sql_session_setting_set; - *out_db = db; return 0; } diff --git a/test/box/gh-4511-access-settings-from-any-frontend.result b/test/box/gh-4511-access-settings-from-any-frontend.result index 579a5a18f..e86efedb4 100644 --- a/test/box/gh-4511-access-settings-from-any-frontend.result +++ b/test/box/gh-4511-access-settings-from-any-frontend.result @@ -65,16 +65,19 @@ test_run:cmd('setopt delimiter ";"') | - true | ... function check_sorting(ss, ts, key) - local is_right = true local iterators_list = {'ALL', 'REQ', 'EQ', 'GE', 'GT', 'LE', 'LT'} for _, it in pairs(iterators_list) do local view_space = ss:select({key}, {iterator = it}) local test_space = ts:select({key}, {iterator = it}) for key, value in pairs(view_space) do - is_right = is_right and (test_space[key].name == value.name) + if test_space[key].name ~= value.name then + return { + err = 'bad sorting', type = it, + exp = test_space[key].name, got = value.name + } + end end ================================================================================ This change simplifies debug. Because a failed test now prints where exactly it has failed. ================================================================================ end - return is_right end; | --- | ... @@ -85,23 +88,18 @@ test_run:cmd('setopt delimiter ""'); check_sorting(s, t) | --- - | - true | ... check_sorting(s, t, 'abcde') | --- - | - true | ... check_sorting(s, t, 'sql_d') | --- - | - true | ... check_sorting(s, t, 'sql_v') | --- - | - true | ... check_sorting(s, t, 'sql_defer_foreign_keys') | --- - | - true | ... t:drop() @@ -242,12 +240,12 @@ s:update('sql_defer_foreign_keys', {{'=', {'value'}, true}}) | --- | - error: Illegal parameters, field id must be a number or a string | ... -s:update('sql_defer_foreign_keys', {{'=', 1, true}}) +s:update('sql_defer_foreign_keys', {{'=', 1, 'new_key'}}) | --- | - error: Attempt to modify a tuple field which is part of index 'primary' in space | '_session_settings' | ... -s:update('sql_defer_foreign_keys', {{'=', 'name', true}}) +s:update('sql_defer_foreign_keys', {{'=', 'name', 'new_key'}}) | --- | - error: Attempt to modify a tuple field which is part of index 'primary' in space | '_session_settings' @@ -263,13 +261,13 @@ s:update('sql_defer_foreign_keys', {{'=', 'some text', true}}) s:update('sql_defer_foreign_keys', {{'=', 'value', 1}}) | --- - | - error: 'Tuple field 2 type does not match one required by operation: expected boolean' + | - error: Session setting sql_defer_foreign_keys expected a value of type boolean | ... s:update('sql_defer_foreign_keys', {{'=', 'value', {1}}}) | --- - | - error: 'Tuple field 2 type does not match one required by operation: expected boolean' + | - error: Session setting sql_defer_foreign_keys expected a value of type boolean | ... s:update('sql_defer_foreign_keys', {{'=', 'value', '1'}}) | --- - | - error: 'Tuple field 2 type does not match one required by operation: expected boolean' + | - error: Session setting sql_defer_foreign_keys expected a value of type boolean | ... diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/gh-4511-access-settings-from-any-frontend.test.lua index 0699aec31..9642f681c 100644 --- a/test/box/gh-4511-access-settings-from-any-frontend.test.lua +++ b/test/box/gh-4511-access-settings-from-any-frontend.test.lua @@ -30,16 +30,19 @@ for _,value in s:pairs() do t:insert(value) end test_run:cmd('setopt delimiter ";"') function check_sorting(ss, ts, key) - local is_right = true local iterators_list = {'ALL', 'REQ', 'EQ', 'GE', 'GT', 'LE', 'LT'} for _, it in pairs(iterators_list) do local view_space = ss:select({key}, {iterator = it}) local test_space = ts:select({key}, {iterator = it}) for key, value in pairs(view_space) do - is_right = is_right and (test_space[key].name == value.name) + if test_space[key].name ~= value.name then + return { + err = 'bad sorting', type = it, + exp = test_space[key].name, got = value.name + } + end end end - return is_right end; test_run:cmd('setopt delimiter ""'); @@ -93,8 +96,8 @@ s:update('sql_defer_foreign_keys', {{1, 'value', true}}) s:update('sql_defer_foreign_keys', {{{1}, 'value', true}}) s:update('sql_defer_foreign_keys', {{'=', {'value'}, true}}) -s:update('sql_defer_foreign_keys', {{'=', 1, true}}) -s:update('sql_defer_foreign_keys', {{'=', 'name', true}}) +s:update('sql_defer_foreign_keys', {{'=', 1, 'new_key'}}) +s:update('sql_defer_foreign_keys', {{'=', 'name', 'new_key'}}) s:update('sql_defer_foreign_keys', {{'=', 3, true}}) s:update('sql_defer_foreign_keys', {{'=', 'some text', true}}) diff --git a/test/box/misc.result b/test/box/misc.result index d2a20307a..004faaaad 100644 --- a/test/box/misc.result +++ b/test/box/misc.result @@ -554,6 +554,7 @@ t; 203: box.error.BOOTSTRAP_READONLY 204: box.error.SQL_FUNC_WRONG_RET_COUNT 205: box.error.FUNC_INVALID_RETURN_TYPE + 206: box.error.SESSION_SETTING_INVALID_VALUE ... test_run:cmd("setopt delimiter ''"); ---