From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 889E44696C4 for ; Thu, 19 Dec 2019 11:33:01 +0300 (MSK) From: imeevma@tarantool.org Date: Thu, 19 Dec 2019 11:33:00 +0300 Message-Id: <71b3e0f017780fe6efcf2d1ccb5e4d46f0d666cd.1576743850.git.imeevma@gmail.com> In-Reply-To: References: Subject: [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: v.shpilevoy@tarantool.org Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for review! I divided the patch into three parts: creation of engine, creation of system space and addition SQL settings to system space. My answers and third part below. On 12/12/19 2:47 AM, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > Overall it is getting better and cooler each time! > > See 10 comments below! > > On 09/12/2019 14:23, imeevma@tarantool.org wrote: >> This patch creates the _session_settings space. This space >> contains names and values of the session settings. User can use >> this system space to view or change session settings. >> >> Part of #4511 >> >> @TarantoolBot document >> Title: _session_settings system space >> The _session_settings system space used to view or change session >> settings. >> >> This space uses a new engine. This allows us to create tuples on >> the fly when the get() or select() methods are called. This >> engine does not support the insert(), replace(), and delete() >> methods. The only way to change the setting value is update(), >> which can only be used with the "=" operation. >> >> Because space creates a tuple on the fly, it allows us to get a >> tuple without saving it anywhere. But this means that every time >> we get a tuple from this system space, it is a new tuple, even if >> they look the same: >> >> tarantool> s = box.space._session_settings >> tarantool> name = 'sql_default_engine' >> tarantool> s:get({name}) == s:get({name}) >> --- >> - false >> ... >> >> Currently, this space contains only SQL settings, since the only >> session settings are SQL settings. >> >> List of currently available session settings: >> >> sql_default_engine >> sql_defer_foreign_keys >> sql_full_column_names >> sql_recursive_triggers >> sql_reverse_unordered_selects >> >> Debug build also have debug settings that could be obtained from >> this sysview: >> >> sql_parser_trace >> sql_select_trace >> sql_trace >> sql_vdbe_addoptrace >> sql_vdbe_debug >> sql_vdbe_eqp >> sql_vdbe_listing >> sql_vdbe_trace >> sql_where_trace >> >> Example of usage: >> tarantool> s = box.space._session_settings >> -- View session settings values. >> tarantool> s:get({'sql_default_engine'}) >> --- >> - ['sql_default_engine', 'memtx'] >> ... >> >> tarantool> s:select() >> --- >> - - ['sql_default_engine', 'memtx'] >> - ['sql_defer_foreign_keys', false] >> - ['sql_full_column_names', false] >> - ['sql_recursive_triggers', true] >> - ['sql_reverse_unordered_selects', false] >> ... >> >> tarantool> s:select('sql_g', {iterator='LE'}) >> --- >> - - ['sql_full_column_names', false] >> - ['sql_defer_foreign_keys', false] >> - ['sql_default_engine', 'memtx'] >> ... >> >> -- Change session setting value. >> tarantool> s:update('sql_default_engine', {{'=', value, 'vinyl'}}) > > 1. Looks like the value should be in quotes, if that is a field > name of the _session_settings space. > Thanks, fixed. >> --- >> - ['sql_default_engine', 'vinyl'] >> ... >> --- >> https://github.com/tarantool/tarantool/issues/4511 >> https://github.com/tarantool/tarantool/tree/imeevma/gh-4511-new-engine >> >> diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt >> index 5cd5cba8..0f94a5b 100644 >> --- a/src/box/CMakeLists.txt >> +++ b/src/box/CMakeLists.txt >> @@ -78,6 +78,7 @@ add_library(box STATIC >> memtx_space.c >> sysview.c >> blackhole.c >> + loophole.c > > 2. I propose to name this engine 'virtual_engine', and name > the files 'virtual_engine.c/.h'. > Thanks, done. >> vinyl.c >> vy_stmt.c >> vy_mem.c >> >> diff --git a/src/box/loophole.c b/src/box/loophole.c >> new file mode 100644 >> index 0000000..775f5a1 >> --- /dev/null >> +++ b/src/box/loophole.c >> @@ -0,0 +1,431 @@ >> +#include "loophole.h" >> +#include "session.h" >> +#include "schema.h" >> +#include "engine.h" >> +#include "tuple.h" >> +#include "xrow.h" >> + >> +struct loophole_index { >> + struct index base; >> + struct tuple_format *format; > > 3. Please, write some comments. Especially why do you > store format in the index. > Done. >> +}; >> + >> +struct loophole_iterator { >> + /** Base iterator. Must be the first member. */ >> + struct iterator base; >> + /** Format of the tuples this iterator returns. */ >> + struct tuple_format *format; >> + /** ID of the option in global list of the options. */ >> + int current_id; >> + /** Decoded key. */ >> + char *key; >> + /** Type of iterator. */ >> + enum iterator_type iterator_type; >> +}; > > 4. I don't like that the whole API below is hardcoded to use settings. > Engine API should not be like that. For example, loophole iterator > explicitly uses 'settings' API. loophole space, index too. > > Virtual engine should be generic. You already have space vtab, so > you can keep all these methods, but rename them to > settings_space_*(), create a separate vtab for that space, and in > virtual_engine_create_space() you check space ID only once. If that > is ID of settings space, you use the settings vtab. > Thanks, I think I fixed this. >> + >> +static void >> +loophole_iterator_free(struct iterator *ptr) >> +{ >> + struct loophole_iterator *it = (struct loophole_iterator *)ptr; >> + free(it->key); >> + free(it); >> +} >> + >> +static int >> +loophole_iterator_next(struct iterator *iterator, struct tuple **ret) >> +{ >> + struct loophole_iterator *it = (struct loophole_iterator *)iterator; >> + it->current_id = session_setting_id_by_name(it->key, it->current_id, >> + it->iterator_type); >> + int id = it->current_id; >> + it->current_id += iterator_type_is_reverse(it->iterator_type) ? -1 : 1; >> + return session_setting_tuple(it->format, id, ret); >> +} >> + >> +static void >> +loophole_index_destroy(struct index *index) >> +{ >> + free_session_settings(); >> + free(index); >> +} >> + >> +static struct iterator * >> +loophole_index_create_iterator(struct index *base, enum iterator_type type, >> + const char *key, uint32_t part_count) >> +{ >> + struct loophole_index *index = (struct loophole_index *)base; >> + char *decoded_key = NULL; >> + if (part_count > 0) { >> + assert(part_count == 1); >> + assert(mp_typeof(*key) == MP_STR); >> + uint32_t len; >> + const char *name = mp_decode_str(&key, &len); >> + decoded_key = (char *)malloc(len + 1); >> + if (decoded_key == NULL) { >> + diag_set(OutOfMemory, len + 1, "malloc", "decoded_key"); >> + return NULL; >> + } >> + memcpy(decoded_key, name, len); >> + decoded_key[len] = '\0'; >> + } >> + struct loophole_iterator *it = >> + (struct loophole_iterator *)malloc(sizeof(*it)); >> + if (it == NULL) { >> + diag_set(OutOfMemory, sizeof(*it), "malloc", "it"); >> + free(decoded_key); >> + return NULL; >> + } >> + iterator_create(&it->base, base); >> + it->base.next = loophole_iterator_next; >> + it->base.free = loophole_iterator_free; >> + it->key = decoded_key; >> + it->iterator_type = type; >> + it->format = index->format; >> + it->current_id = iterator_type_is_reverse(type) ? >> + session_get_settings_max_id() - 1 : 0; >> + return (struct iterator *)it; >> + return NULL; >> +} >> + >> +static int >> +loophole_index_get(struct index *base, const char *key, >> + uint32_t part_count, struct tuple **result) >> +{ >> + struct loophole_index *index = (struct loophole_index *)base; >> + (void)base; > > 5. Why is 'base' void? It is used. Not only in debug. > Fixed. >> + assert(part_count == 1); >> + (void)part_count; >> + uint32_t len; >> + const char *tmp = mp_decode_str(&key, &len); >> + const char *decoded_key = tt_cstr(tmp, len); >> + int id = session_setting_id_by_name(decoded_key, 0, ITER_EQ); >> + return session_setting_tuple(index->format, id, result); >> +} >> + >> +static const struct index_vtab loophole_index_vtab = { >> + /* .destroy = */ loophole_index_destroy, >> + /* .commit_create = */ generic_index_commit_create, >> + /* .abort_create = */ generic_index_abort_create, >> + /* .commit_modify = */ generic_index_commit_modify, >> + /* .commit_drop = */ generic_index_commit_drop, >> + /* .update_def = */ generic_index_update_def, >> + /* .depends_on_pk = */ generic_index_depends_on_pk, >> + /* .def_change_requires_rebuild = */ >> + generic_index_def_change_requires_rebuild, >> + /* .size = */ generic_index_size, >> + /* .bsize = */ generic_index_bsize, >> + /* .min = */ generic_index_min, >> + /* .max = */ generic_index_max, >> + /* .random = */ generic_index_random, >> + /* .count = */ generic_index_count, >> + /* .get = */ loophole_index_get, >> + /* .replace = */ generic_index_replace, >> + /* .create_iterator = */ loophole_index_create_iterator, >> + /* .create_snapshot_iterator = */ >> + generic_index_create_snapshot_iterator, >> + /* .stat = */ generic_index_stat, >> + /* .compact = */ generic_index_compact, >> + /* .reset_stat = */ generic_index_reset_stat, >> + /* .begin_build = */ generic_index_begin_build, >> + /* .reserve = */ generic_index_reserve, >> + /* .build_next = */ generic_index_build_next, >> + /* .end_build = */ generic_index_end_build, >> +}; >> +> +static void >> +loophole_space_destroy(struct space *space) >> +{ >> + free(space); >> +} >> + >> +static int >> +loophole_space_execute_replace(struct space *space, struct txn *txn, >> + struct request *request, struct tuple **result) >> +{ >> + (void)space; >> + (void)txn; >> + (void)request; >> + (void)result; >> + diag_set(ClientError, ER_UNSUPPORTED, "Loophole", "replace()"); >> + return -1; >> +} >> + >> +static int >> +loophole_space_execute_delete(struct space *space, struct txn *txn, >> + struct request *request, struct tuple **result) >> +{ >> + (void)space; >> + (void)txn; >> + (void)request; >> + (void)result; >> + diag_set(ClientError, ER_UNSUPPORTED, "Loophole", "delete()"); >> + return -1; >> +} >> + >> +static int >> +loophole_space_execute_update(struct space *space, struct txn *txn, >> + struct request *request, struct tuple **result) >> +{ >> + (void)txn; > > 6. That is actually something arguable. > > tarantool> box.space._session_settings:get({'sql_default_engine'}) > --- > - ['sql_default_engine', 'memtx'] > ... > > tarantool> box.begin() > --- > ... > > tarantool> box.space._session_settings:update({'sql_default_engine'}, {{'=', 'value', 'vinyl'}}) > --- > - ['sql_default_engine', 'vinyl'] > ... > > tarantool> box.rollback() > --- > ... > > tarantool> box.space._session_settings:get({'sql_default_engine'}) > --- > - ['sql_default_engine', 'vinyl'] > ... > > > This is definitely wrong. And I don't know what to do with that > now. Except that it should be documented at least. > I asked this question, but haven't find an answer. If the issue won't be fixed in this patch-set, I will fill an issue. >> + uint32_t len; >> + const char *tmp; >> + const char *data; > > 7. Lets avoid C89 declaration style. > Fixed. >> + >> + data = request->key; >> + uint32_t key_len = mp_decode_array(&data); >> + if (key_len == 0) { >> + diag_set(ClientError, ER_EXACT_MATCH, 1, 0); >> + return -1; >> + } >> + if (key_len > 1 || mp_typeof(*data) != MP_STR) { >> + diag_set(ClientError, ER_KEY_PART_TYPE, 0, "string"); >> + return -1; >> + } >> + tmp = mp_decode_str(&data, &len); >> + const char *decoded_key = tt_cstr(tmp, len); >> + int id = session_setting_id_by_name(decoded_key, 0, ITER_EQ); >> + if (id == session_get_settings_max_id()) { >> + *result = NULL; >> + return 0; >> + } >> + >> + data = request->tuple; >> + assert(mp_typeof(*data) == MP_ARRAY); >> + uint32_t update_counter = mp_decode_array(&data); >> + if (mp_typeof(*data) != MP_ARRAY) { >> + diag_set(ClientError, ER_ILLEGAL_PARAMS, "update operation " >> + "must be an array {op,..}"); >> + return -1; >> + } >> + if (update_counter > 1) { >> + diag_set(ClientError, ER_UNSUPPORTED, "Loophole", "update of " >> + "more than one field"); >> + return -1; >> + } >> + if (mp_decode_array(&data) != 3) { >> + diag_set(ClientError, ER_UNKNOWN_UPDATE_OP); >> + return -1; >> + } >> + if (mp_typeof(*data) != MP_STR) { >> + diag_set(ClientError, ER_ILLEGAL_PARAMS, "update operation " >> + "name must be a string"); >> + return -1; >> + } >> + tmp = mp_decode_str(&data, &len); >> + if (tmp[0] != '=' || len != 1) { >> + diag_set(ClientError, ER_UNSUPPORTED, "Loophole", "not '='"); >> + return -1; >> + } >> + if (mp_typeof(*data) == MP_UINT) { >> + uint32_t id = mp_decode_uint(&data); >> + if (id - request->index_base == 0) { >> + diag_set(ClientError, ER_CANT_UPDATE_PRIMARY_KEY, >> + space_index(space, 0)->def->name, >> + space_name(space)); >> + return -1; >> + } >> + if (id - request->index_base != 1) { >> + diag_set(ClientError, ER_EXACT_FIELD_COUNT, id, >> + 1 + request->index_base); >> + return -1; >> + } >> + } else if (mp_typeof(*data) == MP_STR) { >> + tmp = mp_decode_str(&data, &len); >> + const char *field_name = tt_cstr(tmp, len); >> + if (strcmp(field_name, "name") == 0) { >> + diag_set(ClientError, ER_CANT_UPDATE_PRIMARY_KEY, >> + space_index(space, 0)->def->name, >> + space_name(space)); >> + return -1; >> + } >> + if (strcmp(field_name, "value") != 0) { >> + diag_set(ClientError, ER_NO_SUCH_FIELD_NAME, >> + field_name); >> + return -1; >> + } >> + } else { >> + diag_set(ClientError, ER_ILLEGAL_PARAMS, >> + "field id must be a number or a string"); >> + return -1; >> + } >> + const char *value = data; >> + >> + if (session_setting_new_value(id, value) != 0) >> + return -1; >> + return session_setting_tuple(space->format, id, result); > > 8. Well, most part of that function could be replaced by: > > struct tuple *old_tuple; > uint32_t new_size = 0, bsize; > struct tuple_format *format = space->format; > const char *old_data = tuple_data_range(old_tuple, &bsize); > session_setting_tuple(format, id, old_tuple); > const char *new_tuple_data = xrow_update_execute( > request->tuple, request->tuple_end, > old_data, old_data + bsize, format->dict, > &new_size, request->index_base, NULL); > > And you have the updated tuple in new_tuple_data. Now you > create a new tuple: > > struct tuple *new_tuple = tuple_new(format, new_tuple_data, new_size); > > From that tuple you get the new value. And you don't need to > implement the update by yourself. That also makes possible to > use any update operations. > > Please, patch schema_def.h with definition of this new space > fields. So as you could obtain the new value by: > > const char *new_value = tuple_field( > new_tuple, BOX_SESSION_SETTINGS_FIELD_VALUE); > Thanks a lot! Fixed. >> +} >> + >> +static int >> +loophole_space_execute_upsert(struct space *space, struct txn *txn, >> + struct request *request) >> +{ >> + (void)space; >> + (void)txn; >> + (void)request; >> + diag_set(ClientError, ER_UNSUPPORTED, "Loophole", "upsert()"); >> + return -1; >> +} >> + >> +static struct index * >> +loophole_space_create_index(struct space *space, struct index_def *def) >> +{ >> + if (space->def->id != BOX_SESSION_SETTINGS_ID) { >> + diag_set(ClientError, ER_UNSUPPORTED, "Loophole", >> + "create_index()"); >> + return NULL; >> + } >> + new_session_settings(); > > 9. That is something I really don't like. See more in the comments > to session setting API. > Fixed, I think. >> diff --git a/src/box/session.cc b/src/box/session.cc >> index 461d1cf..d72d917 100644 >> --- a/src/box/session.cc >> +++ b/src/box/session.cc >> @@ -36,6 +36,7 @@ >> #include "user.h" >> #include "error.h" >> #include "tt_static.h" >> +#include "sql.h" > > 10. This is where my idea was taken wrong. I am trying to > remove explicit session's dependency on SQL. This is possible > in at least two ways: > > - Way 1. All the settings are defined in one namespace. > In one enum. That enum is defined in session.h. > Otherwise, when you will add more settings from other > subsystems, you will get a collision. > In session.c you have an array of triplets > {enum setting, get_function, set_function}. The functions > are all NULL from the beginning. Subsystems, when they > initialize, fill their part in that array (sql_init() > will fill get/set_function values of its SQL settings). > That way may be bad, because session will need to know > names of all settings of all subsystems. > > - Way 2. Make session setting consist of 2 parts: > subsystem ID and setting ID. Then you define subsystem > IDs in session.h, and settings of each subsystem in > any other file. In build.c, in sql.c, whatever. For > example, you store this in session.h: > > enum session_setting_module { > SESSION_SETTING_BOX, > SESSION_SETTING_SQL, > ... > }; > > And keep your current SQL settings in build.c. > > In session.c you have an array with triplets > {enum setting_module, set_function, get_function}. > Get/set functions are filled exactly like in the first > way - during module initializations. > > > We actually already do something similar with > session_vtab_registry global array. Fixed, I think. New patch: >From 71b3e0f017780fe6efcf2d1ccb5e4d46f0d666cd Mon Sep 17 00:00:00 2001 From: Mergen Imeev Date: Sat, 30 Nov 2019 12:59:45 +0300 Subject: [PATCH] box: add SQL settings to _session_settings Part of #4511 @TarantoolBot document Title: _session_settings system space The _session_settings system space used to view or change session settings. This space uses a new engine. This allows us to create tuples on the fly when the get() or select() methods are called. This engine does not support the insert(), replace(), and delete() methods. The only way to change the setting value is update(), which can only be used with the "=" operation. Because space creates a tuple on the fly, it allows us to get a tuple without saving it anywhere. But this means that every time we get a tuple from this system space, it is a new tuple, even if they look the same: tarantool> s = box.space._session_settings tarantool> name = 'sql_default_engine' tarantool> s:get({name}) == s:get({name}) --- - false ... Currently, this space contains only SQL settings, since the only session settings are SQL settings. List of currently available session settings: sql_default_engine sql_defer_foreign_keys sql_full_column_names sql_recursive_triggers sql_reverse_unordered_selects Debug build also have debug settings that could be obtained from this sysview: sql_parser_trace sql_select_trace sql_trace sql_vdbe_addoptrace sql_vdbe_debug sql_vdbe_eqp sql_vdbe_listing sql_vdbe_trace sql_where_trace Example of usage: tarantool> s = box.space._session_settings -- View session settings values. tarantool> s:get({'sql_default_engine'}) --- - ['sql_default_engine', 'memtx'] ... tarantool> s:select() --- - - ['sql_default_engine', 'memtx'] - ['sql_defer_foreign_keys', false] - ['sql_full_column_names', false] - ['sql_recursive_triggers', true] - ['sql_reverse_unordered_selects', false] ... tarantool> s:select('sql_g', {iterator='LE'}) --- - - ['sql_full_column_names', false] - ['sql_defer_foreign_keys', false] - ['sql_default_engine', 'memtx'] ... -- Change session setting value. tarantool> s:update('sql_default_engine', {{'=', 'value', 'vinyl'}}) --- - ['sql_default_engine', 'vinyl'] ... diff --git a/src/box/session_settings.h b/src/box/session_settings.h index e7ecbdb..34d1a25 100644 --- a/src/box/session_settings.h +++ b/src/box/session_settings.h @@ -38,6 +38,7 @@ extern "C" { #endif /* defined(__cplusplus) */ enum session_setting_module { + SESSION_SETTING_MODULE_SQL, SESSION_SETTING_MODULE_max }; diff --git a/src/box/sql.h b/src/box/sql.h index 0fa52fc..3f86568 100644 --- a/src/box/sql.h +++ b/src/box/sql.h @@ -33,6 +33,7 @@ #include #include +#include "iterator_type.h" #if defined(__cplusplus) extern "C" { @@ -70,6 +71,7 @@ struct Select; struct Table; struct sql_trigger; struct space_def; +struct tuple_format; /** * Perform parsing of provided expression. This is done by @@ -420,6 +422,51 @@ 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 51cd7ce..995a32e 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -46,6 +46,7 @@ #include #include "sqlInt.h" #include "vdbeInt.h" +#include "box/tuple.h" #include "tarantoolInt.h" #include "box/box.h" #include "box/ck_constraint.h" @@ -3242,3 +3243,275 @@ sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name, *fieldno = i; return 0; } + +/** + * Identifiers of all SQL session setings. The identifier of the + * option is equal to its place in the sorted list of session + * options of current module. + * + * It is IMPORTANT that these options are sorted by name. If this + * is not the case, the result returned by the _session_settings + * space iterator will not be sorted properly. + */ +enum { + SQL_SESSION_SETTING_DEFAULT_ENGINE = 0, + SQL_SESSION_SETTING_DEFER_FOREIGN_KEYS, + SQL_SESSION_SETTING_FULL_COLUMN_NAMES, +#ifndef NDEBUG + SQL_SESSION_SETTING_PARSER_TRACE, +#endif + SQL_SESSION_SETTING_RECURSIVE_TRIGGERS, + SQL_SESSION_SETTING_REVERSE_UNORDERED_SELECTS, +#ifndef NDEBUG + SQL_SESSION_SETTING_SELECT_TRACE, + SQL_SESSION_SETTING_TRACE, + SQL_SESSION_SETTING_VDBE_ADDOPTRACE, + SQL_SESSION_SETTING_VDBE_DEBUG, + SQL_SESSION_SETTING_VDBE_EQP, + SQL_SESSION_SETTING_VDBE_LISTING, + SQL_SESSION_SETTING_VDBE_TRACE, + SQL_SESSION_SETTING_WHERE_TRACE, +#endif + SQL_SESSION_SETTING_max, +}; + +int +sql_session_settings_count() +{ + return SQL_SESSION_SETTING_max; +} + +/** + * A local structure that allows to establish a connection between + * the name of the parameter, its field type and mask, if it have + * 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. + * + * 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}, + /** SQL_SESSION_SETTING_DEFER_FOREIGN_KEYS */ + {"sql_defer_foreign_keys", FIELD_TYPE_BOOLEAN, SQL_DeferFKs}, + /** SQL_SESSION_SETTING_FULL_COLUMN_NAMES */ + {"sql_full_column_names", FIELD_TYPE_BOOLEAN, SQL_FullColNames}, +#ifndef NDEBUG + /** SQL_SESSION_SETTING_PARSER_TRACE */ + {"sql_parser_trace", FIELD_TYPE_BOOLEAN, PARSER_TRACE_FLAG}, +#endif + /** SQL_SESSION_SETTING_RECURSIVE_TRIGGERS */ + {"sql_recursive_triggers", FIELD_TYPE_BOOLEAN, SQL_RecTriggers}, + /** SQL_SESSION_SETTING_REVERSE_UNORDERED_SELECTS */ + {"sql_reverse_unordered_selects", FIELD_TYPE_BOOLEAN, SQL_ReverseOrder}, +#ifndef NDEBUG + /** SQL_SESSION_SETTING_SELECT_TRACE */ + {"sql_select_trace", FIELD_TYPE_BOOLEAN, SQL_SelectTrace}, + /** SQL_SESSION_SETTING_TRACE */ + {"sql_trace", FIELD_TYPE_BOOLEAN, SQL_SqlTrace}, + /** SQL_SESSION_SETTING_VDBE_ADDOPTRACE */ + {"sql_vdbe_addoptrace", FIELD_TYPE_BOOLEAN, SQL_VdbeAddopTrace}, + /** SQL_SESSION_SETTING_VDBE_DEBUG */ + {"sql_vdbe_debug", FIELD_TYPE_BOOLEAN, + SQL_SqlTrace | SQL_VdbeListing | SQL_VdbeTrace}, + /** SQL_SESSION_SETTING_VDBE_EQP */ + {"sql_vdbe_eqp", FIELD_TYPE_BOOLEAN, SQL_VdbeEQP}, + /** SQL_SESSION_SETTING_VDBE_LISTING */ + {"sql_vdbe_listing", FIELD_TYPE_BOOLEAN, SQL_VdbeListing}, + /** SQL_SESSION_SETTING_VDBE_TRACE */ + {"sql_vdbe_trace", FIELD_TYPE_BOOLEAN, SQL_VdbeTrace}, + /** SQL_SESSION_SETTING_WHERE_TRACE */ + {"sql_where_trace", 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) +{ + if (id < 0 || id >= SQL_SESSION_SETTING_max) { + *result = NULL; + return 0; + } + 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)); + /* + * Currently, SQL session settings are of a boolean or + * string type. + */ + bool is_bool = sql_session_opts[id].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)); + } + + 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)); + if (is_bool) + pos = mp_encode_bool(pos, (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); +} + +static int +sql_set_boolean_option(int id, bool value) +{ + struct session *session = current_session(); + struct sql_option_metadata *option = &sql_session_opts[id]; + assert(option->field_type == FIELD_TYPE_BOOLEAN); + if (value) + session->sql_flags |= option->mask; + else + session->sql_flags &= ~option->mask; +#ifndef NDEBUG + if (id == SQL_SESSION_SETTING_PARSER_TRACE) { + if (value) + sqlParserTrace(stdout, "parser: "); + else + sqlParserTrace(NULL, NULL); + } +#endif + return 0; +} + +static int +sql_set_string_option(int id, const char *value) +{ + assert(sql_session_opts[id].field_type = FIELD_TYPE_STRING); + assert(id == SQL_SESSION_SETTING_DEFAULT_ENGINE); + (void)id; + enum sql_storage_engine engine = STR2ENUM(sql_storage_engine, value); + if (engine == sql_storage_engine_MAX) { + diag_set(ClientError, ER_NO_SUCH_ENGINE, value); + return -1; + } + current_session()->sql_default_engine = engine; + 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; +} + +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); +} diff --git a/src/box/sql/main.c b/src/box/sql/main.c index 0b20f21..826cd1c 100644 --- a/src/box/sql/main.c +++ b/src/box/sql/main.c @@ -39,6 +39,7 @@ #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 @@ -398,6 +399,13 @@ 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 75d53cf..579a5a1 100644 --- a/test/box/gh-4511-access-settings-from-any-frontend.result +++ b/test/box/gh-4511-access-settings-from-any-frontend.result @@ -45,72 +45,231 @@ s:replace({'sql_defer_foreign_keys', true}) | - error: Session_settings space does not support replace() | ... --- Check get() and select(). They should return nothing for now. -s:get({'a'}) +-- +-- Check select() method of session_settings space. Should work +-- the same way as an ordinary space with an index of the type +-- "TREE". +-- +t = box.schema.space.create('settings', {format = s:format()}) + | --- + | ... +_ = t:create_index('primary') + | --- + | ... +for _,value in s:pairs() do t:insert(value) end + | --- + | ... + +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) + end + end + return is_right +end; | --- | ... -s:select() +test_run:cmd('setopt delimiter ""'); | --- - | - [] + | - true | ... -s:select({}, {iterator='EQ'}) + +check_sorting(s, t) | --- - | - [] + | - true | ... -s:select({}, {iterator='ALL'}) +check_sorting(s, t, 'abcde') | --- - | - [] + | - true | ... -s:select({}, {iterator='GE'}) +check_sorting(s, t, 'sql_d') | --- - | - [] + | - true | ... -s:select({}, {iterator='GT'}) +check_sorting(s, t, 'sql_v') | --- - | - [] + | - true | ... -s:select({}, {iterator='REQ'}) +check_sorting(s, t, 'sql_defer_foreign_keys') | --- - | - [] + | - true | ... -s:select({}, {iterator='LE'}) + +t:drop() | --- - | - [] | ... -s:select({}, {iterator='LT'}) + +-- Check get() method of session_settings space. +s:get({'sql_defer_foreign_keys'}) | --- - | - [] + | - ['sql_defer_foreign_keys', false] | ... -s:select({'a'}, {iterator='EQ'}) +s:get({'sql_recursive_triggers'}) | --- - | - [] + | - ['sql_recursive_triggers', true] | ... -s:select({'a'}, {iterator='ALL'}) +s:get({'sql_reverse_unordered_selects'}) | --- - | - [] + | - ['sql_reverse_unordered_selects', false] | ... -s:select({'a'}, {iterator='GE'}) +s:get({'sql_default_engine'}) | --- - | - [] + | - ['sql_default_engine', 'memtx'] | ... -s:select({'a'}, {iterator='GT'}) +s:get({'abcd'}) | --- - | - [] | ... -s:select({'a'}, {iterator='REQ'}) + +-- Check pairs() method of session_settings space. +t = {} | --- - | - [] | ... -s:select({'a'}, {iterator='LE'}) +for key, value in s:pairs() do table.insert(t, {key, value}) end | --- - | - [] | ... -s:select({'a'}, {iterator='LT'}) +#t == s:count() | --- - | - [] + | - true | ... --- Currently there is nothing to update, but update() should work. -s:update('some_option', {{'=', 'value', true}}) +-- Check update() method of session_settings space. + +-- Correct updates. +s:update('sql_defer_foreign_keys', {{'=', 'value', true}}) + | --- + | - ['sql_defer_foreign_keys', true] + | ... +s:update({'sql_defer_foreign_keys'}, {{'=', 2, false}}) + | --- + | - ['sql_defer_foreign_keys', false] + | ... +s:update('sql_default_engine', {{'=', 2, 'vinyl'}}) + | --- + | - ['sql_default_engine', 'vinyl'] + | ... +s:update('sql_default_engine', {{':', 'value', 1, 5, 'memtx'}}) + | --- + | - ['sql_default_engine', 'memtx'] + | ... +s:update('a', {{'=', 2, 1}}) + | --- + | ... + +-- Inorrect updates. +s:update({{'sql_defer_foreign_keys'}}, {{'=', 'value', true}}) + | --- + | - error: 'Supplied key type of part 0 does not match index part type: expected string' + | ... + +s:update('sql_defer_foreign_keys', {'=', 'value', true}) + | --- + | - error: Illegal parameters, update operation must be an array {op,..} + | ... +s:update('sql_defer_foreign_keys', {{'=', 'value', true}, {'=', 2, true}}) + | --- + | - ['sql_defer_foreign_keys', true] + | ... +s:update('sql_defer_foreign_keys', {{}}) + | --- + | - error: Illegal parameters, update operation must be an array {op,..}, got empty + | array + | ... +s:update('sql_defer_foreign_keys', {{'='}}) + | --- + | - error: Unknown UPDATE operation + | ... +s:update('sql_defer_foreign_keys', {{'=', 'value'}}) + | --- + | - error: Unknown UPDATE operation + | ... +s:update('sql_defer_foreign_keys', {{'=', 'value', true, 1}}) + | --- + | - error: Unknown UPDATE operation + | ... + +s:update('sql_defer_foreign_keys', {{'+', 'value', 2}}) + | --- + | - error: 'Argument type in operation ''+'' on field 2 does not match field type: expected + | a number' + | ... +s:update('sql_defer_foreign_keys', {{'-', 'value', 2}}) + | --- + | - error: 'Argument type in operation ''-'' on field 2 does not match field type: expected + | a number' + | ... +s:update('sql_defer_foreign_keys', {{'&', 'value', 2}}) + | --- + | - error: 'Argument type in operation ''&'' on field 2 does not match field type: expected + | a positive integer' + | ... +s:update('sql_defer_foreign_keys', {{'|', 'value', 2}}) + | --- + | - error: 'Argument type in operation ''|'' on field 2 does not match field type: expected + | a positive integer' + | ... +s:update('sql_defer_foreign_keys', {{'^', 'value', 2}}) + | --- + | - error: 'Argument type in operation ''^'' on field 2 does not match field type: expected + | a positive integer' + | ... +s:update('sql_defer_foreign_keys', {{'!', 'value', 2}}) + | --- + | - error: Tuple field count 3 does not match space field count 2 + | ... +s:update('sql_defer_foreign_keys', {{'#', 'value', 2}}) + | --- + | - error: Tuple field count 1 does not match space field count 2 + | ... +s:update('sql_defer_foreign_keys', {{1, 'value', true}}) + | --- + | - error: Illegal parameters, update operation name must be a string + | ... +s:update('sql_defer_foreign_keys', {{{1}, 'value', true}}) + | --- + | - error: Illegal parameters, update operation name must be a string + | ... + +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}}) + | --- + | - 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}}) + | --- + | - error: Attempt to modify a tuple field which is part of index 'primary' in space + | '_session_settings' + | ... +s:update('sql_defer_foreign_keys', {{'=', 3, true}}) + | --- + | - error: Tuple field count 3 does not match space field count 2 + | ... +s:update('sql_defer_foreign_keys', {{'=', 'some text', true}}) + | --- + | - error: Field 'some text' was not found in the tuple + | ... + +s:update('sql_defer_foreign_keys', {{'=', 'value', 1}}) + | --- + | - error: 'Tuple field 2 type does not match one required by operation: expected boolean' + | ... +s:update('sql_defer_foreign_keys', {{'=', 'value', {1}}}) + | --- + | - error: 'Tuple field 2 type does not match one required by operation: expected boolean' + | ... +s:update('sql_defer_foreign_keys', {{'=', 'value', '1'}}) | --- + | - error: 'Tuple field 2 type does not match one required by operation: expected 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 3304454..0699aec 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 @@ -19,23 +19,85 @@ s:insert({'a', 1}) s:delete({'b'}) s:replace({'sql_defer_foreign_keys', true}) --- Check get() and select(). They should return nothing for now. -s:get({'a'}) -s:select() -s:select({}, {iterator='EQ'}) -s:select({}, {iterator='ALL'}) -s:select({}, {iterator='GE'}) -s:select({}, {iterator='GT'}) -s:select({}, {iterator='REQ'}) -s:select({}, {iterator='LE'}) -s:select({}, {iterator='LT'}) -s:select({'a'}, {iterator='EQ'}) -s:select({'a'}, {iterator='ALL'}) -s:select({'a'}, {iterator='GE'}) -s:select({'a'}, {iterator='GT'}) -s:select({'a'}, {iterator='REQ'}) -s:select({'a'}, {iterator='LE'}) -s:select({'a'}, {iterator='LT'}) - --- Currently there is nothing to update, but update() should work. -s:update('some_option', {{'=', 'value', true}}) +-- +-- Check select() method of session_settings space. Should work +-- the same way as an ordinary space with an index of the type +-- "TREE". +-- +t = box.schema.space.create('settings', {format = s:format()}) +_ = t:create_index('primary') +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) + end + end + return is_right +end; +test_run:cmd('setopt delimiter ""'); + +check_sorting(s, t) +check_sorting(s, t, 'abcde') +check_sorting(s, t, 'sql_d') +check_sorting(s, t, 'sql_v') +check_sorting(s, t, 'sql_defer_foreign_keys') + +t:drop() + +-- Check get() method of session_settings space. +s:get({'sql_defer_foreign_keys'}) +s:get({'sql_recursive_triggers'}) +s:get({'sql_reverse_unordered_selects'}) +s:get({'sql_default_engine'}) +s:get({'abcd'}) + +-- Check pairs() method of session_settings space. +t = {} +for key, value in s:pairs() do table.insert(t, {key, value}) end +#t == s:count() + +-- Check update() method of session_settings space. + +-- Correct updates. +s:update('sql_defer_foreign_keys', {{'=', 'value', true}}) +s:update({'sql_defer_foreign_keys'}, {{'=', 2, false}}) +s:update('sql_default_engine', {{'=', 2, 'vinyl'}}) +s:update('sql_default_engine', {{':', 'value', 1, 5, 'memtx'}}) +s:update('a', {{'=', 2, 1}}) + +-- Inorrect updates. +s:update({{'sql_defer_foreign_keys'}}, {{'=', 'value', true}}) + +s:update('sql_defer_foreign_keys', {'=', 'value', true}) +s:update('sql_defer_foreign_keys', {{'=', 'value', true}, {'=', 2, true}}) +s:update('sql_defer_foreign_keys', {{}}) +s:update('sql_defer_foreign_keys', {{'='}}) +s:update('sql_defer_foreign_keys', {{'=', 'value'}}) +s:update('sql_defer_foreign_keys', {{'=', 'value', true, 1}}) + +s:update('sql_defer_foreign_keys', {{'+', 'value', 2}}) +s:update('sql_defer_foreign_keys', {{'-', 'value', 2}}) +s:update('sql_defer_foreign_keys', {{'&', 'value', 2}}) +s:update('sql_defer_foreign_keys', {{'|', 'value', 2}}) +s:update('sql_defer_foreign_keys', {{'^', 'value', 2}}) +s:update('sql_defer_foreign_keys', {{'!', 'value', 2}}) +s:update('sql_defer_foreign_keys', {{'#', 'value', 2}}) +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', {{'=', 3, true}}) +s:update('sql_defer_foreign_keys', {{'=', 'some text', true}}) + +s:update('sql_defer_foreign_keys', {{'=', 'value', 1}}) +s:update('sql_defer_foreign_keys', {{'=', 'value', {1}}}) +s:update('sql_defer_foreign_keys', {{'=', 'value', '1'}})