[tarantool-patches] Re: [PATCH v1 1/2] sql: introduce pragma sql_default_engine

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.