Tarantool development patches archive
 help / color / mirror / Atom feed
From: "Maria Khaydich" <maria.khaydich@tarantool.org>
To: "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org>
Cc: tarantool-patches <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH] box: sql prepare and execute statistics should be reflected in box.stat()
Date: Fri, 06 Mar 2020 14:34:16 +0300	[thread overview]
Message-ID: <1583494456.594178474@f380.i.mail.ru> (raw)
In-Reply-To: <2f34a430-731d-069a-8856-8cdbc30e77ab@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 --]

  reply	other threads:[~2020-03-06 11:34 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-10 18:35 Maria Khaydich
2020-02-10 23:06 ` Nikita Pettik
2020-02-13 21:44 ` Vladislav Shpilevoy
2020-02-19 16:37   ` Maria Khaydich
2020-02-19 17:16     ` Nikita Pettik
2020-02-25 11:08       ` Maria Khaydich
2020-02-25 13:02         ` Nikita Pettik
2020-02-25 20:29           ` Vladislav Shpilevoy
2020-02-26 15:50             ` Maria Khaydich
2020-02-27  0:08               ` Vladislav Shpilevoy
2020-02-27 13:21                 ` Nikita Pettik
2020-03-03 16:42                   ` Maria Khaydich
2020-03-03 22:39                     ` Vladislav Shpilevoy
2020-03-06 11:34                       ` Maria Khaydich [this message]
2020-03-06 14:32                         ` Nikita Pettik
2020-03-06 15:09                           ` Nikita Pettik
2020-03-04 13:47                     ` Nikita Pettik
2020-02-25 22:26 ` Kirill Yukhin
2020-02-25 23:30   ` Nikita Pettik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1583494456.594178474@f380.i.mail.ru \
    --to=maria.khaydich@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] box: sql prepare and execute statistics should be reflected in box.stat()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox