[tarantool-patches] Re: [PATCH v1 1/2] sql: introduce pragma sql_default_engine
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Jun 22 23:04:02 MSK 2018
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.
More information about the Tarantool-patches
mailing list