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