* [Tarantool-patches] [PATCH v2 test-run 1/1] sql: add another way to change SQL default engine
@ 2019-12-29 14:20 imeevma
2019-12-30 3:09 ` Alexander Turenko
0 siblings, 1 reply; 4+ messages in thread
From: imeevma @ 2019-12-29 14:20 UTC (permalink / raw)
To: alexander.turenko; +Cc: tarantool-patches
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@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@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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 test-run 1/1] sql: add another way to change SQL default engine
2019-12-29 14:20 [Tarantool-patches] [PATCH v2 test-run 1/1] sql: add another way to change SQL default engine imeevma
@ 2019-12-30 3:09 ` Alexander Turenko
2019-12-30 9:55 ` Mergen Imeev
0 siblings, 1 reply; 4+ messages in thread
From: Alexander Turenko @ 2019-12-30 3:09 UTC (permalink / raw)
To: Mergen Imeev; +Cc: tarantool-patches
> >> 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.
Ouch, occasionally I misdirected you. Sorry. I intend to say that we
should send the new-style command first and only then (if it fails) send
old-style "pragma ..." command.
This way we'll optimize the case when the new tarantool is tested that
will be most often case after some time.
> > 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 test-run 1/1] sql: add another way to change SQL default engine
2019-12-30 3:09 ` Alexander Turenko
@ 2019-12-30 9:55 ` Mergen Imeev
2019-12-30 17:14 ` Alexander Turenko
0 siblings, 1 reply; 4+ messages in thread
From: Mergen Imeev @ 2019-12-30 9:55 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
Hi! Thank you for review. New patch below.
On Mon, Dec 30, 2019 at 06:09:04AM +0300, Alexander Turenko wrote:
> > >> 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.
>
> Ouch, occasionally I misdirected you. Sorry. I intend to say that we
> should send the new-style command first and only then (if it fails) send
> old-style "pragma ..." command.
>
> This way we'll optimize the case when the new tarantool is tested that
> will be most often case after some time.
>
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.
New patch:
From fd9a8af20393d6df3622ab1ddd8884b7333d1c11 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@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..07cbf0d 100644
--- a/lib/tarantool_server.py
+++ b/lib/tarantool_server.py
@@ -145,6 +145,14 @@ class LuaTest(Test):
return True
engine = self.run_params['engine']
+ 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' and result != "---\n- " +\
+ "null\n- Space '_session_settings' does not exist\n...\n":
+ sys.stdout.write(result)
+ return False
command = "pragma sql_default_engine='{}'".format(engine)
result = self.send_command(command, ts, 'sql')
result = result.replace('\r\n', '\n')
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 test-run 1/1] sql: add another way to change SQL default engine
2019-12-30 9:55 ` Mergen Imeev
@ 2019-12-30 17:14 ` Alexander Turenko
0 siblings, 0 replies; 4+ messages in thread
From: Alexander Turenko @ 2019-12-30 17:14 UTC (permalink / raw)
To: Mergen Imeev; +Cc: tarantool-patches
Applied changes after voice discussion with Mergen (see below). Pushed
to test-run's master. I'll update test-run in tarantool's branches
(master, 2.2 and 1.10) soon.
WBR, Alexander Turenko.
> "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.
I placed "UPDATE ..." first, "pragma ..." second and extracted commands
from the text itself.
> diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
> index 6f71b5b..07cbf0d 100644
> --- a/lib/tarantool_server.py
> +++ b/lib/tarantool_server.py
> @@ -145,6 +145,14 @@ class LuaTest(Test):
> return True
>
> engine = self.run_params['engine']
> + 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' and result != "---\n- " +\
> + "null\n- Space '_session_settings' does not exist\n...\n":
> + sys.stdout.write(result)
> + return False
> command = "pragma sql_default_engine='{}'".format(engine)
> result = self.send_command(command, ts, 'sql')
> result = result.replace('\r\n', '\n')
We discussed in with Mergen voicely. I proposed to eliminate hardcoding
of "Space '_session_settings' does not exist" error just in case: if it
was changed somehow (say, an engine or other info will be added), we'll
need to update test-run again. I would try to avoid it.
Mergen's concern was against ignoring of the first error message: this
may hid useful information in case of some failure.
I think we can just show both results in case of fail and eliminate the
check against 'Space <...> does not exist' error: proceed with 2nd
command after any fail of 1nd.
Resulting patch is below. I verified it with both master
(e13a74e4383744a7ff5ddec83c74e13c82e417dc) and
imeevma/gh-4511-pragma-replaced-by-set branch
(dbe63f9c561bbac3748fcfa22bf09c5b0532d7fd) and it seems everything is
okay.
----
commit dc2cdb554f37efc2d8845e4e0350f53c451f32a7 (HEAD -> master)
Author: Mergen Imeev <imeevma@gmail.com>
Date: Sun Dec 29 16:48:29 2019 +0300
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..48803bf 100644
--- a/README.md
+++ b/README.md
@@ -69,9 +69,15 @@ 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 default engine will be set before executing
+commands from a test file. An engine is set with the following commands:
+
+```sql
+UPDATE "_session_settings" SET "value" = 'memtx|vinyl' WHERE "name" = 'sql_default_engine'
+pragma sql_default_engine='memtx|vinyl'
+```
+
+If the first fails, then the second will be executed. When both fails, fail the test.
#### Python
diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
index 6f71b5b..fd102b9 100644
--- a/lib/tarantool_server.py
+++ b/lib/tarantool_server.py
@@ -145,14 +145,27 @@ 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
- return True
+ # Probe the new way. Pass through on any error.
+ 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')
+ if result_new == '---\n- row_count: 1\n...\n':
+ return True
+
+ # Probe the old way. Fail the test on an error.
+ 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_old == '---\n- row_count: 0\n...\n':
+ return True
+
+ sys.stdout.write(command_new)
+ sys.stdout.write(result_new)
+ sys.stdout.write(command_old)
+ sys.stdout.write(result_old)
+ return False
def set_language(self, ts, language):
for conn in ts.curcon:
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-12-30 17:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-29 14:20 [Tarantool-patches] [PATCH v2 test-run 1/1] sql: add another way to change SQL default engine imeevma
2019-12-30 3:09 ` Alexander Turenko
2019-12-30 9:55 ` Mergen Imeev
2019-12-30 17:14 ` Alexander Turenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox