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.
@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