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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> 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
+})