[Tarantool-patches] [PATCH v4 0/2] introduce _vsession_settings sysview

[Tarantool-patches] [PATCH v4 0/2] introduce _vsession_settings sysview

[imeevma]

This patch-set introduces _vsession_settings system view. This
sysview contains names and current values of the session settings.

https://github.com/tarantool/tarantool/issues/4511
https://github.com/tarantool/tarantool/tree/imeevma/gh-4511-system-view-for-session-settings

Mergen Imeev (2):
  sysview: make get() and create_iterator() methods virtual
  box: introduce _vsession_settings sysview

 src/box/bootstrap.snap             | Bin 5944 -> 5982 bytes
 src/box/lua/space.cc               |   2 +
 src/box/lua/upgrade.lua            |  23 ++++++
 src/box/schema_def.h               |   2 +
 src/box/session.cc                 | 131 ++++++++++++++++++++++++++++++++++
 src/box/session.h                  |  13 ++++
 src/box/sql.h                      |  36 ++++++++++
 src/box/sql/build.c                | 140 +++++++++++++++++++++++++++++++++++++
 src/box/sysview.c                  |  48 +++++++++++--
 test/app-tap/tarantoolctl.test.lua |   4 +-
 test/box-py/bootstrap.result       |   5 +-
 test/box/access_sysview.result     | 124 +++++++++++++++++++++++++++++++-
 test/box/access_sysview.test.lua   |  54 ++++++++++++++
 test/box/alter.result              |   5 +-
 test/wal_off/alter.result          |   2 +-
 15 files changed, 573 insertions(+), 16 deletions(-)

-- 
2.7.4

[Tarantool-patches] [PATCH v4 1/2] sysview: make get() and create_iterator() methods virtual

[imeevma]

This patch makes get() and create_iterator() virtual for sysview
index. This will allow us to create custom version of these
functions.

Needed for #4511
---
 src/box/sysview.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/src/box/sysview.c b/src/box/sysview.c
index 00c320b..745cf09 100644
--- a/src/box/sysview.c
+++ b/src/box/sysview.c
@@ -49,6 +49,12 @@
 #include "session.h"
 
 typedef bool (*sysview_filter_f)(struct space *, struct tuple *);
