[Tarantool-patches] [PATCH v2 test-run 1/1] sql: add another way to change SQL default engine

[imeevma at tarantool.org]

Thank you for review! My answers and new patch below.

On 12/28/19 1:57 PM, Alexander Turenko wrote:
> I don't mind in general, but please consider several notes below.
>
> WBR, Alexander Turenko.
>
> On Fri, Dec 27, 2019 at 03:33:12PM +0300, imeevma at tarantool.org wrote:
>> sql: add another way to change SQL default engine
>>
>> Due to the removal of the sql_default_engine pragma in 2.3.1, this
>> patch adds another way to change this setting.
>
> Nit: I would say 'implements' rather then 'adds', because the letter can
> be read as something configurable by a user.
>
Fixed.

> I would mention that it affects only *.test.sql tests, just in case.
>
Done.

> I think it would be good to link the issue about it here.
>
Done.

>> ---
>> https://github.com/tarantool/test-run/tree/imeevma/fix_sql_default_engine
>>
>>  lib/tarantool_server.py | 19 +++++++++++++------
>>  1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
>> index 6f71b5b..20a398e 100644
>> --- a/lib/tarantool_server.py
>> +++ b/lib/tarantool_server.py
>> @@ -145,12 +145,19 @@ class LuaTest(Test):
>>              return True
>>  
>>          engine = self.run_params['engine']
>> -        command = "pragma sql_default_engine='{}'".format(engine)
>> -        result = self.send_command(command, ts, 'sql')
>> -        result = result.replace('\r\n', '\n')
>> -        if result != '---\n- row_count: 0\n...\n':
>> -            sys.stdout.write(result)
>> -            return False
>> +
>> +        command_new = ("UPDATE \"_session_settings\" SET \"value\" = '{}' " + \
>> +                       "WHERE \"name\" = 'sql_default_engine'").format(engine)
>> +        result_new = self.send_command(command_new, ts, 'sql')
>> +        result_new = result_new.replace('\r\n', '\n')
>> +
>> +        command_old = "pragma sql_default_engine='{}'".format(engine)
>> +        result_old = self.send_command(command_old, ts, 'sql')
>> +        result_old = result_old.replace('\r\n', '\n')
>> +        if result_new != '---\n- row_count: 1\n...\n':
>> +            if result_old != '---\n- row_count: 0\n...\n':
>> +                sys.stdout.write(result_old)
>> +                return False
>
> I would make it like so:
>
> 1. Send "pragma ...", await a result.
>    - If succeeds, return True.
>    - Otherwise proceed with the step 2.
> 2. Send "UPDATE ...", await a result.
>    - If succeeds, return True.
>    - Otherwise return False.
>
Fixed.

> This way we'll save one extra hop on a fresh tarantool. Not so big
> overhead for an action that is run once for a test, but we can overcome
> it and pay nothing, so it looks worthful for me.
>
> Please, look into CI: it reports a flake8 error. You can use `make lint`
> locally to verify python code style.
>
Fixed.

> Please, double-check that everything works as expected for both
> tarantool versions (with pragma support and with _session_settings):
> let's run a .test.sql test and verify that it creates spaces in an
> expected engine. It should be easier using `\set language lua` command:
> it is simple to assess a space engine and compare it with
> test_run:get_cfg('engine').
>
Checked. I haven't found any errors.

> README states that test-run use "pragma <...>" to set an engine. It
> would be good to mention the new way together with the old one.
>
Fixed.

>>  
>>          return True
>>  
>> -- 
>> 2.7.4
>
> BTW, the subject of the email is the following:
>
>  | [PATCH 1/1] sql: add another way to change SQL default engine
>
> I suggest to add a repo prefix when it is not tarantool itself. Look as
> Vlad does it:
>
>  | [PATCH vshard 1/1] travis-ci: deploy packages from tagged revisions
>
> https://lists.tarantool.org/pipermail/tarantool-patches/2019-August/001304.html
Fixed.

New patch:

>From c0f30b2e1e5fe6155c9812d1c7188941b6d2eab5 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma at gmail.com>
Date: Sun, 29 Dec 2019 16:48:29 +0300
Subject: [PATCH] sql: add another way to change SQL default engine

Currently, the pragma sql_default_engine is used in the *.test.sql
tests to change the default SQL engine. But in the issue
tarantool/tarantool#4511, this pragma will be removed. To avoid
failures in these tests because of this, in addition to the
current one, another way to change this parameter was implemented.

diff --git a/README.md b/README.md
index aefeeff..781d003 100644
--- a/README.md
+++ b/README.md
@@ -69,9 +69,10 @@ engine = test_run:get_cfg('engine')
 ```
 
 "engine" value has a special meaning for *.test.sql files: if it is "memtx" or
-"vinyl", then the corresponding engine will be set using "pragma
-sql_default_engine='memtx|vinyl'" command before executing commands from a test
-file.
+"vinyl", then the corresponding engine will be set using either "pragma
+sql_default_engine='memtx|vinyl'" or "UPDATE \\"_session_settings\\" SET
+\\"value\\" = 'memtx|vinyl' WHERE \\"name\\" = 'sql_default_engine';" command
+before executing commands from a test file.
 
 #### Python
 
diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
index 6f71b5b..0b83c2e 100644
--- a/lib/tarantool_server.py
+++ b/lib/tarantool_server.py
@@ -148,7 +148,15 @@ class LuaTest(Test):
         command = "pragma sql_default_engine='{}'".format(engine)
         result = self.send_command(command, ts, 'sql')
         result = result.replace('\r\n', '\n')
-        if result != '---\n- row_count: 0\n...\n':
+        if result != '---\n- row_count: 0\n...\n' and result != "---\n- " +\
+           "null\n- Pragma 'SQL_DEFAULT_ENGINE' does not exist\n...\n":
+            sys.stdout.write(result)
+            return False
+        command = ("UPDATE \"_session_settings\" SET \"value\" = '{}' " +
+                   "WHERE \"name\" = 'sql_default_engine';").format(engine)
+        result = self.send_command(command, ts, 'sql')
+        result = result.replace('\r\n', '\n')
+        if result != '---\n- row_count: 1\n...\n':
             sys.stdout.write(result)
             return False