From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 89E7821A39 for ; Tue, 26 Jun 2018 08:22:26 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Vsh2G_plqWdr for ; Tue, 26 Jun 2018 08:22:26 -0400 (EDT) Received: from smtp15.mail.ru (smtp15.mail.ru [94.100.176.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 493BB21898 for ; Tue, 26 Jun 2018 08:22:26 -0400 (EDT) From: Kirill Shcherbatov Subject: [tarantool-patches] Re: [PATCH v1 1/2] sql: introduce pragma sql_default_engine References: <869f48ed-cf99-7e0a-f89b-8431dee2f72c@tarantool.org> Message-ID: <485804b9-8659-17a1-aaa9-cebc1d0e2893@tarantool.org> Date: Tue, 26 Jun 2018 15:22:23 +0300 MIME-Version: 1.0 In-Reply-To: <869f48ed-cf99-7e0a-f89b-8431dee2f72c@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy > 1. Please, declare engine enums in schema_def.h. If we want to allow > different engines for different languages, we need to see this enum > from non-sql files. And here please assign not to 0, but explicitly > memtx engine enum. You are allowed to assign enum values to int > variables. +++ b/src/box/schema_def.h @@ -243,6 +243,12 @@ enum schema_object_type { schema_object_type_MAX = 8 }; +/** SQL Storage engine. */ +enum sql_storage_engine_type { + SQL_STORAGE_ENGINE_MEMTX = 0, + SQL_STORAGE_ENGINE_VINYL = 1, +}; > 2. Do you really need this function to use in a single place in a > single source file? Yep. > 3. Please, do not use sqlite3_ functions when possible. Why > can not you use strcasecmp here? > 4. You do not need compare lengths. 'engine_name' variable is > zero-terminated already. And you use this fact several lines above. > 5. Bad indentation. > 6. Bad indentation. +++ b/src/box/sql/pragma.c @@ -235,6 +235,29 @@ printActivePragmas(struct session *user_session) } } +/** + * Set tarantool backend default engine for SQL interface. + * @param engine_name to set default. + * @retval -1 on error. + * @retval 0 on success. + */ +static int +sql_default_engine_set(const char *engine_name) +{ + enum sql_storage_engine_type engine; + if (strcasecmp(engine_name, "memtx") == 0) { + engine = SQL_STORAGE_ENGINE_MEMTX; + } else if (strcasecmp(engine_name, "vinyl") == 0) { + engine = SQL_STORAGE_ENGINE_VINYL; + } else { + diag_set(ClientError, ER_NO_SUCH_ENGINE, engine_name); + return -1; + } + current_session()->sql_default_engine = engine; + return 0; +} > 7. On language switch it should be reset, if we do not want to > affect lua. Don't know, what should I do. > 8. Please, remove '_t'. We do not use it for enums. ok > > 9. Please, add a formal comment. I know, it is obvious here, > and I have tried to skip them, but Kostja again asked to write > the comments. ok. > 10. Please, make this function be static inline in pragma.c. ok. > 11. Add a test with correct engines as well. +test:do_catchsql_test( + "pragma-2.2", + [[ + pragma sql_default_engine='vinyl'; + ]], { + 0 +}) + +test:do_catchsql_test( + "pragma-2.3", + [[ + pragma sql_default_engine='memtx'; + ]], { + 0 +})