[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