From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A925B4696C3 for ; Wed, 15 Apr 2020 12:26:26 +0300 (MSK) References: <4d6b0c27784a8abe7573baa3c859a3ebca07f1fb.1586505741.git.lvasiliev@tarantool.org> From: lvasiliev Message-ID: Date: Wed, 15 Apr 2020 12:26:25 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 4/5] iproto: Add session settings for IPROTO List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org 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 ``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 > + * 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] >