+typedef int (*sysview_get_f)(struct index *index, const char *key,
+			     uint32_t part_count, struct tuple **result);
+typedef struct iterator *(*sysview_create_iterator_f)(struct index *index,
+						      enum iterator_type type,
+						      const char *key,
+						      uint32_t part_count);
 
 struct sysview_engine {
 	struct engine base;
@@ -61,6 +67,8 @@ struct sysview_index {
 	uint32_t source_space_id;
 	uint32_t source_index_id;
 	sysview_filter_f filter;
+	sysview_get_f get;
+	sysview_create_iterator_f create_iterator;
 };
 
 struct sysview_iterator {
@@ -140,7 +148,7 @@ sysview_index_create_iterator(struct index *base, enum iterator_type type,
 	it->base.next = sysview_iterator_next;
 	it->base.free = sysview_iterator_free;
 
-	it->source = index_create_iterator(pk, type, key, part_count);
+	it->source = index->create_iterator(pk, type, key, part_count);
 	if (it->source == NULL) {
 		mempool_free(&sysview->iterator_pool, it);
 		return NULL;
@@ -167,7 +175,7 @@ sysview_index_get(struct index *base, const char *key,
 	if (exact_key_validate(pk->def->key_def, key, part_count) != 0)
 		return -1;
 	struct tuple *tuple;
-	if (index_get(pk, key, part_count, &tuple) != 0)
+	if (index->get(pk, key, part_count, &tuple) != 0)
 		return -1;
 	if (tuple == NULL || !index->filter(source, tuple))
 		*result = NULL;
@@ -424,42 +432,58 @@ sysview_space_create_index(struct space *space, struct index_def *def)
 	uint32_t source_space_id;
 	uint32_t source_index_id;
 	sysview_filter_f filter;
+	sysview_get_f get;
+	sysview_create_iterator_f create_iterator;
 
 	switch (def->space_id) {
 	case BOX_VSPACE_ID:
 		source_space_id = BOX_SPACE_ID;
 		source_index_id = def->iid;
 		filter = vspace_filter;
+		get = index_get;
+		create_iterator = index_create_iterator;
 		break;
 	case BOX_VINDEX_ID:
 		source_space_id = BOX_INDEX_ID;
 		source_index_id = def->iid;
 		filter = vspace_filter;
+		get = index_get;
+		create_iterator = index_create_iterator;
 		break;
 	case BOX_VUSER_ID:
 		source_space_id = BOX_USER_ID;
 		source_index_id = def->iid;
 		filter = vuser_filter;
+		get = index_get;
+		create_iterator = index_create_iterator;
 		break;
 	case BOX_VFUNC_ID:
 		source_space_id = BOX_FUNC_ID;
 		source_index_id = def->iid;
 		filter = vfunc_filter;
+		get = index_get;
+		create_iterator = index_create_iterator;
 		break;
 	case BOX_VPRIV_ID:
 		source_space_id = BOX_PRIV_ID;
 		source_index_id = def->iid;
 		filter = vpriv_filter;
+		get = index_get;
+		create_iterator = index_create_iterator;
 		break;
 	case BOX_VSEQUENCE_ID:
 		source_space_id = BOX_SEQUENCE_ID;
 		source_index_id = def->iid;
 		filter = vsequence_filter;
+		get = index_get;
+		create_iterator = index_create_iterator;
 		break;
 	case BOX_VCOLLATION_ID:
 		source_space_id = BOX_COLLATION_ID;
 		source_index_id = def->iid;
 		filter = vcollation_filter;
+		get = index_get;
+		create_iterator = index_create_iterator;
 		break;
 	default:
 		diag_set(ClientError, ER_MODIFY_INDEX,
@@ -484,6 +508,8 @@ sysview_space_create_index(struct space *space, struct index_def *def)
 	index->source_space_id = source_space_id;
 	index->source_index_id = source_index_id;
 	index->filter = filter;
+	index->get = get;
+	index->create_iterator = create_iterator;
 	return &index->base;
 }
 
-- 
2.7.4

[Tarantool-patches] [PATCH v4 2/2] box: introduce _vsession_settings sysview

[imeevma]

This patch creates the _vsession_settings sysview. This sysview
contains names and values of the session settings.

This sysview creates tuples on the fly using its own get() and
create_iterator() methods. This allows us to get a tuple without
having to save it anywhere. But this means that every time we get
a tuple from this system view, it is a new tuple, even if they
look the same:

tarantool> v = box.space._vsession_settings
tarantool> v:get({'sql_default_engine'}) ~= v:get({'sql_default_engine'})
---
- true
...

Currently, only SQL settings can be extracted from this sysview,
since the only session settings are SQL settings.

Part of #4511

@TarantoolBot document
Title: _vsession_settings system view
_vsession_settings sysview is used to get the current session
settings. List of currently available session settings:

sql_default_engine
sql_defer_foreign_keys
sql_full_column_names
sql_recursive_triggers
sql_reverse_unordered_selects

Debug build also have debug settings that could be obtained from
this sysview:

sql_parser_trace
sql_select_trace
sql_trace
sql_vdbe_addoptrace
sql_vdbe_debug
sql_vdbe_eqp
sql_vdbe_listing
sql_vdbe_trace
sql_where_trace

Example of usage:
tarantool> box.space._vsession_settings:get({'sql_default_engine'})
---
- ['sql_default_engine', 'memtx']
...

tarantool> box.space._vsession_settings:select()
---
- - ['sql_default_engine', 'memtx']
  - ['sql_defer_foreign_keys', false]
  - ['sql_full_column_names', false]
  - ['sql_recursive_triggers', true]
  - ['sql_reverse_unordered_selects', false]
...

tarantool> box.space._vsession_settings:select('sql_g', {iterator='LE'})
---
- - ['sql_full_column_names', false]
  - ['sql_defer_foreign_keys', false]
  - ['sql_default_engine', 'memtx']
...
---
 src/box/bootstrap.snap             | Bin 5944 -> 5982 bytes
 src/box/lua/space.cc               |   2 +
 src/box/lua/upgrade.lua            |  23 ++++++
 src/box/schema_def.h               |   2 +
 src/box/session.cc                 | 131 ++++++++++++++++++++++++++++++++++
 src/box/session.h                  |  13 ++++
 src/box/sql.h                      |  36 ++++++++++
 src/box/sql/build.c                | 140 +++++++++++++++++++++++++++++++++++++
 src/box/sysview.c                  |  18 +++--
 test/app-tap/tarantoolctl.test.lua |   4 +-
 test/box-py/bootstrap.result       |   5 +-
 test/box/access_sysview.result     | 124 +++++++++++++++++++++++++++++++-
 test/box/access_sysview.test.lua   |  54 ++++++++++++++
 test/box/alter.result              |   5 +-
 test/wal_off/alter.result          |   2 +-
 15 files changed, 545 insertions(+), 14 deletions(-)

diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index f6e96f0..9b5df28 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -653,6 +653,8 @@ box_lua_space_init(struct lua_State *L)
 	lua_setfield(L, -2, "SPACE_SEQUENCE_ID");
 	lua_pushnumber(L, BOX_FUNC_INDEX_ID);
 	lua_setfield(L, -2, "FUNC_INDEX_ID");
+	lua_pushnumber(L, BOX_VSESSION_SETTINGS_ID);
+	lua_setfield(L, -2, "VSESSION_SETTINGS_ID");
 	lua_pushnumber(L, BOX_SYSTEM_ID_MIN);
 	lua_setfield(L, -2, "SYSTEM_ID_MIN");
 	lua_pushnumber(L, BOX_SYSTEM_ID_MAX);
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index e71b7fb..94e4118 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -943,6 +943,28 @@ local function upgrade_to_2_3_0()
 end
 
 --------------------------------------------------------------------------------
+-- Tarantool 2.3.1
+--------------------------------------------------------------------------------
+
+local function create_vsession_settings_sysview()
+    local _space = box.space[box.schema.SPACE_ID]
+    local _index = box.space[box.schema.INDEX_ID]
+    local format = {}
+    format[1] = {name='name', type='string'}
+    format[2] = {name='value', type='any'}
+    log.info("create space _vsession_settings")
+    _space:insert{box.schema.VSESSION_SETTINGS_ID, ADMIN, '_vsession_settings',
+                  'sysview', 0, setmap({}), format}
+    log.info("create index _vsession_settings:primary")
+    _index:insert{box.schema.VSESSION_SETTINGS_ID, 0, 'primary', 'tree',
+                  {unique = true}, {{0, 'string'}}}
+end
+
+local function upgrade_to_2_3_1()
+    create_vsession_settings_sysview()
+end
+
+--------------------------------------------------------------------------------
 
 local function get_version()
     local version = box.space._schema:get{'version'}
@@ -977,6 +999,7 @@ local function upgrade(options)
         {version = mkversion(2, 1, 3), func = upgrade_to_2_1_3, auto = true},
         {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
         {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
+        {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
     }
 
     for _, handler in ipairs(handlers) do
diff --git a/src/box/schema_def.h b/src/box/schema_def.h
index ba870ff..b9824cb 100644
--- a/src/box/schema_def.h
+++ b/src/box/schema_def.h
@@ -114,6 +114,8 @@ enum {
 	BOX_CK_CONSTRAINT_ID = 364,
 	/** Space id of _func_index. */
 	BOX_FUNC_INDEX_ID = 372,
+	/** Space id of _vsession_settings. */
+	BOX_VSESSION_SETTINGS_ID = 381,
 	/** End of the reserved range of system spaces. */
 	BOX_SYSTEM_ID_MAX = 511,
 	BOX_ID_NIL = 2147483647
diff --git a/src/box/session.cc b/src/box/session.cc
index 461d1cf..93d8b42 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -36,6 +36,9 @@
 #include "user.h"
 #include "error.h"
 #include "tt_static.h"
+#include "index.h"
+#include "schema.h"
+#include "sql.h"
 
 const char *session_type_strs[] = {
 	"background",
@@ -338,3 +341,131 @@ generic_session_sync(struct session *session)
 	(void) session;
 	return 0;
 }
+
+int
+session_options_get(struct index *index, const char *key, uint32_t part_count,
+		    struct tuple **result)
+{
+	assert(part_count == 1);
+	(void)part_count;
+	struct space *space = space_cache_find(index->def->space_id);
+	uint32_t len;
+	const char *name = mp_decode_str(&key, &len);
+	name = tt_cstr(name, len);
+	/*
+	 * Currently, the only session local options are SQL
+	 * options.
+	 */
+	uint32_t id_max = sql_session_opt_id_max();
+	for (uint32_t id = 0; id < id_max; ++id) {
+		if (sql_session_opt_compare(name, id) == 0)
+			return sql_session_opt_tuple(space->format, id, result);
+	}
+	*result = NULL;
+	return 0;
+}
+
+/**
+ * An iterator that iterates over current session options.
+ */
+struct session_options_iterator {
+	/** Base iterator. Must be the first member. */
+	struct iterator base;
+	/** Format of the tuples this iterator returns. */
+	struct tuple_format *format;
+	/** ID of the option in global list of the options. */
+	int current_id;
+	/** Decoded key. */
+	char *key;
+	/** Type of iterator. */
+	enum iterator_type iterator_type;
+};
+
+static int
+session_options_iterator_next(struct iterator *itr, struct tuple **ret)
+{
+	struct session_options_iterator *it =
+		(struct session_options_iterator *)itr;
+	if (it->key == NULL)
+		return sql_session_opt_tuple(it->format, it->current_id++, ret);
+	int id_max = sql_session_opt_id_max();
+	for (; it->current_id < id_max; ++it->current_id) {
+		int compare = sql_session_opt_compare(it->key, it->current_id);
+		if (compare == 0 && (it->iterator_type == ITER_EQ ||
+				     it->iterator_type == ITER_GE ||
+				     it->iterator_type == ITER_ALL))
+			break;
+		if (compare > 0 && (it->iterator_type == ITER_GT ||
+				    it->iterator_type == ITER_GE ||
+				    it->iterator_type == ITER_ALL))
+			break;
+	}
+	return sql_session_opt_tuple(it->format, it->current_id++, ret);
+}
+
+static int
+session_options_iterator_prev(struct iterator *itr, struct tuple **ret)
+{
+	struct session_options_iterator *it =
+		(struct session_options_iterator *)itr;
+	if (it->key == NULL)
+		return sql_session_opt_tuple(it->format, it->current_id--, ret);
+	for (; it->current_id >= 0; --it->current_id) {
+		int compare = sql_session_opt_compare(it->key, it->current_id);
+		if (compare == 0 && (it->iterator_type == ITER_REQ ||
+				     it->iterator_type == ITER_LE))
+			break;
+		if (compare < 0 && (it->iterator_type == ITER_LT ||
+				    it->iterator_type == ITER_LE))
+			break;
+	}
+	return sql_session_opt_tuple(it->format, it->current_id--, ret);
+}
+
+static void
+session_options_iterator_free(struct iterator *itr)
+{
+	struct session_options_iterator *it =
+		(struct session_options_iterator *)itr;
+	free(it->key);
+	free(it);
+}
+
+struct iterator *
+session_options_create_iterator(struct index *index, enum iterator_type type,
+				const char *key, uint32_t part_count)
+{
+	char *decoded_key = NULL;
+	if (part_count > 0) {
+		assert(part_count == 1);
+		assert(mp_typeof(*key) == MP_STR);
+		uint32_t len;
+		const char *name = mp_decode_str(&key, &len);
+		decoded_key = (char *)malloc(len + 1);
+		if (decoded_key == NULL) {
+			diag_set(OutOfMemory, len + 1, "malloc", "decoded_key");
+			return NULL;
+		}
+		memcpy(decoded_key, name, len);
+		decoded_key[len] = '\0';
+	}
+	struct space *space = space_cache_find(index->def->space_id);
+	struct session_options_iterator *it =
+		(session_options_iterator *)malloc(sizeof(*it));
+	if (it == NULL) {
+		diag_set(OutOfMemory, sizeof(*it), "malloc", "it");
+		return NULL;
+	}
+	int current_id = iterator_type_is_reverse(type) ?
+			 sql_session_opt_id_max() - 1 : 0;
+	iterator_create(&it->base, index);
+	it->base.next = iterator_type_is_reverse(type) ?
+			session_options_iterator_prev :
+			session_options_iterator_next;
+	it->base.free = session_options_iterator_free;
+	it->key = decoded_key;
+	it->iterator_type = type;
+	it->format = space->format;
+	it->current_id = current_id;
+	return (struct iterator *)it;
+}
diff --git a/src/box/session.h b/src/box/session.h
index eff3d7a..f5e3cf2 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -36,12 +36,15 @@
 #include "fiber.h"
 #include "user.h"
 #include "authentication.h"
+#include "iterator_type.h"
 
 #if defined(__cplusplus)
 extern "C" {
 #endif /* defined(__cplusplus) */
 
 struct port;
+struct index;
+struct tuple;
 struct session_vtab;
 
 void
@@ -347,6 +350,16 @@ generic_session_fd(struct session *session);
 int64_t
 generic_session_sync(struct session *session);
 
+/** Method get() for _vsession_settings sysview. */
+int
+session_options_get(struct index *index, const char *key, uint32_t part_count,
+		    struct tuple **result);
+
+/** Method create_iterator() for _vsession_settings sysview. */
+struct iterator *
+session_options_create_iterator(struct index *index, enum iterator_type type,
+				const char *key, uint32_t part_count);
+
 #if defined(__cplusplus)
 } /* extern "C" */
 
diff --git a/src/box/sql.h b/src/box/sql.h
index 0fa52fc..604c3d2 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -70,6 +70,7 @@ struct Select;
 struct Table;
 struct sql_trigger;
 struct space_def;
+struct tuple_format;
 
 /**
  * Perform parsing of provided expression. This is done by
@@ -420,6 +421,41 @@ void
 vdbe_field_ref_prepare_tuple(struct vdbe_field_ref *field_ref,
 			     struct tuple *tuple);
 
+/**
+ * Return the next number after the ID of the last SQL session
+ * option.
+ *
+ * @retval Next number after the last SQL option.
+ */
+uint32_t
+sql_session_opt_id_max();
+
+/**
+ * Return an integer less than, equal to, or greater than zero if
+ * name of session option with given ID is found, respectively,
+ * to be less than, to match, or be greater than key.
+ *
+ * @param key Key to compare with option name.
+ * @param id ID of the option.
+ * @retval result of comparison.
+ */
+int
+sql_session_opt_compare(const char *key, uint32_t id);
+
+/**
+ * Create a tuple that contains the name and value of the SQL
+ * session option.
+ *
+ * @param format Format of the tuple.
+ * @param option_id ID of SQL option.
+ * @param[out] result New tuple or NULL.
+ * @retval 0 On success.
+ * @retval < 0 On error.
+ */
+int
+sql_session_opt_tuple(struct tuple_format *format, int option_id,
+		      struct tuple **result);
+
 #if defined(__cplusplus)
 } /* extern "C" { */
 #endif
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 51cd7ce..ce87b88 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -48,6 +48,7 @@
 #include "vdbeInt.h"
 #include "tarantoolInt.h"
 #include "box/box.h"
+#include "box/tuple.h"
 #include "box/ck_constraint.h"
 #include "box/fk_constraint.h"
 #include "box/sequence.h"
@@ -3242,3 +3243,142 @@ sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
 	*fieldno = i;
 	return 0;
 }
+
+/**
+ * Identifiers of all SQL session options that can be viewed. The
+ * identifier of the option is equal to its place in the sorted
+ * list of session options, which starts at 0.
+ *
+ * It is IMPORTANT that these options are sorted by name. If this
+ * is not the case, the result returned by the _vsession_settings
+ * space iterator will not be sorted properly.
+ */
+enum {
+	SQL_SESSION_OPTION_DEFAULT_ENGINE = 0,
+	SQL_SESSION_OPTION_DEFER_FOREIGN_KEYS,
+	SQL_SESSION_OPTION_FULL_COLUMN_NAMES,
+#ifndef NDEBUG
+	SQL_SESSION_OPTION_PARSER_TRACE,
+#endif
+	SQL_SESSION_OPTION_RECURSIVE_TRIGGERS,
+	SQL_SESSION_OPTION_REVERSE_UNORDERED_SELECTS,
+#ifndef NDEBUG
+	SQL_SESSION_OPTION_SELECT_TRACE,
+	SQL_SESSION_OPTION_TRACE,
+	SQL_SESSION_OPTION_VDBE_ADDOPTRACE,
+	SQL_SESSION_OPTION_VDBE_DEBUG,
+	SQL_SESSION_OPTION_VDBE_EQP,
+	SQL_SESSION_OPTION_VDBE_LISTING,
+	SQL_SESSION_OPTION_VDBE_TRACE,
+	SQL_SESSION_OPTION_WHERE_TRACE,
+#endif
+	SQL_SESSION_OPTION_max,
+};
+
+/**
+ * A local structure that allows to establish a connection between
+ * the name of the parameter, its field type and mask, if it have
+ * one.
+ */
+struct sql_option_metadata
+{
+	const char *name;
+	uint32_t field_type;
+	uint32_t mask;
+};
+
+/**
+ * Variable that contains names of the SQL session options, their
+ * field types and mask if they have one or 0 if don't have.
+ *
+ * It is IMPORTANT that these options sorted by name.
+ */
+static struct sql_option_metadata sql_session_opts[] = {
+	/** SQL_SESSION_OPTION_DEFAULT_ENGINE */
+	{"sql_default_engine", FIELD_TYPE_STRING, 0},
+	/** SQL_SESSION_OPTION_DEFER_FOREIGN_KEYS */
+	{"sql_defer_foreign_keys", FIELD_TYPE_BOOLEAN, SQL_DeferFKs},
+	/** SQL_SESSION_OPTION_FULL_COLUMN_NAMES */
+	{"sql_full_column_names", FIELD_TYPE_BOOLEAN, SQL_FullColNames},
+#ifndef NDEBUG
+	/** SQL_SESSION_OPTION_PARSER_TRACE */
+	{"sql_parser_trace", FIELD_TYPE_BOOLEAN, PARSER_TRACE_FLAG},
+#endif
+	/** SQL_SESSION_OPTION_RECURSIVE_TRIGGERS */
+	{"sql_recursive_triggers", FIELD_TYPE_BOOLEAN, SQL_RecTriggers},
+	/** SQL_SESSION_OPTION_REVERSE_UNORDERED_SELECTS */
+	{"sql_reverse_unordered_selects", FIELD_TYPE_BOOLEAN, SQL_ReverseOrder},
+#ifndef NDEBUG
+	/** SQL_SESSION_OPTION_SELECT_TRACE */
+	{"sql_select_trace", FIELD_TYPE_BOOLEAN, SQL_SelectTrace},
+	/** SQL_SESSION_OPTION_TRACE */
+	{"sql_trace", FIELD_TYPE_BOOLEAN, SQL_SqlTrace},
+	/** SQL_SESSION_OPTION_VDBE_ADDOPTRACE */
+	{"sql_vdbe_addoptrace", FIELD_TYPE_BOOLEAN, SQL_VdbeAddopTrace},
+	/** SQL_SESSION_OPTION_VDBE_DEBUG */
+	{"sql_vdbe_debug", FIELD_TYPE_BOOLEAN,
+	 SQL_SqlTrace | SQL_VdbeListing | SQL_VdbeTrace},
+	/** SQL_SESSION_OPTION_VDBE_EQP */
+	{"sql_vdbe_eqp", FIELD_TYPE_BOOLEAN, SQL_VdbeEQP},
+	/** SQL_SESSION_OPTION_VDBE_LISTING */
+	{"sql_vdbe_listing", FIELD_TYPE_BOOLEAN, SQL_VdbeListing},
+	/** SQL_SESSION_OPTION_VDBE_TRACE */
+	{"sql_vdbe_trace", FIELD_TYPE_BOOLEAN, SQL_VdbeTrace},
+	/** SQL_SESSION_OPTION_WHERE_TRACE */
+	{"sql_where_trace", FIELD_TYPE_BOOLEAN, SQL_WhereTrace},
+#endif
+};
+
+uint32_t
+sql_session_opt_id_max()
+{
+	return SQL_SESSION_OPTION_max;
+}
+
+int
+sql_session_opt_compare(const char *key, uint32_t id)
+{
+	assert(id < SQL_SESSION_OPTION_max);
+	return strcmp(sql_session_opts[id].name, key);
+}
+
+int
+sql_session_opt_tuple(struct tuple_format *format, int option_id,
+		 struct tuple **result)
+{
+	if (option_id < 0 || option_id >= SQL_SESSION_OPTION_max) {
+		*result = NULL;
+		return 0;
+	}
+	struct session *session = current_session();
+	uint32_t flags = session->sql_flags;
+	uint32_t mask = sql_session_opts[option_id].mask;
+	const char *engine = NULL;
+	/* Tuple format contains two fields - name and value. */
+	uint32_t column_count = format->min_field_count;
+	assert(column_count == 2);
+	size_t size = mp_sizeof_array(column_count) +
+		      mp_sizeof_str(strlen(sql_session_opts[option_id].name));
+	if (sql_session_opts[option_id].field_type == FIELD_TYPE_BOOLEAN) {
+		size += mp_sizeof_bool(true);
+	} else {
+		assert(option_id == SQL_SESSION_OPTION_DEFAULT_ENGINE);
+		engine = sql_storage_engine_strs[session->sql_default_engine];
+		size += mp_sizeof_str(strlen(engine));
+	}
+
+	char *pos_ret = static_alloc(size);
+	assert(pos_ret != NULL);
+	char *pos = mp_encode_array(pos_ret, column_count);
+	pos = mp_encode_str(pos, sql_session_opts[option_id].name,
+			    strlen(sql_session_opts[option_id].name));
+	if (sql_session_opts[option_id].field_type == FIELD_TYPE_BOOLEAN)
+		pos = mp_encode_bool(pos, (flags & mask) == mask);
+	else
+		pos = mp_encode_str(pos, engine, strlen(engine));
+	struct tuple *tuple = tuple_new(format, pos_ret, pos_ret + size);
+	if (tuple == NULL)
+		return -1;
+	*result = tuple;
+	return 0;
+}
diff --git a/src/box/sysview.c b/src/box/sysview.c
index 745cf09..83a8ccc 100644
--- a/src/box/sysview.c
+++ b/src/box/sysview.c
@@ -411,7 +411,7 @@ vsequence_filter(struct space *source, struct tuple *tuple)
 }
 
 static bool
-vcollation_filter(struct space *source, struct tuple *tuple)
+generic_filter(struct space *source, struct tuple *tuple)
 {
 	(void) source;
 	(void) tuple;
@@ -481,10 +481,17 @@ sysview_space_create_index(struct space *space, struct index_def *def)
 	case BOX_VCOLLATION_ID:
 		source_space_id = BOX_COLLATION_ID;
 		source_index_id = def->iid;
-		filter = vcollation_filter;
+		filter = generic_filter;
 		get = index_get;
 		create_iterator = index_create_iterator;
 		break;
+	case BOX_VSESSION_SETTINGS_ID:
+		source_space_id = BOX_VSESSION_SETTINGS_ID;
+		source_index_id = def->iid;
+		filter = generic_filter;
+		get = session_options_get;
+		create_iterator = session_options_create_iterator;
+		break;
 	default:
 		diag_set(ClientError, ER_MODIFY_INDEX,
 			 def->name, space_name(space),
@@ -569,9 +576,10 @@ sysview_engine_create_space(struct engine *engine, struct space_def *def,
 		return NULL;
 	}
 	struct tuple_format *format =
-		tuple_format_new(NULL, NULL, keys, key_count, def->fields,
-				 def->field_count, def->exact_field_count,
-				 def->dict, def->opts.is_temporary,
+		tuple_format_new(&tuple_format_runtime->vtab, NULL, keys,
+				 key_count, def->fields, def->field_count,
+				 def->exact_field_count, def->dict,
+				 def->opts.is_temporary,
 				 def->opts.is_ephemeral);
 	if (format == NULL) {
 		free(space);
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index 7a07860..4d70595 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -415,8 +415,8 @@ do
             check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json --show-system --replica 1", "\n", 3)
             check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json --show-system --replica 1 --replica 2", "\n", 3)
             check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json --show-system --replica 2", "\n", 0)
-            check_ctlcat_snap(test_i, dir, "--space=280", "---\n", 24)
-            check_ctlcat_snap(test_i, dir, "--space=288", "---\n", 52)
+            check_ctlcat_snap(test_i, dir, "--space=280", "---\n", 25)
+            check_ctlcat_snap(test_i, dir, "--space=288", "---\n", 53)
         end)
     end)
 
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 123aa2f..54b43ec 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -4,7 +4,7 @@ box.internal.bootstrap()
 box.space._schema:select{}
 ---
 - - ['max_id', 511]
-  - ['version', 2, 3, 0]
+  - ['version', 2, 3, 1]
 ...
 box.space._cluster:select{}
 ---
@@ -96,6 +96,8 @@ box.space._space:select{}
         'type': 'boolean'}]]
   - [372, 1, '_func_index', 'memtx', 0, {}, [{'name': 'space_id', 'type': 'unsigned'},
       {'name': 'index_id', 'type': 'unsigned'}, {'name': 'func_id', 'type': 'unsigned'}]]
+  - [381, 1, '_vsession_settings', 'sysview', 0, {}, [{'name': 'name', 'type': 'string'},
+      {'name': 'value', 'type': 'any'}]]
 ...
 box.space._index:select{}
 ---
@@ -153,6 +155,7 @@ box.space._index:select{}
   - [364, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned'], [1, 'string']]]
   - [372, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned'], [1, 'unsigned']]]
   - [372, 1, 'fid', 'tree', {'unique': false}, [[2, 'unsigned']]]
+  - [381, 0, 'primary', 'tree', {'unique': true}, [[0, 'string']]]
 ...
 box.space._user:select{}
 ---
diff --git a/test/box/access_sysview.result b/test/box/access_sysview.result
index 1f33dec..c032248 100644
--- a/test/box/access_sysview.result
+++ b/test/box/access_sysview.result
@@ -246,11 +246,11 @@ box.session.su('guest')
 ...
 #box.space._vspace:select{}
 ---
-- 25
+- 26
 ...
 #box.space._vindex:select{}
 ---
-- 53
+- 54
 ...
 #box.space._vuser:select{}
 ---
@@ -282,7 +282,7 @@ box.session.su('guest')
 ...
 #box.space._vindex:select{}
 ---
-- 53
+- 54
 ...
 #box.space._vuser:select{}
 ---
@@ -743,3 +743,121 @@ s = box.space._space:create_index('owner', {parts = { 2, 'unsigned' }, id = 1, u
 session = nil
 ---
 ...
+--
+-- gh-4511: make sure that _vsession_settings sysview works as
+-- intended.
+--
+v = box.space._vsession_settings
+---
+...
+v:format()
+---
+- [{'name': 'name', 'type': 'string'}, {'name': 'value', 'type': 'any'}]
+...
+s = box.schema.space.create('settings', {format = v:format()})
+---
+...
+_ = s:create_index('primary')
+---
+...
+for _,value in v:pairs() do s:insert(value) end
+---
+...
+env = require('test_run')
+---
+...
+test_run = env.new()
+---
+...
+test_run:cmd('setopt delimiter ";"')
+---
+- true
+...
+function check_sorting(vs, ts, key)
+    local is_right = true
+    local iterators_list = {'ALL', 'REQ', 'EQ', 'GE', 'GT', 'LE', 'LT'}
+    for _, it in pairs(iterators_list) do
+        local view_space = vs:select({key}, {iterator = it})
+        local test_space = ts:select({key}, {iterator = it})
+        for key, value in pairs(view_space) do
+            is_right = is_right and (test_space[key].name == value.name)
+        end
+    end
+    return is_right
+end;
+---
+...
+test_run:cmd('setopt delimiter ""');
+---
+- true
+...
+check_sorting(v, s)
+---
+- true
+...
+check_sorting(v, s, 'abcde')
+---
+- true
+...
+check_sorting(v, s, 'sql_d')
+---
+- true
+...
+check_sorting(v, s, 'sql_v')
+---
+- true
+...
+check_sorting(v, s, 'sql_defer_foreign_keys')
+---
+- true
+...
+-- Check get() method.
+v:get({'sql_defer_foreign_keys'})
+---
+- ['sql_defer_foreign_keys', false]
+...
+v:get({'sql_recursive_triggers'})
+---
+- ['sql_recursive_triggers', true]
+...
+v:get({'sql_reverse_unordered_selects'})
+---
+- ['sql_reverse_unordered_selects', false]
+...
+v:get({'sql_default_engine'})
+---
+- ['sql_default_engine', 'memtx']
+...
+v:get({'abcd'})
+---
+...
+-- Make sure that space is read-only.
+new_record = v:frommap({name='abs', value=123})
+---
+...
+v:insert(new_record)
+---
+- error: View '_vsession_settings' is read-only
+...
+-- Check pairs() method.
+t = {}
+---
+...
+for key, value in v:pairs() do table.insert(t, {key, value}) end
+---
+...
+#t == v:count()
+---
+- true
+...
+-- Make sure that SQL SELECT works.
+box.execute([[SELECT * from "_vsession_settings" WHERE "name" = 'sql_default_engine']])
+---
+- metadata:
+  - name: name
+    type: string
+  - name: value
+    type: any
+  rows:
+  - ['sql_default_engine', 'memtx']
+...
diff --git a/test/box/access_sysview.test.lua b/test/box/access_sysview.test.lua
index 17420ae..a35a79c 100644
--- a/test/box/access_sysview.test.lua
+++ b/test/box/access_sysview.test.lua
@@ -308,3 +308,57 @@ s = box.space._space:create_index('owner', {parts = { 2, 'unsigned' }, id = 1, u
 #box.space._vspace.index[1]:select(1) > 0
 
 session = nil
+
+--
+-- gh-4511: make sure that _vsession_settings sysview works as
+-- intended.
+--
+
+v = box.space._vsession_settings
+v:format()
+
+s = box.schema.space.create('settings', {format = v:format()})
+_ = s:create_index('primary')
+for _,value in v:pairs() do s:insert(value) end
+
+env = require('test_run')
+test_run = env.new()
+test_run:cmd('setopt delimiter ";"')
+function check_sorting(vs, ts, key)
+    local is_right = true
+    local iterators_list = {'ALL', 'REQ', 'EQ', 'GE', 'GT', 'LE', 'LT'}
+    for _, it in pairs(iterators_list) do
+        local view_space = vs:select({key}, {iterator = it})
+        local test_space = ts:select({key}, {iterator = it})
+        for key, value in pairs(view_space) do
+            is_right = is_right and (test_space[key].name == value.name)
+        end
+    end
+    return is_right
+end;
+test_run:cmd('setopt delimiter ""');
+
+check_sorting(v, s)
+check_sorting(v, s, 'abcde')
+check_sorting(v, s, 'sql_d')
+check_sorting(v, s, 'sql_v')
+check_sorting(v, s, 'sql_defer_foreign_keys')
+
+-- Check get() method.
+v:get({'sql_defer_foreign_keys'})
+v:get({'sql_recursive_triggers'})
+v:get({'sql_reverse_unordered_selects'})
+v:get({'sql_default_engine'})
+v:get({'abcd'})
+
+-- Make sure that space is read-only.
+new_record = v:frommap({name='abs', value=123})
+v:insert(new_record)
+
+-- Check pairs() method.
+t = {}
+for key, value in v:pairs() do table.insert(t, {key, value}) end
+#t == v:count()
+
+-- Make sure that SQL SELECT works.
+box.execute([[SELECT * from "_vsession_settings" WHERE "name" = 'sql_default_engine']])
diff --git a/test/box/alter.result b/test/box/alter.result
index 46ce868..2a0eaca 100644
--- a/test/box/alter.result
+++ b/test/box/alter.result
@@ -92,7 +92,7 @@ space = box.space[t[1]]
 ...
 space.id
 ---
-- 373
+- 382
 ...
 space.field_count
 ---
@@ -137,7 +137,7 @@ space_deleted
 ...
 space:replace{0}
 ---
-- error: Space '373' does not exist
+- error: Space '382' does not exist
 ...
 _index:insert{_space.id, 0, 'primary', 'tree', {unique=true}, {{0, 'unsigned'}}}
 ---
@@ -220,6 +220,7 @@ _index:select{}
   - [364, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned'], [1, 'string']]]
   - [372, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned'], [1, 'unsigned']]]
   - [372, 1, 'fid', 'tree', {'unique': false}, [[2, 'unsigned']]]
+  - [381, 0, 'primary', 'tree', {'unique': true}, [[0, 'string']]]
 ...
 -- modify indexes of a system space
 _index:delete{_index.id, 0}
diff --git a/test/wal_off/alter.result b/test/wal_off/alter.result
index 62cb11d..97f7e6f 100644
--- a/test/wal_off/alter.result
+++ b/test/wal_off/alter.result
@@ -28,7 +28,7 @@ end;
 ...
 #spaces;
 ---
-- 65502
+- 65501
 ...
 -- cleanup
 for k, v in pairs(spaces) do
-- 
2.7.4

Re: [Tarantool-patches] [PATCH v4 2/2] box: introduce _vsession_settings sysview

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

> diff --git a/src/box/session.cc b/src/box/session.cc
> index 461d1cf..93d8b42 100644
> --- a/src/box/session.cc
> +++ b/src/box/session.cc
> @@ -338,3 +341,131 @@ generic_session_sync(struct session *session)
> +
> +struct iterator *
> +session_options_create_iterator(struct index *index, enum iterator_type type,
> +				const char *key, uint32_t part_count)
> +{
> +	char *decoded_key = NULL;
> +	if (part_count > 0) {
> +		assert(part_count == 1);
> +		assert(mp_typeof(*key) == MP_STR);
> +		uint32_t len;
> +		const char *name = mp_decode_str(&key, &len);
> +		decoded_key = (char *)malloc(len + 1);
> +		if (decoded_key == NULL) {
> +			diag_set(OutOfMemory, len + 1, "malloc", "decoded_key");
> +			return NULL;
> +		}
> +		memcpy(decoded_key, name, len);
> +		decoded_key[len] = '\0';
> +	}
> +	struct space *space = space_cache_find(index->def->space_id);
> +	struct session_options_iterator *it =
> +		(session_options_iterator *)malloc(sizeof(*it));
> +	if (it == NULL) {

decoded_key leaks here. But heap OOM is such an unlikely event,
and is forgotten for be checked so often, that perhaps we should
reconsider moving this https://github.com/tarantool/tarantool/issues/3534
out of wish list and start using panic in new places. Probably
wrapped into a 'heap_alloc()' function (name is discussable).

I think, Kirill won't be against this.

Re: [Tarantool-patches] [PATCH v4 2/2] box: introduce _vsession_settings sysview

[Konstantin Osipov]

* imeevma@tarantool.org <imeevma@tarantool.org> [19/11/23 23:10]:
> This patch creates the _vsession_settings sysview. This sysview
> contains names and values of the session settings.

Having a session settings in a database that heavily multiplexes
requests over the same connection is a bad idea: the features simply don't work
together. A concurrent statement may tweak the setting while the
previous statement is still in progress.

Having this feature is simply a time bomb waiting to explode:
sooner or later there will be a setting that makes sense to change
asynchronously and that impacts in-flight transactions, and there
will be no sane fix in sight.


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v4 2/2] box: introduce _vsession_settings sysview

[Kirill Yukhin]

Hello,

On 27 ноя 00:12, Konstantin Osipov wrote:
> * imeevma@tarantool.org <imeevma@tarantool.org> [19/11/23 23:10]:
> > This patch creates the _vsession_settings sysview. This sysview
> > contains names and values of the session settings.
> 
> Having a session settings in a database that heavily multiplexes
> requests over the same connection is a bad idea: the features simply don't work
> together. A concurrent statement may tweak the setting while the
> previous statement is still in progress.

We consider pragmas a dead end and not going to extend them further.
However we still need a mechanism for storing session-related things,
like verbosity flags or default engine. We believe, that such settings
are related to the session, not to DB and that is why this patchset
was prepared.

Anyway, thanks a lot for your inputs.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH v4 2/2] box: introduce _vsession_settings sysview

[Mergen Imeev]

Thank you for review! My answer and diff between patches
below.

On Sun, Nov 24, 2019 at 04:27:58PM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> > diff --git a/src/box/session.cc b/src/box/session.cc
> > index 461d1cf..93d8b42 100644
> > --- a/src/box/session.cc
> > +++ b/src/box/session.cc
> > @@ -338,3 +341,131 @@ generic_session_sync(struct session *session)
> > +
> > +struct iterator *
> > +session_options_create_iterator(struct index *index, enum iterator_type type,
> > +				const char *key, uint32_t part_count)
> > +{
> > +	char *decoded_key = NULL;
> > +	if (part_count > 0) {
> > +		assert(part_count == 1);
> > +		assert(mp_typeof(*key) == MP_STR);
> > +		uint32_t len;
> > +		const char *name = mp_decode_str(&key, &len);
> > +		decoded_key = (char *)malloc(len + 1);
> > +		if (decoded_key == NULL) {
> > +			diag_set(OutOfMemory, len + 1, "malloc", "decoded_key");
> > +			return NULL;
> > +		}
> > +		memcpy(decoded_key, name, len);
> > +		decoded_key[len] = '\0';
> > +	}
> > +	struct space *space = space_cache_find(index->def->space_id);
> > +	struct session_options_iterator *it =
> > +		(session_options_iterator *)malloc(sizeof(*it));
> > +	if (it == NULL) {
> 
> decoded_key leaks here. But heap OOM is such an unlikely event,
> and is forgotten for be checked so often, that perhaps we should
> reconsider moving this https://github.com/tarantool/tarantool/issues/3534
> out of wish list and start using panic in new places. Probably
> wrapped into a 'heap_alloc()' function (name is discussable).
> 
> I think, Kirill won't be against this.
Fixed. I asked about issue #3534 in a scrum, but haven’t
received a definite answer yet. I will ask about it again,
but a little later, since now we have to prepare for
release 2.3.1. Also, it looks like we should replace all
malloc() checks in one commit.

diff --git a/src/box/session.cc b/src/box/session.cc
index 93d8b42..fbaabf0 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -454,6 +454,7 @@ session_options_create_iterator(struct index *index, enum iterator_type type,
 		(session_options_iterator *)malloc(sizeof(*it));
 	if (it == NULL) {
 		diag_set(OutOfMemory, sizeof(*it), "malloc", "it");
+		free(decoded_key);
 		return NULL;
 	}
 	int current_id = iterator_type_is_reverse(type) ?

Re: [Tarantool-patches] [PATCH v4 2/2] box: introduce _vsession_settings sysview

[Konstantin Osipov]

Pragmas are of course a dead end, but pragmas themselves are
unimportant, so it's still unclear why one should bother about them,
as opposed to, for example, caring about foreign key checks for all
front ends.

But fine, assuming one wants to remove prgamas, adding extra overhead
to session management is a bad idea. One could have a global sysview
_vsettings with all the settings, perhaps even ones from box.cfg, and
that would be enough.

Re: [Tarantool-patches] [PATCH v4 2/2] box: introduce _vsession_settings sysview

[Mergen Imeev]

On Wed, Nov 27, 2019 at 01:34:32PM +0300, Konstantin Osipov wrote:
> Pragmas are of course a dead end, but pragmas themselves are
> unimportant, so it's still unclear why one should bother about them,
> as opposed to, for example, caring about foreign key checks for all
> front ends.
> 
> But fine, assuming one wants to remove prgamas, adding extra overhead
> to session management is a bad idea. One could have a global sysview
> _vsettings with all the settings, perhaps even ones from box.cfg, and
> that would be enough.

Hi! Thanks for the review. However, I think you are a little
wrong. We do not save these settings anywhere except for the
struct session. When a user reads from the space, it creates
tuples on the fly and passes them to the user. All of this can
actually be read from the commit-message:


box: introduce _vsession_settings sysview

This patch creates the _vsession_settings sysview. This sysview
contains names and values of the session settings.

This sysview creates tuples on the fly using its own get() and
create_iterator() methods. This allows us to get a tuple without
having to save it anywhere. But this means that every time we get
a tuple from this system view, it is a new tuple, even if they
look the same:

tarantool> v = box.space._vsession_settings
tarantool> v:get({'sql_default_engine'}) ~= v:get({'sql_default_engine'})
---
- true
...

Currently, only SQL settings can be extracted from this sysview,
since the only session settings are SQL settings.

Part of #4511

@TarantoolBot document
Title: _vsession_settings system view
_vsession_settings sysview is used to get the current session
settings. List of currently available session settings:

sql_default_engine
sql_defer_foreign_keys
sql_full_column_names
sql_recursive_triggers
sql_reverse_unordered_selects

Debug build also have debug settings that could be obtained from
this sysview:

sql_parser_trace
sql_select_trace
sql_trace
sql_vdbe_addoptrace
sql_vdbe_debug
sql_vdbe_eqp
sql_vdbe_listing
sql_vdbe_trace
sql_where_trace

Example of usage:
tarantool> box.space._vsession_settings:get({'sql_default_engine'})
---
- ['sql_default_engine', 'memtx']
...

tarantool> box.space._vsession_settings:select()
---
- - ['sql_default_engine', 'memtx']
- ['sql_defer_foreign_keys', false]
- ['sql_full_column_names', false]
- ['sql_recursive_triggers', true]
- ['sql_reverse_unordered_selects', false]
...

tarantool> box.space._vsession_settings:select('sql_g', {iterator='LE'})
---
- - ['sql_full_column_names', false]
- ['sql_defer_foreign_keys', false]
- ['sql_default_engine', 'memtx']
...

Re: [Tarantool-patches] [PATCH v4 2/2] box: introduce _vsession_settings sysview

[Konstantin Osipov]

* Mergen Imeev <imeevma@tarantool.org> [19/11/27 13:56]:
> Hi! Thanks for the review. However, I think you are a little
> wrong. We do not save these settings anywhere except for the
> struct session. When a user reads from the space, it creates
> tuples on the fly and passes them to the user. All of this can
> actually be read from the commit-message:
> 
> 
> box: introduce _vsession_settings sysview
> 
> This patch creates the _vsession_settings sysview. This sysview
> contains names and values of the session settings.
> 
> This sysview creates tuples on the fly using its own get() and
> create_iterator() methods. This allows us to get a tuple without
> having to save it anywhere. But this means that every time we get
> a tuple from this system view, it is a new tuple, even if they
> look the same:
> 
> tarantool> v = box.space._vsession_settings
> tarantool> v:get({'sql_default_engine'}) ~= v:get({'sql_default_engine'})
> ---
> - true
> ...
> 
> Currently, only SQL settings can be extracted from this sysview,
> since the only session settings are SQL settings.

Where do you get the value from to build the tuple?

Where is it stored? If it is stored per-session, I mean
struct-session, then my comment is still valid.

I'm not challenging the implementation details (I did not read the
patch indeed), I am challenging the semantics. 

The view is called _vsession_settings, so I assume its state
(semantically) is local to each database connection, to which I
objected.

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v4 2/2] box: introduce _vsession_settings sysview

[Mergen Imeev]

On Wed, Nov 27, 2019 at 02:05:05PM +0300, Konstantin Osipov wrote:
> * Mergen Imeev <imeevma@tarantool.org> [19/11/27 13:56]:
> > Hi! Thanks for the review. However, I think you are a little
> > wrong. We do not save these settings anywhere except for the
> > struct session. When a user reads from the space, it creates
> > tuples on the fly and passes them to the user. All of this can
> > actually be read from the commit-message:
> > 
> > 
> > box: introduce _vsession_settings sysview
> > 
> > This patch creates the _vsession_settings sysview. This sysview
> > contains names and values of the session settings.
> > 
> > This sysview creates tuples on the fly using its own get() and
> > create_iterator() methods. This allows us to get a tuple without
> > having to save it anywhere. But this means that every time we get
> > a tuple from this system view, it is a new tuple, even if they
> > look the same:
> > 
> > tarantool> v = box.space._vsession_settings
> > tarantool> v:get({'sql_default_engine'}) ~= v:get({'sql_default_engine'})
> > ---
> > - true
> > ...
> > 
> > Currently, only SQL settings can be extracted from this sysview,
> > since the only session settings are SQL settings.
> 
> Where do you get the value from to build the tuple?
> 
I get it from current_session()->sql_flags and
current_session()->sql_default_engine.

> Where is it stored? If it is stored per-session, I mean
> struct-session, then my comment is still valid.
> 
There is only one place where all these settings are
stored. If this is not what you mean, then I cannot
understand you at the moment.

> I'm not challenging the implementation details (I did not read the
> patch indeed), I am challenging the semantics. 
> 
> The view is called _vsession_settings, so I assume its state
> (semantically) is local to each database connection, to which I
> objected.
> 
I think that the system view state is the same for all
sessions. It is empty, it has the same methods. However, it
can give different results, because when it gives tuples,
it reads from the session state (current_session()) to
create them.

> -- 
> Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v4 2/2] box: introduce _vsession_settings sysview

[Konstantin Osipov]

* Mergen Imeev <imeevma@tarantool.org> [19/11/27 14:22]:
> > Where do you get the value from to build the tuple?
> > 
> I get it from current_session()->sql_flags and
> current_session()->sql_default_engine.

OK, so my point is that they were put there as a historical
artefact of sqlite conversion. It seemed then as the most logical
place to put them. PRAGMAs were internal, so it was not a big
deal.

But going forward it's a dangerous place to store them, they should be
global - it fattens the session object, and if there are more
settings like these, there will be semantics issues. 

Besides, there will be a need for global counterpart anyway - since the
next logical question is how to reset the session defaults? 

I.e. I want all new sessions to get created with
sql_default_engine = vinyl, how do I do it?

Does my point make sense now?

> I think that the system view state is the same for all
> sessions. It is empty, it has the same methods. However, it
> can give different results, because when it gives tuples,
> it reads from the session state (current_session()) to
> create them.

This is a neat implementation, I agree.

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v4 2/2] box: introduce _vsession_settings sysview

[Mergen Imeev]

On Wed, Nov 27, 2019 at 02:37:41PM +0300, Konstantin Osipov wrote:
> * Mergen Imeev <imeevma@tarantool.org> [19/11/27 14:22]:
> > > Where do you get the value from to build the tuple?
> > > 
> > I get it from current_session()->sql_flags and
> > current_session()->sql_default_engine.
> 
> OK, so my point is that they were put there as a historical
> artefact of sqlite conversion. It seemed then as the most logical
> place to put them. PRAGMAs were internal, so it was not a big
> deal.
> 
> But going forward it's a dangerous place to store them, they should be
> global - it fattens the session object, and if there are more
> settings like these, there will be semantics issues. 
> 
> Besides, there will be a need for global counterpart anyway - since the
> next logical question is how to reset the session defaults? 
> 
> I.e. I want all new sessions to get created with
> sql_default_engine = vinyl, how do I do it?
> 
> Does my point make sense now?
> 
I think all these questions should solved in more global scope -
currently session is just a small object, that contains some
metadata. We cannot save or restore it. As far as I know, there
were some thougths of designing "session". That is all I know,
actually. Not sure that this should be solved in this issue.

As for the global settings - I think we have two ways:
1) Create a normal system space containing all the settings.
2) Create a structure containing these settings and save the
global settings there. Use the same approach as for session
settings after.

The first method looks more standard, but in this case we should
read from space every time we need the value of the setting, or
copy it somewhere else each time setting is changed to speed up
access.

Still, this has not yet been decided.

> > I think that the system view state is the same for all
> > sessions. It is empty, it has the same methods. However, it
> > can give different results, because when it gives tuples,
> > it reads from the session state (current_session()) to
> > create them.
> 
> This is a neat implementation, I agree.
> 
> -- 
> Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v4 2/2] box: introduce _vsession_settings sysview

[Vladislav Shpilevoy]

Thanks for the fixes!

On 27/11/2019 10:53, Mergen Imeev wrote:
> Thank you for review! My answer and diff between patches
> below.
> 
> On Sun, Nov 24, 2019 at 04:27:58PM +0100, Vladislav Shpilevoy wrote:
>> Hi! Thanks for the patch!
>>
>>> diff --git a/src/box/session.cc b/src/box/session.cc
>>> index 461d1cf..93d8b42 100644
>>> --- a/src/box/session.cc
>>> +++ b/src/box/session.cc
>>> @@ -338,3 +341,131 @@ generic_session_sync(struct session *session)
>>> +
>>> +struct iterator *
>>> +session_options_create_iterator(struct index *index, enum iterator_type type,
>>> +				const char *key, uint32_t part_count)
>>> +{
>>> +	char *decoded_key = NULL;
>>> +	if (part_count > 0) {
>>> +		assert(part_count == 1);
>>> +		assert(mp_typeof(*key) == MP_STR);
>>> +		uint32_t len;
>>> +		const char *name = mp_decode_str(&key, &len);
>>> +		decoded_key = (char *)malloc(len + 1);
>>> +		if (decoded_key == NULL) {
>>> +			diag_set(OutOfMemory, len + 1, "malloc", "decoded_key");
>>> +			return NULL;
>>> +		}
>>> +		memcpy(decoded_key, name, len);
>>> +		decoded_key[len] = '\0';
>>> +	}
>>> +	struct space *space = space_cache_find(index->def->space_id);
>>> +	struct session_options_iterator *it =
>>> +		(session_options_iterator *)malloc(sizeof(*it));
>>> +	if (it == NULL) {
>>
>> decoded_key leaks here. But heap OOM is such an unlikely event,
>> and is forgotten for be checked so often, that perhaps we should
>> reconsider moving this https://github.com/tarantool/tarantool/issues/3534
>> out of wish list and start using panic in new places. Probably
>> wrapped into a 'heap_alloc()' function (name is discussable).
>>
>> I think, Kirill won't be against this.
> Fixed. I asked about issue #3534 in a scrum, but haven’t
> received a definite answer yet. I will ask about it again,
> but a little later, since now we have to prepare for
> release 2.3.1. Also, it looks like we should replace all
> malloc() checks in one commit.

No, we should not. I mean, we can, but it is not necessary.
We just could stop using malloc() directly in new code. And
would drop it from the old code eventually, alongside with
other changes.

The patchset LGTM. Technically.

Re: [Tarantool-patches] [PATCH v4 0/2] Introduce _vsession_settings sysview

[Vladislav Shpilevoy]

Thanks for the patch! LGTM. We need a second
opinion from Nikita.

On 28/11/2019 09:46, imeevma@tarantool.org wrote:
> This patch-set introduces _vsession_settings system view. This
> sysview contains names and current values of the session settings.
> 
> https://github.com/tarantool/tarantool/issues/4511
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4511-system-view-for-session-settings
> 
> Mergen Imeev (2):
>   sysview: make get() and create_iterator() methods virtual
>   box: introduce _vsession_settings sysview
> 
>  src/box/bootstrap.snap             | Bin 5944 -> 5982 bytes
>  src/box/lua/space.cc               |   2 +
>  src/box/lua/upgrade.lua            |  23 ++++++
>  src/box/schema_def.h               |   2 +
>  src/box/session.cc                 | 132 ++++++++++++++++++++++++++++++++++
>  src/box/session.h                  |  13 ++++
>  src/box/sql.h                      |  36 ++++++++++
>  src/box/sql/build.c                | 140 +++++++++++++++++++++++++++++++++++++
>  src/box/sysview.c                  |  48 +++++++++++--
>  test/app-tap/tarantoolctl.test.lua |   4 +-
>  test/box-py/bootstrap.result       |   5 +-
>  test/box/access_sysview.result     | 124 +++++++++++++++++++++++++++++++-
>  test/box/access_sysview.test.lua   |  54 ++++++++++++++
>  test/box/alter.result              |   5 +-
>  test/wal_off/alter.result          |   2 +-
>  15 files changed, 574 insertions(+), 16 deletions(-)
> 

[Tarantool-patches] [PATCH v4 0/2] Introduce _vsession_settings sysview

[imeevma]

This patch-set introduces _vsession_settings system view. This
sysview contains names and current values of the session settings.

https://github.com/tarantool/tarantool/issues/4511
https://github.com/tarantool/tarantool/tree/imeevma/gh-4511-system-view-for-session-settings

Mergen Imeev (2):
  sysview: make get() and create_iterator() methods virtual
  box: introduce _vsession_settings sysview

 src/box/bootstrap.snap             | Bin 5944 -> 5982 bytes
 src/box/lua/space.cc               |   2 +
 src/box/lua/upgrade.lua            |  23 ++++++
 src/box/schema_def.h               |   2 +
 src/box/session.cc                 | 132 ++++++++++++++++++++++++++++++++++
 src/box/session.h                  |  13 ++++
 src/box/sql.h                      |  36 ++++++++++
 src/box/sql/build.c                | 140 +++++++++++++++++++++++++++++++++++++
 src/box/sysview.c                  |  48 +++++++++++--
 test/app-tap/tarantoolctl.test.lua |   4 +-
 test/box-py/bootstrap.result       |   5 +-
 test/box/access_sysview.result     | 124 +++++++++++++++++++++++++++++++-
 test/box/access_sysview.test.lua   |  54 ++++++++++++++
 test/box/alter.result              |   5 +-
 test/wal_off/alter.result          |   2 +-
 15 files changed, 574 insertions(+), 16 deletions(-)

-- 
2.7.4