[Tarantool-patches] [PATCH 2/3] box: introduce _session_settings system space
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Dec 21 20:59:12 MSK 2019
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. There are lots of fixes, so review carefully. In my
commit you can find inlined explanations about some changes.
Also see 3 comments below inlined into your commit.
During the review I also thought more about how to implement setting
modules - in a single big namespace of all session settings, each
with individual session settings, or like I proposed and you've done
here, with modules approach.
Looks like I was wrong, and the only extendible option is to have a
single namespace, sorry. This is because not all settings can be split
into submodules and not all of them will have the same prefix. For
example, setting 'read_only'. I can't find a good prefix for it.
Additionally, some SQL settings may migrate down to box in future, such
as foreign key- and trigger- related. And they also likely not to have
one prefix.
Nonetheless, I've decided that we can move settings to one namespace
later, as a separate simple ticket. Here we will finish public API.
We can add individual settings in the current solution too, but it will
look like a module with a single setting.
Also we need to optimize search. Currently it is fullscan what is
not acceptable when we have a sorted array. But also, a subject for a
separate issue.
On 19/12/2019 09:32, imeevma at tarantool.org wrote:
> This patch creates _session_settings system space. This space is
> used to view and change session settings. There are no settings at
> the moment, some will be added in the next patch.
>
> Part of #4511
> ---
> src/box/CMakeLists.txt | 1 +
> src/box/bootstrap.snap | Bin 5921 -> 5975 bytes
> src/box/lua/space.cc | 2 +
> src/box/lua/upgrade.lua | 15 +
> src/box/schema_def.h | 8 +
> src/box/session_settings.c | 409 +++++++++++++++++++++
> src/box/session_settings.h | 61 +++
> src/box/virtual_engine.c | 52 ++-
> test/app-tap/tarantoolctl.test.lua | 4 +-
> test/box-py/bootstrap.result | 3 +
> test/box/access_sysview.result | 6 +-
> test/box/alter.result | 5 +-
> ...h-4511-access-settings-from-any-frontend.result | 108 +++++-
> ...4511-access-settings-from-any-frontend.test.lua | 37 ++
> test/wal_off/alter.result | 2 +-
> 15 files changed, 697 insertions(+), 16 deletions(-)
> create mode 100644 src/box/session_settings.c
> create mode 100644 src/box/session_settings.h
>
> diff --git a/src/box/session_settings.c b/src/box/session_settings.c
> new file mode 100644
> index 0000000..b5b5db8
> --- /dev/null
> +++ b/src/box/session_settings.c
> @@ -0,0 +1,409 @@
> +
> +static int
> +session_settings_iterator_next(struct iterator *iterator, struct tuple **result)
> +{
> + struct session_settings_iterator *it =
> + (struct session_settings_iterator *)iterator;
> + if (it->module_id >= SESSION_SETTING_MODULE_max || it->module_id < 0) {
> + *result = NULL;
> + return 0;
> + }
> +
> + struct tuple *ret = NULL;
> + int i = it->module_id;
> + int j = it->current_id;
> + enum iterator_type type = it->iterator_type;
> + struct tuple_format *format = it->format;
> + struct session_settings_modules *module;
> + const char *key = it->key;
> +
> + if (!iterator_type_is_reverse(type)) {
1. That condition never changes after iteration creation. You could
reuse virtuality of iterator.next to calculate that condition only
once. For that I've split that function in two: next and prev
separately.
> + for (; i < SESSION_SETTING_MODULE_max; ++i) {
> + module = &session_settings_modules[i];
> + if (module->get(format, j, key, type, &j, &ret) != 0)
> + return -1;
> + if (ret != NULL)
> + break;
> + j = 0;
> + }
> + *result = ret;
> + if (i == SESSION_SETTING_MODULE_max) {
> + it->module_id = SESSION_SETTING_MODULE_max;
> + it->current_id = 0;
> + return 0;
> + }
> + ++j;
> + if (j >= (int)module->size) {
> + ++i;
> + j = 0;
> + }
> + it->module_id = i;
> + it->current_id = j;
> + return 0;
> + }
> +
> + for (; i >= 0; --i) {
> + module = &session_settings_modules[i];
> + if (module->get(format, j, key, type, &j, &ret) != 0)
> + return -1;
> + if (ret != NULL)
> + break;
> + if (i > 0)
> + j = session_settings_modules[i - 1].size - 1;
> + }
> + *result = ret;
> + if (i < 0) {
> + it->module_id = -1;
> + it->current_id = 0;
> + return 0;
> + }
> + --j;
> + if (j < 0) {
> + --i;
> + if (i >= 0)
> + j = module[i].size - 1;
> + }
> + it->module_id = i;
> + it->current_id = j;
> + return 0;
> +}
> diff --git a/src/box/session_settings.h b/src/box/session_settings.h
> new file mode 100644
> index 0000000..e7ecbdb
> --- /dev/null
> +++ b/src/box/session_settings.h
> @@ -0,0 +1,61 @@
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
> +
> +enum session_setting_module {
> + SESSION_SETTING_MODULE_max
2. Usually we do vice versa - lowercase enum names,
and uppercase _MAX. That makes possible to use things
such as STR2ENUM.
> +};
> +
> +struct tuple;
> +struct tuple_format;
> +
> +struct session_settings_modules {
> + enum session_setting_module id;
> + uint32_t size;
> + int (*get)(struct tuple_format *format, int id, const char *key,
> + enum iterator_type type, int *end_id, struct tuple **result);
> + int (*set)(struct tuple_format *format, int id, const char *value,
> + struct tuple **result);
3. The thing I mostly didn't like here is that you force submodule
to deal with tuple format, to create tuples, to do search by key.
That is a space's and index's task, not of SQL or other submodule.
Moreover, that logic would be duplicated in each submodule.
I simplified this API so now the session_settings space internals do
all the search, comparisons, tuple creation, etc. And submodules only
provide setting name list, and API to get/set a setting by ID.
> +};
> +
> +extern struct session_settings_modules session_settings_modules[];
> +extern const struct space_vtab session_settings_space_vtab;
> +
My commit with explanations:
==================================================================
commit 38d1392d20ee90f4bf5418e2402dd5ec589cfb1d
Author: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
Date: Sat Dec 21 18:04:14 2019 +0100
Review fixes 2/3
diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index b5b5db81e..dd1874b33 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -31,35 +31,49 @@
#include "session_settings.h"
#include "xrow_update.h"
#include "virtual_engine.h"
+#include "column_mask.h"
#include "session.h"
#include "schema.h"
#include "tuple.h"
#include "xrow.h"
#include "sql.h"
-struct session_settings_modules
- session_settings_modules[SESSION_SETTING_MODULE_max] = {};
+struct session_setting_module
+ session_setting_modules[session_setting_type_MAX] = {};
struct session_settings_index {
/** Base index. Must be the first member. */
struct index base;
- /** Format to create tuples on the fly. */
+ /**
+ * Format of the tuples iterators of this index return. It
+ * is stored here so as not to lookup space each time to
+ * get a format and create an iterator.
+ */
struct tuple_format *format;
};
struct session_settings_iterator {
/** Base iterator. Must be the first member. */
struct iterator base;
- /** Format of the tuples this iterator returns. */
+ /**
+ * Format of the tuples this iterator returns. It is
+ * stored here so as not to lookup space each time to get
+ * a format for selected tuples.
+ */
struct tuple_format *format;
- /** ID of current module in global list of the modules. */
+ /**
+ * ID of the current session settings module in the global
+ * list of the modules.
+ */
int module_id;
/** ID of the setting in current module. */
- int current_id;
+ int setting_id;
/** Decoded key. */
char *key;
- /** Type of iterator. */
- enum iterator_type iterator_type;
+ /** True if the iterator returns only equal keys. */
+ bool is_eq;
+ /** True if the iterator should include equal keys. */
+ bool is_including;
==================================================================
As you can see, I removed iterator type in favor of a couple of
flags. I've found it easier to use these flags in the code than
compare the type with all the known types each time.
==================================================================
};
static void
@@ -72,72 +86,114 @@ session_settings_iterator_free(struct iterator *ptr)
}
static int
-session_settings_iterator_next(struct iterator *iterator, struct tuple **result)
+session_settings_next_in_module(const struct session_setting_module *module,
+ int *sid, const char *key, bool is_eq,
+ bool is_including)
==================================================================
I've moved search by name to the session_settings space. Now
submodules shouldn't care about it. And this is reused by all
modules (even though now there is only one)
==================================================================
{
- struct session_settings_iterator *it =
- (struct session_settings_iterator *)iterator;
- if (it->module_id >= SESSION_SETTING_MODULE_max || it->module_id < 0) {
- *result = NULL;
+ int i = *sid;
+ int count = module->setting_count;
+ if (i >= 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);
+ if ((cmp == 0 && is_including) ||
+ (cmp > 0 && !is_eq)) {
+ *sid = i;
+ return 0;
+ }
}
+ *sid = count;
+ return -1;
+}
- struct tuple *ret = NULL;
- int i = it->module_id;
- int j = it->current_id;
- enum iterator_type type = it->iterator_type;
- struct tuple_format *format = it->format;
- struct session_settings_modules *module;
- const char *key = it->key;
-
- if (!iterator_type_is_reverse(type)) {
- for (; i < SESSION_SETTING_MODULE_max; ++i) {
- module = &session_settings_modules[i];
- if (module->get(format, j, key, type, &j, &ret) != 0)
- return -1;
- if (ret != NULL)
- break;
- j = 0;
- }
- *result = ret;
- if (i == SESSION_SETTING_MODULE_max) {
- it->module_id = SESSION_SETTING_MODULE_max;
- it->current_id = 0;
+static int
+session_settings_prev_in_module(const struct session_setting_module *module,
+ 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 ((cmp == 0 && is_including) ||
+ (cmp < 0 && !is_eq)) {
+ *sid = i;
return 0;
}
- ++j;
- if (j >= (int)module->size) {
- ++i;
- j = 0;
+ }
+ *sid = -1;
+ return -1;
+}
+
+static int
+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;
+ 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 = i;
- it->current_id = j;
+ }
+ it->module_id = mid;
+ 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);
+ *result = box_tuple_new(it->format, mp_pair, mp_pair_end);
==================================================================
Note, I use box_tuple_new(), because iterator API assumes that a
tuple is referenced somewhere, and that box can just bless a tuple
to the userland. Simple tuple_new() does not work here, because it
won't be deleted if anything will go wrong after its creation and
before tuple_bless() in box.
==================================================================
+ return *result != NULL ? 0 : -1;
+}
- for (; i >= 0; --i) {
- module = &session_settings_modules[i];
- if (module->get(format, j, key, type, &j, &ret) != 0)
- return -1;
- if (ret != NULL)
+static int
+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;
+ 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;
- if (i > 0)
- j = session_settings_modules[i - 1].size - 1;
+ }
}
- *result = ret;
- if (i < 0) {
- it->module_id = -1;
- it->current_id = 0;
+ it->module_id = mid;
+ it->setting_id = sid - 1;
+ if (!is_found) {
+ *result = NULL;
return 0;
}
- --j;
- if (j < 0) {
- --i;
- if (i >= 0)
- j = module[i].size - 1;
- }
- it->module_id = i;
- it->current_id = j;
- return 0;
+ const char *mp_pair, *mp_pair_end;
+ module->get(sid, &mp_pair, &mp_pair_end);
+ *result = box_tuple_new(it->format, mp_pair, mp_pair_end);
+ return *result != NULL ? 0 : -1;
}
static void
@@ -175,15 +231,23 @@ session_settings_index_create_iterator(struct index *base,
return NULL;
}
iterator_create(&it->base, base);
- it->base.next = session_settings_iterator_next;
it->base.free = session_settings_iterator_free;
it->key = decoded_key;
- it->iterator_type = type;
+ 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->format = index->format;
- it->module_id = iterator_type_is_reverse(type) ?
- SESSION_SETTING_MODULE_max - 1 : 0;
- it->current_id = iterator_type_is_reverse(type) ?
- session_settings_modules[it->module_id].size - 1 : 0;
+ 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;
+ }
return (struct iterator *)it;
}
@@ -192,23 +256,28 @@ session_settings_index_get(struct index *base, const char *key,
uint32_t part_count, struct tuple **result)
{
struct session_settings_index *index =
- (struct session_settings_index *)base;
+ (struct session_settings_index *) base;
assert(part_count == 1);
- (void)part_count;
+ (void) part_count;
uint32_t len;
- const char *tmp = mp_decode_str(&key, &len);
- const char *decoded_key = tt_cstr(tmp, len);
- struct tuple *ret = NULL;
- struct tuple_format *format = index->format;
- for (int i = 0; i < SESSION_SETTING_MODULE_max; ++i) {
- if (session_settings_modules[i].get(format, 0, decoded_key,
- ITER_EQ, NULL, &ret) != 0)
- return -1;
- if (ret != NULL)
- break;
+ 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;
}
- *result = ret;
+ *result = NULL;
return 0;
+found:;
+ const char *mp_pair;
+ const char *mp_pair_end;
+ module->get(sid, &mp_pair, &mp_pair_end);
+ *result = box_tuple_new(index->format, mp_pair, mp_pair_end);
+ return *result != NULL ? 0 : -1;
}
static const struct index_vtab session_settings_index_vtab = {
@@ -281,70 +350,64 @@ session_settings_space_execute_update(struct space *space, struct txn *txn,
struct tuple **result)
{
(void)txn;
- const char *data = request->key;
- uint32_t key_len = mp_decode_array(&data);
+ struct tuple_format *format = space->format;
+ const char *old_data, *old_data_end, *new_data;
+ struct region *region = &fiber()->gc;
+ size_t used = region_used(region);
+ int rc = -1, sid = 0;
+ struct index_def *pk_def = space->index[0]->def;
+ uint64_t column_mask;
+
+ const char *new_key, *key = request->key;
+ uint32_t new_size, new_key_len, key_len = mp_decode_array(&key);
if (key_len == 0) {
diag_set(ClientError, ER_EXACT_MATCH, 1, 0);
return -1;
}
- if (key_len > 1 || mp_typeof(*data) != MP_STR) {
+ if (key_len > 1 || mp_typeof(*key) != MP_STR) {
diag_set(ClientError, ER_KEY_PART_TYPE, 0, "string");
return -1;
}
- uint32_t len;
- const char *tmp = mp_decode_str(&data, &len);
- const char *decoded_key = tt_cstr(tmp, len);
-
- int id;
- struct tuple *old_tuple = NULL;
- struct tuple_format *format = space->format;
- struct session_settings_modules *module = NULL;
- for (int i = 0; i < SESSION_SETTING_MODULE_max; ++i) {
- module = &session_settings_modules[i];
- if (module->get(format, 0, decoded_key, ITER_EQ, &id,
- &old_tuple) != 0)
- return -1;
- if (old_tuple != NULL)
- break;
+ 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 (old_tuple == NULL)
- return 0;
-
- uint32_t new_size = 0, bsize;
- const char *old_data = tuple_data_range(old_tuple, &bsize);
- const char *new_data =
- xrow_update_execute(request->tuple,request->tuple_end,
- old_data, old_data + bsize, format->dict,
- &new_size, request->index_base, NULL);
+ *result = NULL;
+ return 0;
+found:
+ module->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,
+ &column_mask);
if (new_data == NULL)
- return -1;
+ goto finish;
+ *result = box_tuple_new(format, new_data, new_data + new_size);
+ if (*result == NULL)
+ goto finish;
- uint32_t array_len = mp_decode_array(&new_data);
- if (array_len != 2)
- if (format->exact_field_count != array_len) {
- diag_set(ClientError, ER_EXACT_FIELD_COUNT,
- array_len, format->exact_field_count);
- return -1;
- }
-
- if (mp_typeof(*new_data) != MP_STR) {
- diag_set(ClientError, ER_CANT_UPDATE_PRIMARY_KEY,
- space_index(space, 0)->def->name, space_name(space));
- return -1;
- }
-
- const char *value = new_data;
- mp_next(&value);
- mp_decode_array(&old_data);
- uint32_t size_a = mp_sizeof_str(len);
- uint32_t size_b = value - new_data;
- if (size_a != size_b || memcmp(old_data, new_data, size_a) != 0) {
- diag_set(ClientError, ER_CANT_UPDATE_PRIMARY_KEY,
- space_index(space, 0)->def->name, space_name(space));
- return -1;
+ mp_decode_array(&new_data);
+ new_key = mp_decode_str(&new_data, &new_key_len);
+ if (!key_update_can_be_skipped(pk_def->key_def->column_mask,
+ column_mask)) {
+ if (key_len != new_key_len ||
+ memcmp(key, new_key, key_len) != 0) {
+ diag_set(ClientError, ER_CANT_UPDATE_PRIMARY_KEY,
+ pk_def->name, space_name(space));
+ goto finish;
+ }
}
-
- return module->set(format, id, value, result);
+ if (module->set(sid, new_data) != 0)
+ goto finish;
+ rc = 0;
+finish:
+ region_truncate(region, used);
+ return rc;
}
static int
diff --git a/src/box/session_settings.h b/src/box/session_settings.h
index e7ecbdb08..7415e0eea 100644
--- a/src/box/session_settings.h
+++ b/src/box/session_settings.h
@@ -29,33 +29,41 @@
* THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*/
-#include <stddef.h>
-#include <stdint.h>
-#include "iterator_type.h"
-#if defined(__cplusplus)
-extern "C" {
-#endif /* defined(__cplusplus) */
-
-enum session_setting_module {
- SESSION_SETTING_MODULE_max
+/**
+ * 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.
+ *
+ * The types should be ordered in alphabetical order, because the
+ * type list is used by setting iterators.
+ */
+enum session_setting_type {
+ session_setting_type_MAX,
};
-struct tuple;
-struct tuple_format;
-
-struct session_settings_modules {
- enum session_setting_module id;
- uint32_t size;
- int (*get)(struct tuple_format *format, int id, const char *key,
- enum iterator_type type, int *end_id, struct tuple **result);
- int (*set)(struct tuple_format *format, int id, const char *value,
- struct tuple **result);
+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;
+ /**
+ * Get a MessagePack encoded pair [name, value] for a
+ * setting having index @a id. Index is from the settings
+ * array.
+ */
+ void (*get)(int id, const char **mp_pair, const char **mp_pair_end);
+ /**
+ * Set value of a setting by a given @a id from a
+ * MessagePack encoded buffer. Note, it is not a pair, but
+ * just value.
+ */
+ int (*set)(int id, const char *mp_value);
};
-extern struct session_settings_modules session_settings_modules[];
-extern const struct space_vtab session_settings_space_vtab;
-
-#if defined(__cplusplus)
-} /* extern "C" */
-#endif /* defined(__plusplus) */
+extern struct session_setting_module session_setting_modules[];
diff --git a/src/box/virtual_engine.c b/src/box/virtual_engine.c
index 5ec8015c7..36eec8323 100644
--- a/src/box/virtual_engine.c
+++ b/src/box/virtual_engine.c
@@ -28,10 +28,12 @@
* THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*/
-#include "session_settings.h"
#include "virtual_engine.h"
+#include "tuple.h"
#include "schema.h"
+extern const struct space_vtab session_settings_space_vtab;
+
static void
virtual_engine_shutdown(struct engine *engine)
{
More information about the Tarantool-patches
mailing list