* [Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array
2020-02-17 12:12 [Tarantool-patches] [PATCH 0/4] box: session settings fixes Chris Sosnin
@ 2020-02-17 12:12 ` Chris Sosnin
2020-02-17 12:12 ` [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space Chris Sosnin
` (2 subsequent siblings)
3 siblings, 0 replies; 22+ messages in thread
From: Chris Sosnin @ 2020-02-17 12:12 UTC (permalink / raw)
To: 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 a969d3d10..93d1c8e22 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->dict,
&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 bc50ecbfa..e5fde08cc 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3310,40 +3310,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.
@@ -3361,24 +3327,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},
};
@@ -3386,12 +3352,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;
@@ -3404,7 +3370,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);
@@ -3440,7 +3406,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
@@ -3454,7 +3420,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) {
@@ -3468,7 +3434,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;
@@ -3488,17 +3454,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] 22+ 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: session settings fixes Chris Sosnin
2020-02-17 12:12 ` [Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array Chris Sosnin
@ 2020-02-17 12:12 ` Chris Sosnin
2020-03-16 14:16 ` Nikita Pettik
2020-02-17 12:12 ` [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings Chris Sosnin
2020-02-17 12:12 ` [Tarantool-patches] [PATCH 4/4] sql: " Chris Sosnin
3 siblings, 1 reply; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ messages in thread
* [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings
2020-02-17 12:12 [Tarantool-patches] [PATCH 0/4] box: session settings fixes Chris Sosnin
2020-02-17 12:12 ` [Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array Chris Sosnin
2020-02-17 12:12 ` [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space Chris Sosnin
@ 2020-02-17 12:12 ` Chris Sosnin
2020-03-16 16:08 ` Nikita Pettik
2020-02-17 12:12 ` [Tarantool-patches] [PATCH 4/4] sql: " Chris Sosnin
3 siblings, 1 reply; 22+ messages in thread
From: Chris Sosnin @ 2020-02-17 12:12 UTC (permalink / raw)
To: korablev, tarantool-patches
- space_object:update() is hard to use for configuring session settings,
so we provide box.session.setting 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 | 92 +++++++++++++++++++
src/box/session.cc | 1 +
src/box/session.h | 2 +
src/box/sql.c | 5 -
...rontend.result => session_settings.result} | 63 +++++++++++++
...end.test.lua => session_settings.test.lua} | 20 ++++
6 files changed, 178 insertions(+), 5 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} (85%)
diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index c6a600f6f..c0b4d6184 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,95 @@ lbox_session_on_access_denied(struct lua_State *L)
lbox_push_on_access_denied_event, NULL);
}
+static int
+lbox_session_setting_get(struct lua_State *L)
+{
+ lua_getfield(L, -1, "_id");
+ int sid = lua_tointeger(L, -1);
+ 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 {
+ const char *str = mp_decode_str(&mp_pair, &len);
+ lua_pushlstring(L, str, len);
+ }
+ return 1;
+}
+
+static int
+lbox_session_setting_set(struct lua_State *L)
+{
+ if (lua_gettop(L) != 2)
+ return luaL_error(L, "Usage: box.session.settings:set(value)");
+ int arg_type = lua_type(L, -1);
+ lua_getfield(L, -2, "_id");
+ int sid = lua_tointeger(L, -1);
+ struct session_setting *setting = &session_settings[sid];
+ lua_pop(L, 1);
+ 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_push_nil_and_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_push_nil_and_error(L);
+ break;
+ }
+ default:
+ diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+ session_setting_strs[sid],
+ field_type_strs[setting->field_type]);
+ return luaT_push_nil_and_error(L);
+ }
+ return 0;
+}
+
+static void
+lbox_session_settings_init(struct lua_State *L)
+{
+ lua_createtable(L, 0, 2);
+ lua_pushcfunction(L, lbox_session_setting_get);
+ lua_setfield(L, -2, "__serialize");
+ lua_createtable(L, 0, 1);
+ lua_pushcfunction(L, lbox_session_setting_set);
+ lua_setfield(L, -2, "set");
+ lua_setfield(L, -2, "__index");
+
+ lua_newtable(L);
+ for (int id = 0; id < SESSION_SETTING_COUNT; ++id) {
+ lua_newtable(L);
+ lua_pushinteger(L, id);
+ lua_setfield(L, -2, "_id");
+ lua_pushvalue(L, -3);
+ lua_setmetatable(L, -2);
+ lua_setfield(L, -2, session_setting_strs[id]);
+ }
+ lua_setfield(L, -3, "settings");
+ lua_pop(L, 1);
+}
+
void
session_storage_cleanup(int sid)
{
@@ -478,5 +569,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/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..6d7074e8c 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/session_settings.result
@@ -298,3 +298,66 @@ 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:set('memtx')
+ | ---
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - memtx
+ | ...
+settings.sql_defer_foreign_keys:set(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:set(true)
+ | ---
+ | - null
+ | - Session setting sql_default_engine expected a value of type string
+ | ...
+settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
+ | ---
+ | - error: 'Usage: box.session.settings:set(value)'
+ | ...
+settings.sql_parser_debug:set('string')
+ | ---
+ | - null
+ | - Session setting sql_parser_debug expected a value of type boolean
+ | ...
+
+str = string.rep('a', 20 * 1024)
+ | ---
+ | ...
+box.session.settings.sql_default_engine:set(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 85%
rename from test/box/gh-4511-access-settings-from-any-frontend.test.lua
rename to test/box/session_settings.test.lua
index b243be15e..23799874a 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:set('memtx')
+s:get('sql_default_engine').value
+settings.sql_defer_foreign_keys:set(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:set(true)
+settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
+settings.sql_parser_debug:set('string')
+
+str = string.rep('a', 20 * 1024)
+box.session.settings.sql_default_engine:set(str)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings
2020-02-17 12:12 ` [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings Chris Sosnin
@ 2020-03-16 16:08 ` Nikita Pettik
2020-03-16 22:53 ` Vladislav Shpilevoy
0 siblings, 1 reply; 22+ messages in thread
From: Nikita Pettik @ 2020-03-16 16:08 UTC (permalink / raw)
To: Chris Sosnin; +Cc: tarantool-patches, v.shpilevoy
On 17 Feb 15:12, Chris Sosnin wrote:
> - space_object:update() is hard to use for configuring session settings,
> so we provide box.session.setting 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
> ---
tarantool> box.session.settings.sql_vdbe_debug
---
- false
...
tarantool> box.session.settings.sql_vdbe_debug = true
---
...
tarantool> box.session.settings.sql_vdbe_debug
---
- true
...
tarantool> box.execute("select 1")
---
- metadata:
- name: '1'
type: integer
rows:
- [1]
...
Looks inconsistent. Can we use instead of :set() method simple
table value assignment? Otherwise accessing row table values
should be disallowed. Same concerns :get() method. Why ever
anyone should bother with :get() when one can access table value
via simple indexing?
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings
2020-03-16 16:08 ` Nikita Pettik
@ 2020-03-16 22:53 ` Vladislav Shpilevoy
2020-03-17 14:27 ` Nikita Pettik
0 siblings, 1 reply; 22+ messages in thread
From: Vladislav Shpilevoy @ 2020-03-16 22:53 UTC (permalink / raw)
To: Nikita Pettik, Chris Sosnin; +Cc: tarantool-patches
On 16/03/2020 17:08, Nikita Pettik wrote:
> On 17 Feb 15:12, Chris Sosnin wrote:
>> - space_object:update() is hard to use for configuring session settings,
>> so we provide box.session.setting 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
>> ---
>
> tarantool> box.session.settings.sql_vdbe_debug
> ---
> - false
> ...
>
> tarantool> box.session.settings.sql_vdbe_debug = true
> ---
> ...
>
> tarantool> box.session.settings.sql_vdbe_debug
> ---
> - true
> ...
Yeah, we can ban this. To avoid confusion.
>
> tarantool> box.execute("select 1")
> ---
> - metadata:
> - name: '1'
> type: integer
> rows:
> - [1]
> ...
>
> Looks inconsistent. Can we use instead of :set() method simple
> table value assignment? Otherwise accessing row table values
Assignment would require not to store settings in box.session.settings,
to be able to redefine __newindex metamethod. If we don't store them, we
kill autocompletion, which was asked explicitly by somebody.
But I am on your side here - I don't think autocompletion worth this
complication. Who wants to look at existing settings can just print
box.sessions.settings table.
> should be disallowed. Same concerns :get() method. Why ever
> anyone should bother with :get() when one can access table value
> via simple indexing?
Hm. But there is no :get() method. We didn't implement getting, because
no one asked for this. And indeed, usually you just set settings, without
checking if set really worked. I was thinking we could introduce get if
someone asks for that. But up to you. Can be added now as well.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings
2020-03-16 22:53 ` Vladislav Shpilevoy
@ 2020-03-17 14:27 ` Nikita Pettik
2020-03-17 14:36 ` Chris Sosnin
0 siblings, 1 reply; 22+ messages in thread
From: Nikita Pettik @ 2020-03-17 14:27 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On 16 Mar 23:53, Vladislav Shpilevoy wrote:
> On 16/03/2020 17:08, Nikita Pettik wrote:
> > On 17 Feb 15:12, Chris Sosnin wrote:
> >> - space_object:update() is hard to use for configuring session settings,
> >> so we provide box.session.setting 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
> >> ---
> >
> > tarantool> box.session.settings.sql_vdbe_debug
> > ---
> > - false
> > ...
> >
> > tarantool> box.session.settings.sql_vdbe_debug = true
> > ---
> > ...
> >
> > tarantool> box.session.settings.sql_vdbe_debug
> > ---
> > - true
> > ...
>
> Yeah, we can ban this. To avoid confusion.
>
> >
> > tarantool> box.execute("select 1")
> > ---
> > - metadata:
> > - name: '1'
> > type: integer
> > rows:
> > - [1]
> > ...
> >
> > Looks inconsistent. Can we use instead of :set() method simple
> > table value assignment? Otherwise accessing row table values
>
> Assignment would require not to store settings in box.session.settings,
> to be able to redefine __newindex metamethod. If we don't store them, we
> kill autocompletion, which was asked explicitly by somebody.
>
> But I am on your side here - I don't think autocompletion worth this
> complication. Who wants to look at existing settings can just print
> box.sessions.settings table.
>
> > should be disallowed. Same concerns :get() method. Why ever
> > anyone should bother with :get() when one can access table value
> > via simple indexing?
>
> Hm. But there is no :get() method. We didn't implement getting, because
> no one asked for this.
As far as I remember no one either asked for :set() method (except for me) :)
> And indeed, usually you just set settings, without
> checking if set really worked. I was thinking we could introduce get if
> someone asks for that. But up to you. Can be added now as well.
I do not insist since do not consider this to be prio1 task (as well
as any other issue connected with settings machinery).
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings
2020-03-17 14:27 ` Nikita Pettik
@ 2020-03-17 14:36 ` Chris Sosnin
0 siblings, 0 replies; 22+ messages in thread
From: Chris Sosnin @ 2020-03-17 14:36 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]
> On 17 Mar 2020, at 17:27, Nikita Pettik <korablev@tarantool.org> wrote:
>
> On 16 Mar 23:53, Vladislav Shpilevoy wrote:
>> On 16/03/2020 17:08, Nikita Pettik wrote:
>>> On 17 Feb 15:12, Chris Sosnin wrote:
>>>> - space_object:update() is hard to use for configuring session settings,
>>>> so we provide box.session.setting 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
>>>> ---
>>>
>>> tarantool> box.session.settings.sql_vdbe_debug
>>> ---
>>> - false
>>> ...
>>>
>>> tarantool> box.session.settings.sql_vdbe_debug = true
>>> ---
>>> ...
>>>
>>> tarantool> box.session.settings.sql_vdbe_debug
>>> ---
>>> - true
>>> ...
>>
>> Yeah, we can ban this. To avoid confusion.
>>
>>>
>>> tarantool> box.execute("select 1")
>>> ---
>>> - metadata:
>>> - name: '1'
>>> type: integer
>>> rows:
>>> - [1]
>>> ...
>>>
>>> Looks inconsistent. Can we use instead of :set() method simple
>>> table value assignment? Otherwise accessing row table values
>>
>> Assignment would require not to store settings in box.session.settings,
>> to be able to redefine __newindex metamethod. If we don't store them, we
>> kill autocompletion, which was asked explicitly by somebody.
>>
>> But I am on your side here - I don't think autocompletion worth this
>> complication. Who wants to look at existing settings can just print
>> box.sessions.settings table.
>>
>>> should be disallowed. Same concerns :get() method. Why ever
>>> anyone should bother with :get() when one can access table value
>>> via simple indexing?
>>
>> Hm. But there is no :get() method. We didn't implement getting, because
>> no one asked for this.
>
> As far as I remember no one either asked for :set() method (except for me) :)
This method is a workaround to allow console autocompletion, Lua doesn’t allow you to overload
indexing for the keys already present in the table. But here I agree that this implementation is rather
misleading. I will rework the patch to allow settings.<name> = <value> syntax.
>
>> And indeed, usually you just set settings, without
>> checking if set really worked. I was thinking we could introduce get if
>> someone asks for that. But up to you. Can be added now as well.
>
> I do not insist since do not consider this to be prio1 task (as well
> as any other issue connected with settings machinery).
[-- Attachment #2: Type: text/html, Size: 8115 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings
2020-02-17 12:12 [Tarantool-patches] [PATCH 0/4] box: session settings fixes Chris Sosnin
` (2 preceding siblings ...)
2020-02-17 12:12 ` [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings Chris Sosnin
@ 2020-02-17 12:12 ` Chris Sosnin
2020-03-16 17:02 ` Nikita Pettik
3 siblings, 1 reply; 22+ messages in thread
From: Chris Sosnin @ 2020-02-17 12:12 UTC (permalink / raw)
To: korablev, tarantool-patches
Currently if a user wants to change session setting with sql, he has
to execute non-obvious query, thus, we introduce a more native way to
do this.
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>:set(<new_value>)`.
Example of usage:
```
tarantool> box.session.settings.sql_default_engine
---
- memtx
...
tarantool> box.session.settings.sql_default_engine:set('vinyl')
---
...
```
The table itself represents the (unordered) result of select
from _session_settings space. Every setting is implemented as
a table, so there is no way to retrieve an actual value and use
it until :get() method is introduced.
SQL:
Instead of typing long UPDATE query one can use the SET statement:
`box.execute([[SET "<setting_name>" = <new_value>]])`.
Note, that this query is case sensitive so the name must be quoted.
Example:
```
tarantool> box.execute([[set "sql_default_engine" = 'memtx']])
---
- row_count: 1
...
tarantool> box.execute([[set "sql_defer_foreign_keys" = true]])
---
- row_count: 1
...
```
---
src/box/session_settings.c | 16 ++++++++--
src/box/session_settings.h | 3 ++
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 ++++++++
8 files changed, 169 insertions(+), 3 deletions(-)
diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index 874fc304a..3d06a191a 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/build.c b/src/box/sql/build.c
index e5fde08cc..9cbe25443 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3469,3 +3469,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_Set, 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 cfe1c0012..a01c37e19 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 SET command ////////////////////////////////
+cmd ::= 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 d1fcf4761..3ffae5970 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4510,4 +4510,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 620d74e66..c81486fa6 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
@@ -5248,6 +5249,55 @@ case OP_IncMaxid: {
break;
}
+/* Opcode: Set 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_Set: {
+ 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 6d7074e8c..7c9802af6 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 "sql_default_engine" = 'vinyl']])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - vinyl
+ | ...
+box.execute([[set "sql_default_engine" = 'memtx']])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - memtx
+ | ...
+box.execute([[set "sql_defer_foreign_keys" = true]])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - true
+ | ...
+box.execute([[set "sql_defer_foreign_keys" = ?]], {false})
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - false
+ | ...
+
settings.sql_default_engine:set(true)
| ---
| - null
@@ -361,3 +394,19 @@ box.session.settings.sql_default_engine:set(str)
| ---
| - error: Failed to allocate 20483 bytes in static_alloc for mp_value
| ...
+
+box.execute([[set "sql_def_engine" = true]])
+ | ---
+ | - null
+ | - Session setting sql_def_engine doesn't exist
+ | ...
+box.execute([[set "sql_default_engine" = true]])
+ | ---
+ | - null
+ | - Session setting sql_default_engine expected a value of type string
+ | ...
+box.execute([[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 23799874a..f2d7e03f5 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 "sql_default_engine" = 'vinyl']])
+s:get('sql_default_engine').value
+box.execute([[set "sql_default_engine" = 'memtx']])
+s:get('sql_default_engine').value
+box.execute([[set "sql_defer_foreign_keys" = true]])
+s:get('sql_defer_foreign_keys').value
+box.execute([[set "sql_defer_foreign_keys" = ?]], {false})
+s:get('sql_defer_foreign_keys').value
+
settings.sql_default_engine:set(true)
settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
settings.sql_parser_debug:set('string')
str = string.rep('a', 20 * 1024)
box.session.settings.sql_default_engine:set(str)
+
+box.execute([[set "sql_def_engine" = true]])
+box.execute([[set "sql_default_engine" = true]])
+box.execute([[set "sql_defer_foreign_keys" = 'true']])
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings
2020-02-17 12:12 ` [Tarantool-patches] [PATCH 4/4] sql: " Chris Sosnin
@ 2020-03-16 17:02 ` Nikita Pettik
2020-03-16 22:53 ` Vladislav Shpilevoy
2020-03-17 17:26 ` Chris Sosnin
0 siblings, 2 replies; 22+ messages in thread
From: Nikita Pettik @ 2020-03-16 17:02 UTC (permalink / raw)
To: Chris Sosnin; +Cc: tarantool-patches, v.shpilevoy
On 17 Feb 15:12, Chris Sosnin wrote:
> Currently if a user wants to change session setting with sql, he has
> to execute non-obvious query, thus, we introduce a more native way to
> do this.
It is not about non-obvious queries, but it is all about documentation:
the better feature is described, the clearer its usage turns out to be
for user.
> 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 statement:
> `box.execute([[SET "<setting_name>" = <new_value>]])`.
> Note, that this query is case sensitive so the name must be quoted.
>
> Example:
> ```
> tarantool> box.execute([[set "sql_default_engine" = 'memtx']])
> ---
> - row_count: 1
> ...
>
> tarantool> box.execute([[set "sql_defer_foreign_keys" = true]])
Why didn't you consider show/get command to retrieve setting value?
Otherwise, you simplify way to set session local options, but doesn't
provide the same way to extract current values.
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index d1fcf4761..3ffae5970 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4510,4 +4510,15 @@ int
> sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
> uint32_t *fieldno);
>
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 620d74e66..c81486fa6 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
> @@ -5248,6 +5249,55 @@ case OP_IncMaxid: {
> break;
> }
>
> +/* Opcode: Set 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_Set: {
Please, use more specific opcode name. Like OP_SettingSet.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings
2020-03-16 17:02 ` Nikita Pettik
@ 2020-03-16 22:53 ` Vladislav Shpilevoy
2020-03-17 17:26 ` Chris Sosnin
1 sibling, 0 replies; 22+ messages in thread
From: Vladislav Shpilevoy @ 2020-03-16 22:53 UTC (permalink / raw)
To: Nikita Pettik, Chris Sosnin; +Cc: tarantool-patches
On 16/03/2020 18:02, Nikita Pettik wrote:
> On 17 Feb 15:12, Chris Sosnin wrote:
>> Currently if a user wants to change session setting with sql, he has
>> to execute non-obvious query, thus, we introduce a more native way to
>> do this.
>
> It is not about non-obvious queries, but it is all about documentation:
> the better feature is described, the clearer its usage turns out to be
> for user.
However, on github you voted for this issue, and even agreed that at
least for SQL we need a simpler way to update settings. What changed?
In my opinion, none of system spaces should ever be allowed to be used
by users directly. Especially in such intricate and long way.
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index 620d74e66..c81486fa6 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -5248,6 +5249,55 @@ case OP_IncMaxid: {
>> break;
>> }
>>
>> +/* Opcode: Set 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_Set: {
>
> Please, use more specific opcode name. Like OP_SettingSet.
Should not we then change the grammar? Because it requires just SET to
change a session setting. Not SET SETTING.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings
2020-03-16 17:02 ` Nikita Pettik
2020-03-16 22:53 ` Vladislav Shpilevoy
@ 2020-03-17 17:26 ` Chris Sosnin
2020-03-17 20:12 ` Nikita Pettik
1 sibling, 1 reply; 22+ messages in thread
From: Chris Sosnin @ 2020-03-17 17:26 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
> On 16 Mar 2020, at 20:02, Nikita Pettik <korablev@tarantool.org> wrote:
>
> On 17 Feb 15:12, Chris Sosnin wrote:
>> Currently if a user wants to change session setting with sql, he has
>> to execute non-obvious query, thus, we introduce a more native way to
>> do this.
>
> It is not about non-obvious queries, but it is all about documentation:
> the better feature is described, the clearer its usage turns out to be
> for user.
Should I change the commit message? I though this patchset is about simplifying
the way session settings are updated?
>
>> 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 statement:
>> `box.execute([[SET "<setting_name>" = <new_value>]])`.
>> Note, that this query is case sensitive so the name must be quoted.
>>
>> Example:
>> ```
>> tarantool> box.execute([[set "sql_default_engine" = 'memtx']])
>> ---
>> - row_count: 1
>> ...
>>
>> tarantool> box.execute([[set "sql_defer_foreign_keys" = true]])
>
> Why didn't you consider show/get command to retrieve setting value?
> Otherwise, you simplify way to set session local options, but doesn't
> provide the same way to extract current values.
SELECT * FROM “_session_settings” is simple enough, isn’t it?
>
>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>> index d1fcf4761..3ffae5970 100644
>> --- a/src/box/sql/sqlInt.h
>> +++ b/src/box/sql/sqlInt.h
>> @@ -4510,4 +4510,15 @@ int
>> sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
>> uint32_t *fieldno);
>>
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index 620d74e66..c81486fa6 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
>> @@ -5248,6 +5249,55 @@ case OP_IncMaxid: {
>> break;
>> }
>>
>> +/* Opcode: Set 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_Set: {
>
> Please, use more specific opcode name. Like OP_SettingSet.
Didn’t we accept ’set’ syntax?
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings
2020-03-17 17:26 ` Chris Sosnin
@ 2020-03-17 20:12 ` Nikita Pettik
2020-03-17 21:00 ` Chris Sosnin
2020-03-18 10:00 ` Chris Sosnin
0 siblings, 2 replies; 22+ messages in thread
From: Nikita Pettik @ 2020-03-17 20:12 UTC (permalink / raw)
To: Chris Sosnin; +Cc: tarantool-patches
On 17 Mar 20:26, Chris Sosnin wrote:
>
>
> > On 16 Mar 2020, at 20:02, Nikita Pettik <korablev@tarantool.org> wrote:
> >
> > On 17 Feb 15:12, Chris Sosnin wrote:
> >> Currently if a user wants to change session setting with sql, he has
> >> to execute non-obvious query, thus, we introduce a more native way to
> >> do this.
> >
> > It is not about non-obvious queries, but it is all about documentation:
> > the better feature is described, the clearer its usage turns out to be
> > for user.
>
> Should I change the commit message? I though this patchset is about simplifying
> the way session settings are updated?
I would say:
Currently if a user wants to change session settings via 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 good practice.
To avoid that and a bit simplify user's life, we introduce SQL shortcut command
SET with following syntax: SET <setting_name> = <value>. <setting_name> is
supposed to be name of setting (note that it is uppercased as any other
non-quoted indentifiers in SQL) and <value> - value of corresponding setting to
be set; <value> should be either literal or binding marker. Also it is worth
noting that SET doesn't provide any implicit casts, so <value> must be of the
type corresponding to the setting being updated.
Example:
...
^ It is up to you.
> >
> >> 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 statement:
> >> `box.execute([[SET "<setting_name>" = <new_value>]])`.
> >> Note, that this query is case sensitive so the name must be quoted.
> >>
> >> Example:
> >> ```
> >> tarantool> box.execute([[set "sql_default_engine" = 'memtx']])
> >> ---
> >> - row_count: 1
> >> ...
> >>
> >> tarantool> box.execute([[set "sql_defer_foreign_keys" = true]])
> >
> > Why didn't you consider show/get command to retrieve setting value?
> > Otherwise, you simplify way to set session local options, but doesn't
> > provide the same way to extract current values.
>
> SELECT * FROM “_session_settings” is simple enough, isn’t it?
In this case you'll get list of all settings. To get specific one
we should use this query:
SELECT value FROM "_session_settings" WHERE "name" = 'xxx'
I do not insist on implementing GET/SHOW SQL syntax now tho.
> >> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> >> index d1fcf4761..3ffae5970 100644
> >> --- a/src/box/sql/sqlInt.h
> >> +++ b/src/box/sql/sqlInt.h
> >> @@ -4510,4 +4510,15 @@ int
> >> sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
> >> uint32_t *fieldno);
> >>
> >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> >> index 620d74e66..c81486fa6 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
> >> @@ -5248,6 +5249,55 @@ case OP_IncMaxid: {
> >> break;
> >> }
> >>
> >> +/* Opcode: Set 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_Set: {
> >
> > Please, use more specific opcode name. Like OP_SettingSet.
>
> Didn’t we accept ’set’ syntax?
SET is okay, but I don't mind SET SETTING as well. OP_Set is too general
name - SET keyword is also part of UPDATE syntax, so it may confuse other
developers. Except for this nit patch LGTM.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings
2020-03-17 20:12 ` Nikita Pettik
@ 2020-03-17 21:00 ` Chris Sosnin
2020-03-18 10:00 ` Chris Sosnin
1 sibling, 0 replies; 22+ messages in thread
From: Chris Sosnin @ 2020-03-17 21:00 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 4231 bytes --]
Thank you for your answers!
> On 17 Mar 2020, at 23:12, Nikita Pettik <korablev@tarantool.org> wrote:
>
> On 17 Mar 20:26, Chris Sosnin wrote:
>>
>>
>>> On 16 Mar 2020, at 20:02, Nikita Pettik <korablev@tarantool.org> wrote:
>>>
>>> On 17 Feb 15:12, Chris Sosnin wrote:
>>>> Currently if a user wants to change session setting with sql, he has
>>>> to execute non-obvious query, thus, we introduce a more native way to
>>>> do this.
>>>
>>> It is not about non-obvious queries, but it is all about documentation:
>>> the better feature is described, the clearer its usage turns out to be
>>> for user.
>>
>> Should I change the commit message? I though this patchset is about simplifying
>> the way session settings are updated?
>
> I would say:
>
> Currently if a user wants to change session settings via 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 good practice.
> To avoid that and a bit simplify user's life, we introduce SQL shortcut command
> SET with following syntax: SET <setting_name> = <value>. <setting_name> is
> supposed to be name of setting (note that it is uppercased as any other
> non-quoted indentifiers in SQL) and <value> - value of corresponding setting to
> be set; <value> should be either literal or binding marker. Also it is worth
> noting that SET doesn't provide any implicit casts, so <value> must be of the
> type corresponding to the setting being updated.
>
> Example:
> ...
>
> ^ It is up to you.
>
>>>
>>>> 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 statement:
>>>> `box.execute([[SET "<setting_name>" = <new_value>]])`.
>>>> Note, that this query is case sensitive so the name must be quoted.
>>>>
>>>> Example:
>>>> ```
>>>> tarantool> box.execute([[set "sql_default_engine" = 'memtx']])
>>>> ---
>>>> - row_count: 1
>>>> ...
>>>>
>>>> tarantool> box.execute([[set "sql_defer_foreign_keys" = true]])
>>>
>>> Why didn't you consider show/get command to retrieve setting value?
>>> Otherwise, you simplify way to set session local options, but doesn't
>>> provide the same way to extract current values.
>>
>> SELECT * FROM “_session_settings” is simple enough, isn’t it?
>
> In this case you'll get list of all settings. To get specific one
> we should use this query:
>
> SELECT value FROM "_session_settings" WHERE "name" = 'xxx'
>
> I do not insist on implementing GET/SHOW SQL syntax now tho.
>
>>>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>>>> index d1fcf4761..3ffae5970 100644
>>>> --- a/src/box/sql/sqlInt.h
>>>> +++ b/src/box/sql/sqlInt.h
>>>> @@ -4510,4 +4510,15 @@ int
>>>> sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
>>>> uint32_t *fieldno);
>>>>
>>>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>>>> index 620d74e66..c81486fa6 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
>>>> @@ -5248,6 +5249,55 @@ case OP_IncMaxid: {
>>>> break;
>>>> }
>>>>
>>>> +/* Opcode: Set 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_Set: {
>>>
>>> Please, use more specific opcode name. Like OP_SettingSet.
>>
>> Didn’t we accept ’set’ syntax?
>
> SET is okay, but I don't mind SET SETTING as well. OP_Set is too general
> name - SET keyword is also part of UPDATE syntax, so it may confuse other
> developers. Except for this nit patch LGTM.
I don’t mind this changes, I will work on them tomorrow.
[-- Attachment #2: Type: text/html, Size: 26502 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings
2020-03-17 20:12 ` Nikita Pettik
2020-03-17 21:00 ` Chris Sosnin
@ 2020-03-18 10:00 ` Chris Sosnin
1 sibling, 0 replies; 22+ messages in thread
From: Chris Sosnin @ 2020-03-18 10:00 UTC (permalink / raw)
To: Nikita Pettik, Vladislav Shpilevoy; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 5352 bytes --]
> On 17 Mar 2020, at 23:12, Nikita Pettik <korablev@tarantool.org> wrote:
>
> On 17 Mar 20:26, Chris Sosnin wrote:
>>
>>
>>> On 16 Mar 2020, at 20:02, Nikita Pettik <korablev@tarantool.org> wrote:
>>>
>>> On 17 Feb 15:12, Chris Sosnin wrote:
>>>> Currently if a user wants to change session setting with sql, he has
>>>> to execute non-obvious query, thus, we introduce a more native way to
>>>> do this.
>>>
>>> It is not about non-obvious queries, but it is all about documentation:
>>> the better feature is described, the clearer its usage turns out to be
>>> for user.
>>
>> Should I change the commit message? I though this patchset is about simplifying
>> the way session settings are updated?
>
> I would say:
>
> Currently if a user wants to change session settings via 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 good practice.
> To avoid that and a bit simplify user's life, we introduce SQL shortcut command
> SET with following syntax: SET <setting_name> = <value>. <setting_name> is
> supposed to be name of setting (note that it is uppercased as any other
> non-quoted indentifiers in SQL) and <value> - value of corresponding setting to
> be set; <value> should be either literal or binding marker. Also it is worth
> noting that SET doesn't provide any implicit casts, so <value> must be of the
> type corresponding to the setting being updated.
>
> Example:
> ...
>
> ^ It is up to you.
I took the beginning as long as the rest is described in the doc request (except for
implicit casts note, thanks for pointing that out, fixed):
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.
>>>>
>>>> SQL:
>>>> Instead of typing long UPDATE query one can use the SET statement:
>>>> `box.execute([[SET "<setting_name>" = <new_value>]])`.
>>>> Note, that this query is case sensitive so the name must be quoted.
>>>>
>>>> Example:
>>>> ```
>>>> tarantool> box.execute([[set "sql_default_engine" = 'memtx']])
>>>> ---
>>>> - row_count: 1
>>>> ...
>>>>
>>>> tarantool> box.execute([[set "sql_defer_foreign_keys" = true]])
>>>
>>> Why didn't you consider show/get command to retrieve setting value?
>>> Otherwise, you simplify way to set session local options, but doesn't
>>> provide the same way to extract current values.
>>
>> SELECT * FROM “_session_settings” is simple enough, isn’t it?
>
> In this case you'll get list of all settings. To get specific one
> we should use this query:
>
> SELECT value FROM "_session_settings" WHERE "name" = 'xxx'
>
> I do not insist on implementing GET/SHOW SQL syntax now tho.
>
>>>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>>>> index d1fcf4761..3ffae5970 100644
>>>> --- a/src/box/sql/sqlInt.h
>>>> +++ b/src/box/sql/sqlInt.h
>>>> @@ -4510,4 +4510,15 @@ int
>>>> sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
>>>> uint32_t *fieldno);
>>>>
>>>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>>>> index 620d74e66..c81486fa6 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
>>>> @@ -5248,6 +5249,55 @@ case OP_IncMaxid: {
>>>> break;
>>>> }
>>>>
>>>> +/* Opcode: Set 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_Set: {
>>>
>>> Please, use more specific opcode name. Like OP_SettingSet.
>>
>> Didn’t we accept ’set’ syntax?
>
> SET is okay, but I don't mind SET SETTING as well. OP_Set is too general
> name - SET keyword is also part of UPDATE syntax, so it may confuse other
> developers. Except for this nit patch LGTM.
Here are the fixes:
- sqlVdbeAddOp2(vdbe, OP_Set, index, target);
+ sqlVdbeAddOp2(vdbe, OP_SetSetting, index, target);
-///////////////////////////// The SET command ////////////////////////////////
-cmd ::= SET nm(X) EQ expr(Y). {
+///////////////////////////// The SETTING SET command ////////////////////////
+cmd ::= SETTING SET nm(X) EQ expr(Y). {
-/* Opcode: Set P1 P2 * * *
+/* Opcode: SetSetting P1 P2 * * *
-case OP_Set: {
+case OP_SetSetting: {
And of course I had to change the grammar and tests.
Changes are on the branch with the second version of Lua patch:
https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session-settings-v2
[-- Attachment #2: Type: text/html, Size: 28033 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [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
0 siblings, 1 reply; 22+ 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] 22+ messages in thread
* [Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array
2020-02-03 22:17 [Tarantool-patches] [PATCH v4 1/3] " Vladislav Shpilevoy
@ 2020-02-04 19:29 ` Chris Sosnin
2020-02-06 22:14 ` Vladislav Shpilevoy
0 siblings, 1 reply; 22+ messages in thread
From: Chris Sosnin @ 2020-02-04 19:29 UTC (permalink / raw)
To: v.shpilevoy, tarantool-patches
Hi! Thank you for the review and your fixes!
> 1. I changed the name pattern to SESSION_SETTING_SQL_*. Because since we have
> a single namespace of the settings now, newer settings would look strange.
> For example, read-only setting with the old pattern would be
> READ_ONLY_SESSION_SETTING or something like that. So I made SESSION_SETTING
> a prefix.
This is reasonable, I agree.
> 2. The original mask was uint32. Lets keep it this way.
> Talking of field type - I would try to make it
> enum field_type. Don't know why it was an integer.
I added new include and changed it to be enum and uint32:
+#include "field_def.h"
...
+struct session_setting_metadata {
+ /** Setting's value type. */
+ enum field_type field_type;
+ /** Stores setting's bit flags. */
+ uint32_t mask;
+};
> 3. Please, add comments what is struct session_setting_metadata,
> and what is session_setting.metadata. Every member of a struct
> should have a comment.
I restored the original comment and added some new:
+/**
+ * A structure that allows to establish a connection between
+ * parameter and its field type and mask, if it has one.
+*/
+struct session_setting_metadata {
+ /** Setting's value type. */
+ enum field_type field_type;
+ /** Stores setting's bit flags. */
+ uint32_t mask;
+};
+
+struct session_setting {
+ /** Setting's value type and the corresponding mask. */
+ struct session_setting_metadata metadata;
> 4. String representation of enum values and the values themselves
> should not be stored in different compilation units. Please, keep
> them together. Then you can drop 'session_setting.name' attribute
> and do all the searches in that string array.
I moved the definition of the session_setting_strs into session_settings.c.
See the new version of this patch below:
================================================================================
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
---
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings
issue #1: https://github.com/tarantool/tarantool/issues/4711
issue #2: https://github.com/tarantool/tarantool/issues/4712
src/box/session_settings.c | 121 ++++++++++++++-----------------------
src/box/session_settings.h | 62 +++++++++++++------
src/box/sql/build.c | 95 ++++++++---------------------
3 files changed, 114 insertions(+), 164 deletions(-)
diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index a969d3d10..93d1c8e22 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->dict,
&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..1f18f147b 100644
--- a/src/box/session_settings.h
+++ b/src/box/session_settings.h
@@ -30,29 +30,52 @@
* 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,
-};
-
-struct session_setting_module {
+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,
/**
- * An array of setting names. All of them should have the
- * same prefix.
+ * Follow the pattern for groups of settings:
+ * SESSION_SETTING_<N>_BEGIN = SESSION_SETTING_<N-1>_END,
+ * ...
+ * SESSION_SETTING_<N>_END,
*/
- const char **settings;
- /** Count of settings. */
- int setting_count;
+ SESSION_SETTING_COUNT = SESSION_SETTING_SQL_END,
+};
+
+/**
+ * A structure that allows to establish a connection between
+ * parameter and its field type and mask, if it has one.
+*/
+struct session_setting_metadata {
+ /** Setting's value type. */
+ enum field_type field_type;
+ /** Stores setting's bit flags. */
+ uint32_t mask;
+};
+
+struct session_setting {
+ /** Setting's value type and the corresponding mask. */
+ struct session_setting_metadata metadata;
/**
* Get a MessagePack encoded pair [name, value] for a
* setting having index @a id. Index is from the settings
@@ -67,4 +90,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 bc50ecbfa..3da60df86 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3310,75 +3310,31 @@ 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.
- */
-struct sql_option_metadata
-{
- uint32_t field_type;
- uint32_t mask;
-};
-
/**
* Variable that contains SQL session option field types and masks
* if they have one or 0 if don't have.
*
* It is IMPORTANT that these options sorted by name.
*/
-static struct sql_option_metadata sql_session_opts[] = {
- /** SQL_SESSION_SETTING_DEFAULT_ENGINE */
+static struct session_setting_metadata sql_session_opts[] = {
+ /** 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},
};
@@ -3386,12 +3342,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];
+ struct session_setting_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;
@@ -3404,7 +3360,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);
@@ -3426,7 +3382,7 @@ static int
sql_set_boolean_option(int id, bool value)
{
struct session *session = current_session();
- struct sql_option_metadata *option = &sql_session_opts[id];
+ struct session_setting_metadata *option = &sql_session_opts[id];
assert(option->field_type == FIELD_TYPE_BOOLEAN);
#ifdef NDEBUG
if ((session->sql_flags & SQL_SqlTrace) == 0) {
@@ -3440,7 +3396,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
@@ -3454,7 +3410,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) {
@@ -3468,7 +3424,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;
@@ -3488,17 +3444,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->metadata = sql_session_opts[id];
+ setting->get = sql_session_setting_get;
+ setting->set = sql_session_setting_set;
+ }
}
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array
2020-02-04 19:29 ` [Tarantool-patches] [PATCH 1/4] " Chris Sosnin
@ 2020-02-06 22:14 ` Vladislav Shpilevoy
0 siblings, 0 replies; 22+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-06 22:14 UTC (permalink / raw)
To: Chris Sosnin, tarantool-patches
Hi! Thanks for the fixes!
Now I noticed that struct session_setting_metadata is not
really needed. Mask field is never used. I reverted it back
to build.c, and added field type directly to struct
session_setting.
================================================================================
commit 5b80dce870f045417e63458f6672a82ed5e29bfb
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Thu Feb 6 22:06:14 2020 +0100
Review fixes
diff --git a/src/box/session_settings.h b/src/box/session_settings.h
index 1f18f147b..de24e3c6f 100644
--- a/src/box/session_settings.h
+++ b/src/box/session_settings.h
@@ -62,20 +62,12 @@ enum {
SESSION_SETTING_COUNT = SESSION_SETTING_SQL_END,
};
-/**
- * A structure that allows to establish a connection between
- * parameter and its field type and mask, if it has one.
-*/
-struct session_setting_metadata {
- /** Setting's value type. */
- enum field_type field_type;
- /** Stores setting's bit flags. */
- uint32_t mask;
-};
-
struct session_setting {
- /** Setting's value type and the corresponding mask. */
- struct session_setting_metadata metadata;
+ /**
+ * Setting's value type. Used for error checking and
+ * reporting only.
+ */
+ enum field_type field_type;
/**
* Get a MessagePack encoded pair [name, value] for a
* setting having index @a id. Index is from the settings
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 3da60df86..e5fde08cc 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3310,13 +3310,23 @@ sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
return 0;
}
+/**
+ * A local structure that allows to establish a connection between
+ * parameter and its field type and mask, if it has one.
+ */
+struct sql_option_metadata
+{
+ uint32_t field_type;
+ uint32_t mask;
+};
+
/**
* Variable that contains SQL session option field types and masks
* if they have one or 0 if don't have.
*
* It is IMPORTANT that these options sorted by name.
*/
-static struct session_setting_metadata sql_session_opts[] = {
+static struct sql_option_metadata sql_session_opts[] = {
/** SESSION_SETTING_SQL_DEFAULT_ENGINE */
{FIELD_TYPE_STRING, 0},
/** SESSION_SETTING_SQL_DEFER_FOREIGN_KEYS */
@@ -3345,7 +3355,7 @@ sql_session_setting_get(int id, const char **mp_pair, const char **mp_pair_end)
assert(id >= SESSION_SETTING_SQL_BEGIN && id < SESSION_SETTING_SQL_END);
struct session *session = current_session();
uint32_t flags = session->sql_flags;
- struct session_setting_metadata *opt = &sql_session_opts[id];
+ struct sql_option_metadata *opt = &sql_session_opts[id];
uint32_t mask = opt->mask;
const char *name = session_setting_strs[id];
size_t name_len = strlen(name);
@@ -3382,7 +3392,7 @@ static int
sql_set_boolean_option(int id, bool value)
{
struct session *session = current_session();
- struct session_setting_metadata *option = &sql_session_opts[id];
+ struct sql_option_metadata *option = &sql_session_opts[id];
assert(option->field_type == FIELD_TYPE_BOOLEAN);
#ifdef NDEBUG
if ((session->sql_flags & SQL_SqlTrace) == 0) {
@@ -3454,7 +3464,7 @@ sql_session_settings_init()
for (int id = SESSION_SETTING_SQL_BEGIN; id < SESSION_SETTING_SQL_END;
++id) {
struct session_setting *setting = &session_settings[id];
- setting->metadata = sql_session_opts[id];
+ setting->field_type = sql_session_opts[id].field_type;
setting->get = sql_session_setting_get;
setting->set = sql_session_setting_set;
}
^ permalink raw reply [flat|nested] 22+ messages in thread