Re: [Tarantool-patches] [PATCH v3 18/20] box: introduce prepared statements

[Nikita Pettik <korablev@tarantool.org>]

On 25 Dec 18:23, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for the patch, LGTM with just 2 nits below. 
> 
> Sergos
> 
> On 20 Dec 15:47, Nikita Pettik wrote:
> > This patch introduces local prepared statements. Support of prepared
> > statements in IProto protocol and netbox is added in the next patch.
> > 
> > Prepared statement is an opaque instance of SQL Virtual Machine. It can
> > be executed several times without necessity of query recompilation. To
> > achieve this one can use box.prepare(...) function. It takes string of
> > SQL query to be prepared; returns extended set of meta-information
> > including statement's ID, parameter's types and names, types and names
> > of columns of the resulting set, count of parameters to be bound.  Lua
> > object representing result of :prepare() invocation also features two
> > methods - :execute() and :unprepare(). They correspond to
> > box.execute(stmt.stmt_id) and box.unprepare(stmt.stmt_id), i.e.
> > automatically substitute string of prepared statement to be executed.
> > Statements are held in prepared statement cache - for details see
> > previous commit.  After schema changes all prepared statement located in
> > cache are considered to be expired - they must be re-prepared by
> > separate :prepare() call (or be invalidated with :unrepare()).
> > 
> > Two sessions can share one prepared statements. But in current
> > implementation if statement is executed by one session, another one is
> > not able to use it and will compile it from scratch and than execute.
> 
> It would be nice to mention plans on what should/will be done for
> resolution of this.

I'm going to file an issue to provide optimization for this.
I suppose we can copy VM before executing it. But now VM
consists of several parts which are allocated/deallocated as
separate chunks. So to achieve fast copy and resources finalization
for VM it would be nice to copy them into one chunk before execution.

> Also, my previous question on DDL during the
> execution of the statement is valid - one session can ruin execution
> of a prepared statement in another session with a DDL. Should there 
> be some guard for DDL until all executions of prep statements are finished?

I don't think it is good idea. Firstly, it would lock DB for any DDL
until there's at least one statemenet under execution (taking into
account that Tarantool as a DB is assumed to be under load all the time
it means in fact that DDL would not be allowed at all). Secondly, it is
documented behaviour, so users should be aware of it.

> > SQL cache memory limit is regulated by box{sql_cache_size} which can be
> > set dynamically. However, it can be set to the value which is less than
> > the size of current free space in cache (since otherwise some statements
> > can disappear from cache).
> > 
> > Part of #2592
> > ---
> >  src/box/errcode.h          |   1 +
> >  src/box/execute.c          | 114 ++++++++
> >  src/box/execute.h          |  16 +-
> >  src/box/lua/execute.c      | 213 +++++++++++++-
> >  src/box/lua/execute.h      |   2 +-
> >  src/box/lua/init.c         |   2 +-
> >  src/box/sql/prepare.c      |   9 -
> >  test/box/misc.result       |   3 +
> >  test/sql/engine.cfg        |   3 +
> >  test/sql/prepared.result   | 687 +++++++++++++++++++++++++++++++++++++++++++++
> >  test/sql/prepared.test.lua | 240 ++++++++++++++++
> >  11 files changed, 1267 insertions(+), 23 deletions(-)
> >  create mode 100644 test/sql/prepared.result
> >  create mode 100644 test/sql/prepared.test.lua
> > 
> > diff --git a/src/box/errcode.h b/src/box/errcode.h
> > index ee44f61b3..9e12f3a31 100644
> > --- a/src/box/errcode.h
> > +++ b/src/box/errcode.h
> > @@ -259,6 +259,7 @@ struct errcode_record {
> >  	/*204 */_(ER_SQL_FUNC_WRONG_RET_COUNT,	"SQL expects exactly one argument returned from %s, got %d")\
> >  	/*205 */_(ER_FUNC_INVALID_RETURN_TYPE,	"Function '%s' returned value of invalid type: expected %s got %s") \
> >  	/*206 */_(ER_SQL_PREPARE,		"Failed to prepare SQL statement: %s") \
> > +	/*207 */_(ER_WRONG_QUERY_ID,		"Prepared statement with id %u does not exist") \
> >  
> >  /*
> >   * !IMPORTANT! Please follow instructions at start of the file
> > diff --git a/src/box/execute.c b/src/box/execute.c
> > index 3bc4988b7..09224c23a 100644
> > --- a/src/box/execute.c
> > +++ b/src/box/execute.c
> > @@ -30,6 +30,7 @@
> >   */
> >  #include "execute.h"
> >  
> > +#include "assoc.h"
> >  #include "bind.h"
> >  #include "iproto_constants.h"
> >  #include "sql/sqlInt.h"
> > @@ -45,6 +46,8 @@
> >  #include "tuple.h"
> >  #include "sql/vdbe.h"
> >  #include "box/lua/execute.h"
> > +#include "box/sql_stmt_cache.h"
> > +#include "session.h"
> >  
> >  const char *sql_info_key_strs[] = {
> >  	"row_count",
> > @@ -413,6 +416,81 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
> >  	return 0;
> >  }
> >  
> > +static bool
> > +sql_stmt_check_schema_version(struct sql_stmt *stmt)
> 
> The naming could be better - since you state something as true or false,
> it should be definitive, like sql_stmt_schema_version_is_valid()

Ok, let's rename it to sql_stmt_schema_version_is_valid()

> > +{
> > +	return sql_stmt_schema_version(stmt) == box_schema_version();
> > +}
> > +
> > +int
> > +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);
> > +	if (stmt == NULL) {
> > +		if (sql_stmt_compile(sql, len, NULL, &stmt, NULL) != 0)
> > +			return -1;
> > +		if (sql_stmt_cache_insert(stmt) != 0) {
> > +			sql_stmt_finalize(stmt);
> > +			return -1;
> > +		}
> > +	} else {
> > +		if (! sql_stmt_check_schema_version(stmt)) {
> 
> The unaries should not be space-delimited as per C style $3.1
> https://www.tarantool.io/en/doc/2.2/dev_guide/c_style_guide/

Fixed.