From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 0D2124696C5 for ; Tue, 14 Apr 2020 04:12:09 +0300 (MSK) References: <4d6b0c27784a8abe7573baa3c859a3ebca07f1fb.1586505741.git.lvasiliev@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Tue, 14 Apr 2020 03:12:06 +0200 MIME-Version: 1.0 In-Reply-To: <4d6b0c27784a8abe7573baa3c859a3ebca07f1fb.1586505741.git.lvasiliev@tarantool.org> Content-Type: text/plain; charset=utf-8 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: Leonid Vasiliev Cc: tarantool-patches@dev.tarantool.org 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 ``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]