[Tarantool-patches] [PATCH v4 1/3] box: replace session_settings modules with a single array
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Feb 4 01:17:20 MSK 2020
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 at 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];
More information about the Tarantool-patches
mailing list