Re: [Tarantool-patches] [PATCH] box: sql prepare and execute statistics should be reflected in box.stat()

["Maria Khaydich" <maria.khaydich@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 3968 bytes --]


Thank you for the review. Fixed the comments:
----------------------------------------------------------------------
The patch fixes a bug for the  previous commit when statistics on box.execute
was collected twice. This happened because sql_prepare_and_execute called
sql_execute under the hood, so there's no need to do rmean_collect in both
of them.
 
Follow-up #4756
---
Issue:
https://github.com/tarantool/tarantool/issues/4756  
Branch:
https://github.com/tarantool/tarantool/commit/a9c688b7312c8dc786ea14246eb380f9b5a148cf  

 src/box/execute.c        |  1 -
 test/sql/iproto.result   | 10 +++++++++-
 test/sql/iproto.test.lua |  3 ++-
 3 files changed, 11 insertions(+), 3 deletions(-)
 
diff --git a/src/box/execute.c b/src/box/execute.c
index 3daa09205..24f8529ec 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -735,7 +735,6 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
     if (sql_stmt_compile(sql, len, NULL, &stmt, NULL) != 0)
         return -1;
     assert(stmt != NULL);
-    rmean_collect(rmean_box, IPROTO_EXECUTE, 1);
     enum sql_serialization_format format = sql_column_count(stmt) > 0 ?
                        DQL_EXECUTE : DML_EXECUTE;
     port_sql_create(port, stmt, format, true);
 
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index a391307d1..44ba499a0 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -808,6 +808,14 @@ s:execute({42})
   rows:
   - [42]
 ...
+box.execute('SELECT 1;')
+---
+- metadata:
+  - name: '1'
+    type: integer
+  rows:
+  - [1]
+...
 res, err = box.unprepare(s)
 ---
 ...
@@ -815,7 +823,7 @@ assert(box.stat().PREPARE.total == p + 1)
 ---
 - true
 ...
-assert(box.stat().EXECUTE.total == e + 1)
+assert(box.stat().EXECUTE.total == e + 2)
 ---
 - true
 ...
 
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 9eac91d2c..d884577c5 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -250,10 +250,11 @@ e = box.stat().EXECUTE.total
 
 s = box.prepare([[ SELECT ?; ]])
 s:execute({42})
+box.execute('SELECT 1;')
 res, err = box.unprepare(s)
 
 assert(box.stat().PREPARE.total == p + 1)
-assert(box.stat().EXECUTE.total == e + 1)
+assert(box.stat().EXECUTE.total == e + 2)
 
 -- Cleanup xlog
 box.snapshot()
-- 
2.24.0 
>Среда, 4 марта 2020, 1:39 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
> 
>Thanks for the patch!
>
>See 2 comments below.
>
>> Calling prepare and execute did not update corresponding request statistics
>> in the box.stat table. This happened because methods that collect stats were
>> never called where they should have been.
>
>1. I don't think we can make this commit have the same commit message
>as the original commit (even with an amendment below). And it should not
>be 'Closes'. This is rather 'Follow-up'.
>
>> There was a bug in previous commit resulting in collecting statistics on
>> box.execute twice in some cases.
>>  
>> Closes #4756
>> ---
>> Issue:
>>  https://github.com/tarantool/tarantool/issues/4756
>> Branch:
>>  https://github.com/tarantool/tarantool/tree/eljashm/gh-4756-hotfix-box-stat-execute-prepare  
>> > diff --git a/test/sql/iproto.result b/test/sql/iproto.result
>> index a391307d1..0e046577d 100644
>> --- a/test/sql/iproto.result
>> +++ b/test/sql/iproto.result
>> @@ -800,22 +800,22 @@ e = box.stat().EXECUTE.total
>>  s = box.prepare([[ SELECT ?; ]])
>>  ---
>>  ...
>> -s:execute({42})
>> +res, err = box.unprepare(s)
>>  ---
>> -- metadata:
>> -  - name: '?'
>> -    type: integer
>> -  rows:
>> -  - [42]
>>  ...
>> -res, err = box.unprepare(s)
>> +box.execute('create table test (id integer primary key, a integer)')
>>  ---
>> +- row_count: 1
>> +...
>> +box.execute('DROP TABLE test')
>
>2. Is it important to call DDL? Would 'box.execute('SELECT 1;')' be
>enough? 
 
 
--
Maria Khaydich
 

[-- Attachment #2: Type: text/html, Size: 5662 bytes --]