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 6365D272F9 for ; Fri, 22 Jun 2018 16:04:05 -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 aln7UszdB5Ma for ; Fri, 22 Jun 2018 16:04:05 -0400 (EDT) Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 C61512720A for ; Fri, 22 Jun 2018 16:04:04 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/2] sql: introduce pragma sql_default_engine References: From: Vladislav Shpilevoy Message-ID: <869f48ed-cf99-7e0a-f89b-8431dee2f72c@tarantool.org> Date: Fri, 22 Jun 2018 23:04:02 +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 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, Kirill Shcherbatov Thanks for the patch! See 11 comments below. On 20/06/2018 20:06, Kirill Shcherbatov wrote: > Part of #2199. > > @TarantoolBot document > Title: new pragma sql_default_engine > Now it is allowed to create vinyl spaces using special pragma > setting default engine for SQL requests. > Example: > \set language sql > pragma sql_default_engine='vinyl'; > CREATE TABLE t3(a primary key,b,c); > --- > src/box/session.cc | 1 + > src/box/session.h | 2 ++ > src/box/sql/build.c | 43 +++++++++++++++++++++++++++++++++++- > src/box/sql/pragma.c | 8 +++++++ > src/box/sql/pragma.h | 6 +++++ > src/box/sql/sqliteInt.h | 14 ++++++++++++ > test/sql-tap/gh-2367-pragma.test.lua | 14 +++++++++++- > 7 files changed, 86 insertions(+), 2 deletions(-) > > diff --git a/src/box/session.cc b/src/box/session.cc > index e487280..810dd19 100644 > --- a/src/box/session.cc > +++ b/src/box/session.cc > @@ -107,6 +107,7 @@ session_create(enum session_type type) > memset(&session->meta, 0, sizeof(session->meta)); > session->type = type; > session->sql_flags = default_flags; > + session->sql_default_engine = 0; 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. > > /* For on_connect triggers. */ > credentials_init(&session->credentials, guest_user->auth_token, > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index fff7c19..bcd4d31 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -493,6 +493,43 @@ sqlite3PrimaryKeyIndex(Table * pTab) > } > > /** > + * Get default engine name set in current_session. > + * @retval engine name string. > + */ > +static const char * > +sql_default_engine_name(void) > +{ > + switch (current_session()->sql_default_engine) { > + case SQL_STORAGE_ENGINE_MEMTX: > + return "memtx"; > + case SQL_STORAGE_ENGINE_VINYL: > + return "vinyl"; > + default: > + unreachable(); > + } > +} 2. Do you really need this function to use in a single place in a single source file? >> +int > +sql_default_engine_set(const char *engine_name) > +{ > + enum sql_storage_engine_t engine = 0; > + size_t engine_name_len = strlen(engine_name); > + > + if (engine_name_len == strlen("memtx") && > + sqlite3_stricmp(engine_name, "memtx") == 0) { 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. > + engine = SQL_STORAGE_ENGINE_MEMTX; > + } else if (engine_name_len == strlen("vinyl") && > + sqlite3_stricmp(engine_name, "vinyl") == 0) { > + engine = SQL_STORAGE_ENGINE_VINYL; 6. Bad indentation. > + } else { > + diag_set(ClientError, ER_NO_SUCH_ENGINE, engine_name); > + return -1; > + } > + current_session()->sql_default_engine = engine; 7. On language switch it should be reset, if we do not want to affect lua. Maybe we should ask in a big red chat about this default engine for each language unless we will have to set default engine like this when use iproto: c = netbox.connect(uri) c:execute('PRAGMA default_engine = "memtx"') c:eval('box.space.default_engine = "memtx"') ... -- etc for each language. And it breaks our "celebratory interoperability" so much glorified on T+ conf. Maybe it is a good question for Gulutzan. > + return 0; > +} > + > +/** > * Create and initialize a new SQL Table object. > * All memory except table object itself is allocated on region. > * @param parser SQL Parser object> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index acda23d..8f5ef39 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -4560,6 +4560,20 @@ void sqlite3VdbeIOTraceSql(Vdbe *); > #define sqlite3VdbeIOTraceSql(X) > #endif > > +enum sql_storage_engine_t { > + SQL_STORAGE_ENGINE_MEMTX = 0, > + SQL_STORAGE_ENGINE_VINYL = 1, > +}; 8. Please, remove '_t'. We do not use it for enums. 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. > + > +/** > + * Set tarantool backend default engine for SQL interface. > + * @param engine_name to set default. > + * @retval -1 on error. > + * @retval 0 on success. > + */ > +int > +sql_default_engine_set(const char *engine_name); 10. Please, make this function be static inline in pragma.c. > + > /* > * These routines are available for the mem2.c debugging memory allocator > * only. They are used to verify that different "types" of memory > diff --git a/test/sql-tap/gh-2367-pragma.test.lua b/test/sql-tap/gh-2367-pragma.test.lua > index a41a026..2ee8424 100755 > --- a/test/sql-tap/gh-2367-pragma.test.lua > +++ b/test/sql-tap/gh-2367-pragma.test.lua > @@ -1,7 +1,7 @@ > #!/usr/bin/env tarantool > test = require("sqltester") > > -test:plan(1) > +test:plan(2) > > test:do_catchsql_test( > "pragma-1.3", > @@ -11,4 +11,16 @@ test:do_catchsql_test( > 1, "no such pragma: KEK" > }) > > +--- > +--- gh-2199: SQL default engine pragma > +--- > +test:do_catchsql_test( > + "pragma-2.1", > + [[ > + pragma sql_default_engine='creepy'; > + ]], { > + 1, "Space engine 'creepy' does not exist" > +}) 11. Add a test with correct engines as well.