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

["Maria Khaydich" <maria.khaydich@tarantool.org>]

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