<HTML><BODY><div>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.</div><div> </div><div>> 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. </div><div> </div><div>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. </div><div>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).</div><div> </div><div>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? </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>There was a bug in previous commit resulting in collecting statistics on<br>box.execute twice in some cases.</div><div> </div><div>Closes #4756<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>@ChangeLog<br>- Collecting statistics for box prepare and execute methods<br> gh-4756</div><div> </div><div> src/box/execute.c | 3 +++<br> test/sql/iproto.result | 31 +++++++++++++++++++++++++++++++<br> test/sql/iproto.test.lua | 14 ++++++++++++++<br> 3 files changed, 48 insertions(+)</div><div> </div><div>diff --git a/src/box/execute.c b/src/box/execute.c<br>index dc8dce81c..24f8529ec 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) {</div><div><br>diff --git a/test/sql/iproto.result b/test/sql/iproto.result<br>index 7df11b0bf..0e046577d 100644<br>--- a/test/sql/iproto.result<br>+++ b/test/sql/iproto.result<br>@@ -788,6 +788,37 @@ 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>+res, err = box.unprepare(s)<br>+---<br>+...<br>+box.execute('create table test (id integer primary key, a integer)')<br>+---<br>+- row_count: 1<br>+...<br>+box.execute('DROP TABLE test')<br>+---<br>+- row_count: 1<br>+...<br>+assert(box.stat().PREPARE.total == p + 1)<br>+---<br>+- true<br>+...<br>+assert(box.stat().EXECUTE.total == e + 2)<br>+---<br>+- true<br>+...<br> -- Cleanup xlog<br> box.snapshot()<br> ---</div><div><br>diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua<br>index 4019fb7a4..ec1f37035 100644<br>--- a/test/sql/iproto.test.lua<br>+++ b/test/sql/iproto.test.lua<br>@@ -242,5 +242,19 @@ 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>+res, err = box.unprepare(s)<br>+box.execute('create table test (id integer primary key, a integer)')<br>+box.execute('DROP TABLE test')<br>+<br>+assert(box.stat().PREPARE.total == p + 1)<br>+assert(box.stat().EXECUTE.total == e + 2)<br>+<br> -- Cleanup xlog<br> box.snapshot()<br>-- <br>2.24.0</div></div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Вторник, 25 февраля 2020, 23:29 +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_15826625630358649737_BODY">LGTM as well.<br><br>On 25/02/2020 14:02, Nikita Pettik wrote:<div class="mail-quote-collapse">> On 25 Feb 14:08, Maria Khaydich wrote:<br>>><br>>> Thanks for the review, all done:<br>>> ----------------------------------------------------------------------<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>>> 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> <br>>> <br>>> @ChangeLog<br>>> - Fixed box.stat behavior - now it collects statistics on<br>>> prepare and execute methods as it should.<br>>> gh-4756<br>><br>> LGTM. Vlad, is patch OK with you? If so, I will push it to master, 2.3 and<br>> partially (only part related to EXECUTE statistics) to 2.2.<br>></div></div></div></div></div></blockquote><div> <div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Maria Khaydich</div></div></div><div> </div></div></BODY></HTML>