[Tarantool-patches] [PATCH v4 0/3] box: session settings fixes

[Tarantool-patches] [PATCH v4 0/3] box: session settings fixes

[Chris Sosnin]

Changes in v3:
- add a patch that removes setting modules
- apply Vlad's fixes

Changes in v4:
 - add lua frontend for accessing session settings
 - move settings initialization into session.c

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

Chris Sosnin (3):
  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

 src/box/lua/session.c                         | 106 +++++++++++
 src/box/session_settings.c                    | 180 +++++++++++-------
 src/box/session_settings.h                    |  47 +++--
 src/box/sql.c                                 |   5 -
 src/box/sql/build.c                           |  60 ++----
 ...1-access-settings-from-any-frontend.result |  94 +++++++--
 ...access-settings-from-any-frontend.test.lua |  45 ++++-
 7 files changed, 378 insertions(+), 159 deletions(-)

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v4 1/3] 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 | 103 +++++++++++--------------------------
 src/box/session_settings.h |  47 ++++++++++-------
 src/box/sql/build.c        |  60 ++++++---------------
 3 files changed, 75 insertions(+), 135 deletions(-)

diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index a969d3d10..cdc2d1cf9 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -38,8 +38,8 @@
 #include "xrow.h"
 #include "sql.h"
 
