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

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

Thanks for the answers, LGTM.

Sergos

On 30 Dec 12:27, Nikita Pettik wrote:
> 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.
>