* [Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array
2020-03-30 9:13 [Tarantool-patches] [PATCH 0/4] session settings fixes Chris Sosnin
@ 2020-03-30 9:13 ` Chris Sosnin
2020-04-03 13:32 ` Nikita Pettik
2020-03-30 9:13 ` [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space Chris Sosnin
` (5 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Chris Sosnin @ 2020-03-30 9:13 UTC (permalink / raw)
To: v.shpilevoy, korablev, tarantool-patches
Currently we have an array of modules and each module has
its own array of session_settings. This patch merges these
into one array, as long as it turned out, that we cannot
represent every setting as a part of some module. This
change is also needed for implementing binary search for
_session_settings space.
Part of #4711, #4712
---
src/box/session_settings.c | 121 ++++++++++++++-----------------------
src/box/session_settings.h | 50 +++++++++------
src/box/sql/build.c | 79 +++++++-----------------
3 files changed, 101 insertions(+), 149 deletions(-)
diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index 37d2a3e85..2ecf71f2d 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -38,8 +38,20 @@
#include "xrow.h"
#include "sql.h"
-struct session_setting_module
- session_setting_modules[session_setting_type_MAX] = {};
+struct session_setting session_settings[SESSION_SETTING_COUNT] = {};
+
+/** Corresponding names of session settings. */
+const char *session_setting_strs[SESSION_SETTING_COUNT] = {
+ "sql_default_engine",
+ "sql_defer_foreign_keys",
+ "sql_full_column_names",
+ "sql_full_metadata",
+ "sql_parser_debug",
+ "sql_recursive_triggers",
+ "sql_reverse_unordered_selects",
+ "sql_select_debug",
+ "sql_vdbe_debug",
+};
struct session_settings_index {
/** Base index. Must be the first member. */
@@ -61,12 +73,7 @@ struct session_settings_iterator {
* a format for selected tuples.
*/
struct tuple_format *format;
- /**
- * ID of the current session settings module in the global
- * list of the modules.
- */
- int module_id;
- /** ID of the setting in current module. */
+ /** ID of the setting. */
int setting_id;
/** Decoded key. */
char *key;
@@ -86,46 +93,40 @@ session_settings_iterator_free(struct iterator *ptr)
}
static int
-session_settings_next_in_module(const struct session_setting_module *module,
- int *sid, const char *key, bool is_eq,
- bool is_including)
+session_settings_next(int *sid, const char *key, bool is_eq, bool is_including)
{
int i = *sid;
- int count = module->setting_count;
- if (i >= count)
+ if (i >= SESSION_SETTING_COUNT)
return -1;
if (key == NULL)
return 0;
assert(i >= 0);
- const char **name = &module->settings[i];
- for (; i < count; ++i, ++name) {
- int cmp = strcmp(*name, key);
+ for (; i < SESSION_SETTING_COUNT; ++i) {
+ const char *name = session_setting_strs[i];
+ int cmp = strcmp(name, key);
if ((cmp == 0 && is_including) ||
(cmp > 0 && !is_eq)) {
*sid = i;
return 0;
}
}
- *sid = count;
+ *sid = SESSION_SETTING_COUNT;
return -1;
}
static int
-session_settings_prev_in_module(const struct session_setting_module *module,
- int *sid, const char *key, bool is_eq,
- bool is_including)
+session_settings_prev(int *sid, const char *key, bool is_eq, bool is_including)
{
int i = *sid;
- int count = module->setting_count;
if (i < 0)
return -1;
if (key == NULL)
return 0;
- if (i >= count)
- i = count - 1;
- const char **name = &module->settings[i];
- for (; i >= 0; --i, --name) {
- int cmp = strcmp(*name, key);
+ if (i >= SESSION_SETTING_COUNT)
+ i = SESSION_SETTING_COUNT - 1;
+ for (; i >= 0; --i) {
+ const char *name = session_setting_strs[i];
+ int cmp = strcmp(name, key);
if ((cmp == 0 && is_including) ||
(cmp < 0 && !is_eq)) {
*sid = i;
@@ -141,27 +142,19 @@ session_settings_iterator_next(struct iterator *iterator, struct tuple **result)
{
struct session_settings_iterator *it =
(struct session_settings_iterator *)iterator;
- int mid = it->module_id, sid = it->setting_id;
- struct session_setting_module *module;
+ int sid = it->setting_id;
const char *key = it->key;
bool is_including = it->is_including, is_eq = it->is_eq;
bool is_found = false;
- for (; mid < session_setting_type_MAX; ++mid, sid = 0) {
- module = &session_setting_modules[mid];
- if (session_settings_next_in_module(module, &sid, key, is_eq,
- is_including) == 0) {
- is_found = true;
- break;
- }
- }
- it->module_id = mid;
+ if (session_settings_next(&sid, key, is_eq, is_including) == 0)
+ is_found = true;
it->setting_id = sid + 1;
if (!is_found) {
*result = NULL;
return 0;
}
const char *mp_pair, *mp_pair_end;
- module->get(sid, &mp_pair, &mp_pair_end);
+ session_settings[sid].get(sid, &mp_pair, &mp_pair_end);
*result = box_tuple_new(it->format, mp_pair, mp_pair_end);
return *result != NULL ? 0 : -1;
}
@@ -171,27 +164,19 @@ session_settings_iterator_prev(struct iterator *iterator, struct tuple **result)
{
struct session_settings_iterator *it =
(struct session_settings_iterator *)iterator;
- int mid = it->module_id, sid = it->setting_id;
- struct session_setting_module *module;
+ int sid = it->setting_id;
const char *key = it->key;
bool is_including = it->is_including, is_eq = it->is_eq;
bool is_found = false;
- for (; mid >= 0; --mid, sid = INT_MAX) {
- module = &session_setting_modules[mid];
- if (session_settings_prev_in_module(module, &sid, key, is_eq,
- is_including) == 0) {
- is_found = true;
- break;
- }
- }
- it->module_id = mid;
+ if (session_settings_prev(&sid, key, is_eq, is_including) == 0)
+ is_found = true;
it->setting_id = sid - 1;
if (!is_found) {
*result = NULL;
return 0;
}
const char *mp_pair, *mp_pair_end;
- module->get(sid, &mp_pair, &mp_pair_end);
+ session_settings[sid].get(sid, &mp_pair, &mp_pair_end);
*result = box_tuple_new(it->format, mp_pair, mp_pair_end);
return *result != NULL ? 0 : -1;
}
@@ -239,14 +224,10 @@ session_settings_index_create_iterator(struct index *base,
it->format = index->format;
if (!iterator_type_is_reverse(type)) {
it->base.next = session_settings_iterator_next;
- it->module_id = 0;
it->setting_id = 0;
} else {
it->base.next = session_settings_iterator_prev;
- it->module_id = session_setting_type_MAX - 1;
- struct session_setting_module *module =
- &session_setting_modules[it->module_id];
- it->setting_id = module->setting_count - 1;
+ it->setting_id = SESSION_SETTING_COUNT - 1;
}
return (struct iterator *)it;
}
@@ -262,20 +243,14 @@ session_settings_index_get(struct index *base, const char *key,
uint32_t len;
key = mp_decode_str(&key, &len);
key = tt_cstr(key, len);
- struct session_setting_module *module = &session_setting_modules[0];
- struct session_setting_module *end = module + session_setting_type_MAX;
int sid = 0;
- for (; module < end; ++module, sid = 0) {
- if (session_settings_next_in_module(module, &sid, key, true,
- true) == 0)
- goto found;
+ if (session_settings_next(&sid, key, true, true) != 0) {
+ *result = NULL;
+ return 0;
}
- *result = NULL;
- return 0;
-found:;
const char *mp_pair;
const char *mp_pair_end;
- module->get(sid, &mp_pair, &mp_pair_end);
+ session_settings[sid].get(sid, &mp_pair, &mp_pair_end);
*result = box_tuple_new(index->format, mp_pair, mp_pair_end);
return *result != NULL ? 0 : -1;
}
@@ -370,17 +345,11 @@ session_settings_space_execute_update(struct space *space, struct txn *txn,
}
key = mp_decode_str(&key, &key_len);
key = tt_cstr(key, key_len);
- struct session_setting_module *module = &session_setting_modules[0];
- struct session_setting_module *end = module + session_setting_type_MAX;
- for (; module < end; ++module, sid = 0) {
- if (session_settings_next_in_module(module, &sid, key, true,
- true) == 0)
- goto found;
+ if (session_settings_next(&sid, key, true, true) != 0) {
+ *result = NULL;
+ return 0;
}
- *result = NULL;
- return 0;
-found:
- module->get(sid, &old_data, &old_data_end);
+ session_settings[sid].get(sid, &old_data, &old_data_end);
new_data = xrow_update_execute(request->tuple, request->tuple_end,
old_data, old_data_end, format,
&new_size, request->index_base,
@@ -402,7 +371,7 @@ found:
goto finish;
}
}
- if (module->set(sid, new_data) != 0)
+ if (session_settings[sid].set(sid, new_data) != 0)
goto finish;
rc = 0;
finish:
diff --git a/src/box/session_settings.h b/src/box/session_settings.h
index 25490a7e3..de24e3c6f 100644
--- a/src/box/session_settings.h
+++ b/src/box/session_settings.h
@@ -30,29 +30,44 @@
* SUCH DAMAGE.
*/
+#include "field_def.h"
+
/**
- * 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.
+ * Identifiers of all session settings. The identifier of the
+ * option is equal to its place in the sorted list of session
+ * options.
*
- * The types should be ordered in alphabetical order, because the
- * type list is used by setting iterators.
+ * 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 session_setting_type {
- SESSION_SETTING_SQL,
- session_setting_type_MAX,
+enum {
+ SESSION_SETTING_SQL_BEGIN,
+ SESSION_SETTING_SQL_DEFAULT_ENGINE = SESSION_SETTING_SQL_BEGIN,
+ SESSION_SETTING_SQL_DEFER_FOREIGN_KEYS,
+ SESSION_SETTING_SQL_FULL_COLUMN_NAMES,
+ SESSION_SETTING_SQL_FULL_METADATA,
+ SESSION_SETTING_SQL_PARSER_DEBUG,
+ SESSION_SETTING_SQL_RECURSIVE_TRIGGERS,
+ SESSION_SETTING_SQL_REVERSE_UNORDERED_SELECTS,
+ SESSION_SETTING_SQL_SELECT_DEBUG,
+ SESSION_SETTING_SQL_VDBE_DEBUG,
+ SESSION_SETTING_SQL_END,
+ /**
+ * Follow the pattern for groups of settings:
+ * SESSION_SETTING_<N>_BEGIN = SESSION_SETTING_<N-1>_END,
+ * ...
+ * SESSION_SETTING_<N>_END,
+ */
+ SESSION_SETTING_COUNT = SESSION_SETTING_SQL_END,
};
-struct session_setting_module {
+struct session_setting {
/**
- * An array of setting names. All of them should have the
- * same prefix.
+ * Setting's value type. Used for error checking and
+ * reporting only.
*/
- const char **settings;
- /** Count of settings. */
- int setting_count;
+ enum field_type field_type;
/**
* Get a MessagePack encoded pair [name, value] for a
* setting having index @a id. Index is from the settings
@@ -67,4 +82,5 @@ struct session_setting_module {
int (*set)(int id, const char *mp_value);
};
-extern struct session_setting_module session_setting_modules[];
+extern struct session_setting session_settings[SESSION_SETTING_COUNT];
+extern const char *session_setting_strs[SESSION_SETTING_COUNT];
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 7511fad37..a00da31f9 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3322,40 +3322,6 @@ sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
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,
- SQL_SESSION_SETTING_FULL_METADATA,
- SQL_SESSION_SETTING_PARSER_DEBUG,
- SQL_SESSION_SETTING_RECURSIVE_TRIGGERS,
- SQL_SESSION_SETTING_REVERSE_UNORDERED_SELECTS,
- SQL_SESSION_SETTING_SELECT_DEBUG,
- SQL_SESSION_SETTING_VDBE_DEBUG,
- 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",
- "sql_full_metadata",
- "sql_parser_debug",
- "sql_recursive_triggers",
- "sql_reverse_unordered_selects",
- "sql_select_debug",
- "sql_vdbe_debug",
-};
-
/**
* A local structure that allows to establish a connection between
* parameter and its field type and mask, if it has one.
@@ -3373,24 +3339,24 @@ struct sql_option_metadata
* It is IMPORTANT that these options sorted by name.
*/
static struct sql_option_metadata sql_session_opts[] = {
- /** SQL_SESSION_SETTING_DEFAULT_ENGINE */
+ /** SESSION_SETTING_SQL_DEFAULT_ENGINE */
{FIELD_TYPE_STRING, 0},
- /** SQL_SESSION_SETTING_DEFER_FOREIGN_KEYS */
+ /** SESSION_SETTING_SQL_DEFER_FOREIGN_KEYS */
{FIELD_TYPE_BOOLEAN, SQL_DeferFKs},
- /** SQL_SESSION_SETTING_FULL_COLUMN_NAMES */
+ /** SESSION_SETTING_SQL_FULL_COLUMN_NAMES */
{FIELD_TYPE_BOOLEAN, SQL_FullColNames},
- /** SQL_SESSION_SETTING_FULL_METADATA */
+ /** SESSION_SETTING_SQL_FULL_METADATA */
{FIELD_TYPE_BOOLEAN, SQL_FullMetadata},
- /** SQL_SESSION_SETTING_PARSER_DEBUG */
+ /** SESSION_SETTING_SQL_PARSER_DEBUG */
{FIELD_TYPE_BOOLEAN, SQL_SqlTrace | PARSER_TRACE_FLAG},
- /** SQL_SESSION_SETTING_RECURSIVE_TRIGGERS */
+ /** SESSION_SETTING_SQL_RECURSIVE_TRIGGERS */
{FIELD_TYPE_BOOLEAN, SQL_RecTriggers},
- /** SQL_SESSION_SETTING_REVERSE_UNORDERED_SELECTS */
+ /** SESSION_SETTING_SQL_REVERSE_UNORDERED_SELECTS */
{FIELD_TYPE_BOOLEAN, SQL_ReverseOrder},
- /** SQL_SESSION_SETTING_SELECT_DEBUG */
+ /** SESSION_SETTING_SQL_SELECT_DEBUG */
{FIELD_TYPE_BOOLEAN,
SQL_SqlTrace | SQL_SelectTrace | SQL_WhereTrace},
- /** SQL_SESSION_SETTING_VDBE_DEBUG */
+ /** SESSION_SETTING_SQL_VDBE_DEBUG */
{FIELD_TYPE_BOOLEAN,
SQL_SqlTrace | SQL_VdbeListing | SQL_VdbeTrace},
};
@@ -3398,12 +3364,12 @@ static struct sql_option_metadata sql_session_opts[] = {
static void
sql_session_setting_get(int id, const char **mp_pair, const char **mp_pair_end)
{
- assert(id >= 0 && id < sql_session_setting_MAX);
+ assert(id >= SESSION_SETTING_SQL_BEGIN && id < SESSION_SETTING_SQL_END);
struct session *session = current_session();
uint32_t flags = session->sql_flags;
struct sql_option_metadata *opt = &sql_session_opts[id];
uint32_t mask = opt->mask;
- const char *name = sql_session_setting_strs[id];
+ const char *name = session_setting_strs[id];
size_t name_len = strlen(name);
size_t engine_len;
const char *engine;
@@ -3416,7 +3382,7 @@ sql_session_setting_get(int id, const char **mp_pair, const char **mp_pair_end)
if (is_bool) {
size += mp_sizeof_bool(true);
} else {
- assert(id == SQL_SESSION_SETTING_DEFAULT_ENGINE);
+ assert(id == SESSION_SETTING_SQL_DEFAULT_ENGINE);
engine = sql_storage_engine_strs[session->sql_default_engine];
engine_len = strlen(engine);
size += mp_sizeof_str(engine_len);
@@ -3452,7 +3418,7 @@ sql_set_boolean_option(int id, bool value)
session->sql_flags |= option->mask;
else
session->sql_flags &= ~option->mask;
- if (id == SQL_SESSION_SETTING_PARSER_DEBUG) {
+ if (id == SESSION_SETTING_SQL_PARSER_DEBUG) {
if (value)
sqlParserTrace(stdout, "parser: ");
else
@@ -3466,7 +3432,7 @@ 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);
+ assert(id == SESSION_SETTING_SQL_DEFAULT_ENGINE);
(void)id;
enum sql_storage_engine engine = STR2ENUM(sql_storage_engine, value);
if (engine == sql_storage_engine_MAX) {
@@ -3480,7 +3446,7 @@ sql_set_string_option(int id, const char *value)
static int
sql_session_setting_set(int id, const char *mp_value)
{
- assert(id >= 0 && id < sql_session_setting_MAX);
+ assert(id >= SESSION_SETTING_SQL_BEGIN && id < SESSION_SETTING_SQL_END);
enum mp_type mtype = mp_typeof(*mp_value);
enum field_type stype = sql_session_opts[id].field_type;
uint32_t len;
@@ -3500,17 +3466,18 @@ sql_session_setting_set(int id, const char *mp_value)
unreachable();
}
diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
- sql_session_setting_strs[id], field_type_strs[stype]);
+ session_setting_strs[id], field_type_strs[stype]);
return -1;
}
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;
+ for (int id = SESSION_SETTING_SQL_BEGIN; id < SESSION_SETTING_SQL_END;
+ ++id) {
+ struct session_setting *setting = &session_settings[id];
+ setting->field_type = sql_session_opts[id].field_type;
+ setting->get = sql_session_setting_get;
+ setting->set = sql_session_setting_set;
+ }
}
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 26+ messages in thread* [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space
2020-03-30 9:13 [Tarantool-patches] [PATCH 0/4] session settings fixes Chris Sosnin
2020-03-30 9:13 ` [Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array Chris Sosnin
@ 2020-03-30 9:13 ` Chris Sosnin
2020-04-03 14:00 ` Nikita Pettik
2020-03-30 9:13 ` [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings Chris Sosnin
` (4 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Chris Sosnin @ 2020-03-30 9:13 UTC (permalink / raw)
To: v.shpilevoy, korablev, tarantool-patches
As it is mentioned in implementation, it is important
that _session_settings options are sorted by name, so
there is no need in linear lookup and we can replace it
with binary search.
Closes #4712
---
src/box/session_settings.c | 97 +++++++++++++++++--
...1-access-settings-from-any-frontend.result | 37 ++++---
...access-settings-from-any-frontend.test.lua | 28 ++++--
3 files changed, 133 insertions(+), 29 deletions(-)
diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index 2ecf71f2d..5e4a50427 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -81,6 +81,8 @@ struct session_settings_iterator {
bool is_eq;
/** True if the iterator should include equal keys. */
bool is_including;
+ /** True if the iterator is pointing to an existing setting */
+ bool is_set;
};
static void
@@ -92,6 +94,72 @@ session_settings_iterator_free(struct iterator *ptr)
free(it);
}
+static int
+session_settings_set_forward(int *sid, const char *key, bool is_eq,
+ bool is_including)
+{
+ int low = 0, high = SESSION_SETTING_COUNT - 1;
+ if (key == NULL)
+ return 0;
+ while (low <= high) {
+ int index = (high + low) / 2;
+ const char *name = session_setting_strs[index];
+ int cmp = strcmp(name, key);
+ if (cmp == 0) {
+ if (is_including) {
+ *sid = index;
+ return 0;
+ }
+ *sid = ++index;
+ return index < SESSION_SETTING_COUNT ? 0 : -1;
+ }
+ if (cmp < 0)
+ low = index + 1;
+ else
+ high = index - 1;
+ }
+ if (is_eq) {
+ *sid = SESSION_SETTING_COUNT;
+ return -1;
+ }
+ assert(low > high);
+ *sid = low;
+ return low < SESSION_SETTING_COUNT ? 0 : -1;
+}
+
+static int
+session_settings_set_reverse(int *sid, const char *key, bool is_eq,
+ bool is_including)
+{
+ int low = 0, high = SESSION_SETTING_COUNT - 1;
+ if (key == NULL)
+ return 0;
+ while (low <= high) {
+ int index = (high + low) / 2;
+ const char *name = session_setting_strs[index];
+ int cmp = strcmp(name, key);
+ if (cmp == 0) {
+ if (is_including) {
+ *sid = index;
+ return 0;
+ }
+ *sid = --index;
+ return index >= 0 ? 0 : -1;
+ }
+ if (cmp < 0)
+ low = index + 1;
+ else
+ high = index - 1;
+ }
+ if (is_eq) {
+ *sid = SESSION_SETTING_COUNT;
+ return -1;
+ }
+ assert(low > high);
+ *sid = high;
+ return high >= 0 ? 0 : -1;
+}
+
static int
session_settings_next(int *sid, const char *key, bool is_eq, bool is_including)
{
@@ -145,9 +213,15 @@ session_settings_iterator_next(struct iterator *iterator, struct tuple **result)
int sid = it->setting_id;
const char *key = it->key;
bool is_including = it->is_including, is_eq = it->is_eq;
- bool is_found = false;
- if (session_settings_next(&sid, key, is_eq, is_including) == 0)
- is_found = true;
+ bool is_found;
+ if (!it->is_set) {
+ it->is_set = true;
+ is_found = session_settings_set_forward(&sid, key, is_eq,
+ is_including) == 0;
+ } else {
+ is_found = session_settings_next(&sid, key, is_eq,
+ is_including) == 0;
+ }
it->setting_id = sid + 1;
if (!is_found) {
*result = NULL;
@@ -167,9 +241,15 @@ session_settings_iterator_prev(struct iterator *iterator, struct tuple **result)
int sid = it->setting_id;
const char *key = it->key;
bool is_including = it->is_including, is_eq = it->is_eq;
- bool is_found = false;
- if (session_settings_prev(&sid, key, is_eq, is_including) == 0)
- is_found = true;
+ bool is_found;
+ if (!it->is_set) {
+ it->is_set = true;
+ is_found = session_settings_set_reverse(&sid, key, is_eq,
+ is_including) == 0;
+ } else {
+ is_found = session_settings_prev(&sid, key, is_eq,
+ is_including) == 0;
+ }
it->setting_id = sid - 1;
if (!is_found) {
*result = NULL;
@@ -221,6 +301,7 @@ session_settings_index_create_iterator(struct index *base,
it->is_eq = type == ITER_EQ || type == ITER_REQ;
it->is_including = it->is_eq || type == ITER_GE || type == ITER_ALL ||
type == ITER_LE;
+ it->is_set = false;
it->format = index->format;
if (!iterator_type_is_reverse(type)) {
it->base.next = session_settings_iterator_next;
@@ -244,7 +325,7 @@ session_settings_index_get(struct index *base, const char *key,
key = mp_decode_str(&key, &len);
key = tt_cstr(key, len);
int sid = 0;
- if (session_settings_next(&sid, key, true, true) != 0) {
+ if (session_settings_set_forward(&sid, key, true, true) != 0) {
*result = NULL;
return 0;
}
@@ -345,7 +426,7 @@ session_settings_space_execute_update(struct space *space, struct txn *txn,
}
key = mp_decode_str(&key, &key_len);
key = tt_cstr(key, key_len);
- if (session_settings_next(&sid, key, true, true) != 0) {
+ if (session_settings_set_forward(&sid, key, true, true) != 0) {
*result = NULL;
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 ed340d053..bae77192e 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/gh-4511-access-settings-from-any-frontend.result
@@ -82,6 +82,12 @@ function check_sorting(ss, ts, key)
for _, it in pairs(iterators_list) do
local view_space = ss:select({key}, {iterator = it})
local test_space = ts:select({key}, {iterator = it})
+ if #view_space ~= #test_space then
+ return {
+ err = 'bad sorting', type = it, exp = test_space,
+ got = view_space
+ }
+ end
for key, value in pairs(view_space) do
if test_space[key].name ~= value.name then
return {
@@ -111,32 +117,37 @@ check_sorting(s, t, 'sql_d')
check_sorting(s, t, 'sql_v')
| ---
| ...
-check_sorting(s, t, 'sql_defer_foreign_keys')
+check_sorting(s, t, 'z')
| ---
| ...
-
-t:drop()
+check_sorting(s, t, 'sql_parser_debuf')
+ | ---
+ | ...
+check_sorting(s, t, 'sql_parser_debuh')
| ---
| ...
--- Check get() method of session_settings space.
-s:get({'sql_defer_foreign_keys'})
+name = nil
| ---
- | - ['sql_defer_foreign_keys', false]
| ...
-s:get({'sql_recursive_triggers'})
+err = nil
| ---
- | - ['sql_recursive_triggers', true]
| ...
-s:get({'sql_reverse_unordered_selects'})
+for _, tuple in t:pairs() do \
+ name = tuple.name \
+ err = check_sorting(s, t, name) \
+ if err then \
+ break \
+ end \
+end
| ---
- | - ['sql_reverse_unordered_selects', false]
| ...
-s:get({'sql_default_engine'})
+err and {err, name}
| ---
- | - ['sql_default_engine', 'memtx']
+ | - null
| ...
-s:get({'abcd'})
+
+t:drop()
| ---
| ...
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 366874998..b243be15e 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
@@ -36,6 +36,12 @@ function check_sorting(ss, ts, key)
for _, it in pairs(iterators_list) do
local view_space = ss:select({key}, {iterator = it})
local test_space = ts:select({key}, {iterator = it})
+ if #view_space ~= #test_space then
+ return {
+ err = 'bad sorting', type = it, exp = test_space,
+ got = view_space
+ }
+ end
for key, value in pairs(view_space) do
if test_space[key].name ~= value.name then
return {
@@ -52,17 +58,23 @@ 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')
+check_sorting(s, t, 'z')
+check_sorting(s, t, 'sql_parser_debuf')
+check_sorting(s, t, 'sql_parser_debuh')
+
+name = nil
+err = nil
+for _, tuple in t:pairs() do \
+ name = tuple.name \
+ err = check_sorting(s, t, name) \
+ if err then \
+ break \
+ end \
+end
+err and {err, name}
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
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space
2020-03-30 9:13 ` [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space Chris Sosnin
@ 2020-04-03 14:00 ` Nikita Pettik
2020-04-13 13:40 ` Kirill Yukhin
0 siblings, 1 reply; 26+ messages in thread
From: Nikita Pettik @ 2020-04-03 14:00 UTC (permalink / raw)
To: Chris Sosnin; +Cc: tarantool-patches, v.shpilevoy
On 30 Mar 12:13, Chris Sosnin wrote:
> As it is mentioned in implementation, it is important
> that _session_settings options are sorted by name, so
> there is no need in linear lookup and we can replace it
> with binary search.
>
> Closes #4712
> ---
Still I see no need for this patch. It is unclear which improvements
it introduces except for code complication.
> diff --git a/src/box/session_settings.c b/src/box/session_settings.c
> index 2ecf71f2d..5e4a50427 100644
> --- a/src/box/session_settings.c
> +++ b/src/box/session_settings.c
> @@ -81,6 +81,8 @@ struct session_settings_iterator {
> bool is_eq;
> /** True if the iterator should include equal keys. */
> bool is_including;
> + /** True if the iterator is pointing to an existing setting */
> + bool is_set;
> };
>
> static void
> @@ -92,6 +94,72 @@ session_settings_iterator_free(struct iterator *ptr)
> free(it);
> }
>
> +static int
> +session_settings_set_forward(int *sid, const char *key, bool is_eq,
> + bool is_including)
> +{
> + int low = 0, high = SESSION_SETTING_COUNT - 1;
> + if (key == NULL)
> + return 0;
> + while (low <= high) {
> + int index = (high + low) / 2;
> + const char *name = session_setting_strs[index];
> + int cmp = strcmp(name, key);
> + if (cmp == 0) {
> + if (is_including) {
> + *sid = index;
> + return 0;
> + }
> + *sid = ++index;
> + return index < SESSION_SETTING_COUNT ? 0 : -1;
> + }
> + if (cmp < 0)
> + low = index + 1;
> + else
> + high = index - 1;
> + }
> + if (is_eq) {
> + *sid = SESSION_SETTING_COUNT;
> + return -1;
> + }
> + assert(low > high);
> + *sid = low;
> + return low < SESSION_SETTING_COUNT ? 0 : -1;
> +}
> +
> +static int
> +session_settings_set_reverse(int *sid, const char *key, bool is_eq,
> + bool is_including)
> +{
> + int low = 0, high = SESSION_SETTING_COUNT - 1;
> + if (key == NULL)
> + return 0;
> + while (low <= high) {
> + int index = (high + low) / 2;
> + const char *name = session_setting_strs[index];
> + int cmp = strcmp(name, key);
> + if (cmp == 0) {
> + if (is_including) {
> + *sid = index;
> + return 0;
> + }
> + *sid = --index;
> + return index >= 0 ? 0 : -1;
> + }
> + if (cmp < 0)
> + low = index + 1;
> + else
> + high = index - 1;
> + }
> + if (is_eq) {
> + *sid = SESSION_SETTING_COUNT;
> + return -1;
> + }
> + assert(low > high);
> + *sid = high;
> + return high >= 0 ? 0 : -1;
> +}
> +
> static int
> session_settings_next(int *sid, const char *key, bool is_eq, bool is_including)
> {
> @@ -145,9 +213,15 @@ session_settings_iterator_next(struct iterator *iterator, struct tuple **result)
> int sid = it->setting_id;
> const char *key = it->key;
> bool is_including = it->is_including, is_eq = it->is_eq;
> - bool is_found = false;
> - if (session_settings_next(&sid, key, is_eq, is_including) == 0)
> - is_found = true;
> + bool is_found;
> + if (!it->is_set) {
> + it->is_set = true;
> + is_found = session_settings_set_forward(&sid, key, is_eq,
> + is_including) == 0;
> + } else {
> + is_found = session_settings_next(&sid, key, is_eq,
> + is_including) == 0;
> + }
> it->setting_id = sid + 1;
> if (!is_found) {
> *result = NULL;
> @@ -167,9 +241,15 @@ session_settings_iterator_prev(struct iterator *iterator, struct tuple **result)
> int sid = it->setting_id;
> const char *key = it->key;
> bool is_including = it->is_including, is_eq = it->is_eq;
> - bool is_found = false;
> - if (session_settings_prev(&sid, key, is_eq, is_including) == 0)
> - is_found = true;
> + bool is_found;
> + if (!it->is_set) {
> + it->is_set = true;
> + is_found = session_settings_set_reverse(&sid, key, is_eq,
> + is_including) == 0;
> + } else {
> + is_found = session_settings_prev(&sid, key, is_eq,
> + is_including) == 0;
> + }
> it->setting_id = sid - 1;
> if (!is_found) {
> *result = NULL;
> @@ -221,6 +301,7 @@ session_settings_index_create_iterator(struct index *base,
> it->is_eq = type == ITER_EQ || type == ITER_REQ;
> it->is_including = it->is_eq || type == ITER_GE || type == ITER_ALL ||
> type == ITER_LE;
> + it->is_set = false;
> it->format = index->format;
> if (!iterator_type_is_reverse(type)) {
> it->base.next = session_settings_iterator_next;
> @@ -244,7 +325,7 @@ session_settings_index_get(struct index *base, const char *key,
> key = mp_decode_str(&key, &len);
> key = tt_cstr(key, len);
> int sid = 0;
> - if (session_settings_next(&sid, key, true, true) != 0) {
> + if (session_settings_set_forward(&sid, key, true, true) != 0) {
> *result = NULL;
> return 0;
> }
> @@ -345,7 +426,7 @@ session_settings_space_execute_update(struct space *space, struct txn *txn,
> }
> key = mp_decode_str(&key, &key_len);
> key = tt_cstr(key, key_len);
> - if (session_settings_next(&sid, key, true, true) != 0) {
> + if (session_settings_set_forward(&sid, key, true, true) != 0) {
> *result = NULL;
> 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 ed340d053..bae77192e 100644
> --- a/test/box/gh-4511-access-settings-from-any-frontend.result
> +++ b/test/box/gh-4511-access-settings-from-any-frontend.result
> @@ -82,6 +82,12 @@ function check_sorting(ss, ts, key)
> for _, it in pairs(iterators_list) do
> local view_space = ss:select({key}, {iterator = it})
> local test_space = ts:select({key}, {iterator = it})
> + if #view_space ~= #test_space then
> + return {
> + err = 'bad sorting', type = it, exp = test_space,
> + got = view_space
> + }
> + end
> for key, value in pairs(view_space) do
> if test_space[key].name ~= value.name then
> return {
> @@ -111,32 +117,37 @@ check_sorting(s, t, 'sql_d')
> check_sorting(s, t, 'sql_v')
> | ---
> | ...
> -check_sorting(s, t, 'sql_defer_foreign_keys')
> +check_sorting(s, t, 'z')
> | ---
> | ...
> -
> -t:drop()
> +check_sorting(s, t, 'sql_parser_debuf')
> + | ---
> + | ...
> +check_sorting(s, t, 'sql_parser_debuh')
> | ---
> | ...
>
> --- Check get() method of session_settings space.
> -s:get({'sql_defer_foreign_keys'})
> +name = nil
> | ---
> - | - ['sql_defer_foreign_keys', false]
> | ...
> -s:get({'sql_recursive_triggers'})
> +err = nil
> | ---
> - | - ['sql_recursive_triggers', true]
> | ...
> -s:get({'sql_reverse_unordered_selects'})
> +for _, tuple in t:pairs() do \
> + name = tuple.name \
> + err = check_sorting(s, t, name) \
> + if err then \
> + break \
> + end \
> +end
> | ---
> - | - ['sql_reverse_unordered_selects', false]
> | ...
> -s:get({'sql_default_engine'})
> +err and {err, name}
> | ---
> - | - ['sql_default_engine', 'memtx']
> + | - null
> | ...
> -s:get({'abcd'})
> +
> +t:drop()
> | ---
> | ...
>
> 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 366874998..b243be15e 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
> @@ -36,6 +36,12 @@ function check_sorting(ss, ts, key)
> for _, it in pairs(iterators_list) do
> local view_space = ss:select({key}, {iterator = it})
> local test_space = ts:select({key}, {iterator = it})
> + if #view_space ~= #test_space then
> + return {
> + err = 'bad sorting', type = it, exp = test_space,
> + got = view_space
> + }
> + end
> for key, value in pairs(view_space) do
> if test_space[key].name ~= value.name then
> return {
> @@ -52,17 +58,23 @@ 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')
> +check_sorting(s, t, 'z')
> +check_sorting(s, t, 'sql_parser_debuf')
> +check_sorting(s, t, 'sql_parser_debuh')
> +
> +name = nil
> +err = nil
> +for _, tuple in t:pairs() do \
> + name = tuple.name \
> + err = check_sorting(s, t, name) \
> + if err then \
> + break \
> + end \
> +end
> +err and {err, name}
>
> 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
> --
> 2.21.1 (Apple Git-122.3)
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space
2020-04-03 14:00 ` Nikita Pettik
@ 2020-04-13 13:40 ` Kirill Yukhin
0 siblings, 0 replies; 26+ messages in thread
From: Kirill Yukhin @ 2020-04-13 13:40 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches, v.shpilevoy
Hello,
On 03 Apr 14:00, Nikita Pettik wrote:
> On 30 Mar 12:13, Chris Sosnin wrote:
> > As it is mentioned in implementation, it is important
> > that _session_settings options are sorted by name, so
> > there is no need in linear lookup and we can replace it
> > with binary search.
> >
> > Closes #4712
> > ---
>
> Still I see no need for this patch. It is unclear which improvements
> it introduces except for code complication.
I agree, let's drop it.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings
2020-03-30 9:13 [Tarantool-patches] [PATCH 0/4] session settings fixes Chris Sosnin
2020-03-30 9:13 ` [Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array Chris Sosnin
2020-03-30 9:13 ` [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space Chris Sosnin
@ 2020-03-30 9:13 ` Chris Sosnin
2020-04-03 14:47 ` Nikita Pettik
2020-03-30 9:13 ` [Tarantool-patches] [PATCH 4/4] sql: " Chris Sosnin
` (3 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Chris Sosnin @ 2020-03-30 9:13 UTC (permalink / raw)
To: v.shpilevoy, korablev, tarantool-patches
- space_object:update() is hard to use for configuring session settings,
so we provide box.session.settings table, which can be used in a much more
native way.
- Prior to this patch sql settings were not accessible before box.cfg()
call, even though these flags can be set right after session creation.
Part of #4711
---
src/box/lua/session.c | 111 ++++++++++++++++++
src/box/session.cc | 1 +
src/box/session.h | 2 +
src/box/session_settings.c | 16 ++-
src/box/session_settings.h | 3 +
src/box/sql.c | 5 -
...rontend.result => session_settings.result} | 61 ++++++++++
...end.test.lua => session_settings.test.lua} | 20 ++++
8 files changed, 211 insertions(+), 8 deletions(-)
rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (86%)
rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (86%)
diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index c6a600f6f..103bf22fd 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -42,6 +42,8 @@
#include "box/user.h"
#include "box/schema.h"
#include "box/port.h"
+#include "box/session_settings.h"
+#include "tt_static.h"
static const char *sessionlib_name = "box.session";
@@ -411,6 +413,114 @@ lbox_session_on_access_denied(struct lua_State *L)
lbox_push_on_access_denied_event, NULL);
}
+static int
+lbox_session_setting_get_by_id(struct lua_State *L, int sid)
+{
+ assert(sid >= 0 && sid < SESSION_SETTING_COUNT);
+ const char *mp_pair, *mp_pair_end;
+ session_settings[sid].get(sid, &mp_pair, &mp_pair_end);
+ uint32_t len;
+ mp_decode_array(&mp_pair);
+ mp_decode_str(&mp_pair, &len);
+ enum field_type field_type = session_settings[sid].field_type;
+ if (field_type == FIELD_TYPE_BOOLEAN) {
+ bool value = mp_decode_bool(&mp_pair);
+ lua_pushboolean(L, value);
+ } else {
+ assert(field_type == FIELD_TYPE_STRING);
+ const char *str = mp_decode_str(&mp_pair, &len);
+ lua_pushlstring(L, str, len);
+ }
+ return 1;
+}
+
+static int
+lbox_session_setting_get(struct lua_State *L)
+{
+ assert(lua_gettop(L) == 2);
+ const char *setting_name = lua_tostring(L, -1);
+ int sid = session_setting_find(setting_name);
+ if (sid < 0) {
+ diag_set(ClientError, ER_PROC_LUA, tt_sprintf("Session "\
+ "setting %s doesn't exist", setting_name));
+ return luaT_error(L);
+ }
+ return lbox_session_setting_get_by_id(L, sid);
+}
+
+static int
+lbox_session_setting_set(struct lua_State *L)
+{
+ assert(lua_gettop(L) == 3);
+ int arg_type = lua_type(L, -1);
+ const char *setting_name = lua_tostring(L, -2);
+ int sid = session_setting_find(setting_name);
+ if (sid < 0) {
+ diag_set(ClientError, ER_PROC_LUA, tt_sprintf("Session "\
+ "setting %s doesn't exist", setting_name));
+ return luaT_error(L);
+ }
+ struct session_setting *setting = &session_settings[sid];
+ switch (arg_type) {
+ case LUA_TBOOLEAN: {
+ bool value = lua_toboolean(L, -1);
+ size_t size = mp_sizeof_bool(value);
+ char *mp_value = (char *) static_alloc(size);
+ mp_encode_bool(mp_value, value);
+ if (setting->set(sid, mp_value) != 0)
+ return luaT_error(L);
+ break;
+ }
+ case LUA_TSTRING: {
+ const char *str = lua_tostring(L, -1);
+ size_t len = strlen(str);
+ uint32_t size = mp_sizeof_str(len);
+ char *mp_value = (char *) static_alloc(size);
+ if (mp_value == NULL) {
+ diag_set(OutOfMemory, size, "static_alloc",
+ "mp_value");
+ return luaT_error(L);
+ }
+ mp_encode_str(mp_value, str, len);
+ if (setting->set(sid, mp_value) != 0)
+ return luaT_error(L);
+ break;
+ }
+ default:
+ diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+ session_setting_strs[sid],
+ field_type_strs[setting->field_type]);
+ return luaT_error(L);
+ }
+ return 0;
+}
+
+static int
+lbox_session_settings_serialize(struct lua_State *L)
+{
+ lua_newtable(L);
+ for (int id = 0; id < SESSION_SETTING_COUNT; ++id) {
+ lbox_session_setting_get_by_id(L, id);
+ lua_setfield(L, -2, session_setting_strs[id]);
+ }
+ return 1;
+}
+
+static void
+lbox_session_settings_init(struct lua_State *L)
+{
+ lua_newtable(L);
+ lua_createtable(L, 0, 3);
+ lua_pushcfunction(L, lbox_session_settings_serialize);
+ lua_setfield(L, -2, "__serialize");
+ lua_pushcfunction(L, lbox_session_setting_get);
+ lua_setfield(L, -2, "__index");
+ lua_pushcfunction(L, lbox_session_setting_set);
+ lua_setfield(L, -2, "__newindex");
+ lua_setmetatable(L, -2);
+ lua_setfield(L, -2, "settings");
+}
+
void
session_storage_cleanup(int sid)
{
@@ -478,5 +588,6 @@ box_lua_session_init(struct lua_State *L)
{NULL, NULL}
};
luaL_register_module(L, sessionlib_name, sessionlib);
+ lbox_session_settings_init(L);
lua_pop(L, 1);
}
diff --git a/src/box/session.cc b/src/box/session.cc
index 881318252..b557eed62 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -283,6 +283,7 @@ session_init()
panic("out of memory");
mempool_create(&session_pool, &cord()->slabc, sizeof(struct session));
credentials_create(&admin_credentials, admin_user);
+ sql_session_settings_init();
}
void
diff --git a/src/box/session.h b/src/box/session.h
index 6dfc7cba5..1c47b8986 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -41,6 +41,8 @@
extern "C" {
#endif /* defined(__cplusplus) */
+extern void sql_session_settings_init();
+
struct port;
struct session_vtab;
diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index 5e4a50427..15ab65a97 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -324,8 +324,8 @@ session_settings_index_get(struct index *base, const char *key,
uint32_t len;
key = mp_decode_str(&key, &len);
key = tt_cstr(key, len);
- int sid = 0;
- if (session_settings_set_forward(&sid, key, true, true) != 0) {
+ int sid = session_setting_find(key);
+ if (sid < 0) {
*result = NULL;
return 0;
}
@@ -426,7 +426,8 @@ session_settings_space_execute_update(struct space *space, struct txn *txn,
}
key = mp_decode_str(&key, &key_len);
key = tt_cstr(key, key_len);
- if (session_settings_set_forward(&sid, key, true, true) != 0) {
+ sid = session_setting_find(key);
+ if (sid < 0) {
*result = NULL;
return 0;
}
@@ -520,3 +521,12 @@ const struct space_vtab session_settings_space_vtab = {
/* .prepare_alter = */ generic_space_prepare_alter,
/* .invalidate = */ generic_space_invalidate,
};
+
+int
+session_setting_find(const char *name) {
+ int sid;
+ if (session_settings_set_forward(&sid, name, true, true) == 0)
+ return sid;
+ else
+ return -1;
+}
diff --git a/src/box/session_settings.h b/src/box/session_settings.h
index de24e3c6f..e2adc5289 100644
--- a/src/box/session_settings.h
+++ b/src/box/session_settings.h
@@ -84,3 +84,6 @@ struct session_setting {
extern struct session_setting session_settings[SESSION_SETTING_COUNT];
extern const char *session_setting_strs[SESSION_SETTING_COUNT];
+
+int
+session_setting_find(const char *name);
diff --git a/src/box/sql.c b/src/box/sql.c
index 1256df856..ba98ce5df 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -64,14 +64,9 @@ static const uint32_t default_sql_flags = SQL_EnableTrigger
| 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/test/box/gh-4511-access-settings-from-any-frontend.result b/test/box/session_settings.result
similarity index 86%
rename from test/box/gh-4511-access-settings-from-any-frontend.result
rename to test/box/session_settings.result
index bae77192e..b32a0becb 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/session_settings.result
@@ -298,3 +298,64 @@ s:update('sql_defer_foreign_keys', {{'=', 'value', '1'}})
| ---
| - error: Session setting sql_defer_foreign_keys expected a value of type boolean
| ...
+
+-- gh-4711: Provide a user-friendly frontend for accessing session settings.
+settings = box.session.settings
+ | ---
+ | ...
+assert(settings ~= nil)
+ | ---
+ | - true
+ | ...
+
+s:update('sql_default_engine', {{'=', 2, 'vinyl'}})
+ | ---
+ | - ['sql_default_engine', 'vinyl']
+ | ...
+settings.sql_default_engine
+ | ---
+ | - vinyl
+ | ...
+settings.sql_default_engine = 'memtx'
+ | ---
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - memtx
+ | ...
+settings.sql_defer_foreign_keys = true
+ | ---
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - true
+ | ...
+s:update('sql_defer_foreign_keys', {{'=', 2, false}})
+ | ---
+ | - ['sql_defer_foreign_keys', false]
+ | ...
+settings.sql_defer_foreign_keys
+ | ---
+ | - false
+ | ...
+
+settings.sql_default_engine = true
+ | ---
+ | - error: Session setting sql_default_engine expected a value of type string
+ | ...
+settings.sql_defer_foreign_keys = 'false'
+ | ---
+ | - error: Session setting sql_defer_foreign_keys expected a value of type boolean
+ | ...
+settings.sql_parser_debug = 'string'
+ | ---
+ | - error: Session setting sql_parser_debug expected a value of type boolean
+ | ...
+
+str = string.rep('a', 20 * 1024)
+ | ---
+ | ...
+box.session.settings.sql_default_engine = str
+ | ---
+ | - error: Failed to allocate 20483 bytes in static_alloc for mp_value
+ | ...
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/session_settings.test.lua
similarity index 86%
rename from test/box/gh-4511-access-settings-from-any-frontend.test.lua
rename to test/box/session_settings.test.lua
index b243be15e..440bef7ce 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.test.lua
+++ b/test/box/session_settings.test.lua
@@ -118,3 +118,23 @@ 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'}})
+
+-- gh-4711: Provide a user-friendly frontend for accessing session settings.
+settings = box.session.settings
+assert(settings ~= nil)
+
+s:update('sql_default_engine', {{'=', 2, 'vinyl'}})
+settings.sql_default_engine
+settings.sql_default_engine = 'memtx'
+s:get('sql_default_engine').value
+settings.sql_defer_foreign_keys = true
+s:get('sql_defer_foreign_keys').value
+s:update('sql_defer_foreign_keys', {{'=', 2, false}})
+settings.sql_defer_foreign_keys
+
+settings.sql_default_engine = true
+settings.sql_defer_foreign_keys = 'false'
+settings.sql_parser_debug = 'string'
+
+str = string.rep('a', 20 * 1024)
+box.session.settings.sql_default_engine = str
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 26+ messages in thread* [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings
2020-03-30 9:13 [Tarantool-patches] [PATCH 0/4] session settings fixes Chris Sosnin
` (2 preceding siblings ...)
2020-03-30 9:13 ` [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings Chris Sosnin
@ 2020-03-30 9:13 ` Chris Sosnin
2020-04-03 15:19 ` Nikita Pettik
2020-04-04 21:56 ` Vladislav Shpilevoy
2020-04-02 9:14 ` [Tarantool-patches] [PATCH 0/4] session settings fixes Timur Safin
` (2 subsequent siblings)
6 siblings, 2 replies; 26+ messages in thread
From: Chris Sosnin @ 2020-03-30 9:13 UTC (permalink / raw)
To: v.shpilevoy, korablev, tarantool-patches
Currently if a user wants to change session setting with SQL, one has
to execute UPDATE query like:
[[UPDATE "_session_settings" SET "value" = true WHERE "name" = 'name']]
However, direct access to system spaces isn't considered to be a good practice.
To avoid that and a bit simplify user's life, we introduce SQL shortcut command
SETTING SET.
Closes #4711
@TarantoolBot document
Title: API for accessing _session_settings space.
There are two ways of updating values of session settings:
via Lua and SQL.
Lua:
box.session.settings is a table, which is always accessible
to user. The syntax is the following:
`box.session.settings.<setting_name> = <new_value>`.
Example of usage:
```
tarantool> box.session.settings.sql_default_engine
---
- memtx
...
tarantool> box.session.settings.sql_default_engine = 'vinyl'
---
...
```
The table itself represents the (unordered) result of select
from _session_settings space.
SQL:
Instead of typing long UPDATE query one can use the SETTING SET command:
`box.execute([[SETTING SET "<setting_name>" = <new_value>]])`.
Note, that this query is case sensitive so the name must be quoted.
Also, SETTING SET doesn't prove any implicit casts, so <new_value> must
be of the type corresponding to the setting being updated.
Example:
```
tarantool> box.execute([[setting set "sql_default_engine" = 'memtx']])
---
- row_count: 1
...
tarantool> box.execute([[setting set "sql_defer_foreign_keys" = true]])
---
- row_count: 1
...
```
---
| 1 +
src/box/sql/build.c | 25 +++++++++++++++
src/box/sql/parse.y | 5 +++
src/box/sql/sqlInt.h | 11 +++++++
src/box/sql/vdbe.c | 50 ++++++++++++++++++++++++++++++
test/box/session_settings.result | 49 +++++++++++++++++++++++++++++
test/box/session_settings.test.lua | 13 ++++++++
7 files changed, 154 insertions(+)
--git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
index 006285622..887853529 100644
--- a/extra/mkkeywordhash.c
+++ b/extra/mkkeywordhash.c
@@ -159,6 +159,7 @@ static Keyword aKeywordTable[] = {
{ "SCALAR", "TK_SCALAR", true },
{ "SELECT", "TK_SELECT", true },
{ "SET", "TK_SET", true },
+ { "SETTING", "TK_SETTING", false },
{ "SIMPLE", "TK_SIMPLE", true },
{ "START", "TK_START", true },
{ "STRING", "TK_STRING_KW", true },
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a00da31f9..cdcf8b6d8 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3481,3 +3481,28 @@ sql_session_settings_init()
setting->set = sql_session_setting_set;
}
}
+
+void
+sql_setting_set(struct Parse *parse_context, struct Token *name,
+ struct Expr *expr)
+{
+ struct Vdbe *vdbe = sqlGetVdbe(parse_context);
+ if (vdbe == NULL)
+ goto abort;
+ sqlVdbeCountChanges(vdbe);
+ char *key = sql_name_from_token(parse_context->db, name);
+ if (key == NULL)
+ goto abort;
+ int index = session_setting_find(key);
+ if (index >= 0) {
+ int target = ++parse_context->nMem;
+ sqlExprCode(parse_context, expr, target);
+ sqlVdbeAddOp2(vdbe, OP_SetSetting, index, target);
+ return;
+ }
+ diag_set(ClientError, ER_SQL_PARSER_GENERIC,
+ tt_sprintf("Session setting %s doesn't exist", key));
+abort:
+ parse_context->is_aborted = true;
+ return;
+}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 1a0e89703..56977b003 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1541,6 +1541,11 @@ cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y). {
sql_drop_index(pParse);
}
+///////////////////////////// The SETTING SET command ////////////////////////
+cmd ::= SETTING SET nm(X) EQ expr(Y). {
+ sql_setting_set(pParse,&X,Y.pExpr);
+}
+
///////////////////////////// The PRAGMA command /////////////////////////////
//
cmd ::= PRAGMA nm(X). {
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 1579cc92e..a700dd9c9 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4513,4 +4513,15 @@ int
sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
uint32_t *fieldno);
+/**
+ * Create VDBE instructions to set the new value of the session setting.
+ *
+ * @param parse_context Parsing context.
+ * @param name Name of the session setting.
+ * @param value New value of the session setting.
+ */
+void
+sql_setting_set(struct Parse *parse_context, struct Token *name,
+ struct Expr *value);
+
#endif /* sqlINT_H */
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index e8a029a8a..d6b753a51 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -55,6 +55,7 @@
#include "box/schema.h"
#include "box/space.h"
#include "box/sequence.h"
+#include "box/session_settings.h"
/*
* Invoke this macro on memory cells just prior to changing the
@@ -5252,6 +5253,55 @@ case OP_IncMaxid: {
break;
}
+/* Opcode: SetSetting P1 P2 * * *
+ *
+ * Set the new value of the session setting. P1 is the id of the
+ * setting in the session_settings array, P2 is the register
+ * holding a value.
+ */
+case OP_SetSetting: {
+ int sid = pOp->p1;
+ pIn1 = &aMem[pOp->p2];
+ assert(sid >= 0 && sid < SESSION_SETTING_COUNT);
+ struct session_setting *setting = &session_settings[sid];
+ switch (setting->field_type) {
+ case FIELD_TYPE_BOOLEAN: {
+ if ((pIn1->flags & MEM_Bool) == 0)
+ goto invalid_type;
+ bool value = pIn1->u.b;
+ size_t size = mp_sizeof_bool(value);
+ char *mp_value = (char *) static_alloc(size);
+ mp_encode_bool(mp_value, value);
+ if (setting->set(sid, mp_value) != 0)
+ goto abort_due_to_error;
+ break;
+ }
+ case FIELD_TYPE_STRING: {
+ if ((pIn1->flags & MEM_Str) == 0)
+ goto invalid_type;
+ const char *str = pIn1->z;
+ uint32_t size = mp_sizeof_str(pIn1->n);
+ char *mp_value = (char *) static_alloc(size);
+ if (mp_value == NULL) {
+ diag_set(OutOfMemory, size, "static_alloc", "mp_value");
+ goto abort_due_to_error;
+ }
+ mp_encode_str(mp_value, str, pIn1->n);
+ if (setting->set(sid, mp_value) != 0)
+ goto abort_due_to_error;
+ break;
+ }
+ default:
+ invalid_type:
+ diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+ session_setting_strs[sid],
+ field_type_strs[setting->field_type]);
+ goto abort_due_to_error;
+ }
+ p->nChange++;
+ break;
+}
+
/* Opcode: Noop * * * * *
*
* Do nothing. This instruction is often useful as a jump
diff --git a/test/box/session_settings.result b/test/box/session_settings.result
index b32a0becb..28c79379b 100644
--- a/test/box/session_settings.result
+++ b/test/box/session_settings.result
@@ -339,6 +339,39 @@ settings.sql_defer_foreign_keys
| - false
| ...
+box.execute([[setting set "sql_default_engine" = 'vinyl']])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - vinyl
+ | ...
+box.execute([[setting set "sql_default_engine" = 'memtx']])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - memtx
+ | ...
+box.execute([[setting set "sql_defer_foreign_keys" = true]])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - true
+ | ...
+box.execute([[setting set "sql_defer_foreign_keys" = ?]], {false})
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - false
+ | ...
+
settings.sql_default_engine = true
| ---
| - error: Session setting sql_default_engine expected a value of type string
@@ -359,3 +392,19 @@ box.session.settings.sql_default_engine = str
| ---
| - error: Failed to allocate 20483 bytes in static_alloc for mp_value
| ...
+
+box.execute([[setting set "sql_def_engine" = true]])
+ | ---
+ | - null
+ | - Session setting sql_def_engine doesn't exist
+ | ...
+box.execute([[setting set "sql_default_engine" = true]])
+ | ---
+ | - null
+ | - Session setting sql_default_engine expected a value of type string
+ | ...
+box.execute([[setting set "sql_defer_foreign_keys" = 'true']])
+ | ---
+ | - null
+ | - Session setting sql_defer_foreign_keys expected a value of type boolean
+ | ...
diff --git a/test/box/session_settings.test.lua b/test/box/session_settings.test.lua
index 440bef7ce..c477139f9 100644
--- a/test/box/session_settings.test.lua
+++ b/test/box/session_settings.test.lua
@@ -132,9 +132,22 @@ s:get('sql_defer_foreign_keys').value
s:update('sql_defer_foreign_keys', {{'=', 2, false}})
settings.sql_defer_foreign_keys
+box.execute([[setting set "sql_default_engine" = 'vinyl']])
+s:get('sql_default_engine').value
+box.execute([[setting set "sql_default_engine" = 'memtx']])
+s:get('sql_default_engine').value
+box.execute([[setting set "sql_defer_foreign_keys" = true]])
+s:get('sql_defer_foreign_keys').value
+box.execute([[setting set "sql_defer_foreign_keys" = ?]], {false})
+s:get('sql_defer_foreign_keys').value
+
settings.sql_default_engine = true
settings.sql_defer_foreign_keys = 'false'
settings.sql_parser_debug = 'string'
str = string.rep('a', 20 * 1024)
box.session.settings.sql_default_engine = str
+
+box.execute([[setting set "sql_def_engine" = true]])
+box.execute([[setting set "sql_default_engine" = true]])
+box.execute([[setting set "sql_defer_foreign_keys" = 'true']])
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings
2020-03-30 9:13 ` [Tarantool-patches] [PATCH 4/4] sql: " Chris Sosnin
@ 2020-04-03 15:19 ` Nikita Pettik
2020-04-04 21:56 ` Vladislav Shpilevoy
1 sibling, 0 replies; 26+ messages in thread
From: Nikita Pettik @ 2020-04-03 15:19 UTC (permalink / raw)
To: Chris Sosnin; +Cc: tarantool-patches, v.shpilevoy
On 30 Mar 12:13, Chris Sosnin wrote:
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index a00da31f9..cdcf8b6d8 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -3481,3 +3481,28 @@ sql_session_settings_init()
> setting->set = sql_session_setting_set;
> }
> }
> +
> +void
> +sql_setting_set(struct Parse *parse_context, struct Token *name,
> + struct Expr *expr)
> +{
> + struct Vdbe *vdbe = sqlGetVdbe(parse_context);
> + if (vdbe == NULL)
> + goto abort;
> + sqlVdbeCountChanges(vdbe);
> + char *key = sql_name_from_token(parse_context->db, name);
> + if (key == NULL)
> + goto abort;
> + int index = session_setting_find(key);
> + if (index >= 0) {
> + int target = ++parse_context->nMem;
> + sqlExprCode(parse_context, expr, target);
> + sqlVdbeAddOp2(vdbe, OP_SetSetting, index, target);
> + return;
> + }
> + diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> + tt_sprintf("Session setting %s doesn't exist", key));
Mb it is worth moving this check to runtime? I.e. pass to OP_SetSetting
name of setting to be set. The less appeals to system spaces and
cache look ups are made, the better parsing process is organized.
The rest is ok.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings
2020-03-30 9:13 ` [Tarantool-patches] [PATCH 4/4] sql: " Chris Sosnin
2020-04-03 15:19 ` Nikita Pettik
@ 2020-04-04 21:56 ` Vladislav Shpilevoy
2020-04-10 15:40 ` Chris Sosnin
1 sibling, 1 reply; 26+ messages in thread
From: Vladislav Shpilevoy @ 2020-04-04 21:56 UTC (permalink / raw)
To: Chris Sosnin, korablev, tarantool-patches
Hi! Thanks for the patch!
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index a00da31f9..cdcf8b6d8 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -3481,3 +3481,28 @@ sql_session_settings_init()
> setting->set = sql_session_setting_set;
> }
> }
> +
> +void
> +sql_setting_set(struct Parse *parse_context, struct Token *name,
> + struct Expr *expr)
> +{
> + struct Vdbe *vdbe = sqlGetVdbe(parse_context);
> + if (vdbe == NULL)
> + goto abort;
> + sqlVdbeCountChanges(vdbe);
> + char *key = sql_name_from_token(parse_context->db, name);
> + if (key == NULL)
> + goto abort;
> + int index = session_setting_find(key);
I agree with Nikita. Better make this call part of OP_SetSetting.
> + if (index >= 0) {
> + int target = ++parse_context->nMem;
> + sqlExprCode(parse_context, expr, target);
> + sqlVdbeAddOp2(vdbe, OP_SetSetting, index, target);
> + return;
> + }
> + diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> + tt_sprintf("Session setting %s doesn't exist", key));
> +abort:
> + parse_context->is_aborted = true;
> + return;
> +}
Talking of the syntax, I don't have a strong opinion here
regarding any particular option. I only want the new statement
be short, and not involving _session_settings manipulations.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings
2020-04-04 21:56 ` Vladislav Shpilevoy
@ 2020-04-10 15:40 ` Chris Sosnin
2020-04-11 17:18 ` Vladislav Shpilevoy
2020-04-13 7:50 ` Timur Safin
0 siblings, 2 replies; 26+ messages in thread
From: Chris Sosnin @ 2020-04-10 15:40 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches
> > diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> > index a00da31f9..cdcf8b6d8 100644
> > --- a/src/box/sql/build.c
> > +++ b/src/box/sql/build.c
> > @@ -3481,3 +3481,28 @@ sql_session_settings_init()
> > setting->set = sql_session_setting_set;
> > }
> > }
> > +
> > +void
> > +sql_setting_set(struct Parse *parse_context, struct Token *name,
> > + struct Expr *expr)
> > +{
> > + struct Vdbe *vdbe = sqlGetVdbe(parse_context);
> > + if (vdbe == NULL)
> > + goto abort;
> > + sqlVdbeCountChanges(vdbe);
> > + char *key = sql_name_from_token(parse_context->db, name);
> > + if (key == NULL)
> > + goto abort;
> > + int index = session_setting_find(key);
>
> I agree with Nikita. Better make this call part of OP_SetSetting.
I agree, fixed:
+
+void
+sql_setting_set(struct Parse *parse_context, struct Token *name,
+ struct Expr *expr)
+{
+ struct Vdbe *vdbe = sqlGetVdbe(parse_context);
+ if (vdbe == NULL)
+ goto abort;
+ sqlVdbeCountChanges(vdbe);
+ char *key = sql_name_from_token(parse_context->db, name);
+ if (key == NULL)
+ goto abort;
+ int target = ++parse_context->nMem;
+ sqlExprCode(parse_context, expr, target);
+ sqlVdbeAddOp4(vdbe, OP_SetSession, target, 0, 0, key, P4_DYNAMIC);
+ return;
+abort:
+ parse_context->is_aborted = true;
+ return;
+}
+/* Opcode: SetSession P1 * * P4 *
+ *
+ * Set new value of the session setting. P4 is the name of the
+ * setting being updated, P1 is the register holding a value.
+ */
+case OP_SetSession: {
+ assert(pOp->p4type == P4_DYNAMIC);
+ const char *setting_name = pOp->p4.z;
+ int sid = session_setting_find(setting_name);
+ if (sid < 0) {
+ diag_set(ClientError, ER_NO_SUCH_SESSION_SETTING, setting_name);
+ goto abort_due_to_error;
+ }
+ pIn1 = &aMem[pOp->p1];
...
> Talking of the syntax, I don't have a strong opinion here
> regarding any particular option. I only want the new statement
> be short, and not involving _session_settings manipulations.
I changed it to the following (the main problem with reserved word persists):
+///////////////////////////// The SET SESSION command ////////////////////////
+//
+cmd ::= SET SESSION nm(X) EQ term(Y). {
+ sql_setting_set(pParse,&X,Y.pExpr);
+}
+
I also changed the 3rd commit (Lua frontend) in the series to introduce new error
code (it's present in the diff above):
+ /*213 */_(ER_NO_SUCH_SESSION_SETTING, "Session setting %s doesn't exist") \
as long as searching is moved to vdbe.c and raising parsing error is not correct.
You can see the whole patch below:
===============================================================================
Currently if a user wants to change session setting with SQL, one has
to execute UPDATE query like:
[[UPDATE "_session_settings" SET "value" = true WHERE "name" = 'name']]
However, direct access to system spaces isn't considered to be a good practice.
To avoid that and a bit simplify user's life, we introduce SQL shortcut command
SET SESSION.
Closes #4711
@TarantoolBot document
Title: API for accessing _session_settings space.
There are two ways of updating values of session settings:
via Lua and SQL.
Lua:
box.session.settings is a table, which is always accessible
to user. The syntax is the following:
`box.session.settings.<setting_name> = <new_value>`.
Example of usage:
```
tarantool> box.session.settings.sql_default_engine
---
- memtx
...
tarantool> box.session.settings.sql_default_engine = 'vinyl'
---
...
```
The table itself represents the (unordered) result of select
from _session_settings space.
SQL:
Instead of typing long UPDATE query one can use the SET SESSION command:
`box.execute([[SET SESSION "<setting_name>" = <new_value>]])`.
Note, that this query is case sensitive so the name must be quoted.
Also, SET SESSION doesn't provide any implicit casts, so <new_value> must
be of the type corresponding to the setting being updated.
Example:
```
tarantool> box.execute([[set session "sql_default_engine" = 'memtx']])
---
- row_count: 1
...
tarantool> box.execute([[set session "sql_defer_foreign_keys" = true]])
---
- row_count: 1
...
```
---
| 1 +
src/box/sql/build.c | 20 +++++++++++
src/box/sql/parse.y | 6 ++++
src/box/sql/sqlInt.h | 11 ++++++
src/box/sql/vdbe.c | 54 ++++++++++++++++++++++++++++++
test/box/session_settings.result | 49 +++++++++++++++++++++++++++
test/box/session_settings.test.lua | 13 +++++++
7 files changed, 154 insertions(+)
--git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
index 006285622..dd42c8f5f 100644
--- a/extra/mkkeywordhash.c
+++ b/extra/mkkeywordhash.c
@@ -158,6 +158,7 @@ static Keyword aKeywordTable[] = {
{ "SAVEPOINT", "TK_SAVEPOINT", true },
{ "SCALAR", "TK_SCALAR", true },
{ "SELECT", "TK_SELECT", true },
+ { "SESSION", "TK_SESSION", false },
{ "SET", "TK_SET", true },
{ "SIMPLE", "TK_SIMPLE", true },
{ "START", "TK_START", true },
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a00da31f9..aecba4177 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3481,3 +3481,23 @@ sql_session_settings_init()
setting->set = sql_session_setting_set;
}
}
+
+void
+sql_setting_set(struct Parse *parse_context, struct Token *name,
+ struct Expr *expr)
+{
+ struct Vdbe *vdbe = sqlGetVdbe(parse_context);
+ if (vdbe == NULL)
+ goto abort;
+ sqlVdbeCountChanges(vdbe);
+ char *key = sql_name_from_token(parse_context->db, name);
+ if (key == NULL)
+ goto abort;
+ int target = ++parse_context->nMem;
+ sqlExprCode(parse_context, expr, target);
+ sqlVdbeAddOp4(vdbe, OP_SetSession, target, 0, 0, key, P4_DYNAMIC);
+ return;
+abort:
+ parse_context->is_aborted = true;
+ return;
+}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 1a0e89703..995875566 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1541,6 +1541,12 @@ cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y). {
sql_drop_index(pParse);
}
+///////////////////////////// The SET SESSION command ////////////////////////
+//
+cmd ::= SET SESSION nm(X) EQ term(Y). {
+ sql_setting_set(pParse,&X,Y.pExpr);
+}
+
///////////////////////////// The PRAGMA command /////////////////////////////
//
cmd ::= PRAGMA nm(X). {
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 2d6bad424..e45a7671d 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4517,4 +4517,15 @@ int
sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
uint32_t *fieldno);
+/**
+ * Create VDBE instructions to set the new value of the session setting.
+ *
+ * @param parse_context Parsing context.
+ * @param name Name of the session setting.
+ * @param value New value of the session setting.
+ */
+void
+sql_setting_set(struct Parse *parse_context, struct Token *name,
+ struct Expr *value);
+
#endif /* sqlINT_H */
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index e8a029a8a..c3256c15a 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -55,6 +55,7 @@
#include "box/schema.h"
#include "box/space.h"
#include "box/sequence.h"
+#include "box/session_settings.h"
/*
* Invoke this macro on memory cells just prior to changing the
@@ -5252,6 +5253,59 @@ case OP_IncMaxid: {
break;
}
+/* Opcode: SetSession P1 * * P4 *
+ *
+ * Set new value of the session setting. P4 is the name of the
+ * setting being updated, P1 is the register holding a value.
+ */
+case OP_SetSession: {
+ assert(pOp->p4type == P4_DYNAMIC);
+ const char *setting_name = pOp->p4.z;
+ int sid = session_setting_find(setting_name);
+ if (sid < 0) {
+ diag_set(ClientError, ER_NO_SUCH_SESSION_SETTING, setting_name);
+ goto abort_due_to_error;
+ }
+ pIn1 = &aMem[pOp->p1];
+ struct session_setting *setting = &session_settings[sid];
+ switch (setting->field_type) {
+ case FIELD_TYPE_BOOLEAN: {
+ if ((pIn1->flags & MEM_Bool) == 0)
+ goto invalid_type;
+ bool value = pIn1->u.b;
+ size_t size = mp_sizeof_bool(value);
+ char *mp_value = (char *) static_alloc(size);
+ mp_encode_bool(mp_value, value);
+ if (setting->set(sid, mp_value) != 0)
+ goto abort_due_to_error;
+ break;
+ }
+ case FIELD_TYPE_STRING: {
+ if ((pIn1->flags & MEM_Str) == 0)
+ goto invalid_type;
+ const char *str = pIn1->z;
+ uint32_t size = mp_sizeof_str(pIn1->n);
+ char *mp_value = (char *) static_alloc(size);
+ if (mp_value == NULL) {
+ diag_set(OutOfMemory, size, "static_alloc", "mp_value");
+ goto abort_due_to_error;
+ }
+ mp_encode_str(mp_value, str, pIn1->n);
+ if (setting->set(sid, mp_value) != 0)
+ goto abort_due_to_error;
+ break;
+ }
+ default:
+ invalid_type:
+ diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+ session_setting_strs[sid],
+ field_type_strs[setting->field_type]);
+ goto abort_due_to_error;
+ }
+ p->nChange++;
+ break;
+}
+
/* Opcode: Noop * * * * *
*
* Do nothing. This instruction is often useful as a jump
diff --git a/test/box/session_settings.result b/test/box/session_settings.result
index b32a0becb..b82bdda94 100644
--- a/test/box/session_settings.result
+++ b/test/box/session_settings.result
@@ -339,6 +339,39 @@ settings.sql_defer_foreign_keys
| - false
| ...
+box.execute([[set session "sql_default_engine" = 'vinyl']])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - vinyl
+ | ...
+box.execute([[set session "sql_default_engine" = 'memtx']])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - memtx
+ | ...
+box.execute([[set session "sql_defer_foreign_keys" = true]])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - true
+ | ...
+box.execute([[set session "sql_defer_foreign_keys" = false]])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - false
+ | ...
+
settings.sql_default_engine = true
| ---
| - error: Session setting sql_default_engine expected a value of type string
@@ -359,3 +392,19 @@ box.session.settings.sql_default_engine = str
| ---
| - error: Failed to allocate 20483 bytes in static_alloc for mp_value
| ...
+
+box.execute([[set session "sql_def_engine" = true]])
+ | ---
+ | - null
+ | - Session setting sql_def_engine doesn't exist
+ | ...
+box.execute([[set session "sql_default_engine" = true]])
+ | ---
+ | - null
+ | - Session setting sql_default_engine expected a value of type string
+ | ...
+box.execute([[set session "sql_defer_foreign_keys" = 'true']])
+ | ---
+ | - null
+ | - Session setting sql_defer_foreign_keys expected a value of type boolean
+ | ...
diff --git a/test/box/session_settings.test.lua b/test/box/session_settings.test.lua
index 440bef7ce..d4aac0286 100644
--- a/test/box/session_settings.test.lua
+++ b/test/box/session_settings.test.lua
@@ -132,9 +132,22 @@ s:get('sql_defer_foreign_keys').value
s:update('sql_defer_foreign_keys', {{'=', 2, false}})
settings.sql_defer_foreign_keys
+box.execute([[set session "sql_default_engine" = 'vinyl']])
+s:get('sql_default_engine').value
+box.execute([[set session "sql_default_engine" = 'memtx']])
+s:get('sql_default_engine').value
+box.execute([[set session "sql_defer_foreign_keys" = true]])
+s:get('sql_defer_foreign_keys').value
+box.execute([[set session "sql_defer_foreign_keys" = false]])
+s:get('sql_defer_foreign_keys').value
+
settings.sql_default_engine = true
settings.sql_defer_foreign_keys = 'false'
settings.sql_parser_debug = 'string'
str = string.rep('a', 20 * 1024)
box.session.settings.sql_default_engine = str
+
+box.execute([[set session "sql_def_engine" = true]])
+box.execute([[set session "sql_default_engine" = true]])
+box.execute([[set session "sql_defer_foreign_keys" = 'true']])
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings
2020-04-10 15:40 ` Chris Sosnin
@ 2020-04-11 17:18 ` Vladislav Shpilevoy
2020-04-13 7:50 ` Timur Safin
1 sibling, 0 replies; 26+ messages in thread
From: Vladislav Shpilevoy @ 2020-04-11 17:18 UTC (permalink / raw)
To: Chris Sosnin; +Cc: tarantool-patches
Hi! Thanks for the patchset!
I force pushed the style fix below.
LGTM now.
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 995875566..380fb83d1 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1544,7 +1544,7 @@ cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y). {
///////////////////////////// The SET SESSION command ////////////////////////
//
cmd ::= SET SESSION nm(X) EQ term(Y). {
- sql_setting_set(pParse,&X,Y.pExpr);
+ sql_setting_set(pParse, &X, Y.pExpr);
}
///////////////////////////// The PRAGMA command /////////////////////////////
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings
2020-04-10 15:40 ` Chris Sosnin
2020-04-11 17:18 ` Vladislav Shpilevoy
@ 2020-04-13 7:50 ` Timur Safin
1 sibling, 0 replies; 26+ messages in thread
From: Timur Safin @ 2020-04-13 7:50 UTC (permalink / raw)
To: 'Chris Sosnin', v.shpilevoy; +Cc: tarantool-patches
:
: I changed it to the following (the main problem with reserved word
: persists):
:
: +///////////////////////////// The SET SESSION command
: ////////////////////////
: +//
: +cmd ::= SET SESSION nm(X) EQ term(Y). {
: + sql_setting_set(pParse,&X,Y.pExpr);
: +}
: +
:
..
: ==========================================================================
: =====
:
: Currently if a user wants to change session setting with SQL, one has
: to execute UPDATE query like:
: [[UPDATE "_session_settings" SET "value" = true WHERE "name" = 'name']]
: However, direct access to system spaces isn't considered to be a good
: practice.
: To avoid that and a bit simplify user's life, we introduce SQL shortcut
: command
: SET SESSION.
:
: Closes #4711
:
: @TarantoolBot document
: Title: API for accessing _session_settings space.
: There are two ways of updating values of session settings:
: via Lua and SQL.
:
...
: SQL:
: Instead of typing long UPDATE query one can use the SET SESSION command:
: `box.execute([[SET SESSION "<setting_name>" = <new_value>]])`.
: Note, that this query is case sensitive so the name must be quoted.
: Also, SET SESSION doesn't provide any implicit casts, so <new_value> must
: be of the type corresponding to the setting being updated.
:
: Example:
: ```
: tarantool> box.execute([[set session "sql_default_engine" = 'memtx']])
: ---
: - row_count: 1
: ...
:
: tarantool> box.execute([[set session "sql_defer_foreign_keys" = true]])
: ---
: - row_count: 1
: ...
:
Like it!
(The least intrusive way possible, but still is short for user to write)
Timur
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/4] session settings fixes
2020-03-30 9:13 [Tarantool-patches] [PATCH 0/4] session settings fixes Chris Sosnin
` (3 preceding siblings ...)
2020-03-30 9:13 ` [Tarantool-patches] [PATCH 4/4] sql: " Chris Sosnin
@ 2020-04-02 9:14 ` Timur Safin
2020-04-02 10:18 ` Chris Sosnin
2020-04-03 12:47 ` Nikita Pettik
2020-04-03 13:09 ` Nikita Pettik
2020-04-13 14:18 ` Kirill Yukhin
6 siblings, 2 replies; 26+ messages in thread
From: Timur Safin @ 2020-04-02 9:14 UTC (permalink / raw)
To: 'Chris Sosnin',
v.shpilevoy, korablev, tarantool-patches, Kirill Yukhin
I'm late here on this party, but I very much dislike proposed syntax of
SETTING SET configuration_name = value;
It feels not SQL-ish, and I see no much value introducing new statement
while there is still SET statement available.
Why we could not simply use something like:
SET box.session.settings.name = value;
?
Thanks,
Timur
: -----Original Message-----
: From: Tarantool-patches <tarantool-patches-bounces@dev.tarantool.org> On
: Behalf Of Chris Sosnin
: Sent: Monday, March 30, 2020 12:14 PM
: To: v.shpilevoy@tarantool.org; korablev@tarantool.org; tarantool-
: patches@dev.tarantool.org
: Subject: [Tarantool-patches] [PATCH 0/4] session settings fixes
:
: issue #1:https://github.com/tarantool/tarantool/issues/4711
: issue #2:https://github.com/tarantool/tarantool/issues/4712
: branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-
: session-settings-v2
:
: Chris Sosnin (4):
: box: replace session_settings modules with a single array
: box: add binary search for _session_settings space
: box: provide a user friendly frontend for accessing session settings
: sql: provide a user friendly frontend for accessing session settings
:
: extra/mkkeywordhash.c | 1 +
: src/box/lua/session.c | 111 +++++++++
: src/box/session.cc | 1 +
: src/box/session.h | 2 +
: src/box/session_settings.c | 214 +++++++++++-------
: src/box/session_settings.h | 53 +++--
: src/box/sql.c | 5 -
: src/box/sql/build.c | 104 ++++-----
: src/box/sql/parse.y | 5 +
: src/box/sql/sqlInt.h | 11 +
: src/box/sql/vdbe.c | 50 ++++
: ...rontend.result => session_settings.result} | 147 ++++++++++--
: ...end.test.lua => session_settings.test.lua} | 61 ++++-
: 13 files changed, 589 insertions(+), 176 deletions(-)
: rename test/box/{gh-4511-access-settings-from-any-frontend.result =>
: session_settings.result} (71%)
: rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua =>
: session_settings.test.lua} (64%)
:
: --
: 2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Tarantool-patches] [PATCH 0/4] session settings fixes
2020-04-02 9:14 ` [Tarantool-patches] [PATCH 0/4] session settings fixes Timur Safin
@ 2020-04-02 10:18 ` Chris Sosnin
2020-04-03 12:47 ` Nikita Pettik
1 sibling, 0 replies; 26+ messages in thread
From: Chris Sosnin @ 2020-04-02 10:18 UTC (permalink / raw)
To: Timur Safin; +Cc: tarantool-patches
> On 2 Apr 2020, at 12:14, Timur Safin <tsafin@tarantool.org> wrote:
>
> I'm late here on this party, but I very much dislike proposed syntax of
>
> SETTING SET configuration_name = value;
>
> It feels not SQL-ish, and I see no much value introducing new statement
> while there is still SET statement available.
>
> Why we could not simply use something like:
> SET box.session.settings.name = value;
> ?
>
> Thanks,
> Timur
It was SET <setting_name> = <new_value> in the first version.
However, we agreed on this syntax being too generic for configuring
session settings, so I added SETTING keyword.
>
>
> : -----Original Message-----
> : From: Tarantool-patches <tarantool-patches-bounces@dev.tarantool.org> On
> : Behalf Of Chris Sosnin
> : Sent: Monday, March 30, 2020 12:14 PM
> : To: v.shpilevoy@tarantool.org; korablev@tarantool.org; tarantool-
> : patches@dev.tarantool.org
> : Subject: [Tarantool-patches] [PATCH 0/4] session settings fixes
> :
> : issue #1:https://github.com/tarantool/tarantool/issues/4711
> : issue #2:https://github.com/tarantool/tarantool/issues/4712
> : branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-
> : session-settings-v2
> :
> : Chris Sosnin (4):
> : box: replace session_settings modules with a single array
> : box: add binary search for _session_settings space
> : box: provide a user friendly frontend for accessing session settings
> : sql: provide a user friendly frontend for accessing session settings
> :
> : extra/mkkeywordhash.c | 1 +
> : src/box/lua/session.c | 111 +++++++++
> : src/box/session.cc | 1 +
> : src/box/session.h | 2 +
> : src/box/session_settings.c | 214 +++++++++++-------
> : src/box/session_settings.h | 53 +++--
> : src/box/sql.c | 5 -
> : src/box/sql/build.c | 104 ++++-----
> : src/box/sql/parse.y | 5 +
> : src/box/sql/sqlInt.h | 11 +
> : src/box/sql/vdbe.c | 50 ++++
> : ...rontend.result => session_settings.result} | 147 ++++++++++--
> : ...end.test.lua => session_settings.test.lua} | 61 ++++-
> : 13 files changed, 589 insertions(+), 176 deletions(-)
> : rename test/box/{gh-4511-access-settings-from-any-frontend.result =>
> : session_settings.result} (71%)
> : rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua =>
> : session_settings.test.lua} (64%)
> :
> : --
> : 2.21.1 (Apple Git-122.3)
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Tarantool-patches] [PATCH 0/4] session settings fixes
2020-04-02 9:14 ` [Tarantool-patches] [PATCH 0/4] session settings fixes Timur Safin
2020-04-02 10:18 ` Chris Sosnin
@ 2020-04-03 12:47 ` Nikita Pettik
1 sibling, 0 replies; 26+ messages in thread
From: Nikita Pettik @ 2020-04-03 12:47 UTC (permalink / raw)
To: Timur Safin; +Cc: tarantool-patches, v.shpilevoy
On 02 Apr 12:14, Timur Safin wrote:
> I'm late here on this party, but I very much dislike proposed syntax of
Patch is not merged, so it's still okay to suggest changes.
> SETTING SET configuration_name = value;
>
> It feels not SQL-ish, and I see no much value introducing new statement
> while there is still SET statement available.
SET is not available as a separated statements; it's only a part
of UPDATE statements syntax. Rolling it back (SETTING SET -> SET)
is not a big deal - only one line of parse.y requires patching.
As I already said, I'm okay with all variations, whether it is
SET, SET SETTING or SETTING SET.
> Why we could not simply use something like:
> SET box.session.settings.name = value;
box.session.settings -- too long prefix. Originally, SET was
suggested as a shortcat for normal UPDATE "_session_settings" ...
SQL statement. Since it is shortcat, let's keep it short.
> ?
>
> Thanks,
> Timur
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/4] session settings fixes
2020-03-30 9:13 [Tarantool-patches] [PATCH 0/4] session settings fixes Chris Sosnin
` (4 preceding siblings ...)
2020-04-02 9:14 ` [Tarantool-patches] [PATCH 0/4] session settings fixes Timur Safin
@ 2020-04-03 13:09 ` Nikita Pettik
2020-04-03 14:02 ` Chris Sosnin
2020-04-13 14:18 ` Kirill Yukhin
6 siblings, 1 reply; 26+ messages in thread
From: Nikita Pettik @ 2020-04-03 13:09 UTC (permalink / raw)
To: Chris Sosnin; +Cc: tarantool-patches, v.shpilevoy
On 30 Mar 12:13, Chris Sosnin wrote:
> issue #1:https://github.com/tarantool/tarantool/issues/4711
> issue #2:https://github.com/tarantool/tarantool/issues/4712
> branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session-settings-v2
Nit: while sending next version of patch-set please specify it
explicitly with --subject-prefix='PATCH v2' and attach changelog
between versions. For example: https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/015103.html
> Chris Sosnin (4):
> box: replace session_settings modules with a single array
> box: add binary search for _session_settings space
> box: provide a user friendly frontend for accessing session settings
> sql: provide a user friendly frontend for accessing session settings
>
> extra/mkkeywordhash.c | 1 +
> src/box/lua/session.c | 111 +++++++++
> src/box/session.cc | 1 +
> src/box/session.h | 2 +
> src/box/session_settings.c | 214 +++++++++++-------
> src/box/session_settings.h | 53 +++--
> src/box/sql.c | 5 -
> src/box/sql/build.c | 104 ++++-----
> src/box/sql/parse.y | 5 +
> src/box/sql/sqlInt.h | 11 +
> src/box/sql/vdbe.c | 50 ++++
> ...rontend.result => session_settings.result} | 147 ++++++++++--
> ...end.test.lua => session_settings.test.lua} | 61 ++++-
> 13 files changed, 589 insertions(+), 176 deletions(-)
> rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (71%)
> rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (64%)
>
> --
> 2.21.1 (Apple Git-122.3)
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Tarantool-patches] [PATCH 0/4] session settings fixes
2020-04-03 13:09 ` Nikita Pettik
@ 2020-04-03 14:02 ` Chris Sosnin
0 siblings, 0 replies; 26+ messages in thread
From: Chris Sosnin @ 2020-04-03 14:02 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
It was v2, Vlad asked me to resend the whole patchset for
convenience. Won’t forget to do this next time.
> On 3 Apr 2020, at 16:09, Nikita Pettik <korablev@tarantool.org> wrote:
>
> On 30 Mar 12:13, Chris Sosnin wrote:
>> issue #1:https://github.com/tarantool/tarantool/issues/4711
>> issue #2:https://github.com/tarantool/tarantool/issues/4712
>> branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session-settings-v2
>
> Nit: while sending next version of patch-set please specify it
> explicitly with --subject-prefix='PATCH v2' and attach changelog
> between versions. For example: https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/015103.html
>
>> Chris Sosnin (4):
>> box: replace session_settings modules with a single array
>> box: add binary search for _session_settings space
>> box: provide a user friendly frontend for accessing session settings
>> sql: provide a user friendly frontend for accessing session settings
>>
>> extra/mkkeywordhash.c | 1 +
>> src/box/lua/session.c | 111 +++++++++
>> src/box/session.cc | 1 +
>> src/box/session.h | 2 +
>> src/box/session_settings.c | 214 +++++++++++-------
>> src/box/session_settings.h | 53 +++--
>> src/box/sql.c | 5 -
>> src/box/sql/build.c | 104 ++++-----
>> src/box/sql/parse.y | 5 +
>> src/box/sql/sqlInt.h | 11 +
>> src/box/sql/vdbe.c | 50 ++++
>> ...rontend.result => session_settings.result} | 147 ++++++++++--
>> ...end.test.lua => session_settings.test.lua} | 61 ++++-
>> 13 files changed, 589 insertions(+), 176 deletions(-)
>> rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (71%)
>> rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (64%)
>>
>> --
>> 2.21.1 (Apple Git-122.3)
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/4] session settings fixes
2020-03-30 9:13 [Tarantool-patches] [PATCH 0/4] session settings fixes Chris Sosnin
` (5 preceding siblings ...)
2020-04-03 13:09 ` Nikita Pettik
@ 2020-04-13 14:18 ` Kirill Yukhin
6 siblings, 0 replies; 26+ messages in thread
From: Kirill Yukhin @ 2020-04-13 14:18 UTC (permalink / raw)
To: Chris Sosnin; +Cc: v.shpilevoy, tarantool-patches
Hello,
On 30 Mar 12:13, Chris Sosnin wrote:
> issue #1:https://github.com/tarantool/tarantool/issues/4711
> issue #2:https://github.com/tarantool/tarantool/issues/4712
> branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session-settings-v2
>
> Chris Sosnin (4):
> box: replace session_settings modules with a single array
> box: add binary search for _session_settings space
> box: provide a user friendly frontend for accessing session settings
> sql: provide a user friendly frontend for accessing session settings
>
> extra/mkkeywordhash.c | 1 +
> src/box/lua/session.c | 111 +++++++++
> src/box/session.cc | 1 +
> src/box/session.h | 2 +
> src/box/session_settings.c | 214 +++++++++++-------
> src/box/session_settings.h | 53 +++--
> src/box/sql.c | 5 -
> src/box/sql/build.c | 104 ++++-----
> src/box/sql/parse.y | 5 +
> src/box/sql/sqlInt.h | 11 +
> src/box/sql/vdbe.c | 50 ++++
> ...rontend.result => session_settings.result} | 147 ++++++++++--
> ...end.test.lua => session_settings.test.lua} | 61 ++++-
> 13 files changed, 589 insertions(+), 176 deletions(-)
> rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (71%)
> rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (64%)
I've checked 1, 3 and 4 patches into master.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space
2020-02-17 12:12 [Tarantool-patches] [PATCH 0/4] box: " Chris Sosnin
@ 2020-02-17 12:12 ` Chris Sosnin
2020-03-16 14:16 ` Nikita Pettik
0 siblings, 1 reply; 26+ messages in thread
From: Chris Sosnin @ 2020-02-17 12:12 UTC (permalink / raw)
To: korablev, tarantool-patches
As it is mentioned in implementation, it is important
that _session_settings options are sorted by name, so
there is no need in linear lookup and we can replace it
with binary search.
Closes #4712
---
src/box/session_settings.c | 97 +++++++++++++++++--
...1-access-settings-from-any-frontend.result | 37 ++++---
...access-settings-from-any-frontend.test.lua | 28 ++++--
3 files changed, 133 insertions(+), 29 deletions(-)
diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index 93d1c8e22..874fc304a 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -81,6 +81,8 @@ struct session_settings_iterator {
bool is_eq;
/** True if the iterator should include equal keys. */
bool is_including;
+ /** True if the iterator is pointing to an existing setting */
+ bool is_set;
};
static void
@@ -92,6 +94,72 @@ session_settings_iterator_free(struct iterator *ptr)
free(it);
}
+static int
+session_settings_set_forward(int *sid, const char *key, bool is_eq,
+ bool is_including)
+{
+ int low = 0, high = SESSION_SETTING_COUNT - 1;
+ if (key == NULL)
+ return 0;
+ while (low <= high) {
+ int index = (high + low) / 2;
+ const char *name = session_setting_strs[index];
+ int cmp = strcmp(name, key);
+ if (cmp == 0) {
+ if (is_including) {
+ *sid = index;
+ return 0;
+ }
+ *sid = ++index;
+ return index < SESSION_SETTING_COUNT ? 0 : -1;
+ }
+ if (cmp < 0)
+ low = index + 1;
+ else
+ high = index - 1;
+ }
+ if (is_eq) {
+ *sid = SESSION_SETTING_COUNT;
+ return -1;
+ }
+ assert(low > high);
+ *sid = low;
+ return low < SESSION_SETTING_COUNT ? 0 : -1;
+}
+
+static int
+session_settings_set_reverse(int *sid, const char *key, bool is_eq,
+ bool is_including)
+{
+ int low = 0, high = SESSION_SETTING_COUNT - 1;
+ if (key == NULL)
+ return 0;
+ while (low <= high) {
+ int index = (high + low) / 2;
+ const char *name = session_setting_strs[index];
+ int cmp = strcmp(name, key);
+ if (cmp == 0) {
+ if (is_including) {
+ *sid = index;
+ return 0;
+ }
+ *sid = --index;
+ return index >= 0 ? 0 : -1;
+ }
+ if (cmp < 0)
+ low = index + 1;
+ else
+ high = index - 1;
+ }
+ if (is_eq) {
+ *sid = SESSION_SETTING_COUNT;
+ return -1;
+ }
+ assert(low > high);
+ *sid = high;
+ return high >= 0 ? 0 : -1;
+}
+
static int
session_settings_next(int *sid, const char *key, bool is_eq, bool is_including)
{
@@ -145,9 +213,15 @@ session_settings_iterator_next(struct iterator *iterator, struct tuple **result)
int sid = it->setting_id;
const char *key = it->key;
bool is_including = it->is_including, is_eq = it->is_eq;
- bool is_found = false;
- if (session_settings_next(&sid, key, is_eq, is_including) == 0)
- is_found = true;
+ bool is_found;
+ if (!it->is_set) {
+ it->is_set = true;
+ is_found = session_settings_set_forward(&sid, key, is_eq,
+ is_including) == 0;
+ } else {
+ is_found = session_settings_next(&sid, key, is_eq,
+ is_including) == 0;
+ }
it->setting_id = sid + 1;
if (!is_found) {
*result = NULL;
@@ -167,9 +241,15 @@ session_settings_iterator_prev(struct iterator *iterator, struct tuple **result)
int sid = it->setting_id;
const char *key = it->key;
bool is_including = it->is_including, is_eq = it->is_eq;
- bool is_found = false;
- if (session_settings_prev(&sid, key, is_eq, is_including) == 0)
- is_found = true;
+ bool is_found;
+ if (!it->is_set) {
+ it->is_set = true;
+ is_found = session_settings_set_reverse(&sid, key, is_eq,
+ is_including) == 0;
+ } else {
+ is_found = session_settings_prev(&sid, key, is_eq,
+ is_including) == 0;
+ }
it->setting_id = sid - 1;
if (!is_found) {
*result = NULL;
@@ -221,6 +301,7 @@ session_settings_index_create_iterator(struct index *base,
it->is_eq = type == ITER_EQ || type == ITER_REQ;
it->is_including = it->is_eq || type == ITER_GE || type == ITER_ALL ||
type == ITER_LE;
+ it->is_set = false;
it->format = index->format;
if (!iterator_type_is_reverse(type)) {
it->base.next = session_settings_iterator_next;
@@ -244,7 +325,7 @@ session_settings_index_get(struct index *base, const char *key,
key = mp_decode_str(&key, &len);
key = tt_cstr(key, len);
int sid = 0;
- if (session_settings_next(&sid, key, true, true) != 0) {
+ if (session_settings_set_forward(&sid, key, true, true) != 0) {
*result = NULL;
return 0;
}
@@ -345,7 +426,7 @@ session_settings_space_execute_update(struct space *space, struct txn *txn,
}
key = mp_decode_str(&key, &key_len);
key = tt_cstr(key, key_len);
- if (session_settings_next(&sid, key, true, true) != 0) {
+ if (session_settings_set_forward(&sid, key, true, true) != 0) {
*result = NULL;
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 ed340d053..bae77192e 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/gh-4511-access-settings-from-any-frontend.result
@@ -82,6 +82,12 @@ function check_sorting(ss, ts, key)
for _, it in pairs(iterators_list) do
local view_space = ss:select({key}, {iterator = it})
local test_space = ts:select({key}, {iterator = it})
+ if #view_space ~= #test_space then
+ return {
+ err = 'bad sorting', type = it, exp = test_space,
+ got = view_space
+ }
+ end
for key, value in pairs(view_space) do
if test_space[key].name ~= value.name then
return {
@@ -111,32 +117,37 @@ check_sorting(s, t, 'sql_d')
check_sorting(s, t, 'sql_v')
| ---
| ...
-check_sorting(s, t, 'sql_defer_foreign_keys')
+check_sorting(s, t, 'z')
| ---
| ...
-
-t:drop()
+check_sorting(s, t, 'sql_parser_debuf')
+ | ---
+ | ...
+check_sorting(s, t, 'sql_parser_debuh')
| ---
| ...
--- Check get() method of session_settings space.
-s:get({'sql_defer_foreign_keys'})
+name = nil
| ---
- | - ['sql_defer_foreign_keys', false]
| ...
-s:get({'sql_recursive_triggers'})
+err = nil
| ---
- | - ['sql_recursive_triggers', true]
| ...
-s:get({'sql_reverse_unordered_selects'})
+for _, tuple in t:pairs() do \
+ name = tuple.name \
+ err = check_sorting(s, t, name) \
+ if err then \
+ break \
+ end \
+end
| ---
- | - ['sql_reverse_unordered_selects', false]
| ...
-s:get({'sql_default_engine'})
+err and {err, name}
| ---
- | - ['sql_default_engine', 'memtx']
+ | - null
| ...
-s:get({'abcd'})
+
+t:drop()
| ---
| ...
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 366874998..b243be15e 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
@@ -36,6 +36,12 @@ function check_sorting(ss, ts, key)
for _, it in pairs(iterators_list) do
local view_space = ss:select({key}, {iterator = it})
local test_space = ts:select({key}, {iterator = it})
+ if #view_space ~= #test_space then
+ return {
+ err = 'bad sorting', type = it, exp = test_space,
+ got = view_space
+ }
+ end
for key, value in pairs(view_space) do
if test_space[key].name ~= value.name then
return {
@@ -52,17 +58,23 @@ 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')
+check_sorting(s, t, 'z')
+check_sorting(s, t, 'sql_parser_debuf')
+check_sorting(s, t, 'sql_parser_debuh')
+
+name = nil
+err = nil
+for _, tuple in t:pairs() do \
+ name = tuple.name \
+ err = check_sorting(s, t, name) \
+ if err then \
+ break \
+ end \
+end
+err and {err, name}
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
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space
2020-02-17 12:12 ` [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space Chris Sosnin
@ 2020-03-16 14:16 ` Nikita Pettik
2020-03-16 22:53 ` Vladislav Shpilevoy
0 siblings, 1 reply; 26+ messages in thread
From: Nikita Pettik @ 2020-03-16 14:16 UTC (permalink / raw)
To: Chris Sosnin; +Cc: tarantool-patches, v.shpilevoy
On 17 Feb 15:12, Chris Sosnin wrote:
> As it is mentioned in implementation, it is important
> that _session_settings options are sorted by name, so
> there is no need in linear lookup and we can replace it
> with binary search.
Is there any sufficient performance benefit except for code complication?:)
I really doubt that this part should be optimized for such rare in
terms of usage place. What is more, there's only dozen entries, so
binary search doesn't really make any sence. Idk why Kirill assigned 2.4.1
milestone, IMHO there are way far more important things to elaborate on.
> Closes #4712
> ---
> src/box/session_settings.c | 97 +++++++++++++++++--
> ...1-access-settings-from-any-frontend.result | 37 ++++---
> ...access-settings-from-any-frontend.test.lua | 28 ++++--
> 3 files changed, 133 insertions(+), 29 deletions(-)
>
> diff --git a/src/box/session_settings.c b/src/box/session_settings.c
> index 93d1c8e22..874fc304a 100644
> --- a/src/box/session_settings.c
> +++ b/src/box/session_settings.c
> @@ -81,6 +81,8 @@ struct session_settings_iterator {
> bool is_eq;
> /** True if the iterator should include equal keys. */
> bool is_including;
> + /** True if the iterator is pointing to an existing setting */
> + bool is_set;
> };
>
> static void
> @@ -92,6 +94,72 @@ session_settings_iterator_free(struct iterator *ptr)
> free(it);
> }
>
> +static int
> +session_settings_set_forward(int *sid, const char *key, bool is_eq,
> + bool is_including)
> +{
> + int low = 0, high = SESSION_SETTING_COUNT - 1;
> + if (key == NULL)
> + return 0;
> + while (low <= high) {
> + int index = (high + low) / 2;
> + const char *name = session_setting_strs[index];
> + int cmp = strcmp(name, key);
> + if (cmp == 0) {
> + if (is_including) {
> + *sid = index;
> + return 0;
> + }
> + *sid = ++index;
> + return index < SESSION_SETTING_COUNT ? 0 : -1;
> + }
> + if (cmp < 0)
> + low = index + 1;
> + else
> + high = index - 1;
> + }
> + if (is_eq) {
> + *sid = SESSION_SETTING_COUNT;
> + return -1;
> + }
> + assert(low > high);
> + *sid = low;
> + return low < SESSION_SETTING_COUNT ? 0 : -1;
> +}
> +
> +static int
> +session_settings_set_reverse(int *sid, const char *key, bool is_eq,
> + bool is_including)
> +{
> + int low = 0, high = SESSION_SETTING_COUNT - 1;
> + if (key == NULL)
> + return 0;
> + while (low <= high) {
> + int index = (high + low) / 2;
> + const char *name = session_setting_strs[index];
> + int cmp = strcmp(name, key);
> + if (cmp == 0) {
> + if (is_including) {
> + *sid = index;
> + return 0;
> + }
> + *sid = --index;
> + return index >= 0 ? 0 : -1;
> + }
> + if (cmp < 0)
> + low = index + 1;
> + else
> + high = index - 1;
> + }
> + if (is_eq) {
> + *sid = SESSION_SETTING_COUNT;
> + return -1;
> + }
> + assert(low > high);
> + *sid = high;
> + return high >= 0 ? 0 : -1;
> +}
> +
> static int
> session_settings_next(int *sid, const char *key, bool is_eq, bool is_including)
> {
> @@ -145,9 +213,15 @@ session_settings_iterator_next(struct iterator *iterator, struct tuple **result)
> int sid = it->setting_id;
> const char *key = it->key;
> bool is_including = it->is_including, is_eq = it->is_eq;
> - bool is_found = false;
> - if (session_settings_next(&sid, key, is_eq, is_including) == 0)
> - is_found = true;
> + bool is_found;
> + if (!it->is_set) {
> + it->is_set = true;
> + is_found = session_settings_set_forward(&sid, key, is_eq,
> + is_including) == 0;
> + } else {
> + is_found = session_settings_next(&sid, key, is_eq,
> + is_including) == 0;
> + }
> it->setting_id = sid + 1;
> if (!is_found) {
> *result = NULL;
> @@ -167,9 +241,15 @@ session_settings_iterator_prev(struct iterator *iterator, struct tuple **result)
> int sid = it->setting_id;
> const char *key = it->key;
> bool is_including = it->is_including, is_eq = it->is_eq;
> - bool is_found = false;
> - if (session_settings_prev(&sid, key, is_eq, is_including) == 0)
> - is_found = true;
> + bool is_found;
> + if (!it->is_set) {
> + it->is_set = true;
> + is_found = session_settings_set_reverse(&sid, key, is_eq,
> + is_including) == 0;
> + } else {
> + is_found = session_settings_prev(&sid, key, is_eq,
> + is_including) == 0;
> + }
> it->setting_id = sid - 1;
> if (!is_found) {
> *result = NULL;
> @@ -221,6 +301,7 @@ session_settings_index_create_iterator(struct index *base,
> it->is_eq = type == ITER_EQ || type == ITER_REQ;
> it->is_including = it->is_eq || type == ITER_GE || type == ITER_ALL ||
> type == ITER_LE;
> + it->is_set = false;
> it->format = index->format;
> if (!iterator_type_is_reverse(type)) {
> it->base.next = session_settings_iterator_next;
> @@ -244,7 +325,7 @@ session_settings_index_get(struct index *base, const char *key,
> key = mp_decode_str(&key, &len);
> key = tt_cstr(key, len);
> int sid = 0;
> - if (session_settings_next(&sid, key, true, true) != 0) {
> + if (session_settings_set_forward(&sid, key, true, true) != 0) {
> *result = NULL;
> return 0;
> }
> @@ -345,7 +426,7 @@ session_settings_space_execute_update(struct space *space, struct txn *txn,
> }
> key = mp_decode_str(&key, &key_len);
> key = tt_cstr(key, key_len);
> - if (session_settings_next(&sid, key, true, true) != 0) {
> + if (session_settings_set_forward(&sid, key, true, true) != 0) {
> *result = NULL;
> 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 ed340d053..bae77192e 100644
> --- a/test/box/gh-4511-access-settings-from-any-frontend.result
> +++ b/test/box/gh-4511-access-settings-from-any-frontend.result
> @@ -82,6 +82,12 @@ function check_sorting(ss, ts, key)
> for _, it in pairs(iterators_list) do
> local view_space = ss:select({key}, {iterator = it})
> local test_space = ts:select({key}, {iterator = it})
> + if #view_space ~= #test_space then
> + return {
> + err = 'bad sorting', type = it, exp = test_space,
> + got = view_space
> + }
> + end
> for key, value in pairs(view_space) do
> if test_space[key].name ~= value.name then
> return {
> @@ -111,32 +117,37 @@ check_sorting(s, t, 'sql_d')
> check_sorting(s, t, 'sql_v')
> | ---
> | ...
> -check_sorting(s, t, 'sql_defer_foreign_keys')
> +check_sorting(s, t, 'z')
> | ---
> | ...
> -
> -t:drop()
> +check_sorting(s, t, 'sql_parser_debuf')
> + | ---
> + | ...
> +check_sorting(s, t, 'sql_parser_debuh')
> | ---
> | ...
>
> --- Check get() method of session_settings space.
> -s:get({'sql_defer_foreign_keys'})
> +name = nil
> | ---
> - | - ['sql_defer_foreign_keys', false]
> | ...
> -s:get({'sql_recursive_triggers'})
> +err = nil
> | ---
> - | - ['sql_recursive_triggers', true]
> | ...
> -s:get({'sql_reverse_unordered_selects'})
> +for _, tuple in t:pairs() do \
> + name = tuple.name \
> + err = check_sorting(s, t, name) \
> + if err then \
> + break \
> + end \
> +end
> | ---
> - | - ['sql_reverse_unordered_selects', false]
> | ...
> -s:get({'sql_default_engine'})
> +err and {err, name}
> | ---
> - | - ['sql_default_engine', 'memtx']
> + | - null
> | ...
> -s:get({'abcd'})
> +
> +t:drop()
> | ---
> | ...
>
> 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 366874998..b243be15e 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
> @@ -36,6 +36,12 @@ function check_sorting(ss, ts, key)
> for _, it in pairs(iterators_list) do
> local view_space = ss:select({key}, {iterator = it})
> local test_space = ts:select({key}, {iterator = it})
> + if #view_space ~= #test_space then
> + return {
> + err = 'bad sorting', type = it, exp = test_space,
> + got = view_space
> + }
> + end
> for key, value in pairs(view_space) do
> if test_space[key].name ~= value.name then
> return {
> @@ -52,17 +58,23 @@ 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')
> +check_sorting(s, t, 'z')
> +check_sorting(s, t, 'sql_parser_debuf')
> +check_sorting(s, t, 'sql_parser_debuh')
> +
> +name = nil
> +err = nil
> +for _, tuple in t:pairs() do \
> + name = tuple.name \
> + err = check_sorting(s, t, name) \
> + if err then \
> + break \
> + end \
> +end
> +err and {err, name}
>
> 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
> --
> 2.21.1 (Apple Git-122.3)
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space
2020-03-16 14:16 ` Nikita Pettik
@ 2020-03-16 22:53 ` Vladislav Shpilevoy
2020-03-17 17:24 ` Nikita Pettik
0 siblings, 1 reply; 26+ messages in thread
From: Vladislav Shpilevoy @ 2020-03-16 22:53 UTC (permalink / raw)
To: Nikita Pettik, Chris Sosnin; +Cc: tarantool-patches
Hi! Thanks for your comments!
On 16/03/2020 15:16, Nikita Pettik wrote:
> On 17 Feb 15:12, Chris Sosnin wrote:
>> As it is mentioned in implementation, it is important
>> that _session_settings options are sorted by name, so
>> there is no need in linear lookup and we can replace it
>> with binary search.
>
> Is there any sufficient performance benefit except for code complication?:)
> I really doubt that this part should be optimized for such rare in
> terms of usage place.
We didn't measure perf increase, but for such a simple change it
did not seem necessary.
Talking of rareness of settings change - this is arguable. Firstly,
every new session will change settings, if a user's project uses some
of them permanently. Secondly, some settings, such as default engine,
or constraints related, or similar can be changed on per-request basis,
in case user wants to make some requests not checking FK, and others
should check them, inside one session, for instance. Similar necessity
can arise for other settings. I think we can actually measure how faster
lookup became for that case.
> What is more, there's only dozen entries, so
> binary search doesn't really make any sence. Idk why Kirill assigned 2.4.1
It is only dozen for now. This will change. We already have read-only
session coming.
> milestone, IMHO there are way far more important things to elaborate on.
Honestly, I was thinking there is not much to elaborate. This is
trivial bsearch.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space
2020-03-16 22:53 ` Vladislav Shpilevoy
@ 2020-03-17 17:24 ` Nikita Pettik
0 siblings, 0 replies; 26+ messages in thread
From: Nikita Pettik @ 2020-03-17 17:24 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On 16 Mar 23:53, Vladislav Shpilevoy wrote:
> Hi! Thanks for your comments!
>
> On 16/03/2020 15:16, Nikita Pettik wrote:
> > On 17 Feb 15:12, Chris Sosnin wrote:
> >> As it is mentioned in implementation, it is important
> >> that _session_settings options are sorted by name, so
> >> there is no need in linear lookup and we can replace it
> >> with binary search.
> >
> > Is there any sufficient performance benefit except for code complication?:)
> > I really doubt that this part should be optimized for such rare in
> > terms of usage place.
>
> We didn't measure perf increase, but for such a simple change it
> did not seem necessary.
I mean there can be no even performance benefit at all since
binary search on small data sets can be slower than sequential
access (there's linear search mode even in bps tree, but it is
compile time option).
> Talking of rareness of settings change - this is arguable. Firstly,
> every new session will change settings, if a user's project uses some
> of them permanently. Secondly, some settings, such as default engine,
> or constraints related, or similar can be changed on per-request basis,
> in case user wants to make some requests not checking FK, and others
> should check them, inside one session, for instance. Similar necessity
> can arise for other settings. I think we can actually measure how faster
> lookup became for that case.
>
> > What is more, there's only dozen entries, so
> > binary search doesn't really make any sence. Idk why Kirill assigned 2.4.1
>
> It is only dozen for now. This will change. We already have read-only
> session coming.
>
> > milestone, IMHO there are way far more important things to elaborate on.
>
> Honestly, I was thinking there is not much to elaborate. This is
> trivial bsearch.
I mean work on more improtant features or/and severe bugs. Nevermind,
it is already done anyway. Personally I would reject this patch if
there's no any performance increase (otherwise, it simply complicates
code).
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space
2020-02-03 22:17 [Tarantool-patches] [PATCH v4 2/3] " Vladislav Shpilevoy
@ 2020-02-04 19:30 ` Chris Sosnin
2020-02-06 22:15 ` Vladislav Shpilevoy
0 siblings, 1 reply; 26+ messages in thread
From: Chris Sosnin @ 2020-02-04 19:30 UTC (permalink / raw)
To: v.shpilevoy, tarantool-patches
I agree with your changes, but I send new version of this patch, just in case.
================================================================================
As it is mentioned in implementation, it is important
that _session_settings options are sorted by name, so
there is no need in linear lookup and we can replace it
with binary search.
Closes #4712
---
issue: https://github.com/tarantool/tarantool/issues/4711
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings
src/box/session_settings.c | 97 +++++++++++++++++--
...1-access-settings-from-any-frontend.result | 37 ++++---
...access-settings-from-any-frontend.test.lua | 28 ++++--
3 files changed, 133 insertions(+), 29 deletions(-)
diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index 93d1c8e22..874fc304a 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -81,6 +81,8 @@ struct session_settings_iterator {
bool is_eq;
/** True if the iterator should include equal keys. */
bool is_including;
+ /** True if the iterator is pointing to an existing setting */
+ bool is_set;
};
static void
@@ -92,6 +94,72 @@ session_settings_iterator_free(struct iterator *ptr)
free(it);
}
+static int
+session_settings_set_forward(int *sid, const char *key, bool is_eq,
+ bool is_including)
+{
+ int low = 0, high = SESSION_SETTING_COUNT - 1;
+ if (key == NULL)
+ return 0;
+ while (low <= high) {
+ int index = (high + low) / 2;
+ const char *name = session_setting_strs[index];
+ int cmp = strcmp(name, key);
+ if (cmp == 0) {
+ if (is_including) {
+ *sid = index;
+ return 0;
+ }
+ *sid = ++index;
+ return index < SESSION_SETTING_COUNT ? 0 : -1;
+ }
+ if (cmp < 0)
+ low = index + 1;
+ else
+ high = index - 1;
+ }
+ if (is_eq) {
+ *sid = SESSION_SETTING_COUNT;
+ return -1;
+ }
+ assert(low > high);
+ *sid = low;
+ return low < SESSION_SETTING_COUNT ? 0 : -1;
+}
+
+static int
+session_settings_set_reverse(int *sid, const char *key, bool is_eq,
+ bool is_including)
+{
+ int low = 0, high = SESSION_SETTING_COUNT - 1;
+ if (key == NULL)
+ return 0;
+ while (low <= high) {
+ int index = (high + low) / 2;
+ const char *name = session_setting_strs[index];
+ int cmp = strcmp(name, key);
+ if (cmp == 0) {
+ if (is_including) {
+ *sid = index;
+ return 0;
+ }
+ *sid = --index;
+ return index >= 0 ? 0 : -1;
+ }
+ if (cmp < 0)
+ low = index + 1;
+ else
+ high = index - 1;
+ }
+ if (is_eq) {
+ *sid = SESSION_SETTING_COUNT;
+ return -1;
+ }
+ assert(low > high);
+ *sid = high;
+ return high >= 0 ? 0 : -1;
+}
+
static int
session_settings_next(int *sid, const char *key, bool is_eq, bool is_including)
{
@@ -145,9 +213,15 @@ session_settings_iterator_next(struct iterator *iterator, struct tuple **result)
int sid = it->setting_id;
const char *key = it->key;
bool is_including = it->is_including, is_eq = it->is_eq;
- bool is_found = false;
- if (session_settings_next(&sid, key, is_eq, is_including) == 0)
- is_found = true;
+ bool is_found;
+ if (!it->is_set) {
+ it->is_set = true;
+ is_found = session_settings_set_forward(&sid, key, is_eq,
+ is_including) == 0;
+ } else {
+ is_found = session_settings_next(&sid, key, is_eq,
+ is_including) == 0;
+ }
it->setting_id = sid + 1;
if (!is_found) {
*result = NULL;
@@ -167,9 +241,15 @@ session_settings_iterator_prev(struct iterator *iterator, struct tuple **result)
int sid = it->setting_id;
const char *key = it->key;
bool is_including = it->is_including, is_eq = it->is_eq;
- bool is_found = false;
- if (session_settings_prev(&sid, key, is_eq, is_including) == 0)
- is_found = true;
+ bool is_found;
+ if (!it->is_set) {
+ it->is_set = true;
+ is_found = session_settings_set_reverse(&sid, key, is_eq,
+ is_including) == 0;
+ } else {
+ is_found = session_settings_prev(&sid, key, is_eq,
+ is_including) == 0;
+ }
it->setting_id = sid - 1;
if (!is_found) {
*result = NULL;
@@ -221,6 +301,7 @@ session_settings_index_create_iterator(struct index *base,
it->is_eq = type == ITER_EQ || type == ITER_REQ;
it->is_including = it->is_eq || type == ITER_GE || type == ITER_ALL ||
type == ITER_LE;
+ it->is_set = false;
it->format = index->format;
if (!iterator_type_is_reverse(type)) {
it->base.next = session_settings_iterator_next;
@@ -244,7 +325,7 @@ session_settings_index_get(struct index *base, const char *key,
key = mp_decode_str(&key, &len);
key = tt_cstr(key, len);
int sid = 0;
- if (session_settings_next(&sid, key, true, true) != 0) {
+ if (session_settings_set_forward(&sid, key, true, true) != 0) {
*result = NULL;
return 0;
}
@@ -345,7 +426,7 @@ session_settings_space_execute_update(struct space *space, struct txn *txn,
}
key = mp_decode_str(&key, &key_len);
key = tt_cstr(key, key_len);
- if (session_settings_next(&sid, key, true, true) != 0) {
+ if (session_settings_set_forward(&sid, key, true, true) != 0) {
*result = NULL;
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 ed340d053..f072bafae 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/gh-4511-access-settings-from-any-frontend.result
@@ -82,6 +82,12 @@ function check_sorting(ss, ts, key)
for _, it in pairs(iterators_list) do
local view_space = ss:select({key}, {iterator = it})
local test_space = ts:select({key}, {iterator = it})
+ if #view_space ~= #test_space then
+ return {
+ err = 'bad sorting', type = it, exp = test_space,
+ got = view_space
+ }
+ end
for key, value in pairs(view_space) do
if test_space[key].name ~= value.name then
return {
@@ -111,32 +117,37 @@ check_sorting(s, t, 'sql_d')
check_sorting(s, t, 'sql_v')
| ---
| ...
-check_sorting(s, t, 'sql_defer_foreign_keys')
+check_sorting(s, t, 'z')
| ---
| ...
-
-t:drop()
+check_sorting(s, t, 'sql_parser_debuf')
+ | ---
+ | ...
+check_sorting(s, t, 'sql_parser_debuh')
| ---
| ...
--- Check get() method of session_settings space.
-s:get({'sql_defer_foreign_keys'})
+name = nil
| ---
- | - ['sql_defer_foreign_keys', false]
| ...
-s:get({'sql_recursive_triggers'})
+err = nil
| ---
- | - ['sql_recursive_triggers', true]
| ...
-s:get({'sql_reverse_unordered_selects'})
+for _, tuple in t:pairs() do \
+ name = tuple.name \
+ err = check_sorting(s, t, name) \
+ if err then \
+ break \
+ end \
+end
| ---
- | - ['sql_reverse_unordered_selects', false]
| ...
-s:get({'sql_default_engine'})
+err and {err, name}
| ---
- | - ['sql_default_engine', 'memtx']
+ | - null
| ...
-s:get({'abcd'})
+
+t:drop()
| ---
| ...
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 366874998..40a58ad04 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
@@ -36,6 +36,12 @@ function check_sorting(ss, ts, key)
for _, it in pairs(iterators_list) do
local view_space = ss:select({key}, {iterator = it})
local test_space = ts:select({key}, {iterator = it})
+ if #view_space ~= #test_space then
+ return {
+ err = 'bad sorting', type = it, exp = test_space,
+ got = view_space
+ }
+ end
for key, value in pairs(view_space) do
if test_space[key].name ~= value.name then
return {
@@ -52,17 +58,23 @@ 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')
+check_sorting(s, t, 'z')
+check_sorting(s, t, 'sql_parser_debuf')
+check_sorting(s, t, 'sql_parser_debuh')
+
+name = nil
+err = nil
+for _, tuple in t:pairs() do \
+ name = tuple.name \
+ err = check_sorting(s, t, name) \
+ if err then \
+ break \
+ end \
+end
+err and {err, name}
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
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space
2020-02-04 19:30 ` [Tarantool-patches] [PATCH 2/4] " Chris Sosnin
@ 2020-02-06 22:15 ` Vladislav Shpilevoy
0 siblings, 0 replies; 26+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-06 22:15 UTC (permalink / raw)
To: Chris Sosnin, tarantool-patches
Thanks for the patch!
Here is one another review fix.
================================================================================
commit 0a597ec7e4956a23d5e80420ca1d98fccab8d117
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Thu Feb 6 22:07:50 2020 +0100
Review fix
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 f072bafae..bae77192e 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/gh-4511-access-settings-from-any-frontend.result
@@ -86,7 +86,7 @@ function check_sorting(ss, ts, key)
return {
err = 'bad sorting', type = it, exp = test_space,
got = view_space
- }
+ }
end
for key, value in pairs(view_space) do
if test_space[key].name ~= value.name then
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 40a58ad04..b243be15e 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
@@ -40,7 +40,7 @@ function check_sorting(ss, ts, key)
return {
err = 'bad sorting', type = it, exp = test_space,
got = view_space
- }
+ }
end
for key, value in pairs(view_space) do
if test_space[key].name ~= value.name then
^ permalink raw reply [flat|nested] 26+ messages in thread