From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4416E46970E for ; Mon, 30 Dec 2019 17:15:51 +0300 (MSK) Date: Mon, 30 Dec 2019 17:15:50 +0300 From: Sergey Ostanevich Message-ID: <20191230141550.GF1222@tarantool.org> References: <35b9568be5d828100fefc6e19926757b19ec689a.1576844632.git.korablev@tarantool.org> <20191225152354.GR19594@tarantool.org> <20191230102726.GC29923@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191230102726.GC29923@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v3 18/20] box: introduce prepared statements List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.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. >