[Tarantool-patches] [PATCH 0/4] session settings fixes

[Tarantool-patches] [PATCH 0/4] session settings fixes

[Chris Sosnin]

issue #1:https://github.com/tarantool/tarantool/issues/4711
issue #2:https://github.com/tarantool/tarantool/issues/4712
branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session-settings-v2

Chris Sosnin (4):
  box: replace session_settings modules with a single array
  box: add binary search for _session_settings space
  box: provide a user friendly frontend for accessing session settings
  sql: provide a user friendly frontend for accessing session settings

 extra/mkkeywordhash.c                         |   1 +
 src/box/lua/session.c                         | 111 +++++++++
 src/box/session.cc                            |   1 +
 src/box/session.h                             |   2 +
 src/box/session_settings.c                    | 214 +++++++++++-------
 src/box/session_settings.h                    |  53 +++--
 src/box/sql.c                                 |   5 -
 src/box/sql/build.c                           | 104 ++++-----
 src/box/sql/parse.y                           |   5 +
 src/box/sql/sqlInt.h                          |  11 +
 src/box/sql/vdbe.c                            |  50 ++++
 ...rontend.result => session_settings.result} | 147 ++++++++++--
 ...end.test.lua => session_settings.test.lua} |  61 ++++-
 13 files changed, 589 insertions(+), 176 deletions(-)
 rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (71%)
 rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (64%)

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array

[Chris Sosnin]

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)

[Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space

[Chris Sosnin]

As it is mentioned in implementation, it is important
that _session_settings options are sorted by name, so
there is no need in linear lookup and we can replace it
with binary search.

Closes #4712
---
 src/box/session_settings.c                    | 97 +++++++++++++++++--
 ...1-access-settings-from-any-frontend.result | 37 ++++---
 ...access-settings-from-any-frontend.test.lua | 28 ++++--
 3 files changed, 133 insertions(+), 29 deletions(-)

diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index 2ecf71f2d..5e4a50427 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -81,6 +81,8 @@ struct session_settings_iterator {
 	bool is_eq;
 	/** True if the iterator should include equal keys. */
 	bool is_including;
+	/** True if the iterator is pointing to an existing setting */
+	bool is_set;
 };
 
 static void
@@ -92,6 +94,72 @@ session_settings_iterator_free(struct iterator *ptr)
 	free(it);
 }
 
