Re: [Tarantool-patches] [PATCH v3 20/20] sql: add cache statistics to box.info

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

Thanks for the whole patchset!
I got one major question: in case session is ended - there should be a
refcnt reduction for all prepared stmts of this session. I didn't see
that in the code.

Regards,
Sergos

On 20 Dec 15:47, Nikita Pettik wrote:
> To track current memory occupied by prepared statements and number of
> them, let's extend box.info submodule with .sql statistics: now it
> contains current total size of prepared statements and their count.
> 
> @TarantoolBot document
> Title: Prepared statements in SQL
> 
> Now it is possible to prepare (i.e. compile into byte-code and save to
> the cache) statement and execute it several times. Mechanism is similar
> to ones in other DBs. Prepared statement is identified by numeric
> ID, which are returned alongside with prepared statement handle.
> Note that they are not sequential and represent value of hash function
> applied to the string containing original SQL request.
> Prepared statement holder is shared among all sessions. However, session
> has access only to statements which have been prepared in scope of it.
> There's no eviction policy like in any cache; to remove statement from
> holder explicit unprepare request is required. Alternatively, session's
> disconnect also removes statements from holder.
> Several sessions can share one prepared statement, which will be
> destroyed when all related sessions are disconnected or send unprepare
> request. Memory limit for prepared statements is adjusted by
> box.cfg{sql_cache_size} handle (can be set dynamically;
> 
> Any DDL operation leads to expiration of all prepared statements: they
> should be manually removed or re-prepared.

Does it mean that for user code there should be called unprepare(s.id) and
s = prepare(orginal sql text)? If I got it right from previous patches
the re-prepare is called automatically?

> Prepared statements are available in local mode (i.e. via box.prepare()
> function) and are supported in IProto protocol. In the latter case
> next IProto keys are used to make up/receive requests/responses:
> IPROTO_PREPARE - new IProto command; key is 0x13. It can be sent with
> one of two mandatory keys: IPROTO_SQL_TEXT (0x40 and assumes string value)
> or IPROTO_STMT_ID (0x43 and assumes integer value). Depending on body it
> means to prepare or unprepare SQL statement: IPROTO_SQL_TEXT implies prepare
> request, meanwhile IPROTO_STMT_ID - unprepare;
> IPROTO_BIND_METADATA (0x33 and contains parameters metadata of type map)
> and IPROTO_BIND_COUNT (0x34 and corresponds to the count of parameters to
> be bound) are response keys. They are mandatory members of result of
> IPROTO_PREPARE execution.
> 
> To track statistics of used memory and number of currently prepared
> statements, box.info is extended with SQL statistics:
> 
> box.info:sql().cache.stmt_count - number of prepared statements;
> box.info:sql().cache.size - size of occupied by prepared statements memory.
> 
> Typical workflow with prepared statements is following:
> 
> s = box.prepare("SELECT * FROM t WHERE id = ?;")
> s:execute({1}) or box.execute(s.sql_str, {1})
> s:execute({2}) or box.execute(s.sql_str, {2})
> s:unprepare() or box.unprepare(s.query_id)
> 
> Structure of object is following (member : type):
> 
> - stmt_id: integer
>   execute: function
>   params: map [name : string, type : integer]
>   unprepare: function
>   metadata: map [name : string, type : integer]
>   param_count: integer
> ...
> 
> In terms of remote connection:
> 
> cn = netbox:connect(addr)
> s = cn:prepare("SELECT * FROM t WHERE id = ?;")
> cn:execute(s.sql_str, {1})
> cn:unprepare(s.query_id)
> 
> Closese #2592

Misprint above: Closes

> ---
>  src/box/lua/info.c         | 25 +++++++++++++++++++++++++
>  src/box/sql_stmt_cache.c   | 16 ++++++++++++++++
>  src/box/sql_stmt_cache.h   |  8 ++++++++
>  test/box/info.result       |  1 +
>  test/sql/prepared.result   | 34 +++++++++++++++++++++++++++++++++-
>  test/sql/prepared.test.lua | 12 +++++++++++-
>  6 files changed, 94 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/lua/info.c b/src/box/lua/info.c
> index e029e0e17..8933ea829 100644
> --- a/src/box/lua/info.c
> +++ b/src/box/lua/info.c
> @@ -45,6 +45,7 @@
>  #include "box/gc.h"
>  #include "box/engine.h"
>  #include "box/vinyl.h"
> +#include "box/sql_stmt_cache.h"
>  #include "main.h"
>  #include "version.h"
>  #include "box/box.h"
> @@ -494,6 +495,29 @@ lbox_info_vinyl(struct lua_State *L)
>  	return 1;
>  }
>  
> +static int
> +lbox_info_sql_call(struct lua_State *L)
> +{
> +	struct info_handler h;
> +	luaT_info_handler_create(&h, L);
> +	sql_stmt_cache_stat(&h);
> +
> +	return 1;
> +}
> +
> +static int
> +lbox_info_sql(struct lua_State *L)
> +{
> +	lua_newtable(L);
> +	lua_newtable(L); /* metatable */
> +	lua_pushstring(L, "__call");
> +	lua_pushcfunction(L, lbox_info_sql_call);
> +	lua_settable(L, -3);
> +
> +	lua_setmetatable(L, -2);
> +	return 1;
> +}
> +
>  static const struct luaL_Reg lbox_info_dynamic_meta[] = {
>  	{"id", lbox_info_id},
>  	{"uuid", lbox_info_uuid},
> @@ -509,6 +533,7 @@ static const struct luaL_Reg lbox_info_dynamic_meta[] = {
>  	{"memory", lbox_info_memory},
>  	{"gc", lbox_info_gc},
>  	{"vinyl", lbox_info_vinyl},
> +	{"sql", lbox_info_sql},
>  	{NULL, NULL}
>  };
>  
> diff --git a/src/box/sql_stmt_cache.c b/src/box/sql_stmt_cache.c
> index 742e4135c..a4f5f2745 100644
> --- a/src/box/sql_stmt_cache.c
> +++ b/src/box/sql_stmt_cache.c
> @@ -34,6 +34,7 @@
>  #include "error.h"
>  #include "execute.h"
>  #include "diag.h"
> +#include "info/info.h"
>  
>  static struct sql_stmt_cache sql_stmt_cache;
>  
> @@ -48,6 +49,21 @@ sql_stmt_cache_init()
>  	rlist_create(&sql_stmt_cache.gc_queue);
>  }
>  
> +void
> +sql_stmt_cache_stat(struct info_handler *h)
> +{
> +	info_begin(h);
> +	info_table_begin(h, "cache");
> +	info_append_int(h, "size", sql_stmt_cache.mem_used);
> +	uint32_t entry_count = 0;
> +	mh_int_t i;
> +	mh_foreach(sql_stmt_cache.hash, i)
> +		entry_count++;
> +	info_append_int(h, "stmt_count", entry_count);
> +	info_table_end(h);
> +	info_end(h);
> +}
> +
>  static size_t
>  sql_cache_entry_sizeof(struct sql_stmt *stmt)
>  {
> diff --git a/src/box/sql_stmt_cache.h b/src/box/sql_stmt_cache.h
> index f3935a27f..468cbc9a0 100644
> --- a/src/box/sql_stmt_cache.h
> +++ b/src/box/sql_stmt_cache.h
> @@ -41,6 +41,7 @@ extern "C" {
>  
>  struct sql_stmt;
>  struct mh_i64ptr_t;
> +struct info_handler;
>  
>  struct stmt_cache_entry {
>  	/** Prepared statement itself. */
> @@ -90,6 +91,13 @@ struct sql_stmt_cache {
>  void
>  sql_stmt_cache_init();
>  
> +/**
> + * Store statistics concerning cache (current size and number
> + * of statements in it) into info handler @h.
> + */
> +void
> +sql_stmt_cache_stat(struct info_handler *h);
> +
>  /**
>   * Erase session local hash: unref statements belong to this
>   * session and deallocate hash itself.
> diff --git a/test/box/info.result b/test/box/info.result
> index af81f7add..2e84cbbe3 100644
> --- a/test/box/info.result
> +++ b/test/box/info.result
> @@ -84,6 +84,7 @@ t
>    - replication
>    - ro
>    - signature
> +  - sql
>    - status
>    - uptime
>    - uuid
> diff --git a/test/sql/prepared.result b/test/sql/prepared.result
> index 2f4983b00..9951a4e43 100644
> --- a/test/sql/prepared.result
> +++ b/test/sql/prepared.result
> @@ -64,6 +64,21 @@ test_run:cmd("setopt delimiter ''");
>   | - true
>   | ...
>  
> +-- Check default cache statistics.
> +--
> +box.info.sql()
> + | ---
> + | - cache:
> + |     size: 0
> + |     stmt_count: 0
> + | ...
> +box.info:sql()
> + | ---
> + | - cache:
> + |     size: 0
> + |     stmt_count: 0
> + | ...
> +
>  -- Test local interface and basic capabilities of prepared statements.
>  --
>  execute('CREATE TABLE test (id INT PRIMARY KEY, a NUMBER, b TEXT)')
> @@ -144,6 +159,15 @@ execute(s.stmt_id, {1, 3})
>   |   rows: []
>   | ...
>  
> +assert(box.info.sql().cache.stmt_count ~= 0)
> + | ---
> + | - true
> + | ...
> +assert(box.info.sql().cache.size ~= 0)
> + | ---
> + | - true
> + | ...
> +
>  test_run:cmd("setopt delimiter ';'")
>   | ---
>   | - true
> @@ -584,8 +608,16 @@ unprepare(s1.stmt_id)
>  -- Setting cache size to 0 is possible only in case if
>  -- there's no any prepared statements right now .
>  --
> -box.cfg{sql_cache_size = 0}
> +box.cfg{sql_cache_size = 0 }
> + | ---
> + | ...
> +assert(box.info.sql().cache.stmt_count == 0)
>   | ---
> + | - true
> + | ...
> +assert(box.info.sql().cache.size == 0)
> + | ---
> + | - true
>   | ...
>  prepare("SELECT a FROM test;")
>   | ---
> diff --git a/test/sql/prepared.test.lua b/test/sql/prepared.test.lua
> index c464cc21a..5820525d1 100644
> --- a/test/sql/prepared.test.lua
> +++ b/test/sql/prepared.test.lua
> @@ -43,6 +43,11 @@ end;
>  
>  test_run:cmd("setopt delimiter ''");
>  
> +-- Check default cache statistics.
> +--
> +box.info.sql()
> +box.info:sql()
> +
>  -- Test local interface and basic capabilities of prepared statements.
>  --
>  execute('CREATE TABLE test (id INT PRIMARY KEY, a NUMBER, b TEXT)')
> @@ -60,6 +65,9 @@ s.param_count
>  execute(s.stmt_id, {1, 2})
>  execute(s.stmt_id, {1, 3})
>  
> +assert(box.info.sql().cache.stmt_count ~= 0)
> +assert(box.info.sql().cache.size ~= 0)
> +
>  test_run:cmd("setopt delimiter ';'")
>  if not is_remote then
>      res = s:execute({1, 2})
> @@ -207,7 +215,9 @@ unprepare(s1.stmt_id)
>  -- Setting cache size to 0 is possible only in case if
>  -- there's no any prepared statements right now .
>  --
> -box.cfg{sql_cache_size = 0}
> +box.cfg{sql_cache_size = 0 }
> +assert(box.info.sql().cache.stmt_count == 0)
> +assert(box.info.sql().cache.size == 0)
>  prepare("SELECT a FROM test;")
>  box.cfg{sql_cache_size = 0}
>  
> -- 
> 2.15.1
>