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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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