+static int
+session_settings_set_forward(int *sid, const char *key, bool is_eq,
+			     bool is_including)
+{
+	int low = 0, high = SESSION_SETTING_COUNT - 1;
+	if (key == NULL)
+		return 0;
+	while (low <= high) {
+		int index = (high + low) / 2;
+		const char *name = session_setting_strs[index];
+		int cmp = strcmp(name, key);
+		if (cmp == 0) {
+			if (is_including) {
+				*sid = index;
+				return 0;
+			}
+			*sid = ++index;
+			return index < SESSION_SETTING_COUNT ? 0 : -1;
+		}
+		if (cmp < 0)
+			low = index + 1;
+		else
+			high = index - 1;
+	}
+	if (is_eq) {
+		*sid = SESSION_SETTING_COUNT;
+		return -1;
+	}
+	assert(low > high);
+	*sid = low;
+	return low < SESSION_SETTING_COUNT ? 0 : -1;
+}
+
+static int
+session_settings_set_reverse(int *sid, const char *key, bool is_eq,
+			     bool is_including)
+{
+	int low = 0, high = SESSION_SETTING_COUNT - 1;
+	if (key == NULL)
+		return 0;
+	while (low <= high) {
+		int index = (high + low) / 2;
+		const char *name = session_setting_strs[index];
+		int cmp = strcmp(name, key);
+		if (cmp == 0) {
+			if (is_including) {
+				*sid = index;
+				return 0;
+			}
+			*sid = --index;
+			return index >= 0 ? 0 : -1;
+		}
+		if (cmp < 0)
+			low = index + 1;
+		else
+			high = index - 1;
+	}
+	if (is_eq) {
+		*sid = SESSION_SETTING_COUNT;
+		return -1;
+	}
+	assert(low > high);
+	*sid = high;
+	return high >= 0 ? 0 : -1;
+}
+
 static int
 session_settings_next(int *sid, const char *key, bool is_eq, bool is_including)
 {
@@ -145,9 +213,15 @@ session_settings_iterator_next(struct iterator *iterator, struct tuple **result)
 	int sid = it->setting_id;
 	const char *key = it->key;
 	bool is_including = it->is_including, is_eq = it->is_eq;
-	bool is_found = false;
-	if (session_settings_next(&sid, key, is_eq, is_including) == 0)
-		is_found = true;
+	bool is_found;
+	if (!it->is_set) {
+		it->is_set = true;
+		is_found = session_settings_set_forward(&sid, key, is_eq,
+							is_including) == 0;
+	} else {
+		is_found = session_settings_next(&sid, key, is_eq,
+						 is_including) == 0;
+	}
 	it->setting_id = sid + 1;
 	if (!is_found) {
 		*result = NULL;
@@ -167,9 +241,15 @@ session_settings_iterator_prev(struct iterator *iterator, struct tuple **result)
 	int sid = it->setting_id;
 	const char *key = it->key;
 	bool is_including = it->is_including, is_eq = it->is_eq;
-	bool is_found = false;
-	if (session_settings_prev(&sid, key, is_eq, is_including) == 0)
-		is_found = true;
+	bool is_found;
+	if (!it->is_set) {
+		it->is_set = true;
+		is_found = session_settings_set_reverse(&sid, key, is_eq,
+							is_including) == 0;
+	} else {
+		is_found = session_settings_prev(&sid, key, is_eq,
+						 is_including) == 0;
+	}
 	it->setting_id = sid - 1;
 	if (!is_found) {
 		*result = NULL;
@@ -221,6 +301,7 @@ session_settings_index_create_iterator(struct index *base,
 	it->is_eq = type == ITER_EQ || type == ITER_REQ;
 	it->is_including = it->is_eq || type == ITER_GE || type == ITER_ALL ||
 			   type == ITER_LE;
+	it->is_set = false;
 	it->format = index->format;
 	if (!iterator_type_is_reverse(type)) {
 		it->base.next = session_settings_iterator_next;
@@ -244,7 +325,7 @@ session_settings_index_get(struct index *base, const char *key,
 	key = mp_decode_str(&key, &len);
 	key = tt_cstr(key, len);
 	int sid = 0;
-	if (session_settings_next(&sid, key, true, true) != 0) {
+	if (session_settings_set_forward(&sid, key, true, true) != 0) {
 		*result = NULL;
 		return 0;
 	}
@@ -345,7 +426,7 @@ session_settings_space_execute_update(struct space *space, struct txn *txn,
 	}
 	key = mp_decode_str(&key, &key_len);
 	key = tt_cstr(key, key_len);
-	if (session_settings_next(&sid, key, true, true) != 0) {
+	if (session_settings_set_forward(&sid, key, true, true) != 0) {
 		*result = NULL;
 		return 0;
 	}
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.result b/test/box/gh-4511-access-settings-from-any-frontend.result
index ed340d053..bae77192e 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/gh-4511-access-settings-from-any-frontend.result
@@ -82,6 +82,12 @@ function check_sorting(ss, ts, key)
     for _, it in pairs(iterators_list) do
         local view_space = ss:select({key}, {iterator = it})
         local test_space = ts:select({key}, {iterator = it})
+        if #view_space ~= #test_space then
+            return {
+                err = 'bad sorting', type = it, exp = test_space,
+                got = view_space
+            }
+        end
         for key, value in pairs(view_space) do
             if test_space[key].name ~= value.name then
                 return {
@@ -111,32 +117,37 @@ check_sorting(s, t, 'sql_d')
 check_sorting(s, t, 'sql_v')
  | ---
  | ...
-check_sorting(s, t, 'sql_defer_foreign_keys')
+check_sorting(s, t, 'z')
  | ---
  | ...
-
-t:drop()
+check_sorting(s, t, 'sql_parser_debuf')
+ | ---
+ | ...
+check_sorting(s, t, 'sql_parser_debuh')
  | ---
  | ...
 
--- Check get() method of session_settings space.
-s:get({'sql_defer_foreign_keys'})
+name = nil
  | ---
- | - ['sql_defer_foreign_keys', false]
  | ...
-s:get({'sql_recursive_triggers'})
+err = nil
  | ---
- | - ['sql_recursive_triggers', true]
  | ...
-s:get({'sql_reverse_unordered_selects'})
+for _, tuple in t:pairs() do                    \
+    name = tuple.name                           \
+    err = check_sorting(s, t, name)             \
+    if err then                                 \
+        break                                   \
+    end                                         \
+end
  | ---
- | - ['sql_reverse_unordered_selects', false]
  | ...
-s:get({'sql_default_engine'})
+err and {err, name}
  | ---
- | - ['sql_default_engine', 'memtx']
+ | - null
  | ...
-s:get({'abcd'})
+
+t:drop()
  | ---
  | ...
 
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/gh-4511-access-settings-from-any-frontend.test.lua
index 366874998..b243be15e 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.test.lua
+++ b/test/box/gh-4511-access-settings-from-any-frontend.test.lua
@@ -36,6 +36,12 @@ function check_sorting(ss, ts, key)
     for _, it in pairs(iterators_list) do
         local view_space = ss:select({key}, {iterator = it})
         local test_space = ts:select({key}, {iterator = it})
+        if #view_space ~= #test_space then
+            return {
+                err = 'bad sorting', type = it, exp = test_space,
+                got = view_space
+            }
+        end
         for key, value in pairs(view_space) do
             if test_space[key].name ~= value.name then
                 return {
@@ -52,17 +58,23 @@ check_sorting(s, t)
 check_sorting(s, t, 'abcde')
 check_sorting(s, t, 'sql_d')
 check_sorting(s, t, 'sql_v')
-check_sorting(s, t, 'sql_defer_foreign_keys')
+check_sorting(s, t, 'z')
+check_sorting(s, t, 'sql_parser_debuf')
+check_sorting(s, t, 'sql_parser_debuh')
+
+name = nil
+err = nil
+for _, tuple in t:pairs() do                    \
+    name = tuple.name                           \
+    err = check_sorting(s, t, name)             \
+    if err then                                 \
+        break                                   \
+    end                                         \
+end
+err and {err, name}
 
 t:drop()
 
--- Check get() method of session_settings space.
-s:get({'sql_defer_foreign_keys'})
-s:get({'sql_recursive_triggers'})
-s:get({'sql_reverse_unordered_selects'})
-s:get({'sql_default_engine'})
-s:get({'abcd'})
-
 -- Check pairs() method of session_settings space.
 t = {}
 for key, value in s:pairs() do table.insert(t, {key, value}) end
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings

[Chris Sosnin]

- space_object:update() is hard to use for configuring session settings,
so we provide box.session.settings table, which can be used in a much more
native way.

- Prior to this patch sql settings were not accessible before box.cfg()
call, even though these flags can be set right after session creation.

Part of #4711
---
 src/box/lua/session.c                         | 111 ++++++++++++++++++
 src/box/session.cc                            |   1 +
 src/box/session.h                             |   2 +
 src/box/session_settings.c                    |  16 ++-
 src/box/session_settings.h                    |   3 +
 src/box/sql.c                                 |   5 -
 ...rontend.result => session_settings.result} |  61 ++++++++++
 ...end.test.lua => session_settings.test.lua} |  20 ++++
 8 files changed, 211 insertions(+), 8 deletions(-)
 rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (86%)
 rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (86%)

diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index c6a600f6f..103bf22fd 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -42,6 +42,8 @@
 #include "box/user.h"
 #include "box/schema.h"
 #include "box/port.h"
+#include "box/session_settings.h"
+#include "tt_static.h"
 
 static const char *sessionlib_name = "box.session";
 
@@ -411,6 +413,114 @@ lbox_session_on_access_denied(struct lua_State *L)
 				  lbox_push_on_access_denied_event, NULL);
 }
 
+static int
+lbox_session_setting_get_by_id(struct lua_State *L, int sid)
+{
+	assert(sid >= 0 && sid < SESSION_SETTING_COUNT);
+	const char *mp_pair, *mp_pair_end;
+	session_settings[sid].get(sid, &mp_pair, &mp_pair_end);
+	uint32_t len;
+	mp_decode_array(&mp_pair);
+	mp_decode_str(&mp_pair, &len);
+	enum field_type field_type = session_settings[sid].field_type;
+	if (field_type == FIELD_TYPE_BOOLEAN) {
+		bool value = mp_decode_bool(&mp_pair);
+		lua_pushboolean(L, value);
+	} else {
+		assert(field_type == FIELD_TYPE_STRING);
+		const char *str = mp_decode_str(&mp_pair, &len);
+		lua_pushlstring(L, str, len);
+	}
+	return 1;
+}
+
+static int
+lbox_session_setting_get(struct lua_State *L)
+{
+	assert(lua_gettop(L) == 2);
+	const char *setting_name = lua_tostring(L, -1);
+	int sid = session_setting_find(setting_name);
+	if (sid < 0) {
+		diag_set(ClientError, ER_PROC_LUA, tt_sprintf("Session "\
+			 "setting %s doesn't exist", setting_name));
+		return luaT_error(L);
+	}
+	return lbox_session_setting_get_by_id(L, sid);
+}
+
+static int
+lbox_session_setting_set(struct lua_State *L)
+{
+	assert(lua_gettop(L) == 3);
+	int arg_type = lua_type(L, -1);
+	const char *setting_name = lua_tostring(L, -2);
+	int sid = session_setting_find(setting_name);
+	if (sid < 0) {
+		diag_set(ClientError, ER_PROC_LUA, tt_sprintf("Session "\
+			 "setting %s doesn't exist", setting_name));
+		return luaT_error(L);
+	}
+	struct session_setting *setting = &session_settings[sid];
+	switch (arg_type) {
+	case LUA_TBOOLEAN: {
+		bool value = lua_toboolean(L, -1);
+		size_t size = mp_sizeof_bool(value);
+		char *mp_value = (char *) static_alloc(size);
+		mp_encode_bool(mp_value, value);
+		if (setting->set(sid, mp_value) != 0)
+			return luaT_error(L);
+		break;
+	}
+	case LUA_TSTRING: {
+		const char *str = lua_tostring(L, -1);
+		size_t len = strlen(str);
+		uint32_t size = mp_sizeof_str(len);
+		char *mp_value = (char *) static_alloc(size);
+		if (mp_value == NULL) {
+			diag_set(OutOfMemory, size, "static_alloc",
+				 "mp_value");
+			return luaT_error(L);
+		}
+		mp_encode_str(mp_value, str, len);
+		if (setting->set(sid, mp_value) != 0)
+			return luaT_error(L);
+		break;
+	}
+	default:
+		diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+			 session_setting_strs[sid],
+			 field_type_strs[setting->field_type]);
+		return luaT_error(L);
+	}
+	return 0;
+}
+
+static int
+lbox_session_settings_serialize(struct lua_State *L)
+{
+	lua_newtable(L);
+	for (int id = 0; id < SESSION_SETTING_COUNT; ++id) {
+		lbox_session_setting_get_by_id(L, id);
+		lua_setfield(L, -2, session_setting_strs[id]);
+	}
+	return 1;
+}
+
+static void
+lbox_session_settings_init(struct lua_State *L)
+{
+	lua_newtable(L);
+	lua_createtable(L, 0, 3);
+	lua_pushcfunction(L, lbox_session_settings_serialize);
+	lua_setfield(L, -2, "__serialize");
+	lua_pushcfunction(L, lbox_session_setting_get);
+	lua_setfield(L, -2, "__index");
+	lua_pushcfunction(L, lbox_session_setting_set);
+	lua_setfield(L, -2, "__newindex");
+	lua_setmetatable(L, -2);
+	lua_setfield(L, -2, "settings");
+}
+
 void
 session_storage_cleanup(int sid)
 {
@@ -478,5 +588,6 @@ box_lua_session_init(struct lua_State *L)
 		{NULL, NULL}
 	};
 	luaL_register_module(L, sessionlib_name, sessionlib);
+	lbox_session_settings_init(L);
 	lua_pop(L, 1);
 }
diff --git a/src/box/session.cc b/src/box/session.cc
index 881318252..b557eed62 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -283,6 +283,7 @@ session_init()
 		panic("out of memory");
 	mempool_create(&session_pool, &cord()->slabc, sizeof(struct session));
 	credentials_create(&admin_credentials, admin_user);
+	sql_session_settings_init();
 }
 
 void
diff --git a/src/box/session.h b/src/box/session.h
index 6dfc7cba5..1c47b8986 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -41,6 +41,8 @@
 extern "C" {
 #endif /* defined(__cplusplus) */
 
+extern void sql_session_settings_init();
+
 struct port;
 struct session_vtab;
 
diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index 5e4a50427..15ab65a97 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -324,8 +324,8 @@ session_settings_index_get(struct index *base, const char *key,
 	uint32_t len;
 	key = mp_decode_str(&key, &len);
 	key = tt_cstr(key, len);
-	int sid = 0;
-	if (session_settings_set_forward(&sid, key, true, true) != 0) {
+	int sid = session_setting_find(key);
+	if (sid < 0) {
 		*result = NULL;
 		return 0;
 	}
@@ -426,7 +426,8 @@ session_settings_space_execute_update(struct space *space, struct txn *txn,
 	}
 	key = mp_decode_str(&key, &key_len);
 	key = tt_cstr(key, key_len);
-	if (session_settings_set_forward(&sid, key, true, true) != 0) {
+	sid = session_setting_find(key);
+	if (sid < 0) {
 		*result = NULL;
 		return 0;
 	}
@@ -520,3 +521,12 @@ const struct space_vtab session_settings_space_vtab = {
 	/* .prepare_alter = */ generic_space_prepare_alter,
 	/* .invalidate = */ generic_space_invalidate,
 };
+
+int
+session_setting_find(const char *name) {
+	int sid;
+	if (session_settings_set_forward(&sid, name, true, true) == 0)
+		return sid;
+	else
+		return -1;
+}
diff --git a/src/box/session_settings.h b/src/box/session_settings.h
index de24e3c6f..e2adc5289 100644
--- a/src/box/session_settings.h
+++ b/src/box/session_settings.h
@@ -84,3 +84,6 @@ struct session_setting {
 
 extern struct session_setting session_settings[SESSION_SETTING_COUNT];
 extern const char *session_setting_strs[SESSION_SETTING_COUNT];
+
+int
+session_setting_find(const char *name);
diff --git a/src/box/sql.c b/src/box/sql.c
index 1256df856..ba98ce5df 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -64,14 +64,9 @@ static const uint32_t default_sql_flags = SQL_EnableTrigger
 					  | SQL_AutoIndex
 					  | SQL_RecTriggers;
 
-extern void
-sql_session_settings_init();
-
 void
 sql_init()
 {
-	sql_session_settings_init();
-
 	default_flags |= default_sql_flags;
 
 	current_session()->sql_flags |= default_sql_flags;
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.result b/test/box/session_settings.result
similarity index 86%
rename from test/box/gh-4511-access-settings-from-any-frontend.result
rename to test/box/session_settings.result
index bae77192e..b32a0becb 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/session_settings.result
@@ -298,3 +298,64 @@ s:update('sql_defer_foreign_keys', {{'=', 'value', '1'}})
  | ---
  | - error: Session setting sql_defer_foreign_keys expected a value of type boolean
  | ...
+
+-- gh-4711: Provide a user-friendly frontend for accessing session settings.
+settings = box.session.settings
+ | ---
+ | ...
+assert(settings ~= nil)
+ | ---
+ | - true
+ | ...
+
+s:update('sql_default_engine', {{'=', 2, 'vinyl'}})
+ | ---
+ | - ['sql_default_engine', 'vinyl']
+ | ...
+settings.sql_default_engine
+ | ---
+ | - vinyl
+ | ...
+settings.sql_default_engine = 'memtx'
+ | ---
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - memtx
+ | ...
+settings.sql_defer_foreign_keys = true
+ | ---
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - true
+ | ...
+s:update('sql_defer_foreign_keys', {{'=', 2, false}})
+ | ---
+ | - ['sql_defer_foreign_keys', false]
+ | ...
+settings.sql_defer_foreign_keys
+ | ---
+ | - false
+ | ...
+
+settings.sql_default_engine = true
+ | ---
+ | - error: Session setting sql_default_engine expected a value of type string
+ | ...
+settings.sql_defer_foreign_keys = 'false'
+ | ---
+ | - error: Session setting sql_defer_foreign_keys expected a value of type boolean
+ | ...
+settings.sql_parser_debug = 'string'
+ | ---
+ | - error: Session setting sql_parser_debug expected a value of type boolean
+ | ...
+
+str = string.rep('a', 20 * 1024)
+ | ---
+ | ...
+box.session.settings.sql_default_engine = str
+ | ---
+ | - error: Failed to allocate 20483 bytes in static_alloc for mp_value
+ | ...
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/session_settings.test.lua
similarity index 86%
rename from test/box/gh-4511-access-settings-from-any-frontend.test.lua
rename to test/box/session_settings.test.lua
index b243be15e..440bef7ce 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.test.lua
+++ b/test/box/session_settings.test.lua
@@ -118,3 +118,23 @@ s:update('sql_defer_foreign_keys', {{'=', 'some text', true}})
 s:update('sql_defer_foreign_keys', {{'=', 'value', 1}})
 s:update('sql_defer_foreign_keys', {{'=', 'value', {1}}})
 s:update('sql_defer_foreign_keys', {{'=', 'value', '1'}})
+
+-- gh-4711: Provide a user-friendly frontend for accessing session settings.
+settings = box.session.settings
+assert(settings ~= nil)
+
+s:update('sql_default_engine', {{'=', 2, 'vinyl'}})
+settings.sql_default_engine
+settings.sql_default_engine = 'memtx'
+s:get('sql_default_engine').value
+settings.sql_defer_foreign_keys = true
+s:get('sql_defer_foreign_keys').value
+s:update('sql_defer_foreign_keys', {{'=', 2, false}})
+settings.sql_defer_foreign_keys
+
+settings.sql_default_engine = true
+settings.sql_defer_foreign_keys = 'false'
+settings.sql_parser_debug = 'string'
+
+str = string.rep('a', 20 * 1024)
+box.session.settings.sql_default_engine = str
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings

[Chris Sosnin]

Currently if a user wants to change session setting with SQL, one has
to execute UPDATE query like:
[[UPDATE "_session_settings" SET "value" = true WHERE "name" = 'name']]
However, direct access to system spaces isn't considered to be a good practice.
To avoid that and a bit simplify user's life, we introduce SQL shortcut command
SETTING SET.

Closes #4711

@TarantoolBot document
Title: API for accessing _session_settings space.
There are two ways of updating values of session settings:
via Lua and SQL.

Lua:
box.session.settings is a table, which is always accessible
to user. The syntax is the following:
`box.session.settings.<setting_name> = <new_value>`.

Example of usage:
```
tarantool> box.session.settings.sql_default_engine
---
- memtx
...

tarantool> box.session.settings.sql_default_engine = 'vinyl'
---
...

```

The table itself represents the (unordered) result of select
from _session_settings space.

SQL:
Instead of typing long UPDATE query one can use the SETTING SET command:
`box.execute([[SETTING SET "<setting_name>" = <new_value>]])`.
Note, that this query is case sensitive so the name must be quoted.
Also, SETTING SET doesn't prove any implicit casts, so <new_value> must
be of the type corresponding to the setting being updated.

Example:
```
tarantool> box.execute([[setting set "sql_default_engine" = 'memtx']])
---
- row_count: 1
...

tarantool> box.execute([[setting set "sql_defer_foreign_keys" = true]])
---
- row_count: 1
...

```
---
 extra/mkkeywordhash.c              |  1 +
 src/box/sql/build.c                | 25 +++++++++++++++
 src/box/sql/parse.y                |  5 +++
 src/box/sql/sqlInt.h               | 11 +++++++
 src/box/sql/vdbe.c                 | 50 ++++++++++++++++++++++++++++++
 test/box/session_settings.result   | 49 +++++++++++++++++++++++++++++
 test/box/session_settings.test.lua | 13 ++++++++
 7 files changed, 154 insertions(+)

diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
index 006285622..887853529 100644
--- a/extra/mkkeywordhash.c
+++ b/extra/mkkeywordhash.c
@@ -159,6 +159,7 @@ static Keyword aKeywordTable[] = {
   { "SCALAR",                 "TK_SCALAR",      true  },
   { "SELECT",                 "TK_SELECT",      true  },
   { "SET",                    "TK_SET",         true  },
+  { "SETTING",                "TK_SETTING",     false },
   { "SIMPLE",                 "TK_SIMPLE",      true  },
   { "START",                  "TK_START",       true  },
   { "STRING",                 "TK_STRING_KW",   true  },
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a00da31f9..cdcf8b6d8 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3481,3 +3481,28 @@ sql_session_settings_init()
 		setting->set = sql_session_setting_set;
 	}
 }
+
+void
+sql_setting_set(struct Parse *parse_context, struct Token *name,
+		struct Expr *expr)
+{
+	struct Vdbe *vdbe = sqlGetVdbe(parse_context);
+	if (vdbe == NULL)
+		goto abort;
+	sqlVdbeCountChanges(vdbe);
+	char *key = sql_name_from_token(parse_context->db, name);
+	if (key == NULL)
+		goto abort;
+	int index = session_setting_find(key);
+	if (index >= 0) {
+		int target = ++parse_context->nMem;
+		sqlExprCode(parse_context, expr, target);
+		sqlVdbeAddOp2(vdbe, OP_SetSetting, index, target);
+		return;
+	}
+	diag_set(ClientError, ER_SQL_PARSER_GENERIC,
+		 tt_sprintf("Session setting %s doesn't exist", key));
+abort:
+	parse_context->is_aborted = true;
+	return;
+}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 1a0e89703..56977b003 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1541,6 +1541,11 @@ cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y).   {
   sql_drop_index(pParse);
 }
 
+///////////////////////////// The SETTING SET command ////////////////////////
+cmd ::= SETTING SET nm(X) EQ expr(Y).  {
+    sql_setting_set(pParse,&X,Y.pExpr);
+}
+
 ///////////////////////////// The PRAGMA command /////////////////////////////
 //
 cmd ::= PRAGMA nm(X).                        {
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 1579cc92e..a700dd9c9 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4513,4 +4513,15 @@ int
 sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
 		    uint32_t *fieldno);
 
+/**
+ * Create VDBE instructions to set the new value of the session setting.
+ *
+ * @param parse_context Parsing context.
+ * @param name Name of the session setting.
+ * @param value New value of the session setting.
+ */
+void
+sql_setting_set(struct Parse *parse_context, struct Token *name,
+		struct Expr *value);
+
 #endif				/* sqlINT_H */
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index e8a029a8a..d6b753a51 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -55,6 +55,7 @@
 #include "box/schema.h"
 #include "box/space.h"
 #include "box/sequence.h"
+#include "box/session_settings.h"
 
 /*
  * Invoke this macro on memory cells just prior to changing the
@@ -5252,6 +5253,55 @@ case OP_IncMaxid: {
 	break;
 }
 
+/* Opcode: SetSetting P1 P2 * * *
+ *
+ * Set the new value of the session setting. P1 is the id of the
+ * setting in the session_settings array, P2 is the register
+ * holding a value.
+ */
+case OP_SetSetting: {
+	int sid = pOp->p1;
+	pIn1 = &aMem[pOp->p2];
+	assert(sid >= 0 && sid < SESSION_SETTING_COUNT);
+	struct session_setting *setting = &session_settings[sid];
+	switch (setting->field_type) {
+	case FIELD_TYPE_BOOLEAN: {
+		if ((pIn1->flags & MEM_Bool) == 0)
+			goto invalid_type;
+		bool value = pIn1->u.b;
+		size_t size = mp_sizeof_bool(value);
+		char *mp_value = (char *) static_alloc(size);
+		mp_encode_bool(mp_value, value);
+		if (setting->set(sid, mp_value) != 0)
+			goto abort_due_to_error;
+		break;
+	}
+	case FIELD_TYPE_STRING: {
+		if ((pIn1->flags & MEM_Str) == 0)
+			goto invalid_type;
+		const char *str = pIn1->z;
+		uint32_t size = mp_sizeof_str(pIn1->n);
+		char *mp_value = (char *) static_alloc(size);
+		if (mp_value == NULL) {
+			diag_set(OutOfMemory, size, "static_alloc", "mp_value");
+			goto abort_due_to_error;
+		}
+		mp_encode_str(mp_value, str, pIn1->n);
+		if (setting->set(sid, mp_value) != 0)
+			goto abort_due_to_error;
+		break;
+	}
+	default:
+	invalid_type:
+		diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+			 session_setting_strs[sid],
+			 field_type_strs[setting->field_type]);
+		goto abort_due_to_error;
+	}
+	p->nChange++;
+	break;
+}
+
 /* Opcode: Noop * * * * *
  *
  * Do nothing.  This instruction is often useful as a jump
diff --git a/test/box/session_settings.result b/test/box/session_settings.result
index b32a0becb..28c79379b 100644
--- a/test/box/session_settings.result
+++ b/test/box/session_settings.result
@@ -339,6 +339,39 @@ settings.sql_defer_foreign_keys
  | - false
  | ...
 
+box.execute([[setting set "sql_default_engine" = 'vinyl']])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - vinyl
+ | ...
+box.execute([[setting set "sql_default_engine" = 'memtx']])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - memtx
+ | ...
+box.execute([[setting set "sql_defer_foreign_keys" = true]])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - true
+ | ...
+box.execute([[setting set "sql_defer_foreign_keys" = ?]], {false})
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - false
+ | ...
+
 settings.sql_default_engine = true
  | ---
  | - error: Session setting sql_default_engine expected a value of type string
@@ -359,3 +392,19 @@ box.session.settings.sql_default_engine = str
  | ---
  | - error: Failed to allocate 20483 bytes in static_alloc for mp_value
  | ...
+
+box.execute([[setting set "sql_def_engine" = true]])
+ | ---
+ | - null
+ | - Session setting sql_def_engine doesn't exist
+ | ...
+box.execute([[setting set "sql_default_engine" = true]])
+ | ---
+ | - null
+ | - Session setting sql_default_engine expected a value of type string
+ | ...
+box.execute([[setting set "sql_defer_foreign_keys" = 'true']])
+ | ---
+ | - null
+ | - Session setting sql_defer_foreign_keys expected a value of type boolean
+ | ...
diff --git a/test/box/session_settings.test.lua b/test/box/session_settings.test.lua
index 440bef7ce..c477139f9 100644
--- a/test/box/session_settings.test.lua
+++ b/test/box/session_settings.test.lua
@@ -132,9 +132,22 @@ s:get('sql_defer_foreign_keys').value
 s:update('sql_defer_foreign_keys', {{'=', 2, false}})
 settings.sql_defer_foreign_keys
 
+box.execute([[setting set "sql_default_engine" = 'vinyl']])
+s:get('sql_default_engine').value
+box.execute([[setting set "sql_default_engine" = 'memtx']])
+s:get('sql_default_engine').value
+box.execute([[setting set "sql_defer_foreign_keys" = true]])
+s:get('sql_defer_foreign_keys').value
+box.execute([[setting set "sql_defer_foreign_keys" = ?]], {false})
+s:get('sql_defer_foreign_keys').value
+
 settings.sql_default_engine = true
 settings.sql_defer_foreign_keys = 'false'
 settings.sql_parser_debug = 'string'
 
 str = string.rep('a', 20 * 1024)
 box.session.settings.sql_default_engine = str
+
+box.execute([[setting set "sql_def_engine" = true]])
+box.execute([[setting set "sql_default_engine" = true]])
+box.execute([[setting set "sql_defer_foreign_keys" = 'true']])
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Timur Safin]

I'm late here on this party, but I very much dislike proposed syntax of

	SETTING SET configuration_name = value;

It feels not SQL-ish, and I see no much value introducing new statement 
while there is still SET statement available. 

Why we could not simply use something like:
	SET box.session.settings.name = value;
?

Thanks,
Timur


: -----Original Message-----
: From: Tarantool-patches <tarantool-patches-bounces@dev.tarantool.org> On
: Behalf Of Chris Sosnin
: Sent: Monday, March 30, 2020 12:14 PM
: To: v.shpilevoy@tarantool.org; korablev@tarantool.org; tarantool-
: patches@dev.tarantool.org
: Subject: [Tarantool-patches] [PATCH 0/4] session settings fixes
: 
: issue #1:https://github.com/tarantool/tarantool/issues/4711
: issue #2:https://github.com/tarantool/tarantool/issues/4712
: branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-
: session-settings-v2
: 
: Chris Sosnin (4):
:   box: replace session_settings modules with a single array
:   box: add binary search for _session_settings space
:   box: provide a user friendly frontend for accessing session settings
:   sql: provide a user friendly frontend for accessing session settings
: 
:  extra/mkkeywordhash.c                         |   1 +
:  src/box/lua/session.c                         | 111 +++++++++
:  src/box/session.cc                            |   1 +
:  src/box/session.h                             |   2 +
:  src/box/session_settings.c                    | 214 +++++++++++-------
:  src/box/session_settings.h                    |  53 +++--
:  src/box/sql.c                                 |   5 -
:  src/box/sql/build.c                           | 104 ++++-----
:  src/box/sql/parse.y                           |   5 +
:  src/box/sql/sqlInt.h                          |  11 +
:  src/box/sql/vdbe.c                            |  50 ++++
:  ...rontend.result => session_settings.result} | 147 ++++++++++--
:  ...end.test.lua => session_settings.test.lua} |  61 ++++-
:  13 files changed, 589 insertions(+), 176 deletions(-)
:  rename test/box/{gh-4511-access-settings-from-any-frontend.result =>
: session_settings.result} (71%)
:  rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua =>
: session_settings.test.lua} (64%)
: 
: --
: 2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Chris Sosnin]

> On 2 Apr 2020, at 12:14, Timur Safin <tsafin@tarantool.org> wrote:
> 
> I'm late here on this party, but I very much dislike proposed syntax of
> 
> 	SETTING SET configuration_name = value;
> 
> It feels not SQL-ish, and I see no much value introducing new statement 
> while there is still SET statement available. 
> 
> Why we could not simply use something like:
> 	SET box.session.settings.name = value;
> ?
> 
> Thanks,
> Timur

It was SET <setting_name> = <new_value> in the first version.

However, we agreed on this syntax being too generic for configuring
session settings, so I added SETTING keyword.

> 
> 
> : -----Original Message-----
> : From: Tarantool-patches <tarantool-patches-bounces@dev.tarantool.org> On
> : Behalf Of Chris Sosnin
> : Sent: Monday, March 30, 2020 12:14 PM
> : To: v.shpilevoy@tarantool.org; korablev@tarantool.org; tarantool-
> : patches@dev.tarantool.org
> : Subject: [Tarantool-patches] [PATCH 0/4] session settings fixes
> : 
> : issue #1:https://github.com/tarantool/tarantool/issues/4711
> : issue #2:https://github.com/tarantool/tarantool/issues/4712
> : branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-
> : session-settings-v2
> : 
> : Chris Sosnin (4):
> :   box: replace session_settings modules with a single array
> :   box: add binary search for _session_settings space
> :   box: provide a user friendly frontend for accessing session settings
> :   sql: provide a user friendly frontend for accessing session settings
> : 
> :  extra/mkkeywordhash.c                         |   1 +
> :  src/box/lua/session.c                         | 111 +++++++++
> :  src/box/session.cc                            |   1 +
> :  src/box/session.h                             |   2 +
> :  src/box/session_settings.c                    | 214 +++++++++++-------
> :  src/box/session_settings.h                    |  53 +++--
> :  src/box/sql.c                                 |   5 -
> :  src/box/sql/build.c                           | 104 ++++-----
> :  src/box/sql/parse.y                           |   5 +
> :  src/box/sql/sqlInt.h                          |  11 +
> :  src/box/sql/vdbe.c                            |  50 ++++
> :  ...rontend.result => session_settings.result} | 147 ++++++++++--
> :  ...end.test.lua => session_settings.test.lua} |  61 ++++-
> :  13 files changed, 589 insertions(+), 176 deletions(-)
> :  rename test/box/{gh-4511-access-settings-from-any-frontend.result =>
> : session_settings.result} (71%)
> :  rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua =>
> : session_settings.test.lua} (64%)
> : 
> : --
> : 2.21.1 (Apple Git-122.3)
> 
> 

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Nikita Pettik]

On 02 Apr 12:14, Timur Safin wrote:
> I'm late here on this party, but I very much dislike proposed syntax of

Patch is not merged, so it's still okay to suggest changes.

> 	SETTING SET configuration_name = value;
> 
> It feels not SQL-ish, and I see no much value introducing new statement 
> while there is still SET statement available. 

SET is not available as a separated statements; it's only a part
of UPDATE statements syntax. Rolling it back (SETTING SET -> SET)
is not a big deal - only one line of parse.y requires patching.

As I already said, I'm okay with all variations, whether it is
SET, SET SETTING or SETTING SET.
 
> Why we could not simply use something like:
> 	SET box.session.settings.name = value;

box.session.settings -- too long prefix. Originally, SET was
suggested as a shortcat for normal UPDATE "_session_settings" ...
SQL statement. Since it is shortcat, let's keep it short.

> ?
> 
> Thanks,
> Timur
> 

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Nikita Pettik]

On 30 Mar 12:13, Chris Sosnin wrote:
> issue #1:https://github.com/tarantool/tarantool/issues/4711
> issue #2:https://github.com/tarantool/tarantool/issues/4712
> branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session-settings-v2

Nit: while sending next version of patch-set please specify it
explicitly with --subject-prefix='PATCH v2' and attach changelog
between versions. For example: https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/015103.html
 
> Chris Sosnin (4):
>   box: replace session_settings modules with a single array
>   box: add binary search for _session_settings space
>   box: provide a user friendly frontend for accessing session settings
>   sql: provide a user friendly frontend for accessing session settings
> 
>  extra/mkkeywordhash.c                         |   1 +
>  src/box/lua/session.c                         | 111 +++++++++
>  src/box/session.cc                            |   1 +
>  src/box/session.h                             |   2 +
>  src/box/session_settings.c                    | 214 +++++++++++-------
>  src/box/session_settings.h                    |  53 +++--
>  src/box/sql.c                                 |   5 -
>  src/box/sql/build.c                           | 104 ++++-----
>  src/box/sql/parse.y                           |   5 +
>  src/box/sql/sqlInt.h                          |  11 +
>  src/box/sql/vdbe.c                            |  50 ++++
>  ...rontend.result => session_settings.result} | 147 ++++++++++--
>  ...end.test.lua => session_settings.test.lua} |  61 ++++-
>  13 files changed, 589 insertions(+), 176 deletions(-)
>  rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (71%)
>  rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (64%)
> 
> -- 
> 2.21.1 (Apple Git-122.3)
> 

Re: [Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array

[Nikita Pettik]

On 30 Mar 12:13, Chris Sosnin wrote:
> 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

LGTM

Re: [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space

[Nikita Pettik]

On 30 Mar 12:13, Chris Sosnin wrote:
> As it is mentioned in implementation, it is important
> that _session_settings options are sorted by name, so
> there is no need in linear lookup and we can replace it
> with binary search.
> 
> Closes #4712
> ---

Still I see no need for this patch. It is unclear which improvements
it introduces except for code complication.
 
> diff --git a/src/box/session_settings.c b/src/box/session_settings.c
> index 2ecf71f2d..5e4a50427 100644
> --- a/src/box/session_settings.c
> +++ b/src/box/session_settings.c
> @@ -81,6 +81,8 @@ struct session_settings_iterator {
>  	bool is_eq;
>  	/** True if the iterator should include equal keys. */
>  	bool is_including;
> +	/** True if the iterator is pointing to an existing setting */
> +	bool is_set;
>  };
>  
>  static void
> @@ -92,6 +94,72 @@ session_settings_iterator_free(struct iterator *ptr)
>  	free(it);
>  }
>  
> +static int
> +session_settings_set_forward(int *sid, const char *key, bool is_eq,
> +			     bool is_including)
> +{
> +	int low = 0, high = SESSION_SETTING_COUNT - 1;
> +	if (key == NULL)
> +		return 0;
> +	while (low <= high) {
> +		int index = (high + low) / 2;
> +		const char *name = session_setting_strs[index];
> +		int cmp = strcmp(name, key);
> +		if (cmp == 0) {
> +			if (is_including) {
> +				*sid = index;
> +				return 0;
> +			}
> +			*sid = ++index;
> +			return index < SESSION_SETTING_COUNT ? 0 : -1;
> +		}
> +		if (cmp < 0)
> +			low = index + 1;
> +		else
> +			high = index - 1;
> +	}
> +	if (is_eq) {
> +		*sid = SESSION_SETTING_COUNT;
> +		return -1;
> +	}
> +	assert(low > high);
> +	*sid = low;
> +	return low < SESSION_SETTING_COUNT ? 0 : -1;
> +}
> +
> +static int
> +session_settings_set_reverse(int *sid, const char *key, bool is_eq,
> +			     bool is_including)
> +{
> +	int low = 0, high = SESSION_SETTING_COUNT - 1;
> +	if (key == NULL)
> +		return 0;
> +	while (low <= high) {
> +		int index = (high + low) / 2;
> +		const char *name = session_setting_strs[index];
> +		int cmp = strcmp(name, key);
> +		if (cmp == 0) {
> +			if (is_including) {
> +				*sid = index;
> +				return 0;
> +			}
> +			*sid = --index;
> +			return index >= 0 ? 0 : -1;
> +		}
> +		if (cmp < 0)
> +			low = index + 1;
> +		else
> +			high = index - 1;
> +	}
> +	if (is_eq) {
> +		*sid = SESSION_SETTING_COUNT;
> +		return -1;
> +	}
> +	assert(low > high);
> +	*sid = high;
> +	return high >= 0 ? 0 : -1;
> +}
> +
>  static int
>  session_settings_next(int *sid, const char *key, bool is_eq, bool is_including)
>  {
> @@ -145,9 +213,15 @@ session_settings_iterator_next(struct iterator *iterator, struct tuple **result)
>  	int sid = it->setting_id;
>  	const char *key = it->key;
>  	bool is_including = it->is_including, is_eq = it->is_eq;
> -	bool is_found = false;
> -	if (session_settings_next(&sid, key, is_eq, is_including) == 0)
> -		is_found = true;
> +	bool is_found;
> +	if (!it->is_set) {
> +		it->is_set = true;
> +		is_found = session_settings_set_forward(&sid, key, is_eq,
> +							is_including) == 0;
> +	} else {
> +		is_found = session_settings_next(&sid, key, is_eq,
> +						 is_including) == 0;
> +	}
>  	it->setting_id = sid + 1;
>  	if (!is_found) {
>  		*result = NULL;
> @@ -167,9 +241,15 @@ session_settings_iterator_prev(struct iterator *iterator, struct tuple **result)
>  	int sid = it->setting_id;
>  	const char *key = it->key;
>  	bool is_including = it->is_including, is_eq = it->is_eq;
> -	bool is_found = false;
> -	if (session_settings_prev(&sid, key, is_eq, is_including) == 0)
> -		is_found = true;
> +	bool is_found;
> +	if (!it->is_set) {
> +		it->is_set = true;
> +		is_found = session_settings_set_reverse(&sid, key, is_eq,
> +							is_including) == 0;
> +	} else {
> +		is_found = session_settings_prev(&sid, key, is_eq,
> +						 is_including) == 0;
> +	}
>  	it->setting_id = sid - 1;
>  	if (!is_found) {
>  		*result = NULL;
> @@ -221,6 +301,7 @@ session_settings_index_create_iterator(struct index *base,
>  	it->is_eq = type == ITER_EQ || type == ITER_REQ;
>  	it->is_including = it->is_eq || type == ITER_GE || type == ITER_ALL ||
>  			   type == ITER_LE;
> +	it->is_set = false;
>  	it->format = index->format;
>  	if (!iterator_type_is_reverse(type)) {
>  		it->base.next = session_settings_iterator_next;
> @@ -244,7 +325,7 @@ session_settings_index_get(struct index *base, const char *key,
>  	key = mp_decode_str(&key, &len);
>  	key = tt_cstr(key, len);
>  	int sid = 0;
> -	if (session_settings_next(&sid, key, true, true) != 0) {
> +	if (session_settings_set_forward(&sid, key, true, true) != 0) {
>  		*result = NULL;
>  		return 0;
>  	}
> @@ -345,7 +426,7 @@ session_settings_space_execute_update(struct space *space, struct txn *txn,
>  	}
>  	key = mp_decode_str(&key, &key_len);
>  	key = tt_cstr(key, key_len);
> -	if (session_settings_next(&sid, key, true, true) != 0) {
> +	if (session_settings_set_forward(&sid, key, true, true) != 0) {
>  		*result = NULL;
>  		return 0;
>  	}
> diff --git a/test/box/gh-4511-access-settings-from-any-frontend.result b/test/box/gh-4511-access-settings-from-any-frontend.result
> index ed340d053..bae77192e 100644
> --- a/test/box/gh-4511-access-settings-from-any-frontend.result
> +++ b/test/box/gh-4511-access-settings-from-any-frontend.result
> @@ -82,6 +82,12 @@ function check_sorting(ss, ts, key)
>      for _, it in pairs(iterators_list) do
>          local view_space = ss:select({key}, {iterator = it})
>          local test_space = ts:select({key}, {iterator = it})
> +        if #view_space ~= #test_space then
> +            return {
> +                err = 'bad sorting', type = it, exp = test_space,
> +                got = view_space
> +            }
> +        end
>          for key, value in pairs(view_space) do
>              if test_space[key].name ~= value.name then
>                  return {
> @@ -111,32 +117,37 @@ check_sorting(s, t, 'sql_d')
>  check_sorting(s, t, 'sql_v')
>   | ---
>   | ...
> -check_sorting(s, t, 'sql_defer_foreign_keys')
> +check_sorting(s, t, 'z')
>   | ---
>   | ...
> -
> -t:drop()
> +check_sorting(s, t, 'sql_parser_debuf')
> + | ---
> + | ...
> +check_sorting(s, t, 'sql_parser_debuh')
>   | ---
>   | ...
>  
> --- Check get() method of session_settings space.
> -s:get({'sql_defer_foreign_keys'})
> +name = nil
>   | ---
> - | - ['sql_defer_foreign_keys', false]
>   | ...
> -s:get({'sql_recursive_triggers'})
> +err = nil
>   | ---
> - | - ['sql_recursive_triggers', true]
>   | ...
> -s:get({'sql_reverse_unordered_selects'})
> +for _, tuple in t:pairs() do                    \
> +    name = tuple.name                           \
> +    err = check_sorting(s, t, name)             \
> +    if err then                                 \
> +        break                                   \
> +    end                                         \
> +end
>   | ---
> - | - ['sql_reverse_unordered_selects', false]
>   | ...
> -s:get({'sql_default_engine'})
> +err and {err, name}
>   | ---
> - | - ['sql_default_engine', 'memtx']
> + | - null
>   | ...
> -s:get({'abcd'})
> +
> +t:drop()
>   | ---
>   | ...
>  
> diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/gh-4511-access-settings-from-any-frontend.test.lua
> index 366874998..b243be15e 100644
> --- a/test/box/gh-4511-access-settings-from-any-frontend.test.lua
> +++ b/test/box/gh-4511-access-settings-from-any-frontend.test.lua
> @@ -36,6 +36,12 @@ function check_sorting(ss, ts, key)
>      for _, it in pairs(iterators_list) do
>          local view_space = ss:select({key}, {iterator = it})
>          local test_space = ts:select({key}, {iterator = it})
> +        if #view_space ~= #test_space then
> +            return {
> +                err = 'bad sorting', type = it, exp = test_space,
> +                got = view_space
> +            }
> +        end
>          for key, value in pairs(view_space) do
>              if test_space[key].name ~= value.name then
>                  return {
> @@ -52,17 +58,23 @@ check_sorting(s, t)
>  check_sorting(s, t, 'abcde')
>  check_sorting(s, t, 'sql_d')
>  check_sorting(s, t, 'sql_v')
> -check_sorting(s, t, 'sql_defer_foreign_keys')
> +check_sorting(s, t, 'z')
> +check_sorting(s, t, 'sql_parser_debuf')
> +check_sorting(s, t, 'sql_parser_debuh')
> +
> +name = nil
> +err = nil
> +for _, tuple in t:pairs() do                    \
> +    name = tuple.name                           \
> +    err = check_sorting(s, t, name)             \
> +    if err then                                 \
> +        break                                   \
> +    end                                         \
> +end
> +err and {err, name}
>  
>  t:drop()
>  
> --- Check get() method of session_settings space.
> -s:get({'sql_defer_foreign_keys'})
> -s:get({'sql_recursive_triggers'})
> -s:get({'sql_reverse_unordered_selects'})
> -s:get({'sql_default_engine'})
> -s:get({'abcd'})
> -
>  -- Check pairs() method of session_settings space.
>  t = {}
>  for key, value in s:pairs() do table.insert(t, {key, value}) end
> -- 
> 2.21.1 (Apple Git-122.3)
> 

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Chris Sosnin]

It was v2, Vlad asked me to resend the whole patchset for 
convenience. Won’t forget to do this next time.

> On 3 Apr 2020, at 16:09, Nikita Pettik <korablev@tarantool.org> wrote:
> 
> On 30 Mar 12:13, Chris Sosnin wrote:
>> issue #1:https://github.com/tarantool/tarantool/issues/4711
>> issue #2:https://github.com/tarantool/tarantool/issues/4712
>> branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session-settings-v2
> 
> Nit: while sending next version of patch-set please specify it
> explicitly with --subject-prefix='PATCH v2' and attach changelog
> between versions. For example: https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/015103.html
> 
>> Chris Sosnin (4):
>>  box: replace session_settings modules with a single array
>>  box: add binary search for _session_settings space
>>  box: provide a user friendly frontend for accessing session settings
>>  sql: provide a user friendly frontend for accessing session settings
>> 
>> extra/mkkeywordhash.c                         |   1 +
>> src/box/lua/session.c                         | 111 +++++++++
>> src/box/session.cc                            |   1 +
>> src/box/session.h                             |   2 +
>> src/box/session_settings.c                    | 214 +++++++++++-------
>> src/box/session_settings.h                    |  53 +++--
>> src/box/sql.c                                 |   5 -
>> src/box/sql/build.c                           | 104 ++++-----
>> src/box/sql/parse.y                           |   5 +
>> src/box/sql/sqlInt.h                          |  11 +
>> src/box/sql/vdbe.c                            |  50 ++++
>> ...rontend.result => session_settings.result} | 147 ++++++++++--
>> ...end.test.lua => session_settings.test.lua} |  61 ++++-
>> 13 files changed, 589 insertions(+), 176 deletions(-)
>> rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (71%)
>> rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (64%)
>> 
>> -- 
>> 2.21.1 (Apple Git-122.3)
>> 

Re: [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings

[Nikita Pettik]

On 30 Mar 12:13, Chris Sosnin wrote:
> - space_object:update() is hard to use for configuring session settings,
> so we provide box.session.settings table, which can be used in a much more
> native way.
> 
> - Prior to this patch sql settings were not accessible before box.cfg()
> call, even though these flags can be set right after session creation.
> 
> Part of #4711
> ---

LGTM

Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings

[Nikita Pettik]

On 30 Mar 12:13, Chris Sosnin wrote:
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index a00da31f9..cdcf8b6d8 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -3481,3 +3481,28 @@ sql_session_settings_init()
>  		setting->set = sql_session_setting_set;
>  	}
>  }
> +
> +void
> +sql_setting_set(struct Parse *parse_context, struct Token *name,
> +		struct Expr *expr)
> +{
> +	struct Vdbe *vdbe = sqlGetVdbe(parse_context);
> +	if (vdbe == NULL)
> +		goto abort;
> +	sqlVdbeCountChanges(vdbe);
> +	char *key = sql_name_from_token(parse_context->db, name);
> +	if (key == NULL)
> +		goto abort;
> +	int index = session_setting_find(key);
> +	if (index >= 0) {
> +		int target = ++parse_context->nMem;
> +		sqlExprCode(parse_context, expr, target);
> +		sqlVdbeAddOp2(vdbe, OP_SetSetting, index, target);
> +		return;
> +	}
> +	diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> +		 tt_sprintf("Session setting %s doesn't exist", key));

Mb it is worth moving this check to runtime? I.e. pass to OP_SetSetting
name of setting to be set. The less appeals to system spaces and
cache look ups are made, the better parsing process is organized.

The rest is ok.

Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index a00da31f9..cdcf8b6d8 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -3481,3 +3481,28 @@ sql_session_settings_init()
>  		setting->set = sql_session_setting_set;
>  	}
>  }
> +
> +void
> +sql_setting_set(struct Parse *parse_context, struct Token *name,
> +		struct Expr *expr)
> +{
> +	struct Vdbe *vdbe = sqlGetVdbe(parse_context);
> +	if (vdbe == NULL)
> +		goto abort;
> +	sqlVdbeCountChanges(vdbe);
> +	char *key = sql_name_from_token(parse_context->db, name);
> +	if (key == NULL)
> +		goto abort;
> +	int index = session_setting_find(key);

I agree with Nikita. Better make this call part of OP_SetSetting.

> +	if (index >= 0) {
> +		int target = ++parse_context->nMem;
> +		sqlExprCode(parse_context, expr, target);
> +		sqlVdbeAddOp2(vdbe, OP_SetSetting, index, target);
> +		return;
> +	}
> +	diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> +		 tt_sprintf("Session setting %s doesn't exist", key));
> +abort:
> +	parse_context->is_aborted = true;
> +	return;
> +}

Talking of the syntax, I don't have a strong opinion here
regarding any particular option. I only want the new statement
be short, and not involving _session_settings manipulations.

Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings

[Chris Sosnin]

> > diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> > index a00da31f9..cdcf8b6d8 100644
> > --- a/src/box/sql/build.c
> > +++ b/src/box/sql/build.c
> > @@ -3481,3 +3481,28 @@ sql_session_settings_init()
> >  		setting->set = sql_session_setting_set;
> >  	}
> >  }
> > +
> > +void
> > +sql_setting_set(struct Parse *parse_context, struct Token *name,
> > +		struct Expr *expr)
> > +{
> > +	struct Vdbe *vdbe = sqlGetVdbe(parse_context);
> > +	if (vdbe == NULL)
> > +		goto abort;
> > +	sqlVdbeCountChanges(vdbe);
> > +	char *key = sql_name_from_token(parse_context->db, name);
> > +	if (key == NULL)
> > +		goto abort;
> > +	int index = session_setting_find(key);
> 
> I agree with Nikita. Better make this call part of OP_SetSetting.

I agree, fixed:

+
+void
+sql_setting_set(struct Parse *parse_context, struct Token *name,
+		struct Expr *expr)
+{
+	struct Vdbe *vdbe = sqlGetVdbe(parse_context);
+	if (vdbe == NULL)
+		goto abort;
+	sqlVdbeCountChanges(vdbe);
+	char *key = sql_name_from_token(parse_context->db, name);
+	if (key == NULL)
+		goto abort;
+	int target = ++parse_context->nMem;
+	sqlExprCode(parse_context, expr, target);
+	sqlVdbeAddOp4(vdbe, OP_SetSession, target, 0, 0, key, P4_DYNAMIC);
+	return;
+abort:
+	parse_context->is_aborted = true;
+	return;
+}


+/* Opcode: SetSession P1 * * P4 *
+ *
+ * Set new value of the session setting. P4 is the name of the
+ * setting being updated, P1 is the register holding a value.
+ */
+case OP_SetSession: {
+	assert(pOp->p4type == P4_DYNAMIC);
+	const char *setting_name = pOp->p4.z;
+	int sid = session_setting_find(setting_name);
+	if (sid < 0) {
+		diag_set(ClientError, ER_NO_SUCH_SESSION_SETTING, setting_name);
+		goto abort_due_to_error;
+	}
+	pIn1 = &aMem[pOp->p1];
...

> Talking of the syntax, I don't have a strong opinion here
> regarding any particular option. I only want the new statement
> be short, and not involving _session_settings manipulations.

I changed it to the following (the main problem with reserved word persists):

+///////////////////////////// The SET SESSION command ////////////////////////
+//
+cmd ::= SET SESSION nm(X) EQ term(Y).  {
+    sql_setting_set(pParse,&X,Y.pExpr);
+}
+

I also changed the 3rd commit (Lua frontend) in the series to introduce new error
code (it's present in the diff above):

+       /*213 */_(ER_NO_SUCH_SESSION_SETTING,   "Session setting %s doesn't exist") \

as long as searching is moved to vdbe.c and raising parsing error is not correct.

You can see the whole patch below:

===============================================================================

Currently if a user wants to change session setting with SQL, one has
to execute UPDATE query like:
[[UPDATE "_session_settings" SET "value" = true WHERE "name" = 'name']]
However, direct access to system spaces isn't considered to be a good practice.
To avoid that and a bit simplify user's life, we introduce SQL shortcut command
SET SESSION.

Closes #4711

@TarantoolBot document
Title: API for accessing _session_settings space.
There are two ways of updating values of session settings:
via Lua and SQL.

Lua:
box.session.settings is a table, which is always accessible
to user. The syntax is the following:
`box.session.settings.<setting_name> = <new_value>`.

Example of usage:
```
tarantool> box.session.settings.sql_default_engine
---
- memtx
...

tarantool> box.session.settings.sql_default_engine = 'vinyl'
---
...

```

The table itself represents the (unordered) result of select
from _session_settings space.

SQL:
Instead of typing long UPDATE query one can use the SET SESSION command:
`box.execute([[SET SESSION "<setting_name>" = <new_value>]])`.
Note, that this query is case sensitive so the name must be quoted.
Also, SET SESSION doesn't provide any implicit casts, so <new_value> must
be of the type corresponding to the setting being updated.

Example:
```
tarantool> box.execute([[set session "sql_default_engine" = 'memtx']])
---
- row_count: 1
...

tarantool> box.execute([[set session "sql_defer_foreign_keys" = true]])
---
- row_count: 1
...

```
---
 extra/mkkeywordhash.c              |  1 +
 src/box/sql/build.c                | 20 +++++++++++
 src/box/sql/parse.y                |  6 ++++
 src/box/sql/sqlInt.h               | 11 ++++++
 src/box/sql/vdbe.c                 | 54 ++++++++++++++++++++++++++++++
 test/box/session_settings.result   | 49 +++++++++++++++++++++++++++
 test/box/session_settings.test.lua | 13 +++++++
 7 files changed, 154 insertions(+)

diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
index 006285622..dd42c8f5f 100644
--- a/extra/mkkeywordhash.c
+++ b/extra/mkkeywordhash.c
@@ -158,6 +158,7 @@ static Keyword aKeywordTable[] = {
   { "SAVEPOINT",              "TK_SAVEPOINT",   true  },
   { "SCALAR",                 "TK_SCALAR",      true  },
   { "SELECT",                 "TK_SELECT",      true  },
+  { "SESSION",                "TK_SESSION",     false },
   { "SET",                    "TK_SET",         true  },
   { "SIMPLE",                 "TK_SIMPLE",      true  },
   { "START",                  "TK_START",       true  },
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a00da31f9..aecba4177 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3481,3 +3481,23 @@ sql_session_settings_init()
 		setting->set = sql_session_setting_set;
 	}
 }
+
+void
+sql_setting_set(struct Parse *parse_context, struct Token *name,
+		struct Expr *expr)
+{
+	struct Vdbe *vdbe = sqlGetVdbe(parse_context);
+	if (vdbe == NULL)
+		goto abort;
+	sqlVdbeCountChanges(vdbe);
+	char *key = sql_name_from_token(parse_context->db, name);
+	if (key == NULL)
+		goto abort;
+	int target = ++parse_context->nMem;
+	sqlExprCode(parse_context, expr, target);
+	sqlVdbeAddOp4(vdbe, OP_SetSession, target, 0, 0, key, P4_DYNAMIC);
+	return;
+abort:
+	parse_context->is_aborted = true;
+	return;
+}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 1a0e89703..995875566 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1541,6 +1541,12 @@ cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y).   {
   sql_drop_index(pParse);
 }
 
+///////////////////////////// The SET SESSION command ////////////////////////
+//
+cmd ::= SET SESSION nm(X) EQ term(Y).  {
+    sql_setting_set(pParse,&X,Y.pExpr);
+}
+
 ///////////////////////////// The PRAGMA command /////////////////////////////
 //
 cmd ::= PRAGMA nm(X).                        {
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 2d6bad424..e45a7671d 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4517,4 +4517,15 @@ int
 sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
 		    uint32_t *fieldno);
 
+/**
+ * Create VDBE instructions to set the new value of the session setting.
+ *
+ * @param parse_context Parsing context.
+ * @param name Name of the session setting.
+ * @param value New value of the session setting.
+ */
+void
+sql_setting_set(struct Parse *parse_context, struct Token *name,
+		struct Expr *value);
+
 #endif				/* sqlINT_H */
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index e8a029a8a..c3256c15a 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -55,6 +55,7 @@
 #include "box/schema.h"
 #include "box/space.h"
 #include "box/sequence.h"
+#include "box/session_settings.h"
 
 /*
  * Invoke this macro on memory cells just prior to changing the
@@ -5252,6 +5253,59 @@ case OP_IncMaxid: {
 	break;
 }
 
+/* Opcode: SetSession P1 * * P4 *
+ *
+ * Set new value of the session setting. P4 is the name of the
+ * setting being updated, P1 is the register holding a value.
+ */
+case OP_SetSession: {
+	assert(pOp->p4type == P4_DYNAMIC);
+	const char *setting_name = pOp->p4.z;
+	int sid = session_setting_find(setting_name);
+	if (sid < 0) {
+		diag_set(ClientError, ER_NO_SUCH_SESSION_SETTING, setting_name);
+		goto abort_due_to_error;
+	}
+	pIn1 = &aMem[pOp->p1];
+	struct session_setting *setting = &session_settings[sid];
+	switch (setting->field_type) {
+	case FIELD_TYPE_BOOLEAN: {
+		if ((pIn1->flags & MEM_Bool) == 0)
+			goto invalid_type;
+		bool value = pIn1->u.b;
+		size_t size = mp_sizeof_bool(value);
+		char *mp_value = (char *) static_alloc(size);
+		mp_encode_bool(mp_value, value);
+		if (setting->set(sid, mp_value) != 0)
+			goto abort_due_to_error;
+		break;
+	}
+	case FIELD_TYPE_STRING: {
+		if ((pIn1->flags & MEM_Str) == 0)
+			goto invalid_type;
+		const char *str = pIn1->z;
+		uint32_t size = mp_sizeof_str(pIn1->n);
+		char *mp_value = (char *) static_alloc(size);
+		if (mp_value == NULL) {
+			diag_set(OutOfMemory, size, "static_alloc", "mp_value");
+			goto abort_due_to_error;
+		}
+		mp_encode_str(mp_value, str, pIn1->n);
+		if (setting->set(sid, mp_value) != 0)
+			goto abort_due_to_error;
+		break;
+	}
+	default:
+	invalid_type:
+		diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+			 session_setting_strs[sid],
+			 field_type_strs[setting->field_type]);
+		goto abort_due_to_error;
+	}
+	p->nChange++;
+	break;
+}
+
 /* Opcode: Noop * * * * *
  *
  * Do nothing.  This instruction is often useful as a jump
diff --git a/test/box/session_settings.result b/test/box/session_settings.result
index b32a0becb..b82bdda94 100644
--- a/test/box/session_settings.result
+++ b/test/box/session_settings.result
@@ -339,6 +339,39 @@ settings.sql_defer_foreign_keys
  | - false
  | ...
 
+box.execute([[set session "sql_default_engine" = 'vinyl']])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - vinyl
+ | ...
+box.execute([[set session "sql_default_engine" = 'memtx']])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - memtx
+ | ...
+box.execute([[set session "sql_defer_foreign_keys" = true]])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - true
+ | ...
+box.execute([[set session "sql_defer_foreign_keys" = false]])
+ | ---
+ | - row_count: 1
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - false
+ | ...
+
 settings.sql_default_engine = true
  | ---
  | - error: Session setting sql_default_engine expected a value of type string
@@ -359,3 +392,19 @@ box.session.settings.sql_default_engine = str
  | ---
  | - error: Failed to allocate 20483 bytes in static_alloc for mp_value
  | ...
+
+box.execute([[set session "sql_def_engine" = true]])
+ | ---
+ | - null
+ | - Session setting sql_def_engine doesn't exist
+ | ...
+box.execute([[set session "sql_default_engine" = true]])
+ | ---
+ | - null
+ | - Session setting sql_default_engine expected a value of type string
+ | ...
+box.execute([[set session "sql_defer_foreign_keys" = 'true']])
+ | ---
+ | - null
+ | - Session setting sql_defer_foreign_keys expected a value of type boolean
+ | ...
diff --git a/test/box/session_settings.test.lua b/test/box/session_settings.test.lua
index 440bef7ce..d4aac0286 100644
--- a/test/box/session_settings.test.lua
+++ b/test/box/session_settings.test.lua
@@ -132,9 +132,22 @@ s:get('sql_defer_foreign_keys').value
 s:update('sql_defer_foreign_keys', {{'=', 2, false}})
 settings.sql_defer_foreign_keys
 
+box.execute([[set session "sql_default_engine" = 'vinyl']])
+s:get('sql_default_engine').value
+box.execute([[set session "sql_default_engine" = 'memtx']])
+s:get('sql_default_engine').value
+box.execute([[set session "sql_defer_foreign_keys" = true]])
+s:get('sql_defer_foreign_keys').value
+box.execute([[set session "sql_defer_foreign_keys" = false]])
+s:get('sql_defer_foreign_keys').value
+
 settings.sql_default_engine = true
 settings.sql_defer_foreign_keys = 'false'
 settings.sql_parser_debug = 'string'
 
 str = string.rep('a', 20 * 1024)
 box.session.settings.sql_default_engine = str
+
+box.execute([[set session "sql_def_engine" = true]])
+box.execute([[set session "sql_default_engine" = true]])
+box.execute([[set session "sql_defer_foreign_keys" = 'true']])
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings

[Vladislav Shpilevoy]

Hi! Thanks for the patchset!

I force pushed the style fix below.

LGTM now.

diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 995875566..380fb83d1 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1544,7 +1544,7 @@ cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y).   {
 ///////////////////////////// The SET SESSION command ////////////////////////
 //
 cmd ::= SET SESSION nm(X) EQ term(Y).  {
-    sql_setting_set(pParse,&X,Y.pExpr);
+    sql_setting_set(pParse, &X, Y.pExpr);
 }
 
 ///////////////////////////// The PRAGMA command /////////////////////////////

Re: [Tarantool-patches] [PATCH 4/4] sql: provide a user friendly frontend for accessing session settings

[Timur Safin]

: 
: I changed it to the following (the main problem with reserved word
: persists):
: 
: +///////////////////////////// The SET SESSION command
: ////////////////////////
: +//
: +cmd ::= SET SESSION nm(X) EQ term(Y).  {
: +    sql_setting_set(pParse,&X,Y.pExpr);
: +}
: +
: 
..

: ==========================================================================
: =====
: 
: Currently if a user wants to change session setting with SQL, one has
: to execute UPDATE query like:
: [[UPDATE "_session_settings" SET "value" = true WHERE "name" = 'name']]
: However, direct access to system spaces isn't considered to be a good
: practice.
: To avoid that and a bit simplify user's life, we introduce SQL shortcut
: command
: SET SESSION.
: 
: Closes #4711
: 
: @TarantoolBot document
: Title: API for accessing _session_settings space.
: There are two ways of updating values of session settings:
: via Lua and SQL.
: 
...
: SQL:
: Instead of typing long UPDATE query one can use the SET SESSION command:
: `box.execute([[SET SESSION "<setting_name>" = <new_value>]])`.
: Note, that this query is case sensitive so the name must be quoted.
: Also, SET SESSION doesn't provide any implicit casts, so <new_value> must
: be of the type corresponding to the setting being updated.
: 
: Example:
: ```
: tarantool> box.execute([[set session "sql_default_engine" = 'memtx']])
: ---
: - row_count: 1
: ...
: 
: tarantool> box.execute([[set session "sql_defer_foreign_keys" = true]])
: ---
: - row_count: 1
: ...
: 

Like it! 
(The least intrusive way possible, but still is short for user to write)

Timur

Re: [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space

[Kirill Yukhin]

Hello,

On 03 Apr 14:00, Nikita Pettik wrote:
> On 30 Mar 12:13, Chris Sosnin wrote:
> > As it is mentioned in implementation, it is important
> > that _session_settings options are sorted by name, so
> > there is no need in linear lookup and we can replace it
> > with binary search.
> > 
> > Closes #4712
> > ---
> 
> Still I see no need for this patch. It is unclear which improvements
> it introduces except for code complication.

I agree, let's drop it.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Kirill Yukhin]

Hello,

On 30 Mar 12:13, Chris Sosnin wrote:
> issue #1:https://github.com/tarantool/tarantool/issues/4711
> issue #2:https://github.com/tarantool/tarantool/issues/4712
> branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session-settings-v2
> 
> Chris Sosnin (4):
>   box: replace session_settings modules with a single array
>   box: add binary search for _session_settings space
>   box: provide a user friendly frontend for accessing session settings
>   sql: provide a user friendly frontend for accessing session settings
> 
>  extra/mkkeywordhash.c                         |   1 +
>  src/box/lua/session.c                         | 111 +++++++++
>  src/box/session.cc                            |   1 +
>  src/box/session.h                             |   2 +
>  src/box/session_settings.c                    | 214 +++++++++++-------
>  src/box/session_settings.h                    |  53 +++--
>  src/box/sql.c                                 |   5 -
>  src/box/sql/build.c                           | 104 ++++-----
>  src/box/sql/parse.y                           |   5 +
>  src/box/sql/sqlInt.h                          |  11 +
>  src/box/sql/vdbe.c                            |  50 ++++
>  ...rontend.result => session_settings.result} | 147 ++++++++++--
>  ...end.test.lua => session_settings.test.lua} |  61 ++++-
>  13 files changed, 589 insertions(+), 176 deletions(-)
>  rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (71%)
>  rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (64%)

I've checked 1, 3 and 4 patches into master.

--
Regards, Kirill Yukhin

[Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array

[Chris Sosnin]

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)

Re: [Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array

[Vladislav Shpilevoy]

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;
 	}

[Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array

[Chris Sosnin]

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)