-struct session_setting_module
-	session_setting_modules[session_setting_type_MAX] = {};
+struct session_setting session_settings[session_setting_MAX] = {};
+static const int session_setting_count = session_setting_MAX;
 
 struct session_settings_index {
 	/** Base index. Must be the first member. */
@@ -61,12 +61,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 +81,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);
+	struct session_setting *setting = &session_settings[i];
+	for (; i < session_setting_count; ++i, ++setting) {
+		int cmp = strcmp(setting->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;
+	struct session_setting *setting = &session_settings[i];
+	for (; i >= 0; --i, --setting) {
+		int cmp = strcmp(setting->name, key);
 		if ((cmp == 0 && is_including) ||
 		    (cmp < 0 && !is_eq)) {
 			*sid = i;
@@ -141,27 +130,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 +152,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 +212,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 +231,15 @@ 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)
+		goto found;
 	*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 +334,12 @@ 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)
+		goto found;
 	*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 +361,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..2975bcaf0 100644
--- a/src/box/session_settings.h
+++ b/src/box/session_settings.h
@@ -31,28 +31,37 @@
  */
 
 /**
- * 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 setings. 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 {
+	sql_session_setting_BEGIN = 0,
+	SQL_SESSION_SETTING_DEFAULT_ENGINE = sql_session_setting_BEGIN,
+	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_END = SQL_SESSION_SETTING_VDBE_DEBUG,
+	session_setting_MAX,
 };
 
-struct session_setting_module {
-	/**
-	 * An array of setting names. All of them should have the
-	 * same prefix.
-	 */
-	const char **settings;
-	/** Count of settings. */
-	int setting_count;
+struct session_setting_metadata {
+	unsigned field_type;
+	unsigned mask;
+};
+
+struct session_setting {
+	const char *name;
+	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 +76,4 @@ 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[];
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index bc50ecbfa..7dcf7b858 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3310,29 +3310,7 @@ 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] = {
+static const char *sql_session_setting_strs[] = {
 	"sql_default_engine",
 	"sql_defer_foreign_keys",
 	"sql_full_column_names",
@@ -3344,23 +3322,13 @@ static const char *sql_session_setting_strs[sql_session_setting_MAX] = {
 	"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[] = {
+static struct session_setting_metadata sql_session_opts[] = {
 	/** SQL_SESSION_SETTING_DEFAULT_ENGINE */
 	{FIELD_TYPE_STRING, 0},
 	/** SQL_SESSION_SETTING_DEFER_FOREIGN_KEYS */
@@ -3386,10 +3354,11 @@ 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 >= sql_session_setting_BEGIN &&
+	       id <= sql_session_setting_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];
 	size_t name_len = strlen(name);
@@ -3426,7 +3395,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) {
@@ -3468,7 +3437,8 @@ 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 >= sql_session_setting_BEGIN &&
+	       id <= sql_session_setting_END);
 	enum mp_type mtype = mp_typeof(*mp_value);
 	enum field_type stype = sql_session_opts[id].field_type;
 	uint32_t len;
@@ -3495,10 +3465,12 @@ sql_session_setting_set(int id, const char *mp_value)
 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;
+	int id = sql_session_setting_BEGIN;
+	for (; id <= sql_session_setting_END; ++id) {
+		struct session_setting *setting = &session_settings[id];
+		setting->name = sql_session_setting_strs[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)

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

[Vladislav Shpilevoy]

Thanks for the patch!

I've pushed my review fixes on top of this commit. See it below
and on the branch. If you agree, then squash. Otherwise lets
discuss.

See 4 comments below.

On 28/01/2020 13:50, 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
> ---
>  src/box/session_settings.c | 103 +++++++++++--------------------------
>  src/box/session_settings.h |  47 ++++++++++-------
>  src/box/sql/build.c        |  60 ++++++---------------
>  3 files changed, 75 insertions(+), 135 deletions(-)
> 
> diff --git a/src/box/session_settings.h b/src/box/session_settings.h
> index 25490a7e3..2975bcaf0 100644
> --- a/src/box/session_settings.h
> +++ b/src/box/session_settings.h
> @@ -31,28 +31,37 @@
>   */
>  
>  /**
> - * 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 setings. 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 {
> +	sql_session_setting_BEGIN = 0,
> +	SQL_SESSION_SETTING_DEFAULT_ENGINE = sql_session_setting_BEGIN,
> +	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_END = SQL_SESSION_SETTING_VDBE_DEBUG,
> +	session_setting_MAX,
>  };

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.

> -struct session_setting_module {
> -	/**
> -	 * An array of setting names. All of them should have the
> -	 * same prefix.
> -	 */
> -	const char **settings;
> -	/** Count of settings. */
> -	int setting_count;
> +struct session_setting_metadata {
> +	unsigned field_type;
> +	unsigned mask;

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.

> +};
> +
> +struct session_setting {
> +	const char *name;
> +	struct session_setting_metadata metadata;

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.

>  	/**
>  	 * 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 bc50ecbfa..7dcf7b858 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -3310,29 +3310,7 @@ 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] = {
> +static const char *sql_session_setting_strs[] = {

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.

Or you can merge this array and session_settings[], and then
declare these strings in session_settings.c in the static declaration
of session_settings[] array, like this:

    session_settings[count] = {
        {.name = "..."},
        {.name = "..."},
        ...
    }

Not sure that C allows that though. Don't remember.

>  	"sql_default_engine",
>  	"sql_defer_foreign_keys",
>  	"sql_full_column_names",

Otherwise the patch is surprisingly simple, thanks for that.

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

commit 7b1bff5bad9c7b8217364f3da902b31ef5dbbdb7
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Mon Feb 3 21:57:41 2020 +0100

    Review fixes

diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index cdc2d1cf9..6f42b787f 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -38,8 +38,7 @@
 #include "xrow.h"
 #include "sql.h"
 
-struct session_setting session_settings[session_setting_MAX] = {};
-static const int session_setting_count = session_setting_MAX;
+struct session_setting session_settings[SESSION_SETTING_COUNT] = {};

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

I changed session_setting_MAX to SESSION_SETTING_COUNT,
because originally _MAX was used in case it will be
needed, for example, in STR2ENUM(). But seems it won't
happen.

==================================================
 
 struct session_settings_index {
 	/** Base index. Must be the first member. */
@@ -84,13 +83,13 @@ static int
 session_settings_next(int *sid, const char *key, bool is_eq, bool is_including)
 {
 	int i = *sid;
-	if (i >= session_setting_count)
+	if (i >= SESSION_SETTING_COUNT)
 		return -1;
 	if (key == NULL)
 		return 0;
 	assert(i >= 0);
 	struct session_setting *setting = &session_settings[i];
-	for (; i < session_setting_count; ++i, ++setting) {
+	for (; i < SESSION_SETTING_COUNT; ++i, ++setting) {
 		int cmp = strcmp(setting->name, key);
 		if ((cmp == 0 && is_including) ||
 		    (cmp > 0 && !is_eq)) {
@@ -98,7 +97,7 @@ session_settings_next(int *sid, const char *key, bool is_eq, bool is_including)
 			return 0;
 		}
 	}
-	*sid = session_setting_count;
+	*sid = SESSION_SETTING_COUNT;
 	return -1;
 }
 
@@ -110,8 +109,8 @@ session_settings_prev(int *sid, const char *key, bool is_eq, bool is_including)
 		return -1;
 	if (key == NULL)
 		return 0;
-	if (i >= session_setting_count)
-		i = session_setting_count - 1;
+	if (i >= SESSION_SETTING_COUNT)
+		i = SESSION_SETTING_COUNT - 1;
 	struct session_setting *setting = &session_settings[i];
 	for (; i >= 0; --i, --setting) {
 		int cmp = strcmp(setting->name, key);
@@ -215,7 +214,7 @@ session_settings_index_create_iterator(struct index *base,
 		it->setting_id = 0;
 	} else {
 		it->base.next = session_settings_iterator_prev;
-		it->setting_id = session_setting_count - 1;
+		it->setting_id = SESSION_SETTING_COUNT - 1;
 	}
 	return (struct iterator *)it;
 }
@@ -232,11 +231,10 @@ 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)
-		goto found;
-	*result = NULL;
-	return 0;
-found:;
+	if (session_settings_next(&sid, key, true, true) != 0) {
+		*result = NULL;
+		return 0;
+	}
 	const char *mp_pair;
 	const char *mp_pair_end;
 	session_settings[sid].get(sid, &mp_pair, &mp_pair_end);
@@ -334,11 +332,10 @@ 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)
-		goto found;
-	*result = NULL;
-	return 0;
-found:
+	if (session_settings_next(&sid, key, true, true) != 0) {
+		*result = NULL;
+		return 0;
+	}
 	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,
diff --git a/src/box/session_settings.h b/src/box/session_settings.h
index 2975bcaf0..bcd7c0273 100644
--- a/src/box/session_settings.h
+++ b/src/box/session_settings.h
@@ -31,7 +31,7 @@
  */
 
 /**
- * Identifiers of all session setings. The identifier of the
+ * Identifiers of all session settings. The identifier of the
  * option is equal to its place in the sorted list of session
  * options.
  *
@@ -40,18 +40,24 @@
  * space iterator will not be sorted properly.
  */
 enum {
-	sql_session_setting_BEGIN = 0,
-	SQL_SESSION_SETTING_DEFAULT_ENGINE = sql_session_setting_BEGIN,
-	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_END = SQL_SESSION_SETTING_VDBE_DEBUG,
-	session_setting_MAX,
+	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,

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

So as not to write *_END = previous and first = *_BEGIN I decided
to at least drop this from the _END part by making a thing similar
to iterators in SQL, where END means after the last. So now we
only need to assign _BEGIN explicitly, not counting the first one,
which is just always 0.

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

+	/**
+	 * 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_metadata {
@@ -76,4 +82,4 @@ struct session_setting {
 	int (*set)(int id, const char *mp_value);
 };
 
-extern struct session_setting session_settings[];
+extern struct session_setting session_settings[SESSION_SETTING_COUNT];
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 7dcf7b858..59c72eeaf 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3329,24 +3329,24 @@ static const char *sql_session_setting_strs[] = {
  * It is IMPORTANT that these options sorted by name.
  */
 static struct session_setting_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},
 };
@@ -3354,8 +3354,7 @@ static struct session_setting_metadata sql_session_opts[] = {
 static void
 sql_session_setting_get(int id, const char **mp_pair, const char **mp_pair_end)
 {
-	assert(id >= sql_session_setting_BEGIN &&
-	       id <= sql_session_setting_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];
@@ -3373,7 +3372,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);
@@ -3409,7 +3408,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
@@ -3423,7 +3422,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) {
@@ -3437,8 +3436,7 @@ sql_set_string_option(int id, const char *value)
 static int
 sql_session_setting_set(int id, const char *mp_value)
 {
-	assert(id >= sql_session_setting_BEGIN &&
-	       id <= sql_session_setting_END);
+	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;
@@ -3465,8 +3463,8 @@ sql_session_setting_set(int id, const char *mp_value)
 void
 sql_session_settings_init()
 {
-	int id = sql_session_setting_BEGIN;
-	for (; id <= sql_session_setting_END; ++id) {
+	for (int id = SESSION_SETTING_SQL_BEGIN; id < SESSION_SETTING_SQL_END;
+	     ++id) {
 		struct session_setting *setting = &session_settings[id];
 		setting->name = sql_session_setting_strs[id];
 		setting->metadata = sql_session_opts[id];

[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)

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 v4 2/3] 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 cdc2d1cf9..051ce0603 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -69,6 +69,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
@@ -80,6 +82,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_settings[index].name;
+		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_settings[index].name;
+		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)
 {
@@ -130,12 +198,18 @@ session_settings_iterator_next(struct iterator *iterator, struct tuple **result)
 {
 	struct session_settings_iterator *it =
 		(struct session_settings_iterator *)iterator;
-	int sid = it->setting_id;
+	int rc, 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;
+	if (!it->is_set) {
+		it->is_set = true;
+		rc = session_settings_set_forward(&sid, key, is_eq,
+						  is_including);
+	} else {
+		rc = session_settings_next(&sid, key, is_eq, is_including);
+	}
+	is_found = rc == 0;
 	it->setting_id = sid + 1;
 	if (!is_found) {
 		*result = NULL;
@@ -152,12 +226,18 @@ session_settings_iterator_prev(struct iterator *iterator, struct tuple **result)
 {
 	struct session_settings_iterator *it =
 		(struct session_settings_iterator *)iterator;
-	int sid = it->setting_id;
+	int rc, 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;
+	if (!it->is_set) {
+		it->is_set = true;
+		rc = session_settings_set_reverse(&sid, key, is_eq,
+						  is_including);
+	} else {
+		rc = session_settings_prev(&sid, key, is_eq, is_including);
+	}
+	is_found = rc == 0;
 	it->setting_id = sid - 1;
 	if (!is_found) {
 		*result = NULL;
@@ -209,6 +289,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;
@@ -232,7 +313,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)
 		goto found;
 	*result = NULL;
 	return 0;
@@ -334,7 +415,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)
 		goto found;
 	*result = NULL;
 	return 0;
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.result b/test/box/gh-4511-access-settings-from-any-frontend.result
index ed340d053..f072bafae 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/gh-4511-access-settings-from-any-frontend.result
@@ -82,6 +82,12 @@ function check_sorting(ss, ts, key)
     for _, it in pairs(iterators_list) do
         local view_space = ss:select({key}, {iterator = it})
         local test_space = ts:select({key}, {iterator = it})
+        if #view_space ~= #test_space then
+            return {
+                err = 'bad sorting', type = it, exp = test_space,
+                got = view_space
+        }
+        end
         for key, value in pairs(view_space) do
             if test_space[key].name ~= value.name then
                 return {
@@ -111,32 +117,37 @@ check_sorting(s, t, 'sql_d')
 check_sorting(s, t, 'sql_v')
  | ---
  | ...
-check_sorting(s, t, 'sql_defer_foreign_keys')
+check_sorting(s, t, 'z')
  | ---
  | ...
-
-t:drop()
+check_sorting(s, t, 'sql_parser_debuf')
+ | ---
+ | ...
+check_sorting(s, t, 'sql_parser_debuh')
  | ---
  | ...
 
--- Check get() method of session_settings space.
-s:get({'sql_defer_foreign_keys'})
+name = nil
  | ---
- | - ['sql_defer_foreign_keys', false]
  | ...
-s:get({'sql_recursive_triggers'})
+err = nil
  | ---
- | - ['sql_recursive_triggers', true]
  | ...
-s:get({'sql_reverse_unordered_selects'})
+for _, tuple in t:pairs() do                    \
+    name = tuple.name                           \
+    err = check_sorting(s, t, name)             \
+    if err then                                 \
+        break                                   \
+    end                                         \
+end
  | ---
- | - ['sql_reverse_unordered_selects', false]
  | ...
-s:get({'sql_default_engine'})
+err and {err, name}
  | ---
- | - ['sql_default_engine', 'memtx']
+ | - null
  | ...
-s:get({'abcd'})
+
+t:drop()
  | ---
  | ...
 
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/gh-4511-access-settings-from-any-frontend.test.lua
index 366874998..40a58ad04 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.test.lua
+++ b/test/box/gh-4511-access-settings-from-any-frontend.test.lua
@@ -36,6 +36,12 @@ function check_sorting(ss, ts, key)
     for _, it in pairs(iterators_list) do
         local view_space = ss:select({key}, {iterator = it})
         local test_space = ts:select({key}, {iterator = it})
+        if #view_space ~= #test_space then
+            return {
+                err = 'bad sorting', type = it, exp = test_space,
+                got = view_space
+        }
+        end
         for key, value in pairs(view_space) do
             if test_space[key].name ~= value.name then
                 return {
@@ -52,17 +58,23 @@ check_sorting(s, t)
 check_sorting(s, t, 'abcde')
 check_sorting(s, t, 'sql_d')
 check_sorting(s, t, 'sql_v')
-check_sorting(s, t, 'sql_defer_foreign_keys')
+check_sorting(s, t, 'z')
+check_sorting(s, t, 'sql_parser_debuf')
+check_sorting(s, t, 'sql_parser_debuh')
+
+name = nil
+err = nil
+for _, tuple in t:pairs() do                    \
+    name = tuple.name                           \
+    err = check_sorting(s, t, name)             \
+    if err then                                 \
+        break                                   \
+    end                                         \
+end
+err and {err, name}
 
 t:drop()
 
--- Check get() method of session_settings space.
-s:get({'sql_defer_foreign_keys'})
-s:get({'sql_recursive_triggers'})
-s:get({'sql_reverse_unordered_selects'})
-s:get({'sql_default_engine'})
-s:get({'abcd'})
-
 -- Check pairs() method of session_settings space.
 t = {}
 for key, value in s:pairs() do table.insert(t, {key, value}) end
-- 
2.21.1 (Apple Git-122.3)

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

[Vladislav Shpilevoy]

Thanks for the patch!

I've pushed my review fixes on top of this commit. See it below
and on the branch. If you agree, then squash. Otherwise lets
discuss.

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

commit 2d72be324e331055f13d5ca8e515b0dbf8837cc6
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Mon Feb 3 22:03:57 2020 +0100

    Review fixes

diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index bd98bd360..653a56b77 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -197,18 +197,18 @@ session_settings_iterator_next(struct iterator *iterator, struct tuple **result)
 {
 	struct session_settings_iterator *it =
 		(struct session_settings_iterator *)iterator;
-	int rc, sid = it->setting_id;
+	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;
+	bool is_found;
 	if (!it->is_set) {
 		it->is_set = true;
-		rc = session_settings_set_forward(&sid, key, is_eq,
-						  is_including);
+		is_found = session_settings_set_forward(&sid, key, is_eq,
+							is_including) == 0;
 	} else {
-		rc = session_settings_next(&sid, key, is_eq, is_including);
+		is_found = session_settings_next(&sid, key, is_eq,
+						 is_including) == 0;
 	}
-	is_found = rc == 0;
 	it->setting_id = sid + 1;
 	if (!is_found) {
 		*result = NULL;
@@ -225,18 +225,18 @@ session_settings_iterator_prev(struct iterator *iterator, struct tuple **result)
 {
 	struct session_settings_iterator *it =
 		(struct session_settings_iterator *)iterator;
-	int rc, sid = it->setting_id;
+	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;
+	bool is_found;
 	if (!it->is_set) {
 		it->is_set = true;
-		rc = session_settings_set_reverse(&sid, key, is_eq,
-						  is_including);
+		is_found = session_settings_set_reverse(&sid, key, is_eq,
+							is_including) == 0;
 	} else {
-		rc = session_settings_prev(&sid, key, is_eq, is_including);
+		is_found = session_settings_prev(&sid, key, is_eq,
+						 is_including) == 0;
 	}
-	is_found = rc == 0;
 	it->setting_id = sid - 1;
 	if (!is_found) {
 		*result = NULL;

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

[Chris Sosnin]

I agree with your changes, but I send new version of this patch, just in case.
================================================================================

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

Closes #4712
---
issue: https://github.com/tarantool/tarantool/issues/4711
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings

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

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

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

[Vladislav Shpilevoy]

Thanks for the patch!

Here is one another review fix.

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

commit 0a597ec7e4956a23d5e80420ca1d98fccab8d117
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Thu Feb 6 22:07:50 2020 +0100

    Review fix

diff --git a/test/box/gh-4511-access-settings-from-any-frontend.result b/test/box/gh-4511-access-settings-from-any-frontend.result
index f072bafae..bae77192e 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/gh-4511-access-settings-from-any-frontend.result
@@ -86,7 +86,7 @@ function check_sorting(ss, ts, key)
             return {
                 err = 'bad sorting', type = it, exp = test_space,
                 got = view_space
-        }
+            }
         end
         for key, value in pairs(view_space) do
             if test_space[key].name ~= value.name then
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/gh-4511-access-settings-from-any-frontend.test.lua
index 40a58ad04..b243be15e 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.test.lua
+++ b/test/box/gh-4511-access-settings-from-any-frontend.test.lua
@@ -40,7 +40,7 @@ function check_sorting(ss, ts, key)
             return {
                 err = 'bad sorting', type = it, exp = test_space,
                 got = view_space
-        }
+            }
         end
         for key, value in pairs(view_space) do
             if test_space[key].name ~= value.name then

[Tarantool-patches] [PATCH v4 3/3] 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.setting table, which can be used in a much more
native way.

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

Part of #4711
---
 src/box/lua/session.c                         | 106 ++++++++++++++++++
 src/box/sql.c                                 |   5 -
 ...1-access-settings-from-any-frontend.result |  57 ++++++++++
 ...access-settings-from-any-frontend.test.lua |  17 +++
 4 files changed, 180 insertions(+), 5 deletions(-)

diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index c6a600f6f..3ba1253c7 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -42,6 +42,9 @@
 #include "box/user.h"
 #include "box/schema.h"
 #include "box/port.h"
+#include "box/session_settings.h"
+
+extern void sql_session_settings_init();
 
 static const char *sessionlib_name = "box.session";
 
@@ -411,6 +414,108 @@ lbox_session_on_access_denied(struct lua_State *L)
 				  lbox_push_on_access_denied_event, NULL);
 }
 
+static int
+session_setting_serialize(lua_State *L)
+{
+	lua_getfield(L, -1, "_id");
+	int sid = lua_tointeger(L, -1);
+	const char *mp_pair, *mp_pair_end;
+	session_settings[sid].get(sid, &mp_pair, &mp_pair_end);
+	uint32_t len;
+	mp_decode_array(&mp_pair); /* [setting_name : value] */
+	mp_decode_str(&mp_pair, &len);
+	uint32_t field_type = session_settings[sid].metadata.field_type;
+	if (field_type == FIELD_TYPE_BOOLEAN) {
+		bool value = mp_decode_bool(&mp_pair);
+		lua_pushboolean(L, value);
+	} else {
+		const char *str = mp_decode_str(&mp_pair, &len);
+		lua_pushlstring(L, str, len);
+	}
+	return 1;
+}
+
+static int
+session_setting_set(lua_State *L)
+{
+	if (lua_gettop(L) != 2)
+		goto error;
+	int arg_type = lua_type(L, -1);
+	lua_getfield(L, -2, "_id");
+	int sid = lua_tointeger(L, -1);
+	struct session_setting *setting = &session_settings[sid];
+	lua_pop(L, 1);
+	switch (arg_type) {
+		case LUA_TBOOLEAN: {
+			bool value = lua_toboolean(L, -1);
+			size_t size = mp_sizeof_bool(value);
+			char *mp_value = (char *)region_alloc(&fiber()->gc,
+							      size);
+			mp_encode_bool(mp_value, value);
+			if (setting->set(sid, mp_value) != 0)
+				luaT_error(L);
+			lua_pushstring(L, setting->name);
+			lua_pushboolean(L, value);
+			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 *)region_alloc(&fiber()->gc,
+							      size);
+			mp_encode_str(mp_value, str, len);
+			if (setting->set(sid, mp_value) != 0)
+				luaT_error(L);
+			lua_pushstring(L, setting->name);
+			lua_pushstring(L, str);
+			break;
+		}
+		default:
+			goto error;
+	}
+	return 2;
+error:
+	luaL_error(L, "box.session.settings.set(): bad arguments");
+	return -1;
+}
+
+static void
+session_setting_create(struct lua_State *L, int id)
+{
+	lua_newtable(L);
+	lua_pushstring(L, "_id");
+	lua_pushinteger(L, id);
+	lua_settable(L, -3);
+	lua_newtable(L); /* setting metatable */
+	lua_pushstring(L, "__serialize");
+	lua_pushcfunction(L, session_setting_serialize);
+	lua_settable(L, -3);
+	lua_setmetatable(L, -2);
+	lua_pushcfunction(L, session_setting_set);
+	lua_setfield(L, -2, "set");
+}
+
+void
+session_settings_init(struct lua_State *L)
+{
+	/* Init settings that are avaliable right after session creation. */
+	sql_session_settings_init();
+	static const struct luaL_Reg settingslib[] = {
+		{NULL, NULL}
+	};
+	luaL_register_module(L, "box.session.settings", settingslib);
+	int id = sql_session_setting_BEGIN;
+	for (; id <= sql_session_setting_END; ++id) {
+		/* Store settings as name : id */
+		struct session_setting *setting = &session_settings[id];
+		lua_pushstring(L, setting->name);
+		session_setting_create(L, id);
+		lua_settable(L, -3);
+	}
+	lua_pop(L, 1);
+}
+
 void
 session_storage_cleanup(int sid)
 {
@@ -448,6 +553,7 @@ exit:
 void
 box_lua_session_init(struct lua_State *L)
 {
+	session_settings_init(L);
 	static const struct luaL_Reg session_internal_lib[] = {
 		{"create", lbox_session_create},
 		{"run_on_connect",    lbox_session_run_on_connect},
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/gh-4511-access-settings-from-any-frontend.result
index f072bafae..1c3ca7661 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/gh-4511-access-settings-from-any-frontend.result
@@ -298,3 +298,60 @@ s:update('sql_defer_foreign_keys', {{'=', 'value', '1'}})
  | ---
  | - error: Session setting sql_defer_foreign_keys expected a value of type boolean
  | ...
+
+-- gh-4711: Provide a user-friendly frontend for accessing session settings.
+settings = box.session.settings
+ | ---
+ | ...
+assert(settings ~= nil)
+ | ---
+ | - true
+ | ...
+
+s:update('sql_default_engine', {{'=', 2, 'vinyl'}})
+ | ---
+ | - ['sql_default_engine', 'vinyl']
+ | ...
+settings.sql_default_engine
+ | ---
+ | - vinyl
+ | ...
+settings.sql_default_engine:set('memtx')
+ | ---
+ | - sql_default_engine
+ | - memtx
+ | ...
+assert(s:get('sql_default_engine').value == 'memtx')
+ | ---
+ | - true
+ | ...
+settings.sql_defer_foreign_keys:set(true)
+ | ---
+ | - sql_defer_foreign_keys
+ | - true
+ | ...
+assert(s:get('sql_defer_foreign_keys').value == true)
+ | ---
+ | - true
+ | ...
+s:update('sql_defer_foreign_keys', {{'=', 2, false}})
+ | ---
+ | - ['sql_defer_foreign_keys', false]
+ | ...
+settings.sql_defer_foreign_keys
+ | ---
+ | - false
+ | ...
+
+settings.sql_default_engine:set(true)
+ | ---
+ | - error: Session setting sql_default_engine expected a value of type string
+ | ...
+settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
+ | ---
+ | - error: 'box.session.settings.set(): bad arguments'
+ | ...
+settings.sql_parser_debug:set('string')
+ | ---
+ | - error: Session setting sql_parser_debug expected a value of type boolean
+ | ...
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/gh-4511-access-settings-from-any-frontend.test.lua
index 40a58ad04..53f03450d 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
@@ -118,3 +118,20 @@ s:update('sql_defer_foreign_keys', {{'=', 'some text', true}})
 s:update('sql_defer_foreign_keys', {{'=', 'value', 1}})
 s:update('sql_defer_foreign_keys', {{'=', 'value', {1}}})
 s:update('sql_defer_foreign_keys', {{'=', 'value', '1'}})
+
+-- gh-4711: Provide a user-friendly frontend for accessing session settings.
+settings = box.session.settings
+assert(settings ~= nil)
+
+s:update('sql_default_engine', {{'=', 2, 'vinyl'}})
+settings.sql_default_engine
+settings.sql_default_engine:set('memtx')
+assert(s:get('sql_default_engine').value == 'memtx')
+settings.sql_defer_foreign_keys:set(true)
+assert(s:get('sql_defer_foreign_keys').value == true)
+s:update('sql_defer_foreign_keys', {{'=', 2, false}})
+settings.sql_defer_foreign_keys
+
+settings.sql_default_engine:set(true)
+settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
+settings.sql_parser_debug:set('string')
-- 
2.21.1 (Apple Git-122.3)

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

[Vladislav Shpilevoy]

Thanks for the patch!

See 12 comments below.

On 28/01/2020 13:50, Chris Sosnin wrote:
> - space_object:update() is hard to use for configuring session settings,
> so we provide box.session.setting table, which can be used in a much more
> native way.
> 
> - Prior to this patch sql settings were not accessible before box.cfg()
> call, even though these flags can be set right after session creation.
> 
> Part of #4711
> ---
>  src/box/lua/session.c                         | 106 ++++++++++++++++++
>  src/box/sql.c                                 |   5 -
>  ...1-access-settings-from-any-frontend.result |  57 ++++++++++
>  ...access-settings-from-any-frontend.test.lua |  17 +++
>  4 files changed, 180 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/lua/session.c b/src/box/lua/session.c
> index c6a600f6f..3ba1253c7 100644
> --- a/src/box/lua/session.c
> +++ b/src/box/lua/session.c
> @@ -42,6 +42,9 @@
>  #include "box/user.h"
>  #include "box/schema.h"
>  #include "box/port.h"
> +#include "box/session_settings.h"
> +
> +extern void sql_session_settings_init();

1. Please, don't mix SQL and Lua. At least here. Call it in
session.cc in session_init(). This file should not initialize
any session backends.

>  static const char *sessionlib_name = "box.session";
>  
> @@ -411,6 +414,108 @@ lbox_session_on_access_denied(struct lua_State *L)
>  				  lbox_push_on_access_denied_event, NULL);
>  }
>  
> +static int
> +session_setting_serialize(lua_State *L)
> +{
> +	lua_getfield(L, -1, "_id");
> +	int sid = lua_tointeger(L, -1);
> +	const char *mp_pair, *mp_pair_end;
> +	session_settings[sid].get(sid, &mp_pair, &mp_pair_end);
> +	uint32_t len;
> +	mp_decode_array(&mp_pair); /* [setting_name : value] */

2. Avoid writing comments in the same line as the code, unless
this is a special case like a structure inlined initialization.
This has simple reasons: lines are shorter and therefore easier
to read; diff is smaller when only the code or only the comment
are changed.

> +	mp_decode_str(&mp_pair, &len);
> +	uint32_t field_type = session_settings[sid].metadata.field_type;
> +	if (field_type == FIELD_TYPE_BOOLEAN) {
> +		bool value = mp_decode_bool(&mp_pair);
> +		lua_pushboolean(L, value);
> +	} else {
> +		const char *str = mp_decode_str(&mp_pair, &len);
> +		lua_pushlstring(L, str, len);
> +	}
> +	return 1;
> +}
> +
> +static int
> +session_setting_set(lua_State *L)
> +{
> +	if (lua_gettop(L) != 2)
> +		goto error;
> +	int arg_type = lua_type(L, -1);
> +	lua_getfield(L, -2, "_id");
> +	int sid = lua_tointeger(L, -1);
> +	struct session_setting *setting = &session_settings[sid];
> +	lua_pop(L, 1);
> +	switch (arg_type) {
> +		case LUA_TBOOLEAN: {
> +			bool value = lua_toboolean(L, -1);
> +			size_t size = mp_sizeof_bool(value);
> +			char *mp_value = (char *)region_alloc(&fiber()->gc,
> +							      size);

3. Just use static_alloc. The same below.

> +			mp_encode_bool(mp_value, value);
> +			if (setting->set(sid, mp_value) != 0)
> +				luaT_error(L);

4. According to the new agreement, all new Lua functions should
return nil, err when an error happens. Except OOM and invalid
usage of API. So if set() returns != 0, then please, use
luaT_push_nil_and_error().

> +			lua_pushstring(L, setting->name);
> +			lua_pushboolean(L, value);
> +			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 *)region_alloc(&fiber()->gc,
> +							      size);
> +			mp_encode_str(mp_value, str, len);
> +			if (setting->set(sid, mp_value) != 0)
> +				luaT_error(L);
> +			lua_pushstring(L, setting->name);
> +			lua_pushstring(L, str);
> +			break;
> +		}
> +		default:
> +			goto error;
> +	}
> +	return 2;
> +error:
> +	luaL_error(L, "box.session.settings.set(): bad arguments");
> +	return -1;
> +}
> +
> +static void
> +session_setting_create(struct lua_State *L, int id)
> +{
> +	lua_newtable(L);
> +	lua_pushstring(L, "_id");
> +	lua_pushinteger(L, id);
> +	lua_settable(L, -3);
> +	lua_newtable(L); /* setting metatable */

5. The same as above about code + comment in one line. Also,
please, use capital letter to start a sentence, and put a dot
in the end.

> +	lua_pushstring(L, "__serialize");
> +	lua_pushcfunction(L, session_setting_serialize);
> +	lua_settable(L, -3);
> +	lua_setmetatable(L, -2);

6. You create lots of duplicate metatables here. I would
try to create it only once and use that one object for
all lua_setmetatable().

> +	lua_pushcfunction(L, session_setting_set);
> +	lua_setfield(L, -2, "set");
> +}
> +
> +void
> +session_settings_init(struct lua_State *L)
> +{
> +	/* Init settings that are avaliable right after session creation. */

7. avaliable -> available.

8. Please, keep comments in 66 line width limit.

> +	sql_session_settings_init();
> +	static const struct luaL_Reg settingslib[] = {
> +		{NULL, NULL}
> +	};
> +	luaL_register_module(L, "box.session.settings", settingslib);
> +	int id = sql_session_setting_BEGIN;

9. If you start from SQL, you will need to change this code
in case something will be added prior to SQL settings. Lets
start from 0 and iterate to SESSION_SETTING_COUNT.

> +	for (; id <= sql_session_setting_END; ++id) {
> +		/* Store settings as name : id */

10. Not sure this comment is still correct. You use
name : table, rather than name : id. But honestly here
this is obvious anyway, so I would drop that comment
at all.

> +		struct session_setting *setting = &session_settings[id];
> +		lua_pushstring(L, setting->name);
> +		session_setting_create(L, id);
> +		lua_settable(L, -3);
> +	}
> +	lua_pop(L, 1);
> +}
> +
>  void
>  session_storage_cleanup(int sid)
>  {
> @@ -448,6 +553,7 @@ exit:
>  void
>  box_lua_session_init(struct lua_State *L)
>  {
> +	session_settings_init(L);

11. I propose you to move it to the end of this function. You
initialize box.session.settings before box.session is
created. If you move it to the end, you will be able to drop
this dummy module registration, which you are doing above:

    static const struct luaL_Reg settingslib[] = {
    	{NULL, NULL}
    };

Because after

    luaL_register_module(L, sessionlib_name, sessionlib)

you have box.session on the stack and can add whatever you
want there, including settings.

>  	static const struct luaL_Reg session_internal_lib[] = {
>  		{"create", lbox_session_create},
>  		{"run_on_connect",    lbox_session_run_on_connect},
> diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/gh-4511-access-settings-from-any-frontend.test.lua
> index 40a58ad04..53f03450d 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
> @@ -118,3 +118,20 @@ 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.

12. The file has name 'gh-4511-*', so you can't add issue 4711 tests
here. Either add to a new file, or rename the test file to
box/session_settings.

> +settings = box.session.settings
> +assert(settings ~= nil)
> +
> +s:update('sql_default_engine', {{'=', 2, 'vinyl'}})
> +settings.sql_default_engine
> +settings.sql_default_engine:set('memtx')
> +assert(s:get('sql_default_engine').value == 'memtx')
> +settings.sql_defer_foreign_keys:set(true)
> +assert(s:get('sql_defer_foreign_keys').value == true)
> +s:update('sql_defer_foreign_keys', {{'=', 2, false}})
> +settings.sql_defer_foreign_keys
> +
> +settings.sql_default_engine:set(true)
> +settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
> +settings.sql_parser_debug:set('string')
> 

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

[Chris Sosnin]

Thank you for the review and your comments!
My answers below:

> 1. Please, don't mix SQL and Lua. At least here. Call it in
> session.cc in session_init(). This file should not initialize
> any session backends.

Done.

> 2. Avoid writing comments in the same line as the code, unless
> this is a special case like a structure inlined initialization.
> This has simple reasons: lines are shorter and therefore easier
> to read; diff is smaller when only the code or only the comment
> are changed.

Actually, I decided that comments are not essential here, so I
dropped them. Starting from here, everything about comments is
skipped.

> 3. Just use static_alloc. The same below.
+char *mp_value = static_alloc(size);

> 4. According to the new agreement, all new Lua functions should
> return nil, err when an error happens. Except OOM and invalid
> usage of API. So if set() returns != 0, then please, use
> luaT_push_nil_and_error().
+return luaT_push_nil_and_error(L);

> 6. You create lots of duplicate metatables here. I would
> try to create it only once and use that one object for
> all lua_setmetatable().

I did it the following way:
+	static const struct luaL_Reg session_setting_meta[] = {
+		{"__serialize", session_setting_serialize},
+		{"set", session_setting_set},
+		{NULL, NULL}
+	};
+	luaL_register_type(L, "session_setting", session_setting_meta);
...
+	luaL_getmetatable(L, "session_setting");
+	lua_setmetatable(L, -2);

> 9. If you start from SQL, you will need to change this code
> in case something will be added prior to SQL settings. Lets
> start from 0 and iterate to SESSION_SETTING_COUNT.

Well, I did it because SQL settings are available right after session
creation, if we add some settings that are not, then trying to access
them would lead to crash. However, currently it won't affect anything,
so ok.

+	for (int id = 0; id < SESSION_SETTING_COUNT; ++id) {
+		lua_pushstring(L, session_setting_strs[id]);
+		session_setting_create(L, id);
+		lua_settable(L, -3);
+	}

> 11. I propose you to move it to the end of this function. You
> initialize box.session.settings before box.session is
> created. If you move it to the end, you will be able to drop
> this dummy module registration, which you are doing above:

Done:
 	luaL_register_module(L, sessionlib_name, sessionlib);
+	session_settings_init(L);
 	lua_pop(L, 1);

+void
+session_settings_init(struct lua_State *L)
+{
+	lua_pushstring(L, "settings");
+	lua_newtable(L);
+	for (int id = 0; id < SESSION_SETTING_COUNT; ++id) {
+		lua_pushstring(L, session_setting_strs[id]);
+		session_setting_create(L, id);
+		lua_settable(L, -3);
+	}
+	lua_settable(L, -3);
+}

> 12. The file has name 'gh-4511-*', so you can't add issue 4711 tests
> here. Either add to a new file, or rename the test file to
> box/session_settings.

I renamed it.

See new version of the patch below:
================================================================================

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

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

Part of #4711
---
issue:https://github.com/tarantool/tarantool/issues/4711
branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings

 src/box/lua/session.c                         | 95 +++++++++++++++++++
 src/box/session.cc                            |  1 +
 src/box/session.h                             |  2 +
 src/box/sql.c                                 |  5 -
 ...rontend.result => session_settings.result} | 59 ++++++++++++
 ...end.test.lua => session_settings.test.lua} | 17 ++++
 6 files changed, 174 insertions(+), 5 deletions(-)
 rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (87%)
 rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (87%)

diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index c6a600f6f..b324cdab0 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,91 @@ lbox_session_on_access_denied(struct lua_State *L)
 				  lbox_push_on_access_denied_event, NULL);
 }
 
+static int
+session_setting_serialize(lua_State *L)
+{
+	lua_getfield(L, -1, "_id");
+	int sid = lua_tointeger(L, -1);
+	const char *mp_pair, *mp_pair_end;
+	session_settings[sid].get(sid, &mp_pair, &mp_pair_end);
+	uint32_t len;
+	mp_decode_array(&mp_pair);
+	mp_decode_str(&mp_pair, &len);
+	enum field_type field_type = session_settings[sid].metadata.field_type;
+	if (field_type == FIELD_TYPE_BOOLEAN) {
+		bool value = mp_decode_bool(&mp_pair);
+		lua_pushboolean(L, value);
+	} else {
+		const char *str = mp_decode_str(&mp_pair, &len);
+		lua_pushlstring(L, str, len);
+	}
+	return 1;
+}
+
+static int
+session_setting_set(lua_State *L)
+{
+	if (lua_gettop(L) != 2)
+		goto error;
+	int arg_type = lua_type(L, -1);
+	lua_getfield(L, -2, "_id");
+	int sid = lua_tointeger(L, -1);
+	struct session_setting *setting = &session_settings[sid];
+	lua_pop(L, 1);
+	switch (arg_type) {
+		case LUA_TBOOLEAN: {
+			bool value = lua_toboolean(L, -1);
+			size_t size = mp_sizeof_bool(value);
+			char *mp_value = static_alloc(size);
+			mp_encode_bool(mp_value, value);
+			if (setting->set(sid, mp_value) != 0)
+				return luaT_push_nil_and_error(L);
+			break;
+		}
+		case LUA_TSTRING: {
+			const char *str = lua_tostring(L, -1);
+			size_t len = strlen(str);
+			uint32_t size = mp_sizeof_str(len);
+			char *mp_value = static_alloc(size);
+			mp_encode_str(mp_value, str, len);
+			if (setting->set(sid, mp_value) != 0)
+				return luaT_push_nil_and_error(L);
+			break;
+		}
+		default:
+			goto error;
+	}
+	lua_pushstring(L, session_setting_strs[sid]);
+	lua_pushvalue(L, -2);
+	return 2;
+error:
+	return luaL_error(L, "Usage: setting:set(value)");
+}
+
+static void
+session_setting_create(struct lua_State *L, int id)
+{
+	lua_newtable(L);
+	lua_pushstring(L, "_id");
+	lua_pushinteger(L, id);
+	lua_settable(L, -3);
+	luaL_getmetatable(L, "session_setting");
+	lua_setmetatable(L, -2);
+}
+
+void
+session_settings_init(struct lua_State *L)
+{
+	lua_pushstring(L, "settings");
+	lua_newtable(L);
+	for (int id = 0; id < SESSION_SETTING_COUNT; ++id) {
+		lua_pushstring(L, session_setting_strs[id]);
+		session_setting_create(L, id);
+		lua_settable(L, -3);
+	}
+	lua_settable(L, -3);
+}
+
 void
 session_storage_cleanup(int sid)
 {
@@ -458,6 +545,13 @@ box_lua_session_init(struct lua_State *L)
 	luaL_register(L, "box.internal.session", session_internal_lib);
 	lua_pop(L, 1);
 
+	static const struct luaL_Reg session_setting_meta[] = {
+		{"__serialize", session_setting_serialize},
+		{"set", session_setting_set},
+		{NULL, NULL}
+	};
+	luaL_register_type(L, "session_setting", session_setting_meta);
+	
 	static const struct luaL_Reg sessionlib[] = {
 		{"id", lbox_session_id},
 		{"type", lbox_session_type},
@@ -478,5 +572,6 @@ box_lua_session_init(struct lua_State *L)
 		{NULL, NULL}
 	};
 	luaL_register_module(L, sessionlib_name, sessionlib);
+	session_settings_init(L);
 	lua_pop(L, 1);
 }
diff --git a/src/box/session.cc b/src/box/session.cc
index 881318252..b557eed62 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -283,6 +283,7 @@ session_init()
 		panic("out of memory");
 	mempool_create(&session_pool, &cord()->slabc, sizeof(struct session));
 	credentials_create(&admin_credentials, admin_user);
+	sql_session_settings_init();
 }
 
 void
diff --git a/src/box/session.h b/src/box/session.h
index 6dfc7cba5..1c47b8986 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -41,6 +41,8 @@
 extern "C" {
 #endif /* defined(__cplusplus) */
 
+extern void sql_session_settings_init();
+
 struct port;
 struct session_vtab;
 
diff --git a/src/box/sql.c b/src/box/sql.c
index 1256df856..ba98ce5df 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -64,14 +64,9 @@ static const uint32_t default_sql_flags = SQL_EnableTrigger
 					  | SQL_AutoIndex
 					  | SQL_RecTriggers;
 
-extern void
-sql_session_settings_init();
-
 void
 sql_init()
 {
-	sql_session_settings_init();
-
 	default_flags |= default_sql_flags;
 
 	current_session()->sql_flags |= default_sql_flags;
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.result b/test/box/session_settings.result
similarity index 87%
rename from test/box/gh-4511-access-settings-from-any-frontend.result
rename to test/box/session_settings.result
index f072bafae..403a4506c 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/session_settings.result
@@ -298,3 +298,62 @@ s:update('sql_defer_foreign_keys', {{'=', 'value', '1'}})
  | ---
  | - error: Session setting sql_defer_foreign_keys expected a value of type boolean
  | ...
+
+-- gh-4711: Provide a user-friendly frontend for accessing session settings.
+settings = box.session.settings
+ | ---
+ | ...
+assert(settings ~= nil)
+ | ---
+ | - true
+ | ...
+
+s:update('sql_default_engine', {{'=', 2, 'vinyl'}})
+ | ---
+ | - ['sql_default_engine', 'vinyl']
+ | ...
+settings.sql_default_engine
+ | ---
+ | - vinyl
+ | ...
+settings.sql_default_engine:set('memtx')
+ | ---
+ | - sql_default_engine
+ | - memtx
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - memtx
+ | ...
+settings.sql_defer_foreign_keys:set(true)
+ | ---
+ | - 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:set(true)
+ | ---
+ | - null
+ | - Session setting sql_default_engine expected a value of type string
+ | ...
+settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
+ | ---
+ | - error: 'Usage: setting:set(value)'
+ | ...
+settings.sql_parser_debug:set('string')
+ | ---
+ | - null
+ | - Session setting sql_parser_debug expected a value of type boolean
+ | ...
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/session_settings.test.lua
similarity index 87%
rename from test/box/gh-4511-access-settings-from-any-frontend.test.lua
rename to test/box/session_settings.test.lua
index 40a58ad04..99559100d 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.test.lua
+++ b/test/box/session_settings.test.lua
@@ -118,3 +118,20 @@ s:update('sql_defer_foreign_keys', {{'=', 'some text', true}})
 s:update('sql_defer_foreign_keys', {{'=', 'value', 1}})
 s:update('sql_defer_foreign_keys', {{'=', 'value', {1}}})
 s:update('sql_defer_foreign_keys', {{'=', 'value', '1'}})
+
+-- gh-4711: Provide a user-friendly frontend for accessing session settings.
+settings = box.session.settings
+assert(settings ~= nil)
+
+s:update('sql_default_engine', {{'=', 2, 'vinyl'}})
+settings.sql_default_engine
+settings.sql_default_engine:set('memtx')
+s:get('sql_default_engine').value
+settings.sql_defer_foreign_keys:set(true)
+s:get('sql_defer_foreign_keys').value
+s:update('sql_defer_foreign_keys', {{'=', 2, false}})
+settings.sql_defer_foreign_keys
+
+settings.sql_default_engine:set(true)
+settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
+settings.sql_parser_debug:set('string')
-- 
2.21.1 (Apple Git-122.3)

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

[Vladislav Shpilevoy]

Thanks for the patch!

I fixed code style by adding lbox_ prefix to functions
related to Lua box.session.*; added 'struct' to lua_State
mentions; fixed a crash in setting set(); dropped global
type 'session_setting'; renamed session_setting_serialize
to session_setting_get, because in future we likely will
add setting:get() method, and we could use the same function
as __serialize for this.

Consider my fixes below and on the branch.

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

commit ff63c003f7b7332f4b21c92cdcc8d85ac8f2e002
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Thu Feb 6 22:17:14 2020 +0100

    Review fixes

diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index 9aaf9e8dd..a0bce2333 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -414,7 +414,7 @@ lbox_session_on_access_denied(struct lua_State *L)
 }
 
 static int
-session_setting_serialize(lua_State *L)
+lbox_session_setting_get(struct lua_State *L)
 {
 	lua_getfield(L, -1, "_id");
 	int sid = lua_tointeger(L, -1);
@@ -435,10 +435,10 @@ session_setting_serialize(lua_State *L)
 }
 
 static int
-session_setting_set(lua_State *L)
+lbox_session_setting_set(struct lua_State *L)
 {
 	if (lua_gettop(L) != 2)
-		goto error;
+		return luaL_error(L, "Usage: box.session.settings:set(value)");
 	int arg_type = lua_type(L, -1);
 	lua_getfield(L, -2, "_id");
 	int sid = lua_tointeger(L, -1);
@@ -459,43 +459,47 @@ session_setting_set(lua_State *L)
 			size_t len = strlen(str);
 			uint32_t size = mp_sizeof_str(len);
 			char *mp_value = static_alloc(size);
+			if (mp_value == NULL) {
+				diag_set(OutOfMemory, size, "static_alloc",
+					 "mp_value");
+				return luaT_error(L);
+			}
 			mp_encode_str(mp_value, str, len);
 			if (setting->set(sid, mp_value) != 0)
 				return luaT_push_nil_and_error(L);
 			break;
 		}
 		default:
-			goto error;
+			diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+				 session_setting_strs[sid],
+				 field_type_strs[setting->field_type]);
+			return luaT_push_nil_and_error(L);
 	}
-	lua_pushstring(L, session_setting_strs[sid]);
-	lua_pushvalue(L, -2);
-	return 2;
-error:
-	return luaL_error(L, "Usage: setting:set(value)");
+	return 0;
 }
 
 static void
-session_setting_create(struct lua_State *L, int id)
+lbox_session_settings_init(struct lua_State *L)
 {
-	lua_newtable(L);
-	lua_pushstring(L, "_id");
-	lua_pushinteger(L, id);
-	lua_settable(L, -3);
-	luaL_getmetatable(L, "session_setting");
-	lua_setmetatable(L, -2);
-}
+	lua_createtable(L, 0, 2);
+	lua_pushcfunction(L, lbox_session_setting_get);
+	lua_setfield(L, -2, "__serialize");
+	lua_createtable(L, 0, 1);
+	lua_pushcfunction(L, lbox_session_setting_set);
+	lua_setfield(L, -2, "set");
+	lua_setfield(L, -2, "__index");
 
-void
-session_settings_init(struct lua_State *L)
-{
-	lua_pushstring(L, "settings");
 	lua_newtable(L);
 	for (int id = 0; id < SESSION_SETTING_COUNT; ++id) {
-		lua_pushstring(L, session_setting_strs[id]);
-		session_setting_create(L, id);
-		lua_settable(L, -3);
+		lua_newtable(L);
+		lua_pushinteger(L, id);
+		lua_setfield(L, -2, "_id");
+		lua_pushvalue(L, -3);
+		lua_setmetatable(L, -2);
+		lua_setfield(L, -2, session_setting_strs[id]);
 	}
-	lua_settable(L, -3);
+	lua_setfield(L, -3, "settings");
+	lua_pop(L, 1);
 }
 
 void
@@ -545,13 +549,6 @@ box_lua_session_init(struct lua_State *L)
 	luaL_register(L, "box.internal.session", session_internal_lib);
 	lua_pop(L, 1);
 
-	static const struct luaL_Reg session_setting_meta[] = {
-		{"__serialize", session_setting_serialize},
-		{"set", session_setting_set},
-		{NULL, NULL}
-	};
-	luaL_register_type(L, "session_setting", session_setting_meta);
-	
 	static const struct luaL_Reg sessionlib[] = {
 		{"id", lbox_session_id},
 		{"type", lbox_session_type},
@@ -572,6 +569,6 @@ box_lua_session_init(struct lua_State *L)
 		{NULL, NULL}
 	};
 	luaL_register_module(L, sessionlib_name, sessionlib);
-	session_settings_init(L);
+	lbox_session_settings_init(L);
 	lua_pop(L, 1);
 }
diff --git a/test/box/session_settings.result b/test/box/session_settings.result
index 1bb3757ff..6d7074e8c 100644
--- a/test/box/session_settings.result
+++ b/test/box/session_settings.result
@@ -318,8 +318,6 @@ settings.sql_default_engine
  | ...
 settings.sql_default_engine:set('memtx')
  | ---
- | - sql_default_engine
- | - memtx
  | ...
 s:get('sql_default_engine').value
  | ---
@@ -327,8 +325,6 @@ s:get('sql_default_engine').value
  | ...
 settings.sql_defer_foreign_keys:set(true)
  | ---
- | - sql_defer_foreign_keys
- | - true
  | ...
 s:get('sql_defer_foreign_keys').value
  | ---
@@ -350,10 +346,18 @@ settings.sql_default_engine:set(true)
  | ...
 settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
  | ---
- | - error: 'Usage: setting:set(value)'
+ | - error: 'Usage: box.session.settings:set(value)'
  | ...
 settings.sql_parser_debug:set('string')
  | ---
  | - null
  | - Session setting sql_parser_debug expected a value of type boolean
  | ...
+
+str = string.rep('a', 20 * 1024)
+ | ---
+ | ...
+box.session.settings.sql_default_engine:set(str)
+ | ---
+ | - error: Failed to allocate 20483 bytes in static_alloc for mp_value
+ | ...
diff --git a/test/box/session_settings.test.lua b/test/box/session_settings.test.lua
index 89b98dde2..23799874a 100644
--- a/test/box/session_settings.test.lua
+++ b/test/box/session_settings.test.lua
@@ -135,3 +135,6 @@ settings.sql_defer_foreign_keys
 settings.sql_default_engine:set(true)
 settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
 settings.sql_parser_debug:set('string')
+
+str = string.rep('a', 20 * 1024)
+box.session.settings.sql_default_engine:set(str)

Re: [Tarantool-patches] [PATCH v4 0/3] box: session settings fixes

[Vladislav Shpilevoy]

Hi! Thanks for the patchset!

I see that you have 1 more commit on top of the ones listed
below. Please, next time send 2 separate branches.

On 28/01/2020 13:50, Chris Sosnin wrote:
> Changes in v3:
> - add a patch that removes setting modules
> - apply Vlad's fixes
> 
> Changes in v4:
>  - add lua frontend for accessing session settings
>  - move settings initialization into session.c
> 
> 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
> 
> Chris Sosnin (3):
>   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
> 
>  src/box/lua/session.c                         | 106 +++++++++++
>  src/box/session_settings.c                    | 180 +++++++++++-------
>  src/box/session_settings.h                    |  47 +++--
>  src/box/sql.c                                 |   5 -
>  src/box/sql/build.c                           |  60 ++----
>  ...1-access-settings-from-any-frontend.result |  94 +++++++--
>  ...access-settings-from-any-frontend.test.lua |  45 ++++-
>  7 files changed, 378 insertions(+), 159 deletions(-)
> 

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

[Chris Sosnin]

The first patch just merges all modules into one array, so the
binary search will work once for all settings.

The second patch is implementation of the binary search.

The last two patches add frontend for accessing session settings:
Lua table and SQL statement respectively.

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

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

 src/box/lua/session.c                         |  92 ++++++++
 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} | 149 ++++++++++--
 ...end.test.lua => session_settings.test.lua} |  61 ++++-
 12 files changed, 571 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} (65%)

-- 
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 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)

[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)

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