[tarantool-patches] Re: [PATCH v1 1/2] sql: introduce pragma sql_default_engine
Kirill Shcherbatov
kshcherbatov at tarantool.org
Tue Jun 26 15:22:23 MSK 2018
> 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
+})
More information about the Tarantool-patches
mailing list