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