[Tarantool-patches] [PATCH v2 4/5] iproto: Add session settings for IPROTO
lvasiliev
lvasiliev at tarantool.org
Wed Apr 15 12:26:25 MSK 2020
Hi! Thanks for the review.
On 14.04.2020 4:12, Vladislav Shpilevoy wrote:
> 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.
This was discussed in TPT chat with Mons and Nazarov, and after that
with you, Turenko, Mons ... in zoom. I was orienteted by the net.box
session and I might not know something. Where do you suggest storing the
setting?
>
>> 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.
ok
>
>> 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.
I don't mind.
>
>> + 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.
Sorry, I think this should not work until the release.
>
>> + 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.
It's network hop only if option will be set.As optimization it can be
included to auth packet in future.
>
> 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.
>
I applied your patch and added you to Co-authored.
> 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