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

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

[Maria Khaydich]

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


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.
 
Closes #4756

---
Issue:
https://github.com/tarantool/tarantool/issues/4756
Branch:
https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated
 
 src/box/execute.c         |  4 ++++
 test/box-tap/cfg.test.lua | 16 +++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/src/box/execute.c b/src/box/execute.c
index dc8dce81c..e775055b4 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -48,6 +48,7 @@
 #include "box/lua/execute.h"
 #include "box/sql_stmt_cache.h"
 #include "session.h"
+#include "rmean.h"
 
 const char *sql_info_key_strs[] = {
     "row_count",
@@ -608,6 +609,7 @@ sql_prepare(const char *sql, int len, struct port *port)
 {
     uint32_t stmt_id = sql_stmt_calculate_id(sql, len);
     struct sql_stmt *stmt = sql_stmt_cache_find(stmt_id);
+    rmean_collect(rmean_box, IPROTO_PREPARE, 1);
     if (stmt == NULL) {
         if (sql_stmt_compile(sql, len, NULL, &stmt, NULL) != 0)
             return -1;
@@ -669,6 +671,7 @@ static inline int
 sql_execute(struct sql_stmt *stmt, struct port *port, struct region *region)
 {
     int rc, column_count = sql_column_count(stmt);
+    rmean_collect(rmean_box, IPROTO_EXECUTE, 1);
     if (column_count > 0) {
         /* Either ROW or DONE or ERROR. */
         while ((rc = sql_step(stmt)) == SQL_ROW) {
@@ -732,6 +735,7 @@ 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_PREPARE, 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/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
index d529447bb..d367aab07 100755
--- a/test/box-tap/cfg.test.lua
+++ b/test/box-tap/cfg.test.lua
@@ -6,7 +6,7 @@ local socket = require('socket')
 local fio = require('fio')
 local uuid = require('uuid')
 local msgpack = require('msgpack')
-test:plan(104)
+test:plan(106)
 
 --------------------------------------------------------------------------------
 -- Invalid values
@@ -592,6 +592,16 @@ box.cfg{read_only=true}
 ]]
 test:is(run_script(code), PANIC, "panic on bootstrapping a read-only instance as master")
 
+--
+-- gh-4756: PREPARE and EXECUTE statistics should be present in box.stat()
+--
+local p = box.stat().PREPARE.total
+local e = box.stat().EXECUTE.total
 
-test:check()
-os.exit(0)
+s = box.prepare([[ SELECT ?; ]])
+s:execute({42})
+
+test:is(box.stat().PREPARE.total, p + 1, "prepare stat is incremented")
+test:is(box.stat().EXECUTE.total, e + 1, "execute stat is incremented")
+
+os.exit(test:check() and 0 or 1)
-- 
2.24.0
 
--
Maria Khaydich
 

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

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

[Nikita Pettik]

On 10 Feb 21:35, Maria Khaydich wrote:
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index dc8dce81c..e775055b4 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -48,6 +48,7 @@
>  #include "box/lua/execute.h"
>  #include "box/sql_stmt_cache.h"
>  #include "session.h"
> +#include "rmean.h"
>  
>  const char *sql_info_key_strs[] = {
>      "row_count",
> @@ -608,6 +609,7 @@ sql_prepare(const char *sql, int len, struct port *port)
>  {
>      uint32_t stmt_id = sql_stmt_calculate_id(sql, len);
>      struct sql_stmt *stmt = sql_stmt_cache_find(stmt_id);
> +    rmean_collect(rmean_box, IPROTO_PREPARE, 1);
>      if (stmt == NULL) {
>          if (sql_stmt_compile(sql, len, NULL, &stmt, NULL) != 0)
>              return -1;
> @@ -669,6 +671,7 @@ static inline int
>  sql_execute(struct sql_stmt *stmt, struct port *port, struct region *region)
>  {
>      int rc, column_count = sql_column_count(stmt);
> +    rmean_collect(rmean_box, IPROTO_EXECUTE, 1);
>      if (column_count > 0) {
>          /* Either ROW or DONE or ERROR. */
>          while ((rc = sql_step(stmt)) == SQL_ROW) {
> @@ -732,6 +735,7 @@ 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_PREPARE, 1);

prepare_and_execute() is supposed to handle IPROTO_EXECUTE request.

>      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/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
> index d529447bb..d367aab07 100755
> --- a/test/box-tap/cfg.test.lua
> +++ b/test/box-tap/cfg.test.lua

box-tap/cfg.test is unlikely to be proper place for this test.
Look at sql/ suite (for instance, sql/iproto.test.lua).

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

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 10/02/2020 19:35, Maria Khaydich wrote:
> 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.
>  
> Closes #4756
> 
> ---
> Issue:
> https://github.com/tarantool/tarantool/issues/4756
> Branch:
> https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated <https://github.com/tarantool/tarantool/issues/4756Branch:https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated>

Something is broken with the links. When I click on them, all the links are
pasted into the address string of my browser concatenated. And this is what
I see in the source:

https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated <https://github.com/tarantool/tarantool/issues/4756Branch:https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated>

So everything is a one huge line. This does not happen with other emails
and links.

See 2 comments below.

>  src/box/execute.c         |  4 ++++
>  test/box-tap/cfg.test.lua | 16 +++++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)
> diff --git a/src/box/execute.c b/src/box/execute.c
> index dc8dce81c..e775055b4 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -732,6 +735,7 @@ 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_PREPARE, 1);

1. Nikita is right, this is called when IPROTO_EXECUTE arrives. You can
see that in iproto.cc. This is a separate question, why do we collect
IPROTO_* statistics out of iproto. For now lets just use IPROTO_EXECUTE
here.

>      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/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
> index d529447bb..d367aab07 100755
> --- a/test/box-tap/cfg.test.lua
> +++ b/test/box-tap/cfg.test.lua
> @@ -6,7 +6,7 @@ local socket = require('socket')
>  local fio = require('fio')
>  local uuid = require('uuid')
>  local msgpack = require('msgpack')
> -test:plan(104)
> +test:plan(106)

2. Here Nikita is also right. This file is for box.cfg() function
tests. For iproto statistics, indeed, sql/iproto.test.lua would
fit well.

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

[Maria Khaydich]

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


Thank you for the review!
Proposed fixes are done.
 
----------------------------------------------------------------------
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.
 
Closes #4756

---
Issue:
https://github.com/tarantool/tarantool/issues/4756  
Branch:
https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated  
 
 src/box/execute.c        |  4 ++++
 test/sql/iproto.result   | 28 ++++++++++++++++++++++++++++
 test/sql/iproto.test.lua | 12 ++++++++++++
 3 files changed, 44 insertions(+)
 
diff --git a/src/box/execute.c b/src/box/execute.c
index dc8dce81c..3daa09205 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -48,6 +48,7 @@
 #include "box/lua/execute.h"
 #include "box/sql_stmt_cache.h"
 #include "session.h"
+#include "rmean.h"
 
 const char *sql_info_key_strs[] = {
     "row_count",
@@ -608,6 +609,7 @@ sql_prepare(const char *sql, int len, struct port *port)
 {
     uint32_t stmt_id = sql_stmt_calculate_id(sql, len);
     struct sql_stmt *stmt = sql_stmt_cache_find(stmt_id);
+    rmean_collect(rmean_box, IPROTO_PREPARE, 1);
     if (stmt == NULL) {
         if (sql_stmt_compile(sql, len, NULL, &stmt, NULL) != 0)
             return -1;
@@ -669,6 +671,7 @@ static inline int
 sql_execute(struct sql_stmt *stmt, struct port *port, struct region *region)
 {
     int rc, column_count = sql_column_count(stmt);
+    rmean_collect(rmean_box, IPROTO_EXECUTE, 1);
     if (column_count > 0) {
         /* Either ROW or DONE or ERROR. */
         while ((rc = sql_step(stmt)) == SQL_ROW) {
@@ -732,6 +735,7 @@ 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 7df11b0bf..b4a05e4f8 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -788,6 +788,34 @@ box.schema.user.revoke('guest', 'create', 'space')
 space = nil
 ---
 ...
+--
+-- gh-4756: PREPARE and EXECUTE statistics should be present in box.stat()
+--
+p = box.stat().PREPARE.total
+---
+...
+e = box.stat().EXECUTE.total
+---
+...
+s = box.prepare([[ SELECT ?; ]])
+---
+...
+s:execute({42})
+---
+- metadata:
+  - name: '?'
+    type: integer
+  rows:
+  - [42]
+...
+box.stat().PREPARE.total == p + 1
+---
+- true
+...
+box.stat().EXECUTE.total == e + 1
+---
+- true
+...
 -- Cleanup xlog
 box.snapshot()
 ---
 
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 4019fb7a4..1417aa32b 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -242,5 +242,17 @@ box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'create', 'space')
 space = nil
 
+--
+-- gh-4756: PREPARE and EXECUTE statistics should be present in box.stat()
+--
+p = box.stat().PREPARE.total
+e = box.stat().EXECUTE.total
+
+s = box.prepare([[ SELECT ?; ]])
+s:execute({42})
+
+box.stat().PREPARE.total == p + 1
+box.stat().EXECUTE.total == e + 1
+
 -- Cleanup xlog
 box.snapshot()
-- 
2.24.0 
>Пятница, 14 февраля 2020, 0:44 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
> 
>Hi! Thanks for the patch!
>
>On 10/02/2020 19:35, Maria Khaydich wrote:
>> 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.
>>  
>> Closes #4756
>>
>> ---
>> Issue:
>>  https://github.com/tarantool/tarantool/issues/4756
>> Branch:
>>  https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated < https://github.com/tarantool/tarantool/issues/4756Branch:https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated >
>
>Something is broken with the links. When I click on them, all the links are
>pasted into the address string of my browser concatenated. And this is what
>I see in the source:
>
>https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated < https://github.com/tarantool/tarantool/issues/4756Branch:https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated >
>
>So everything is a one huge line. This does not happen with other emails
>and links.
>
>See 2 comments below.
>
>>  src/box/execute.c         |  4 ++++
>>  test/box-tap/cfg.test.lua | 16 +++++++++++++---
>>  2 files changed, 17 insertions(+), 3 deletions(-)
>> diff --git a/src/box/execute.c b/src/box/execute.c
>> index dc8dce81c..e775055b4 100644
>> --- a/src/box/execute.c
>> +++ b/src/box/execute.c
>> @@ -732,6 +735,7 @@ 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_PREPARE, 1);
>
>1. Nikita is right, this is called when IPROTO_EXECUTE arrives. You can
>see that in iproto.cc. This is a separate question, why do we collect
>IPROTO_* statistics out of iproto. For now lets just use IPROTO_EXECUTE
>here.
>
>>      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/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
>> index d529447bb..d367aab07 100755
>> --- a/test/box-tap/cfg.test.lua
>> +++ b/test/box-tap/cfg.test.lua
>> @@ -6,7 +6,7 @@ local socket = require('socket')
>>  local fio = require('fio')
>>  local uuid = require('uuid')
>>  local msgpack = require('msgpack')
>> -test:plan(104)
>> +test:plan(106)
>
>2. Here Nikita is also right. This file is for box.cfg() function
>tests. For iproto statistics, indeed, sql/iproto.test.lua would
>fit well. 
 
 
--
Maria Khaydich
 

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

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

[Nikita Pettik]

On 19 Feb 19:37, Maria Khaydich wrote:
> 
> Thank you for the review!
> Proposed fixes are done.
>  
> ----------------------------------------------------------------------
> 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.
>  
> Closes #4756

Please also add @ChangeLog according to our new sop guide. For instance,
see: https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014301.html
 
>  space = nil
>  ---
>  ...
> +--
> +-- gh-4756: PREPARE and EXECUTE statistics should be present in box.stat()
> +--
> +p = box.stat().PREPARE.total
> +---
> +...
> +e = box.stat().EXECUTE.total
> +---
> +...
> +s = box.prepare([[ SELECT ?; ]])
> +---
> +...
> +s:execute({42})
> +---
> +- metadata:
> +  - name: '?'
> +    type: integer
> +  rows:
> +  - [42]
> +...
> +box.stat().PREPARE.total == p + 1
> +---
> +- true
> +...
> +box.stat().EXECUTE.total == e + 1
> +---
> +- true
> +...
>  -- Cleanup xlog
>  box.snapshot()
>  ---
>  
> diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
> index 4019fb7a4..1417aa32b 100644
> --- a/test/sql/iproto.test.lua
> +++ b/test/sql/iproto.test.lua
> @@ -242,5 +242,17 @@ box.schema.user.revoke('guest', 'read,write,execute', 'universe')
>  box.schema.user.revoke('guest', 'create', 'space')
>  space = nil
>  
> +--
> +-- gh-4756: PREPARE and EXECUTE statistics should be present in box.stat()
> +--
> +p = box.stat().PREPARE.total
> +e = box.stat().EXECUTE.total
> +
> +s = box.prepare([[ SELECT ?; ]])
> +s:execute({42})

You also have to unprepare statement. Otherwise it gets stuck in
cache and sql/prepare.test.lua may fail since at the start of
execution it checks cahce size:
https://travis-ci.org/tarantool/tarantool/jobs/652552114?utm_medium=notification&utm_source=github_status

It is sort of basic clean-up which is provided for any schema object
(spaces, triggers etc).

> +box.stat().PREPARE.total == p + 1
> +box.stat().EXECUTE.total == e + 1
> +

Nit: I'd better wrap these statements into asssertions:

assert(box.stat().PREPARE.total == p + 1)

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

[Maria Khaydich]

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


Thanks for the review, all done:
----------------------------------------------------------------------
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.
 
Closes #4756
---
Issue:
https://github.com/tarantool/tarantool/issues/4756  
Branch:
https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated  
 
@ChangeLog
- Fixed box.stat behavior - now it collects statistics on
    prepare and execute methods as it should.
    gh-4756
 
 src/box/execute.c        |  4 ++++
 test/sql/iproto.result   | 31 +++++++++++++++++++++++++++++++
 test/sql/iproto.test.lua | 13 +++++++++++++
 3 files changed, 48 insertions(+)
 
diff --git a/src/box/execute.c b/src/box/execute.c
index dc8dce81c..3daa09205 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -48,6 +48,7 @@
 #include "box/lua/execute.h"
 #include "box/sql_stmt_cache.h"
 #include "session.h"
+#include "rmean.h"
 
 const char *sql_info_key_strs[] = {
     "row_count",
@@ -608,6 +609,7 @@ sql_prepare(const char *sql, int len, struct port *port)
 {
     uint32_t stmt_id = sql_stmt_calculate_id(sql, len);
     struct sql_stmt *stmt = sql_stmt_cache_find(stmt_id);
+    rmean_collect(rmean_box, IPROTO_PREPARE, 1);
     if (stmt == NULL) {
         if (sql_stmt_compile(sql, len, NULL, &stmt, NULL) != 0)
             return -1;
@@ -669,6 +671,7 @@ static inline int
 sql_execute(struct sql_stmt *stmt, struct port *port, struct region *region)
 {
     int rc, column_count = sql_column_count(stmt);
+    rmean_collect(rmean_box, IPROTO_EXECUTE, 1);
     if (column_count > 0) {
         /* Either ROW or DONE or ERROR. */
         while ((rc = sql_step(stmt)) == SQL_ROW) {
@@ -732,6 +735,7 @@ 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 7df11b0bf..a391307d1 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -788,6 +788,37 @@ box.schema.user.revoke('guest', 'create', 'space')
 space = nil
 ---
 ...
+--
+-- gh-4756: PREPARE and EXECUTE statistics should be present in box.stat()
+--
+p = box.stat().PREPARE.total
+---
+...
+e = box.stat().EXECUTE.total
+---
+...
+s = box.prepare([[ SELECT ?; ]])
+---
+...
+s:execute({42})
+---
+- metadata:
+  - name: '?'
+    type: integer
+  rows:
+  - [42]
+...
+res, err = box.unprepare(s)
+---
+...
+assert(box.stat().PREPARE.total == p + 1)
+---
+- true
+...
+assert(box.stat().EXECUTE.total == e + 1)
+---
+- true
+...
 -- Cleanup xlog
 box.snapshot()
 ---

diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 4019fb7a4..9eac91d2c 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -242,5 +242,18 @@ box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'create', 'space')
 space = nil
 
+--
+-- gh-4756: PREPARE and EXECUTE statistics should be present in box.stat()
+--
+p = box.stat().PREPARE.total
+e = box.stat().EXECUTE.total
+
+s = box.prepare([[ SELECT ?; ]])
+s:execute({42})
+res, err = box.unprepare(s)
+
+assert(box.stat().PREPARE.total == p + 1)
+assert(box.stat().EXECUTE.total == e + 1)
+
 -- Cleanup xlog
 box.snapshot()
-- 
2.24.0 
>Среда, 19 февраля 2020, 20:16 +03:00 от Nikita Pettik <korablev@tarantool.org>:
> 
>On 19 Feb 19:37, Maria Khaydich wrote:
>>
>> Thank you for the review!
>> Proposed fixes are done.
>>  
>> ----------------------------------------------------------------------
>> 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.
>>  
>> Closes #4756
>
>Please also add @ChangeLog according to our new sop guide. For instance,
>see:  https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014301.html
> 
>>  space = nil
>>  ---
>>  ...
>> +--
>> +-- gh-4756: PREPARE and EXECUTE statistics should be present in box.stat()
>> +--
>> +p = box.stat().PREPARE.total
>> +---
>> +...
>> +e = box.stat().EXECUTE.total
>> +---
>> +...
>> +s = box.prepare([[ SELECT ?; ]])
>> +---
>> +...
>> +s:execute({42})
>> +---
>> +- metadata:
>> +  - name: '?'
>> +    type: integer
>> +  rows:
>> +  - [42]
>> +...
>> +box.stat().PREPARE.total == p + 1
>> +---
>> +- true
>> +...
>> +box.stat().EXECUTE.total == e + 1
>> +---
>> +- true
>> +...
>>  -- Cleanup xlog
>>  box.snapshot()
>>  ---
>>  
>> diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
>> index 4019fb7a4..1417aa32b 100644
>> --- a/test/sql/iproto.test.lua
>> +++ b/test/sql/iproto.test.lua
>> @@ -242,5 +242,17 @@ box.schema.user.revoke('guest', 'read,write,execute', 'universe')
>>  box.schema.user.revoke('guest', 'create', 'space')
>>  space = nil
>>  
>> +--
>> +-- gh-4756: PREPARE and EXECUTE statistics should be present in box.stat()
>> +--
>> +p = box.stat().PREPARE.total
>> +e = box.stat().EXECUTE.total
>> +
>> +s = box.prepare([[ SELECT ?; ]])
>> +s:execute({42})
>
>You also have to unprepare statement. Otherwise it gets stuck in
>cache and sql/prepare.test.lua may fail since at the start of
>execution it checks cahce size:
>https://travis-ci.org/tarantool/tarantool/jobs/652552114?utm_medium=notification&utm_source=github_status
>
>It is sort of basic clean-up which is provided for any schema object
>(spaces, triggers etc).
>
>> +box.stat().PREPARE.total == p + 1
>> +box.stat().EXECUTE.total == e + 1
>> +
>
>Nit: I'd better wrap these statements into asssertions:
>
>assert(box.stat().PREPARE.total == p + 1)
>  
 
 
--
Maria Khaydich
 

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

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

[Nikita Pettik]

On 25 Feb 14:08, Maria Khaydich wrote:
> 
> Thanks for the review, all done:
> ----------------------------------------------------------------------
> 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.
>  
> Closes #4756
> ---
> Issue:
> https://github.com/tarantool/tarantool/issues/4756  
> Branch:
> https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated  
>  
> @ChangeLog
> - Fixed box.stat behavior - now it collects statistics on
>     prepare and execute methods as it should.
>     gh-4756

LGTM. Vlad, is patch OK with you? If so, I will push it to master, 2.3 and
partially (only part related to EXECUTE statistics) to 2.2.

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

[Vladislav Shpilevoy]

LGTM as well.

On 25/02/2020 14:02, Nikita Pettik wrote:
> On 25 Feb 14:08, Maria Khaydich wrote:
>>
>> Thanks for the review, all done:
>> ----------------------------------------------------------------------
>> 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.
>>  
>> Closes #4756
>> ---
>> Issue:
>> https://github.com/tarantool/tarantool/issues/4756  
>> Branch:
>> https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated  
>>  
>> @ChangeLog
>> - Fixed box.stat behavior - now it collects statistics on
>>     prepare and execute methods as it should.
>>     gh-4756
> 
> LGTM. Vlad, is patch OK with you? If so, I will push it to master, 2.3 and
> partially (only part related to EXECUTE statistics) to 2.2.
> 

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

[Kirill Yukhin]

Hello,

On 10 фев 21:35, Maria Khaydich wrote:
> 
> 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.
>  
> Closes #4756
> 
> ---
> Issue:
> https://github.com/tarantool/tarantool/issues/4756
> Branch:
> https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated

I've checked your patch into 2.3 and master.

--
Regards, Kirill Yukhin

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

[Nikita Pettik]

On 26 Feb 01:26, Kirill Yukhin wrote:
> Hello,
> 
> On 10 фев 21:35, Maria Khaydich wrote:
> > 
> > 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.
> >  
> > Closes #4756
> > 
> > ---
> > Issue:
> > https://github.com/tarantool/tarantool/issues/4756
> > Branch:
> > https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated
> 
> I've checked your patch into 2.3 and master.

Kirill, it also should be backported to 2.2 skipping part related to
'prepare' feature.
 
> --
> Regards, Kirill Yukhin

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

[Maria Khaydich]

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


Hey, I’ve been doing the 2.2 version for the same issue and realized there’s a bug in the commit that was already merged to master.
 
> 1. Nikita is right, this is called when IPROTO_EXECUTE arrives. You can
see that in iproto.cc. This is a separate question, why do we collect
IPROTO_* statistics out of iproto. For now lets just use IPROTO_EXECUTE
here. 
 
The reason I was collecting stats for box.prepare here in the first place is because it looked to me like the execution of method sql_prepare_and_execute implied the user is preparing _and_ executing something at the same time, and whereas all other methods boiled down to either sql_prepare or sql_execute in the end, this one collected statistics only for IPROTO_EXECUTE, so I thought it was necessary to cover IPROTO_PREPARE as well. But your argument was aimed at something a bit different, so I missed the global picture here. 
The point is, there’s no need to do another rmean_collect(), as it happens later when sql_execute is being called. Otherwise the stats would have been collected twice for each executed statement as it was obvious in different tests (that I also added in the follow up patch).
 
Also, I’m wondering if we need to collect the stats for IPROTO_PREPARE in sql_reprepare as well? It seems relevant due the nature of this method: «Re-compile statement and refresh global prepared statement cache with the newest value». Meaning, it’s another instance of prepare, right? 
 
----------------------------------------------------------------------
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.
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/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated  
 
@ChangeLog
- Collecting statistics for box prepare and execute methods
    gh-4756
 
 src/box/execute.c        |  3 +++
 test/sql/iproto.result   | 31 +++++++++++++++++++++++++++++++
 test/sql/iproto.test.lua | 14 ++++++++++++++
 3 files changed, 48 insertions(+)
 
diff --git a/src/box/execute.c b/src/box/execute.c
index dc8dce81c..24f8529ec 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -48,6 +48,7 @@
 #include "box/lua/execute.h"
 #include "box/sql_stmt_cache.h"
 #include "session.h"
+#include "rmean.h"
 
 const char *sql_info_key_strs[] = {
     "row_count",
@@ -608,6 +609,7 @@ sql_prepare(const char *sql, int len, struct port *port)
 {
     uint32_t stmt_id = sql_stmt_calculate_id(sql, len);
     struct sql_stmt *stmt = sql_stmt_cache_find(stmt_id);
+    rmean_collect(rmean_box, IPROTO_PREPARE, 1);
     if (stmt == NULL) {
         if (sql_stmt_compile(sql, len, NULL, &stmt, NULL) != 0)
             return -1;
@@ -669,6 +671,7 @@ static inline int
 sql_execute(struct sql_stmt *stmt, struct port *port, struct region *region)
 {
     int rc, column_count = sql_column_count(stmt);
+    rmean_collect(rmean_box, IPROTO_EXECUTE, 1);
     if (column_count > 0) {
         /* Either ROW or DONE or ERROR. */
         while ((rc = sql_step(stmt)) == SQL_ROW) {

diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 7df11b0bf..0e046577d 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -788,6 +788,37 @@ box.schema.user.revoke('guest', 'create', 'space')
 space = nil
 ---
 ...
+--
+-- gh-4756: PREPARE and EXECUTE statistics should be present in box.stat()
+--
+p = box.stat().PREPARE.total
+---
+...
+e = box.stat().EXECUTE.total
+---
+...
+s = box.prepare([[ SELECT ?; ]])
+---
+...
+res, err = box.unprepare(s)
+---
+...
+box.execute('create table test (id integer primary key, a integer)')
+---
+- row_count: 1
+...
+box.execute('DROP TABLE test')
+---
+- row_count: 1
+...
+assert(box.stat().PREPARE.total == p + 1)
+---
+- true
+...
+assert(box.stat().EXECUTE.total == e + 2)
+---
+- true
+...
 -- Cleanup xlog
 box.snapshot()
 ---

diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 4019fb7a4..ec1f37035 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -242,5 +242,19 @@ box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'create', 'space')
 space = nil
 
+--
+-- gh-4756: PREPARE and EXECUTE statistics should be present in box.stat()
+--
+p = box.stat().PREPARE.total
+e = box.stat().EXECUTE.total
+
+s = box.prepare([[ SELECT ?; ]])
+res, err = box.unprepare(s)
+box.execute('create table test (id integer primary key, a integer)')
+box.execute('DROP TABLE test')
+
+assert(box.stat().PREPARE.total == p + 1)
+assert(box.stat().EXECUTE.total == e + 2)
+
 -- Cleanup xlog
 box.snapshot()
-- 
2.24.0
  
>Вторник, 25 февраля 2020, 23:29 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
> 
>LGTM as well.
>
>On 25/02/2020 14:02, Nikita Pettik wrote:
>> On 25 Feb 14:08, Maria Khaydich wrote:
>>>
>>> Thanks for the review, all done:
>>> ----------------------------------------------------------------------
>>> 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.
>>>  
>>> Closes #4756
>>> ---
>>> Issue:
>>>  https://github.com/tarantool/tarantool/issues/4756  
>>> Branch:
>>>  https://github.com/tarantool/tarantool/compare/eljashm/gh-4756-box-stat-execute-and-prepare-not-updated  
>>>  
>>> @ChangeLog
>>> - Fixed box.stat behavior - now it collects statistics on
>>>     prepare and execute methods as it should.
>>>     gh-4756
>>
>> LGTM. Vlad, is patch OK with you? If so, I will push it to master, 2.3 and
>> partially (only part related to EXECUTE statistics) to 2.2.
>>
 
 
--
Maria Khaydich
 

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

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

[Vladislav Shpilevoy]

Good catch, and shame on me for lgtming the patch!

On 26/02/2020 16:50, Maria Khaydich wrote:
> Hey, I’ve been doing the 2.2 version for the same issue and realized there’s a bug in the commit that was already merged to master.
>  
>> 1. Nikita is right, this is called when IPROTO_EXECUTE arrives. You can
> see that in iproto.cc. This is a separate question, why do we collect
> IPROTO_* statistics out of iproto. For now lets just use IPROTO_EXECUTE
> here. 
>  
> The reason I was collecting stats for box.prepare here in the first place is because it looked to me like the execution of method sql_prepare_and_execute implied the user is preparing _and_ executing something at the same time,

From what I know, this method is invoked when we execute a non prepared
statement. Such a query is not cached in the prep statements cache, and
was not sent as PREPARE. So this would be a surprise for a user to see
increased PREPARE after a usual execute().

> and whereas all other methods boiled down to either sql_prepare or sql_execute in the end, this one collected statistics only for IPROTO_EXECUTE, so I thought it was necessary to cover IPROTO_PREPARE as well. But your argument was aimed at something a bit different, so I missed the global picture here. 
> The point is, there’s no need to do another rmean_collect(), as it happens later when sql_execute is being called. Otherwise the stats would have been collected twice for each executed statement as it was obvious in different tests (that I also added in the follow up patch).
>  
> Also, I’m wondering if we need to collect the stats for IPROTO_PREPARE in sql_reprepare as well? It seems relevant due the nature of this method: «Re-compile statement and refresh global prepared statement cache with the newest value». Meaning, it’s another instance of prepare, right? 

This would be good to account somewhere, but certainly not here.
box.stat() is not for such internal things. This is just request counters.
Only for explicit requests. Reprepare would be good to add to box.stat.sql()
some day.

I propose to just drop rmean_collect() from sql_prepare_and_execute(). This
would solve the problem.

sql_prepare_and_execute() will be accounted as EXECUTE due to calling sql_execute().
sql_execute() will be accounted as EXECUTE.
sql_prepare() as PREPARE.

Nothing more.

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

[Nikita Pettik]

On 27 Feb 01:08, Vladislav Shpilevoy wrote:
> Good catch, and shame on me for lgtming the patch!
> 
> On 26/02/2020 16:50, Maria Khaydich wrote:
> > Hey, I’ve been doing the 2.2 version for the same issue and realized there’s a bug in the commit that was already merged to master.
> >  
> >> 1. Nikita is right, this is called when IPROTO_EXECUTE arrives. You can
> > see that in iproto.cc. This is a separate question, why do we collect
> > IPROTO_* statistics out of iproto. For now lets just use IPROTO_EXECUTE
> > here. 
> >  
> > The reason I was collecting stats for box.prepare here in the first place is because it looked to me like the execution of method sql_prepare_and_execute implied the user is preparing _and_ executing something at the same time,
> 
> From what I know, this method is invoked when we execute a non prepared
> statement. Such a query is not cached in the prep statements cache, and
> was not sent as PREPARE. So this would be a surprise for a user to see
> increased PREPARE after a usual execute().
> 
> > and whereas all other methods boiled down to either sql_prepare or sql_execute in the end, this one collected statistics only for IPROTO_EXECUTE, so I thought it was necessary to cover IPROTO_PREPARE as well. But your argument was aimed at something a bit different, so I missed the global picture here. 
> > The point is, there’s no need to do another rmean_collect(), as it happens later when sql_execute is being called. Otherwise the stats would have been collected twice for each executed statement as it was obvious in different tests (that I also added in the follow up patch).
> >  
> > Also, I’m wondering if we need to collect the stats for IPROTO_PREPARE in sql_reprepare as well?

sql_reprepare is a part of sql_prepare so we shouldn't account it.

> > It seems relevant due the nature of this method: «Re-compile statement and refresh global prepared statement cache with the newest value». Meaning, it’s another instance of prepare, right? 
> 
> This would be good to account somewhere, but certainly not here.
> box.stat() is not for such internal things. This is just request counters.
> Only for explicit requests. Reprepare would be good to add to box.stat.sql()
> some day.
> 
> I propose to just drop rmean_collect() from sql_prepare_and_execute(). This
> would solve the problem.

Exactly. Please send smth like 'hotfix' for the last commit, I guess it
is too late to force push to master.

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

[Maria Khaydich]

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


> Please send smth like 'hotfix' for the last commit, I guess it is too late to force push to master.
 
If I understood you correctly:
----------------------------------------------------------------------
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.

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  

 src/box/execute.c        |  1 -
 test/sql/iproto.result   | 16 ++++++++--------
 test/sql/iproto.test.lua |  5 +++--
 3 files changed, 11 insertions(+), 11 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..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')
+---
+- row_count: 1
 ...
 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..ec1f37035 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -249,11 +249,12 @@ p = box.stat().PREPARE.total
 e = box.stat().EXECUTE.total
 
 s = box.prepare([[ SELECT ?; ]])
-s:execute({42})
 res, err = box.unprepare(s)
+box.execute('create table test (id integer primary key, a integer)')
+box.execute('DROP TABLE test')
 
 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
 
----------------------------------------------------------------------
Also that was delegated.
>  Kirill, it also should be backported to 2.2 skipping part related to  'prepare' feature.
 
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.
 
Version for 2.2 only includes box.execute.
 
Closes #4756
---
Branch:
https://github.com/tarantool/tarantool/tree/eljashm/gh-4756-box-stat-execute-prepare-2.2-version  
 
 src/box/execute.c        |  2 ++
 test/sql/iproto.result   | 18 ++++++++++++++++++
 test/sql/iproto.test.lua |  9 +++++++++
 3 files changed, 29 insertions(+)
diff --git a/src/box/execute.c b/src/box/execute.c
index 68e94e442..cf4d8afe6 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -45,6 +45,7 @@
 #include "tuple.h"
 #include "sql/vdbe.h"
 #include "box/lua/execute.h"
+#include "rmean.h"
 
 const char *sql_info_key_strs[] = {
     "row_count",
@@ -417,6 +418,7 @@ static inline int
 sql_execute(struct sql_stmt *stmt, struct port *port, struct region *region)
 {
     int rc, column_count = sql_column_count(stmt);
+    rmean_collect(rmean_box, IPROTO_EXECUTE, 1);
     if (column_count > 0) {
         /* Either ROW or DONE or ERROR. */
         while ((rc = sql_step(stmt)) == SQL_ROW) {
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index f43e87117..8a9bfa405 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -820,6 +820,24 @@ box.schema.user.revoke('guest', 'create', 'space')
 space = nil
 ---
 ...
+--
+-- gh-4756: EXECUTE statistics should be present in box.stat()
+--
+e = box.stat().EXECUTE.total
+---
+...
+box.execute('create table test3 (id int primary key, a NUMBER, b text)')
+---
+- row_count: 1
+...
+box.execute('drop table test3')
+---
+- row_count: 1
+...
+assert(box.stat().EXECUTE.total == e + 2)
+---
+- true
+...
 -- Cleanup xlog
 box.snapshot()
 ---
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index dd60afe79..d0e979133 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -250,5 +250,14 @@ box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'create', 'space')
 space = nil
 
+--
+-- gh-4756: PREPARE and EXECUTE statistics should be present in box.stat()
+--
+e = box.stat().EXECUTE.total
+box.execute('create table test3 (id int primary key, a NUMBER, b text)')
+box.execute('drop table test3')
+
+assert(box.stat().EXECUTE.total == e + 2)
+
 -- Cleanup xlog
 box.snapshot()
-- 
2.24.0
 
--
Maria Khaydich
 
 

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

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

[Vladislav Shpilevoy]

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?

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

[Nikita Pettik]

On 03 Mar 19:42, Maria Khaydich wrote:
> 
> > Please send smth like 'hotfix' for the last commit, I guess it is too late to force push to master.
>  
> If I understood you correctly:
> ----------------------------------------------------------------------
> 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.
> 
> There was a bug in previous commit resulting in collecting statistics on
> box.execute twice in some cases.
>  
> Closes #4756
> ---
> diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
> index 9eac91d2c..ec1f37035 100644
> --- a/test/sql/iproto.test.lua
> +++ b/test/sql/iproto.test.lua
> @@ -249,11 +249,12 @@ p = box.stat().PREPARE.total
>  e = box.stat().EXECUTE.total
>  
>  s = box.prepare([[ SELECT ?; ]])
> -s:execute({42})

Why did you get rid of s:execute() call? Now both prepared:execute()
and box.execute() use the same function (sql_execute()) under the hood,
but it may change in future. Please, leave both test cases.

Lgtm otherwise.

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

[Maria Khaydich]

[-- 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 --]

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

[Nikita Pettik]

On 06 Mar 14:34, Maria Khaydich wrote:
> 
> 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  

LGTM. Pushed to master as obvious.
 

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

[Nikita Pettik]

On 06 Mar 14:32, Nikita Pettik wrote:
> On 06 Mar 14:34, Maria Khaydich wrote:
> > 
> > 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  
> 
> LGTM. Pushed to master as obvious.
>

Pushed also to 2.3 and 2.2, update changelog for 2.2.3 release.