[Tarantool-patches] [PATCH v2 4/5] iproto: Add session settings for IPROTO
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Apr 14 04:12:06 MSK 2020
Thanks for the patch!
Sorry, some of my comments may contain typos, because it is
very late and I can't continue polishing it today.
See 5 comments below, review fixes in the end of the email,
and on a new branch in a separate commit.
On 10/04/2020 10:10, Leonid Vasiliev wrote:
> IPROTO session settings for error transmission in various formats
> depending on session settings have been added.
> This is required for backward compatibility.
What backward compatibility? Keep in mind, that the team consists not
only of you, me, and Alexander T. Other people don't know what a
backward compatibility is being tried to be preserved here. Just
'backward compatibility' does not tell anything at all.
> @TarantoolBot document
> Title: Add session_setting
The doc request is corrupted, docbot won't understand that because
of leading whitespaces.
> iproto_error_format setting has been added to _session_settings
Looks like there is a lack of global setting similar to what we had
for tracebacks. Currently, when the option is false (by default), and
I want to use the new format everywhere, I need to find every single
place where I create a new session, and put there code saying
box.session.settings.error_format = new/old/whatever
I think there should be a global option when a user's application is
ready to switch to the new format completely. Otherwise it is going
to be hell to find all places where a new session is created, and patch
them.
Just a reminder - every fiber.new(), fiber.create() creates a session,
every iproto connection is a session.
> Used to set the error transmission format in the session.
> Old format: transmits as string at IPROTO_BODY
> New format: transmits as error object at IPROTO_BODY
>
> Needed for #4398
> ---
> src/box/iproto.cc | 97 ++++++++++++++++++++++++++++++++++++++++++++++
> src/box/lua/net_box.lua | 12 ++++++
> src/box/session.cc | 3 ++
> src/box/session.h | 3 ++
> src/box/session_settings.h | 1 +
> src/lua/utils.h | 20 ++++++++++
> 6 files changed, 136 insertions(+)
>
> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index 9dad43b..92be645 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -2183,6 +2184,100 @@ iproto_session_push(struct session *session, uint64_t sync, struct port *port)
>
> /** }}} */
>
> +enum {
> + IPROTO_SESSION_SETTING_ERR_FORMAT = 0,
> + iproto_session_setting_MAX,
> +};
1. The new setting is not about iproto only. It is about
error serialization to MessagePack. MessagePack != IProto.
So iproto.cc is not a good place for the new setting.
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 1e0cd7a..c8f76b0 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -1047,6 +1047,18 @@ local function new_sm(host, port, opts, connection, greeting)
> if opts.wait_connected ~= false then
> remote._transport.wait_state('active', tonumber(opts.wait_connected))
> end
> +
> + -- Set extended error format for session.
> + if opts.error_extended then
2. You are inconsistent in what an option do you want. In _session_setting
you call it 'iproto_error_format' assuming it is a format. In practice it
is a number, which, I assume, means version number. In the public API it
is called 'error_extended' assuming it is a flag - either true or false.
So what is it?
In my review fixes I made it a flag. Because there is no way we will support
multiple 'degrees' of error extension.
> + local ext_err_supported = version_at_least(remote.peer_version_id, 2, 4, 1)
3. This won't work in case it is an instance bootstrapped from the master
branch before 2.4.1 was released. I don't know how to fix it now.
> + if not ext_err_supported then
> + box.error(box.error.PROC_LUA,
> + "Server doesn't support extended error format")
> + end
> + remote.space._session_settings:update('iproto_error_format',
> + {{'=', 2, 1}})
4. This is additional network hop. I don't think it should be done
automatically. I would let users do that.
Besides, you didn't document that option, so how were they supposed
to know about it?
> + end
> +
> return remote
> end
>
> diff --git a/src/box/session.h b/src/box/session.h
> index 6dfc7cb..a8903aa 100644
> --- a/src/box/session.h
> +++ b/src/box/session.h
> @@ -36,6 +36,7 @@
> #include "fiber.h"
> #include "user.h"
> #include "authentication.h"
> +#include "lua/utils.h"
5. This is clearly broken dependency. lua/utils.h is from src/lua/,
and it obviously depends on Lua language.
Session is from src/box/. So it is from different hierarchy, and
is not related to any language.
Consider my review fixes below and on a new branch in a
separate commit.
Branch is:
lvasiliev/gh-4398-expose-error-module-4-review
====================
Review fixes
New commit message proposal:
error: add session setting for error type marshaling
Errors are encoded as a string when serialized to MessagePack to
be sent over IProto or when just saved into a buffer via Lua
modules msgpackffi and msgpack.
That is not very useful on client-side, because most of the error
metadata is lost: code, type, trace - everything except the
message.
Next commits are going to dedicate a new MP_EXT type to error
objects so as everything could be encoded, and on client side it
would be possible to restore types.
But this is a breaking change in case some users use old
connectors when work with newer Tarantool instances. So to smooth
the upgrade there is a new session setting -
'error_marshaling_enabled'.
By default it is false. When it is true, all fibers of the given
session will serialize error objects as MP_EXT.
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 92be645d2..9dad43b0b 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -55,7 +55,6 @@
#include "call.h"
#include "tuple_convert.h"
#include "session.h"
-#include "session_settings.h"
#include "xrow.h"
#include "schema.h" /* schema_version */
#include "replication.h" /* instance_uuid */
@@ -2184,100 +2183,6 @@ iproto_session_push(struct session *session, uint64_t sync, struct port *port)
/** }}} */
-enum {
- IPROTO_SESSION_SETTING_ERR_FORMAT = 0,
- iproto_session_setting_MAX,
-};
-
-static const char *iproto_session_setting_strs[iproto_session_setting_MAX] = {
- "iproto_error_format",
-};
-
-static int iproto_session_field_type[] = {
- /** IPROTO_SESSION_SETTING_ERR_FORMAT */
- FIELD_TYPE_UNSIGNED,
-};
-
-static void
-iproto_session_setting_get(int id, const char **mp_pair,
- const char **mp_pair_end)
-{
- if (id < 0 || id >= iproto_session_setting_MAX) {
- diag_set(ClientError, ER_ILLEGAL_PARAMS,
- "unknown session setting");
- return;
- }
- struct session *session = current_session();
-
- const char *name = iproto_session_setting_strs[id];
- size_t name_len = strlen(name);
-
- /* Now we have only one iproto session setting. */
- size_t size = mp_sizeof_array(2) + mp_sizeof_str(name_len)
- + mp_sizeof_uint(session->serializer_ctx.err_format_ver);
-
- char *pos = (char*)static_alloc(size);
- assert(pos != NULL);
- char *pos_end = mp_encode_array(pos, 2);
- pos_end = mp_encode_str(pos_end, name, name_len);
- pos_end = mp_encode_uint(pos_end,
- session->serializer_ctx.err_format_ver);
- *mp_pair = pos;
- *mp_pair_end = pos_end;
-}
-
-static int
-iproto_session_setting_set(int id, const char *mp_value)
-{
- if (id < 0 || id >= iproto_session_setting_MAX) {
- diag_set(ClientError, ER_ILLEGAL_PARAMS,
- "unknown session setting");
- return -1;
- }
- /*Current IPROTO session settings are used only for BINARY session */
- if (current_session()->type != SESSION_TYPE_BINARY)
- return -1;
-
- enum mp_type mtype = mp_typeof(*mp_value);
- int stype = iproto_session_field_type[id];
- switch(stype) {
- case FIELD_TYPE_UNSIGNED: {
- if (mtype != MP_UINT)
- break;
- int val = mp_decode_uint(&mp_value);
- switch (id) {
- case IPROTO_SESSION_SETTING_ERR_FORMAT:
- if (val >= ERR_FORMAT_UNK)
- break;
- current_session()->serializer_ctx.err_format_ver = val;
- return 0;
- default:
- diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
- iproto_session_setting_strs[id],
- field_type_strs[stype]);
- return -1;
- }
- break;
- }
- default:
- unreachable();
- }
- diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
- iproto_session_setting_strs[id], field_type_strs[stype]);
- return -1;
-}
-
-void
-iproto_session_settings_init()
-{
- struct session_setting_module *module =
- &session_setting_modules[SESSION_SETTING_IPROTO];
- module->settings = iproto_session_setting_strs;
- module->setting_count = iproto_session_setting_MAX;
- module->get = iproto_session_setting_get;
- module->set = iproto_session_setting_set;
-}
-
/** Initialize the iproto subsystem and start network io thread */
void
iproto_init()
@@ -2296,8 +2201,6 @@ iproto_init()
/* .sync = */ iproto_session_sync,
};
session_vtab_registry[SESSION_TYPE_BINARY] = iproto_session_vtab;
-
- iproto_session_settings_init();
}
/** Available iproto configuration changes. */
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index a1e9ee745..5c07bb07c 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -1042,18 +1042,6 @@ local function new_sm(host, port, opts, connection, greeting)
if opts.wait_connected ~= false then
remote._transport.wait_state('active', tonumber(opts.wait_connected))
end
-
- -- Set extended error format for session.
- if opts.error_extended then
- local ext_err_supported = version_at_least(remote.peer_version_id, 2, 4, 1)
- if not ext_err_supported then
- box.error(box.error.PROC_LUA,
- "Server doesn't support extended error format")
- end
- remote.space._session_settings:update('iproto_error_format',
- {{'=', 2, 1}})
- end
-
return remote
end
diff --git a/src/box/session.cc b/src/box/session.cc
index 89435ee0c..08a10924a 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -144,9 +144,6 @@ session_create(enum session_type type)
session->sql_default_engine = SQL_STORAGE_ENGINE_MEMTX;
session->sql_stmts = NULL;
- /* Set default Lua serializer context */
- session->serializer_ctx.err_format_ver = ERR_FORMAT_DEF;
-
/* For on_connect triggers. */
credentials_create(&session->credentials, guest_user);
struct mh_i64ptr_node_t node;
@@ -278,6 +275,9 @@ session_find(uint64_t sid)
mh_i64ptr_node(session_registry, k)->val;
}
+extern "C" void
+session_settings_init(void);
+
void
session_init()
{
@@ -286,7 +286,7 @@ session_init()
panic("out of memory");
mempool_create(&session_pool, &cord()->slabc, sizeof(struct session));
credentials_create(&admin_credentials, admin_user);
- sql_session_settings_init();
+ session_settings_init();
}
void
diff --git a/src/box/session.h b/src/box/session.h
index fdc9f03d7..500a88b22 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -36,14 +36,12 @@
#include "fiber.h"
#include "user.h"
#include "authentication.h"
-#include "lua/utils.h"
+#include "serializer_opts.h"
#if defined(__cplusplus)
extern "C" {
#endif /* defined(__cplusplus) */
-extern void sql_session_settings_init();
-
struct port;
struct session_vtab;
@@ -91,6 +89,8 @@ struct session_meta {
};
/** Console output format. */
enum output_format output_format;
+ /** Session-specific serialization options. */
+ struct serializer_opts serializer_opts;
};
/**
@@ -123,8 +123,6 @@ struct session {
struct credentials credentials;
/** Trigger for fiber on_stop to cleanup created on-demand session */
struct trigger fiber_on_stop;
- /** Session Lua serializer context */
- struct luaL_serializer_ctx serializer_ctx;
};
struct session_vtab {
diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index 79c4b8d3c..dbbbf2461 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -42,6 +42,7 @@ struct session_setting session_settings[SESSION_SETTING_COUNT] = {};
/** Corresponding names of session settings. */
const char *session_setting_strs[SESSION_SETTING_COUNT] = {
+ "error_marshaling_enabled",
"sql_default_engine",
"sql_defer_foreign_keys",
"sql_full_column_names",
@@ -449,3 +450,58 @@ session_setting_find(const char *name) {
else
return -1;
}
+
+/* Module independent session settings. */
+
+static void
+session_setting_error_marshaling_enabled_get(int id, const char **mp_pair,
+ const char **mp_pair_end)
+{
+ assert(id == SESSION_SETTING_ERROR_MARSHALING_ENABLED);
+ struct session *session = current_session();
+ const char *name = session_setting_strs[id];
+ size_t name_len = strlen(name);
+ bool value = session->meta.serializer_opts.error_marshaling_enabled;
+ size_t size = mp_sizeof_array(2) + mp_sizeof_str(name_len) +
+ mp_sizeof_bool(value);
+
+ char *pos = (char*)static_alloc(size);
+ assert(pos != NULL);
+ char *pos_end = mp_encode_array(pos, 2);
+ pos_end = mp_encode_str(pos_end, name, name_len);
+ pos_end = mp_encode_bool(pos_end, value);
+ *mp_pair = pos;
+ *mp_pair_end = pos_end;
+}
+
+static int
+session_setting_error_marshaling_enabled_set(int id, const char *mp_value)
+{
+ assert(id == SESSION_SETTING_ERROR_MARSHALING_ENABLED);
+ enum mp_type mtype = mp_typeof(*mp_value);
+ enum field_type stype = session_settings[id].field_type;
+ if (mtype != MP_BOOL) {
+ diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+ session_setting_strs[id], field_type_strs[stype]);
+ return -1;
+ }
+ struct session *session = current_session();
+ session->meta.serializer_opts.error_marshaling_enabled =
+ mp_decode_bool(&mp_value);
+ return 0;
+}
+
+extern void
+sql_session_settings_init();
+
+void
+session_settings_init(void)
+{
+ struct session_setting *s =
+ &session_settings[SESSION_SETTING_ERROR_MARSHALING_ENABLED];
+ s->field_type = FIELD_TYPE_BOOLEAN;
+ s->get = session_setting_error_marshaling_enabled_get;
+ s->set = session_setting_error_marshaling_enabled_set;
+
+ sql_session_settings_init();
+}
diff --git a/src/lua/utils.h b/src/lua/utils.h
index cac9c57b8..4bc041796 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -270,26 +270,6 @@ struct luaL_serializer {
struct rlist on_update;
};
-/**
- * An error serialization formats
- */
-enum error_formats {
- /** Default(old) format */
- ERR_FORMAT_DEF,
- /** Extended format */
- ERR_FORMAT_EX,
- /** The max version of error format */
- ERR_FORMAT_UNK
-};
-
-/**
- * A serializer context (additional settings for a serializer)
- */
-struct luaL_serializer_ctx {
- /** Version of a format for an error transmission */
- uint8_t err_format_ver;
-};
-
extern int luaL_nil_ref;
extern int luaL_map_metatable_ref;
extern int luaL_array_metatable_ref;
diff --git a/src/serializer_opts.h b/src/serializer_opts.h
new file mode 100644
index 000000000..9e2c15eff
--- /dev/null
+++ b/src/serializer_opts.h
@@ -0,0 +1,44 @@
+#pragma once
+/*
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ * copyright notice, this list of conditions and the
+ * following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer in the documentation and/or other materials
+ * provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+/**
+ * Serializer options which can regulate how to serialize
+ * something in scope of one session.
+ */
+struct serializer_opts {
+ /**
+ * When enabled, error objects get their own MP_EXT
+ * MessagePack type and therefore can be type-safely
+ * transmitted over the network.
+ */
+ bool error_marshaling_enabled;
+};
diff --git a/test/box/session_settings.result b/test/box/session_settings.result
index ea6302dff..149cc4bd5 100644
--- a/test/box/session_settings.result
+++ b/test/box/session_settings.result
@@ -52,7 +52,8 @@ s:replace({'sql_defer_foreign_keys', true})
--
s:select()
| ---
- | - - ['sql_default_engine', 'memtx']
+ | - - ['error_marshaling_enabled', false]
+ | - ['sql_default_engine', 'memtx']
| - ['sql_defer_foreign_keys', false]
| - ['sql_full_column_names', false]
| - ['sql_full_metadata', false]
More information about the Tarantool-patches
mailing list