[Tarantool-patches] [PATCH v2 00/16] sql: prepared statements

[Tarantool-patches] [PATCH v2 00/16] sql: prepared statements

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/tree/np/gh-2592-prepared-statemets-v2
Issue: https://github.com/tarantool/tarantool/issues/2592

Patch v1: https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/012274.html

Changes in this iteration:

- Now cache is global and statements are shared among all session
  (main reason for this change is to increase performance);
- Cache follows next eviction policy: when there's no space for
  another one statement, statement which was prepared earlier than others
  is removed from cache (strickly speaking it's not LRU policy, but rather
  FIFO; however, the same approach is already used for tuple cache and it
  is called LRU anyway);
- Owing to the previous point, there's no need in 'unprepare' invocation
  at all (it concerns both manual request and on_disconnect clean-up);
- Also due to using replacement policy eviction from cache should
  not be "visible" to user, i.e. statement should still be capable of being
  executed. Keeping only numeric ids doesn't allow this, so now we use
  string ids (which consist of original SQL request);
- Statement is automatically re-prepared (on demand) if it is expired
  as a result of DDL request;
- In case statement is busy (is executed right now by other session), it
  is compiled from scratch and then executed.

Nikita Pettik (16):
  sql: remove sql_prepare_v2()
  sql: refactor sql_prepare() and sqlPrepare()
  sql: move sql_prepare() declaration to box/execute.h
  sql: rename sqlPrepare() to sql_compile()
  sql: move sql_finalize() to execute.h
  port: increase padding of struct port
  port: add dump format and request type to port_sql
  sql: resurrect sql_bind_parameter_count() function
  sql: resurrect sql_bind_parameter_name()
  sql: add sql_stmt_schema_version()
  sql: introduce sql_stmt_sizeof() function
  box: increment schema_version on ddl operations
  sql: introduce sql_stmt_query_str() method
  sql: introduce cache for prepared statemets
  box: introduce prepared statements
  netbox: introduce prepared statements

 src/box/CMakeLists.txt          |   1 +
 src/box/alter.cc                |   3 +
 src/box/box.cc                  |  20 ++
 src/box/box.h                   |   1 +
 src/box/ck_constraint.c         |   1 +
 src/box/errcode.h               |   1 +
 src/box/execute.c               | 213 ++++++++++++++-
 src/box/execute.h               |  51 ++++
 src/box/iproto.cc               |  25 +-
 src/box/iproto_constants.c      |   6 +-
 src/box/iproto_constants.h      |   4 +
 src/box/lua/cfg.cc              |  12 +
 src/box/lua/execute.c           | 161 ++++++++++-
 src/box/lua/execute.h           |   2 +-
 src/box/lua/init.c              |   2 +-
 src/box/lua/load_cfg.lua        |   3 +
 src/box/lua/net_box.c           |  79 ++++++
 src/box/lua/net_box.lua         |  13 +
 src/box/prep_stmt.c             | 186 +++++++++++++
 src/box/prep_stmt.h             | 112 ++++++++
 src/box/session.cc              |   1 +
 src/box/sql.c                   |   3 +
 src/box/sql/analyze.c           |  16 +-
 src/box/sql/legacy.c            |   3 +-
 src/box/sql/prepare.c           |  56 +---
 src/box/sql/sqlInt.h            |  44 +--
 src/box/sql/vdbe.h              |   2 +-
 src/box/sql/vdbeInt.h           |   1 -
 src/box/sql/vdbeapi.c           |  95 +++++--
 src/box/sql/vdbeaux.c           |   5 +-
 src/box/xrow.c                  |   2 +-
 src/box/xrow.h                  |   2 +-
 src/lib/core/port.h             |   2 +-
 test/app-tap/init_script.result |  37 +--
 test/box/admin.result           |   2 +
 test/box/cfg.result             |   7 +
 test/box/cfg.test.lua           |   1 +
 test/box/misc.result            |   3 +
 test/sql/engine.cfg             |   4 +
 test/sql/prepared.result        | 592 ++++++++++++++++++++++++++++++++++++++++
 test/sql/prepared.test.lua      | 227 +++++++++++++++
 41 files changed, 1851 insertions(+), 150 deletions(-)
 create mode 100644 src/box/prep_stmt.c
 create mode 100644 src/box/prep_stmt.h
 create mode 100644 test/sql/prepared.result
 create mode 100644 test/sql/prepared.test.lua

-- 
2.15.1

[Tarantool-patches] [PATCH v2 01/16] sql: remove sql_prepare_v2()

[Nikita Pettik]

There are two versions of the same function (sql_prepare()) which are
almost identical. Let's keep more relevant version sql_prepare_v2() but
rename it to sql_prepare() in order to avoid any mess.

Needed for #3292
---
 src/box/execute.c     |  2 +-
 src/box/sql/legacy.c  |  2 +-
 src/box/sql/prepare.c | 32 ++++----------------------------
 src/box/sql/sqlInt.h  | 25 +++++++++++--------------
 src/box/sql/vdbeapi.c |  2 +-
 5 files changed, 18 insertions(+), 45 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index e8b012e5b..130a3f675 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -443,7 +443,7 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
 {
 	struct sql_stmt *stmt;
 	struct sql *db = sql_get();
-	if (sql_prepare_v2(db, sql, len, &stmt, NULL) != 0)
+	if (sql_prepare(db, sql, len, &stmt, NULL) != 0)
 		return -1;
 	assert(stmt != NULL);
 	port_sql_create(port, stmt);
diff --git a/src/box/sql/legacy.c b/src/box/sql/legacy.c
index 0b1370f4a..bfd1e32b9 100644
--- a/src/box/sql/legacy.c
+++ b/src/box/sql/legacy.c
@@ -70,7 +70,7 @@ sql_exec(sql * db,	/* The database on which the SQL executes */
 		char **azVals = 0;
 
 		pStmt = 0;
-		rc = sql_prepare_v2(db, zSql, -1, &pStmt, &zLeftover);
+		rc = sql_prepare(db, zSql, -1, &pStmt, &zLeftover);
 		assert(rc == 0 || pStmt == NULL);
 		if (rc != 0)
 			continue;
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 0ecc676e2..35e81212d 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -204,36 +204,12 @@ sqlReprepare(Vdbe * p)
 	return 0;
 }
 
-/*
- * Two versions of the official API.  Legacy and new use.  In the legacy
- * version, the original SQL text is not saved in the prepared statement
- * and so if a schema change occurs, an error is returned by
- * sql_step().  In the new version, the original SQL text is retained
- * and the statement is automatically recompiled if an schema change
- * occurs.
- */
-int
-sql_prepare(sql * db,		/* Database handle. */
-		const char *zSql,	/* UTF-8 encoded SQL statement. */
-		int nBytes,		/* Length of zSql in bytes. */
-		sql_stmt ** ppStmt,	/* OUT: A pointer to the prepared statement */
-		const char **pzTail)	/* OUT: End of parsed string */
-{
-	int rc = sqlPrepare(db, zSql, nBytes, 0, 0, ppStmt, pzTail);
-	assert(rc == 0 || ppStmt == NULL || *ppStmt == NULL);	/* VERIFY: F13021 */
-	return rc;
-}
-
 int
-sql_prepare_v2(sql * db,	/* Database handle. */
-		   const char *zSql,	/* UTF-8 encoded SQL statement. */
-		   int nBytes,	/* Length of zSql in bytes. */
-		   sql_stmt ** ppStmt,	/* OUT: A pointer to the prepared statement */
-		   const char **pzTail	/* OUT: End of parsed string */
-    )
+sql_prepare(struct sql *db, const char *sql, int length, struct sql_stmt **stmt,
+	    const char **sql_tail)
 {
-	int rc = sqlPrepare(db, zSql, nBytes, 1, 0, ppStmt, pzTail);
-	assert(rc == 0 || ppStmt == NULL || *ppStmt == NULL);	/* VERIFY: F13021 */
+	int rc = sqlPrepare(db, sql, length, 1, 0, stmt, sql_tail);
+	assert(rc == 0 || stmt == NULL || *stmt == NULL);
 	return rc;
 }
 
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 2594b73e0..7bd952a17 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -468,21 +468,18 @@ typedef void (*sql_destructor_type) (void *);
 #define SQL_STATIC      ((sql_destructor_type)0)
 #define SQL_TRANSIENT   ((sql_destructor_type)-1)
 
+/**
+ * Prepare (compile into VDBE byte-code) statement.
+ *
+ * @param db Database handle.
+ * @param sql UTF-8 encoded SQL statement.
+ * @param length Length of @param sql in bytes.
+ * @param[out] stmt A pointer to the prepared statement.
+ * @param[out] sql_tail End of parsed string.
+ */
 int
-sql_prepare(sql * db,	/* Database handle */
-		const char *zSql,	/* SQL statement, UTF-8 encoded */
-		int nByte,	/* Maximum length of zSql in bytes. */
-		sql_stmt ** ppStmt,	/* OUT: Statement handle */
-		const char **pzTail	/* OUT: Pointer to unused portion of zSql */
-	);
-
-int
-sql_prepare_v2(sql * db,	/* Database handle */
-		   const char *zSql,	/* SQL statement, UTF-8 encoded */
-		   int nByte,	/* Maximum length of zSql in bytes. */
-		   sql_stmt ** ppStmt,	/* OUT: Statement handle */
-		   const char **pzTail	/* OUT: Pointer to unused portion of zSql */
-	);
+sql_prepare(struct sql *db, const char *sql, int length, struct sql_stmt **stmt,
+	    const char **sql_tail);
 
 int
 sql_step(sql_stmt *);
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 685212d91..12449d3bc 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -452,7 +452,7 @@ sqlStep(Vdbe * p)
 		checkProfileCallback(db, p);
 
 	if (p->isPrepareV2 && rc != SQL_ROW && rc != SQL_DONE) {
-		/* If this statement was prepared using sql_prepare_v2(), and an
+		/* If this statement was prepared using sql_prepare(), and an
 		 * error has occurred, then return an error.
 		 */
 		if (p->is_aborted)
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 01/16] sql: remove sql_prepare_v2()

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> There are two versions of the same function (sql_prepare()) which are
> almost identical. Let's keep more relevant version sql_prepare_v2() but
> rename it to sql_prepare() in order to avoid any mess.
> 
> Needed for #3292

lgtm


-- 
Konstantin Osipov, Moscow, Russia

[Tarantool-patches] [PATCH v2 02/16] sql: refactor sql_prepare() and sqlPrepare()

[Nikita Pettik]

- Removed saveSqlFlag as argument from sqlPrepare(). It was used to
  indicate that its caller is sql_prepare_v2() not sql_prepare().
  Since in previous commit we've left only one version of this function
  let's remove this flag at all.

- Removed struct db from list of sql_prepare() arguments. There's one
  global database handler and it can be obtained by sql_get() call.
  Hence, it makes no sense to pass around this argument.

Needed for #3292
---
 src/box/execute.c     |  3 +--
 src/box/sql/analyze.c | 15 +++++++--------
 src/box/sql/legacy.c  |  2 +-
 src/box/sql/prepare.c | 10 ++++------
 src/box/sql/sqlInt.h  |  3 +--
 src/box/sql/vdbe.h    |  2 +-
 src/box/sql/vdbeInt.h |  1 -
 src/box/sql/vdbeapi.c |  2 +-
 src/box/sql/vdbeaux.c |  5 +----
 9 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index 130a3f675..0b21386b5 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -442,8 +442,7 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
 			struct region *region)
 {
 	struct sql_stmt *stmt;
-	struct sql *db = sql_get();
-	if (sql_prepare(db, sql, len, &stmt, NULL) != 0)
+	if (sql_prepare(sql, len, &stmt, NULL) != 0)
 		return -1;
 	assert(stmt != NULL);
 	port_sql_create(port, stmt);
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index b9858c8d6..1eba1e206 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1343,7 +1343,7 @@ sample_compare(const void *a, const void *b, void *arg)
  * @retval 0 on success, -1 otherwise.
  */
 static int
-load_stat_from_space(struct sql *db, const char *sql_select_prepare,
+load_stat_from_space(const char *sql_select_prepare,
 		     const char *sql_select_load, struct index_stat *stats)
 {
 	struct index **indexes = NULL;
@@ -1359,7 +1359,7 @@ load_stat_from_space(struct sql *db, const char *sql_select_prepare,
 		}
 	}
 	sql_stmt *stmt = NULL;
-	int rc = sql_prepare(db, sql_select_prepare, -1, &stmt, 0);
+	int rc = sql_prepare(sql_select_prepare, -1, &stmt, 0);
 	if (rc)
 		goto finalize;
 	uint32_t current_idx_count = 0;
@@ -1427,7 +1427,7 @@ load_stat_from_space(struct sql *db, const char *sql_select_prepare,
 	rc = sql_finalize(stmt);
 	if (rc)
 		goto finalize;
-	rc = sql_prepare(db, sql_select_load, -1, &stmt, 0);
+	rc = sql_prepare(sql_select_load, -1, &stmt, 0);
 	if (rc)
 		goto finalize;
 	struct index *prev_index = NULL;
@@ -1505,12 +1505,11 @@ load_stat_from_space(struct sql *db, const char *sql_select_prepare,
 }
 
 static int
-load_stat_to_index(struct sql *db, const char *sql_select_load,
-		   struct index_stat **stats)
+load_stat_to_index(const char *sql_select_load, struct index_stat **stats)
 {
 	assert(stats != NULL && *stats != NULL);
 	struct sql_stmt *stmt = NULL;
-	if (sql_prepare(db, sql_select_load, -1, &stmt, 0) != 0)
+	if (sql_prepare(sql_select_load, -1, &stmt, 0) != 0)
 		return -1;
 	uint32_t current_idx_count = 0;
 	while (sql_step(stmt) == SQL_ROW) {
@@ -1696,7 +1695,7 @@ sql_analysis_load(struct sql *db)
 	const char *load_query = "SELECT \"tbl\",\"idx\",\"neq\",\"nlt\","
 				 "\"ndlt\",\"sample\" FROM \"_sql_stat4\"";
 	/* Load the statistics from the _sql_stat4 table. */
-	if (load_stat_from_space(db, init_query, load_query, stats) != 0)
+	if (load_stat_from_space(init_query, load_query, stats) != 0)
 		goto fail;
 	/*
 	 * Now we have complete statistics for each index
@@ -1739,7 +1738,7 @@ sql_analysis_load(struct sql *db)
 	 */
 	const char *order_query = "SELECT \"tbl\",\"idx\" FROM "
 				  "\"_sql_stat4\" GROUP BY \"tbl\",\"idx\"";
-	if (load_stat_to_index(db, order_query, heap_stats) == 0)
+	if (load_stat_to_index(order_query, heap_stats) == 0)
 		return box_txn_commit();
 fail:
 	box_txn_rollback();
diff --git a/src/box/sql/legacy.c b/src/box/sql/legacy.c
index bfd1e32b9..16507b334 100644
--- a/src/box/sql/legacy.c
+++ b/src/box/sql/legacy.c
@@ -70,7 +70,7 @@ sql_exec(sql * db,	/* The database on which the SQL executes */
 		char **azVals = 0;
 
 		pStmt = 0;
-		rc = sql_prepare(db, zSql, -1, &pStmt, &zLeftover);
+		rc = sql_prepare(zSql, -1, &pStmt, &zLeftover);
 		assert(rc == 0 || pStmt == NULL);
 		if (rc != 0)
 			continue;
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 35e81212d..520b52d64 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -46,7 +46,6 @@ static int
 sqlPrepare(sql * db,	/* Database handle. */
 	       const char *zSql,	/* UTF-8 encoded SQL statement. */
 	       int nBytes,	/* Length of zSql in bytes. */
-	       int saveSqlFlag,	/* True to copy SQL text into the sql_stmt */
 	       Vdbe * pReprepare,	/* VM being reprepared */
 	       sql_stmt ** ppStmt,	/* OUT: A pointer to the prepared statement */
 	       const char **pzTail	/* OUT: End of parsed string */
@@ -156,8 +155,7 @@ sqlPrepare(sql * db,	/* Database handle. */
 
 	if (db->init.busy == 0) {
 		Vdbe *pVdbe = sParse.pVdbe;
-		sqlVdbeSetSql(pVdbe, zSql, (int)(sParse.zTail - zSql),
-				  saveSqlFlag);
+		sqlVdbeSetSql(pVdbe, zSql, (int)(sParse.zTail - zSql));
 	}
 	if (sParse.pVdbe != NULL && (rc != 0 || db->mallocFailed)) {
 		sqlVdbeFinalize(sParse.pVdbe);
@@ -192,7 +190,7 @@ sqlReprepare(Vdbe * p)
 	zSql = sql_sql((sql_stmt *) p);
 	assert(zSql != 0);	/* Reprepare only called for prepare_v2() statements */
 	db = sqlVdbeDb(p);
-	if (sqlPrepare(db, zSql, -1, 0, p, &pNew, 0) != 0) {
+	if (sqlPrepare(db, zSql, -1, p, &pNew, 0) != 0) {
 		assert(pNew == 0);
 		return -1;
 	}
@@ -205,10 +203,10 @@ sqlReprepare(Vdbe * p)
 }
 
 int
-sql_prepare(struct sql *db, const char *sql, int length, struct sql_stmt **stmt,
+sql_prepare(const char *sql, int length, struct sql_stmt **stmt,
 	    const char **sql_tail)
 {
-	int rc = sqlPrepare(db, sql, length, 1, 0, stmt, sql_tail);
+	int rc = sqlPrepare(sql_get(), sql, length, 0, stmt, sql_tail);
 	assert(rc == 0 || stmt == NULL || *stmt == NULL);
 	return rc;
 }
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 7bd952a17..ac1d8ce42 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -471,14 +471,13 @@ typedef void (*sql_destructor_type) (void *);
 /**
  * Prepare (compile into VDBE byte-code) statement.
  *
- * @param db Database handle.
  * @param sql UTF-8 encoded SQL statement.
  * @param length Length of @param sql in bytes.
  * @param[out] stmt A pointer to the prepared statement.
  * @param[out] sql_tail End of parsed string.
  */
 int
-sql_prepare(struct sql *db, const char *sql, int length, struct sql_stmt **stmt,
+sql_prepare(const char *sql, int length, struct sql_stmt **stmt,
 	    const char **sql_tail);
 
 int
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 582d48a1f..573577355 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -251,7 +251,7 @@ void sqlVdbeSetNumCols(Vdbe *, int);
 int sqlVdbeSetColName(Vdbe *, int, int, const char *, void (*)(void *));
 void sqlVdbeCountChanges(Vdbe *);
 sql *sqlVdbeDb(Vdbe *);
-void sqlVdbeSetSql(Vdbe *, const char *z, int n, int);
+void sqlVdbeSetSql(Vdbe *, const char *z, int n);
 void sqlVdbeSwap(Vdbe *, Vdbe *);
 VdbeOp *sqlVdbeTakeOpArray(Vdbe *, int *, int *);
 sql_value *sqlVdbeGetBoundValue(Vdbe *, int, u8);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 0f32b4cd6..078ebc34e 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -421,7 +421,6 @@ struct Vdbe {
 	bft explain:2;		/* True if EXPLAIN present on SQL command */
 	bft changeCntOn:1;	/* True to update the change-counter */
 	bft runOnlyOnce:1;	/* Automatically expire on reset */
-	bft isPrepareV2:1;	/* True if prepared with prepare_v2() */
 	u32 aCounter[5];	/* Counters used by sql_stmt_status() */
 	char *zSql;		/* Text of the SQL statement that generated this */
 	void *pFree;		/* Free this when deleting the vdbe */
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 12449d3bc..db7936e78 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -451,7 +451,7 @@ sqlStep(Vdbe * p)
 	if (rc != SQL_ROW)
 		checkProfileCallback(db, p);
 
-	if (p->isPrepareV2 && rc != SQL_ROW && rc != SQL_DONE) {
+	if (rc != SQL_ROW && rc != SQL_DONE) {
 		/* If this statement was prepared using sql_prepare(), and an
 		 * error has occurred, then return an error.
 		 */
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index a1d658648..619105820 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -89,14 +89,12 @@ sql_vdbe_prepare(struct Vdbe *vdbe)
  * Remember the SQL string for a prepared statement.
  */
 void
-sqlVdbeSetSql(Vdbe * p, const char *z, int n, int isPrepareV2)
+sqlVdbeSetSql(Vdbe * p, const char *z, int n)
 {
-	assert(isPrepareV2 == 1 || isPrepareV2 == 0);
 	if (p == 0)
 		return;
 	assert(p->zSql == 0);
 	p->zSql = sqlDbStrNDup(p->db, z, n);
-	p->isPrepareV2 = (u8) isPrepareV2;
 }
 
 /*
@@ -120,7 +118,6 @@ sqlVdbeSwap(Vdbe * pA, Vdbe * pB)
 	zTmp = pA->zSql;
 	pA->zSql = pB->zSql;
 	pB->zSql = zTmp;
-	pB->isPrepareV2 = pA->isPrepareV2;
 }
 
 /*
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 02/16] sql: refactor sql_prepare() and sqlPrepare()

[Vladislav Shpilevoy]

Thanks for the patch!

> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index b9858c8d6..1eba1e206 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -1343,7 +1343,7 @@ sample_compare(const void *a, const void *b, void *arg)
>   * @retval 0 on success, -1 otherwise.
>   */
>  static int
> -load_stat_from_space(struct sql *db, const char *sql_select_prepare,
> +load_stat_from_space(const char *sql_select_prepare,

Please, drop db from the comment too.

Re: [Tarantool-patches] [PATCH v2 02/16] sql: refactor sql_prepare() and sqlPrepare()

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> - Removed saveSqlFlag as argument from sqlPrepare(). It was used to
>   indicate that its caller is sql_prepare_v2() not sql_prepare().
>   Since in previous commit we've left only one version of this function
>   let's remove this flag at all.
> 
> - Removed struct db from list of sql_prepare() arguments. There's one
>   global database handler and it can be obtained by sql_get() call.
>   Hence, it makes no sense to pass around this argument.

lgtm


-- 
Konstantin Osipov, Moscow, Russia

[Tarantool-patches] [PATCH v2 03/16] sql: move sql_prepare() declaration to box/execute.h

[Nikita Pettik]

We are going to split sql_prepare_and_execute() into several explicit
and logically separated steps:

1. sql_prepare() -- compile VDBE byte-code
2. sql_bind() -- bind variables (if there are any)
3. sql_execute() -- query (byte-code) execution in virtual machine

For instance, for dry-run we are interested only in query preparation.
Contrary, if we had prepared statement cache, we could skip query
preparation and handle only bind and execute steps.

To avoid inclusion of sql/sqlInt.h header (which gathers almost all SQL
specific functions and constants) let's move sql_prepare() to
box/execute.h header (which already holds sql_prepare_and_execute()).

Needed for #3292
---
 src/box/execute.h     | 12 ++++++++++++
 src/box/sql/analyze.c |  1 +
 src/box/sql/legacy.c  |  1 +
 src/box/sql/sqlInt.h  | 12 ------------
 4 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/src/box/execute.h b/src/box/execute.h
index a2fd4d1b7..a6000c08b 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -89,6 +89,18 @@ struct port_sql {
 
 extern const struct port_vtab port_sql_vtab;
 
+/**
+ * Prepare (compile into VDBE byte-code) statement.
+ *
+ * @param sql UTF-8 encoded SQL statement.
+ * @param length Length of @param sql in bytes.
+ * @param[out] stmt A pointer to the prepared statement.
+ * @param[out] sql_tail End of parsed string.
+ */
+int
+sql_prepare(const char *sql, int length, struct sql_stmt **stmt,
+	    const char **sql_tail);
+
 #if defined(__cplusplus)
 } /* extern "C" { */
 #endif
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 1eba1e206..9a66f8254 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -106,6 +106,7 @@
  */
 
 #include "box/box.h"
+#include "box/execute.h"
 #include "box/index.h"
 #include "box/key_def.h"
 #include "box/schema.h"
diff --git a/src/box/sql/legacy.c b/src/box/sql/legacy.c
index 16507b334..e3a2c77ca 100644
--- a/src/box/sql/legacy.c
+++ b/src/box/sql/legacy.c
@@ -37,6 +37,7 @@
  */
 
 #include "sqlInt.h"
+#include "box/execute.h"
 #include "box/session.h"
 
 /*
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index ac1d8ce42..3ca10778e 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -468,18 +468,6 @@ typedef void (*sql_destructor_type) (void *);
 #define SQL_STATIC      ((sql_destructor_type)0)
 #define SQL_TRANSIENT   ((sql_destructor_type)-1)
 
-/**
- * Prepare (compile into VDBE byte-code) statement.
- *
- * @param sql UTF-8 encoded SQL statement.
- * @param length Length of @param sql in bytes.
- * @param[out] stmt A pointer to the prepared statement.
- * @param[out] sql_tail End of parsed string.
- */
-int
-sql_prepare(const char *sql, int length, struct sql_stmt **stmt,
-	    const char **sql_tail);
-
 int
 sql_step(sql_stmt *);
 
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 03/16] sql: move sql_prepare() declaration to box/execute.h

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> index 16507b334..e3a2c77ca 100644
> --- a/src/box/sql/legacy.c
> +++ b/src/box/sql/legacy.c
> @@ -37,6 +37,7 @@
>   */
>  
>  #include "sqlInt.h"
> +#include "box/execute.h"
>  #include "box/session.h"

looks like a stray change?


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 03/16] sql: move sql_prepare() declaration to box/execute.h

[Nikita Pettik]

On 04 Dec 14:37, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> > index 16507b334..e3a2c77ca 100644
> > --- a/src/box/sql/legacy.c
> > +++ b/src/box/sql/legacy.c
> > @@ -37,6 +37,7 @@
> >   */
> >  
> >  #include "sqlInt.h"
> > +#include "box/execute.h"
> >  #include "box/session.h"
> 
> looks like a stray change?

No, it's not. Without it compilation fails:

/Users/n.pettik/tarantool/src/box/sql/legacy.c:73:8: error: implicit declaration of function 'sql_prepare' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                rc = sql_prepare(zSql, -1, &pStmt, &zLeftover);
                     ^
/Users/n.pettik/tarantool/src/box/sql/legacy.c:73:8: note: did you mean 'sqlReprepare'?
/Users/n.pettik/tarantool/src/box/sql/sqlInt.h:4181:5: note: 'sqlReprepare' declared here
int sqlReprepare(Vdbe *);
    ^

Declaration of sql_prepare() is moved to box/execute.h, so to
use it in sql/legacy.c we should include box/execute.h
 
> 
> -- 
> Konstantin Osipov, Moscow, Russia

[Tarantool-patches] [PATCH v2 04/16] sql: rename sqlPrepare() to sql_compile()

[Nikita Pettik]

sql_prepare() is going not only to compile statement, but also to save it
to the prepared statement cache. So we'd better rename sqlPrepare()
which is static wrapper around sql_prepare() and make it non-static.
Where it is possible let's use sql_compile() instead of sql_prepare().

Needed for #2592
---
 src/box/execute.c     |  2 +-
 src/box/sql/analyze.c |  6 +++---
 src/box/sql/legacy.c  |  2 +-
 src/box/sql/prepare.c | 21 ++++++---------------
 src/box/sql/sqlInt.h  | 13 +++++++++++++
 5 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index 0b21386b5..83680b70f 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -442,7 +442,7 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
 			struct region *region)
 {
 	struct sql_stmt *stmt;
-	if (sql_prepare(sql, len, &stmt, NULL) != 0)
+	if (sql_compile(sql, len, NULL, &stmt, NULL) != 0)
 		return -1;
 	assert(stmt != NULL);
 	port_sql_create(port, stmt);
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 9a66f8254..a887a2bf3 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1360,7 +1360,7 @@ load_stat_from_space(const char *sql_select_prepare,
 		}
 	}
 	sql_stmt *stmt = NULL;
-	int rc = sql_prepare(sql_select_prepare, -1, &stmt, 0);
+	int rc = sql_compile(sql_select_prepare, -1, NULL, &stmt, 0);
 	if (rc)
 		goto finalize;
 	uint32_t current_idx_count = 0;
@@ -1428,7 +1428,7 @@ load_stat_from_space(const char *sql_select_prepare,
 	rc = sql_finalize(stmt);
 	if (rc)
 		goto finalize;
-	rc = sql_prepare(sql_select_load, -1, &stmt, 0);
+	rc = sql_compile(sql_select_load, -1, NULL, &stmt, 0);
 	if (rc)
 		goto finalize;
 	struct index *prev_index = NULL;
@@ -1510,7 +1510,7 @@ load_stat_to_index(const char *sql_select_load, struct index_stat **stats)
 {
 	assert(stats != NULL && *stats != NULL);
 	struct sql_stmt *stmt = NULL;
-	if (sql_prepare(sql_select_load, -1, &stmt, 0) != 0)
+	if (sql_compile(sql_select_load, -1, NULL, &stmt, 0) != 0)
 		return -1;
 	uint32_t current_idx_count = 0;
 	while (sql_step(stmt) == SQL_ROW) {
diff --git a/src/box/sql/legacy.c b/src/box/sql/legacy.c
index e3a2c77ca..93f927dea 100644
--- a/src/box/sql/legacy.c
+++ b/src/box/sql/legacy.c
@@ -71,7 +71,7 @@ sql_exec(sql * db,	/* The database on which the SQL executes */
 		char **azVals = 0;
 
 		pStmt = 0;
-		rc = sql_prepare(zSql, -1, &pStmt, &zLeftover);
+		rc = sql_compile(zSql, -1, NULL, &pStmt, &zLeftover);
 		assert(rc == 0 || pStmt == NULL);
 		if (rc != 0)
 			continue;
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 520b52d64..47e40223d 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -39,18 +39,11 @@
 #include "box/space.h"
 #include "box/session.h"
 
-/*
- * Compile the UTF-8 encoded SQL statement zSql into a statement handle.
- */
-static int
-sqlPrepare(sql * db,	/* Database handle. */
-	       const char *zSql,	/* UTF-8 encoded SQL statement. */
-	       int nBytes,	/* Length of zSql in bytes. */
-	       Vdbe * pReprepare,	/* VM being reprepared */
-	       sql_stmt ** ppStmt,	/* OUT: A pointer to the prepared statement */
-	       const char **pzTail	/* OUT: End of parsed string */
-    )
+int
+sql_compile(const char *zSql, int nBytes, struct Vdbe *pReprepare,
+	    sql_stmt **ppStmt, const char **pzTail)
 {
+	struct sql *db = sql_get();
 	int rc = 0;	/* Result code */
 	Parse sParse;		/* Parsing context */
 	sql_parser_create(&sParse, db, current_session()->sql_flags);
@@ -185,12 +178,10 @@ sqlReprepare(Vdbe * p)
 {
 	sql_stmt *pNew;
 	const char *zSql;
-	sql *db;
 
 	zSql = sql_sql((sql_stmt *) p);
 	assert(zSql != 0);	/* Reprepare only called for prepare_v2() statements */
-	db = sqlVdbeDb(p);
-	if (sqlPrepare(db, zSql, -1, p, &pNew, 0) != 0) {
+	if (sql_compile(zSql, -1, p, &pNew, 0) != 0) {
 		assert(pNew == 0);
 		return -1;
 	}
@@ -206,7 +197,7 @@ int
 sql_prepare(const char *sql, int length, struct sql_stmt **stmt,
 	    const char **sql_tail)
 {
-	int rc = sqlPrepare(sql_get(), sql, length, 0, stmt, sql_tail);
+	int rc = sql_compile(sql, length, 0, stmt, sql_tail);
 	assert(rc == 0 || stmt == NULL || *stmt == NULL);
 	return rc;
 }
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 3ca10778e..07c26e932 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -468,6 +468,19 @@ typedef void (*sql_destructor_type) (void *);
 #define SQL_STATIC      ((sql_destructor_type)0)
 #define SQL_TRANSIENT   ((sql_destructor_type)-1)
 
+/**
+ * Compile the UTF-8 encoded SQL statement zSql into a statement handle.
+ *
+ * @param sql UTF-8 encoded SQL statement.
+ * @param sql_len Length of @sql in bytes.
+ * @param re_prepared VM being re-compiled. Can be NULL.
+ * @param[out] stmt A pointer to the compiled statement.
+ * @param[out] sql_tail End of parsed string.
+ */
+int
+sql_compile(const char *sql, int bytes_count, struct Vdbe *re_prepared,
+	    sql_stmt **stmt, const char **sql_tail);
+
 int
 sql_step(sql_stmt *);
 
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 04/16] sql: rename sqlPrepare() to sql_compile()

[Vladislav Shpilevoy]

Thanks for the patch!

> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 3ca10778e..07c26e932 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -468,6 +468,19 @@ typedef void (*sql_destructor_type) (void *);
>  #define SQL_STATIC      ((sql_destructor_type)0)
>  #define SQL_TRANSIENT   ((sql_destructor_type)-1)
>  
> +/**
> + * Compile the UTF-8 encoded SQL statement zSql into a statement handle.

Please, keep 66 symbols border, and remove 'zSql'.

> + *
> + * @param sql UTF-8 encoded SQL statement.
> + * @param sql_len Length of @sql in bytes.
> + * @param re_prepared VM being re-compiled. Can be NULL.
> + * @param[out] stmt A pointer to the compiled statement.
> + * @param[out] sql_tail End of parsed string.
> + */
> +int
> +sql_compile(const char *sql, int bytes_count, struct Vdbe *re_prepared,
> +	    sql_stmt **stmt, const char **sql_tail);
> +
>  int
>  sql_step(sql_stmt *);
>  
> 

Re: [Tarantool-patches] [PATCH v2 04/16] sql: rename sqlPrepare() to sql_compile()

[Nikita Pettik]

On 03 Dec 23:51, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> > index 3ca10778e..07c26e932 100644
> > --- a/src/box/sql/sqlInt.h
> > +++ b/src/box/sql/sqlInt.h
> > @@ -468,6 +468,19 @@ typedef void (*sql_destructor_type) (void *);
> >  #define SQL_STATIC      ((sql_destructor_type)0)
> >  #define SQL_TRANSIENT   ((sql_destructor_type)-1)
> >  
> > +/**
> > + * Compile the UTF-8 encoded SQL statement zSql into a statement handle.
> 
> Please, keep 66 symbols border, and remove 'zSql'.

diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 07c26e932..3bdbc0c38 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -469,7 +469,8 @@ typedef void (*sql_destructor_type) (void *);
 #define SQL_TRANSIENT   ((sql_destructor_type)-1)
 
 /**
- * Compile the UTF-8 encoded SQL statement zSql into a statement handle.
+ * Compile the UTF-8 encoded SQL statement into
+ * a statement handle (struct Vdbe).
  *
  * @param sql UTF-8 encoded SQL statement.
  * @param sql_len Length of @sql in bytes.> 

Re: [Tarantool-patches] [PATCH v2 04/16] sql: rename sqlPrepare() to sql_compile()

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> sql_prepare() is going not only to compile statement, but also to save it
> to the prepared statement cache. So we'd better rename sqlPrepare()
> which is static wrapper around sql_prepare() and make it non-static.
> Where it is possible let's use sql_compile() instead of sql_prepare().

I'd call it sql_stmt_create, but sql_compile is fine as well.
My point is, I would try to make the api more object-oriented.

Maybe sql_stmt_compile?

Anyway, lgtm.

-- 
Konstantin Osipov, Moscow, Russia

[Tarantool-patches] [PATCH v2 05/16] sql: move sql_finalize() to execute.h

[Nikita Pettik]

We are going to make prepared statement cache be session local. Hence,
when sessions is destroyed we should erase its cache and deallocate each
prepared statement in it. As a consequence, we should be able to call
sql_finalize() from box/ submodule. So let's move its signature to
box/execute.h

 Need for #2592
---
 src/box/ck_constraint.c | 1 +
 src/box/execute.h       | 3 +++
 src/box/sql/sqlInt.h    | 3 ---
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
index a2c66ce00..1b0e85943 100644
--- a/src/box/ck_constraint.c
+++ b/src/box/ck_constraint.c
@@ -29,6 +29,7 @@
  * SUCH DAMAGE.
  */
 #include "box/session.h"
+#include "execute.h"
 #include "bind.h"
 #include "ck_constraint.h"
 #include "errcode.h"
diff --git a/src/box/execute.h b/src/box/execute.h
index a6000c08b..a85fca5fc 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -89,6 +89,9 @@ struct port_sql {
 
 extern const struct port_vtab port_sql_vtab;
 
+int
+sql_finalize(struct sql_stmt *stmt);
+
 /**
  * Prepare (compile into VDBE byte-code) statement.
  *
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 07c26e932..bd0dca703 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -519,9 +519,6 @@ sql_value *
 sql_column_value(sql_stmt *,
 		     int iCol);
 
-int
-sql_finalize(sql_stmt * pStmt);
-
 /*
  * Terminate the current execution of an SQL statement and reset
  * it back to its starting state so that it can be reused.
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 05/16] sql: move sql_finalize() to execute.h

[Vladislav Shpilevoy]

Thanks for the patch!

On 20/11/2019 22:28, Nikita Pettik wrote:
> We are going to make prepared statement cache be session local. Hence,

Session local? I thought we've decided to use a global
cache.

> when sessions is destroyed we should erase its cache and deallocate each
> prepared statement in it. As a consequence, we should be able to call
> sql_finalize() from box/ submodule. So let's move its signature to
> box/execute.h
> 
>  Need for #2592

Re: [Tarantool-patches] [PATCH v2 05/16] sql: move sql_finalize() to execute.h

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/12/04 01:57]:
> Thanks for the patch!
> 
> On 20/11/2019 22:28, Nikita Pettik wrote:
> > We are going to make prepared statement cache be session local. Hence,
> 
> Session local? I thought we've decided to use a global
> cache.

Second that sentiment.


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 05/16] sql: move sql_finalize() to execute.h

[Nikita Pettik]

On 03 Dec 23:51, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> On 20/11/2019 22:28, Nikita Pettik wrote:
> > We are going to make prepared statement cache be session local. Hence,
> 
> Session local? I thought we've decided to use a global
> cache.

Fixed commit message:

Author: Nikita Pettik <korablev@tarantool.org>
Date:   Wed Oct 23 00:04:20 2019 +0300

    sql: move sql_finalize() to execute.h
    
    We are going to introduce prepared statement cache. On statement's
    deallocation we should release all resources which is done by
    sql_finalize(). Now it is declared in sql/sqlInt.h header, which
    accumulates almost all SQL related functions. To avoid including such a
    huge header to use single function, let's move its signature to
    box/execute.h
    
    Need for #2592
 
> > when sessions is destroyed we should erase its cache and deallocate each
> > prepared statement in it. As a consequence, we should be able to call
> > sql_finalize() from box/ submodule. So let's move its signature to
> > box/execute.h
> > 
> >  Need for #2592

Re: [Tarantool-patches] [PATCH v2 05/16] sql: move sql_finalize() to execute.h

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> index a2c66ce00..1b0e85943 100644
> --- a/src/box/ck_constraint.c
> +++ b/src/box/ck_constraint.c
> @@ -29,6 +29,7 @@
>   * SUCH DAMAGE.
>   */
>  #include "box/session.h"
> +#include "execute.h"
>  #include "bind.h"
>  #include "ck_constraint.h"
>  #include "errcode.h"

All such additions require an explanation in the CS comment
otherwise they look stray.

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 05/16] sql: move sql_finalize() to execute.h

[Nikita Pettik]

On 04 Dec 14:40, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> > index a2c66ce00..1b0e85943 100644
> > --- a/src/box/ck_constraint.c
> > +++ b/src/box/ck_constraint.c
> > @@ -29,6 +29,7 @@
> >   * SUCH DAMAGE.
> >   */
> >  #include "box/session.h"
> > +#include "execute.h"
> >  #include "bind.h"
> >  #include "ck_constraint.h"
> >  #include "errcode.h"
> 
> All such additions require an explanation in the CS comment
> otherwise they look stray.

Function's declaration just has been moved to a different header,
so now we have to include it to be able to use sql_finalize()
 
> -- 
> Konstantin Osipov, Moscow, Russia

[Tarantool-patches] [PATCH v2 06/16] port: increase padding of struct port

[Nikita Pettik]

We are going to extend context of struct port_sql. One already inherits
struct port_tuple, which makes it size barely fits into 48 bytes of
padding of basic structure (struct port). Hence, let's increase padding
a bit to be able to add at least one more member to struct port_sql.
---
 src/lib/core/port.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lib/core/port.h b/src/lib/core/port.h
index d61342287..bfdfa4656 100644
--- a/src/lib/core/port.h
+++ b/src/lib/core/port.h
@@ -122,7 +122,7 @@ struct port {
 	 * Implementation dependent content. Needed to declare
 	 * an abstract port instance on stack.
 	 */
-	char pad[48];
+	char pad[52];
 };
 
 /** Is not inlined just to be exported. */
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 06/16] port: increase padding of struct port

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> We are going to extend context of struct port_sql. One already inherits
> struct port_tuple, which makes it size barely fits into 48 bytes of
> padding of basic structure (struct port). Hence, let's increase padding
> a bit to be able to add at least one more member to struct port_sql.

This is very verbose but still unclear :) What makes you certain 4
bytes is enough?  Be it a pointer or integer, it may be 4 or 8
bytes depending on the platform.
Please explain.

Otherwise lgtm.

> ---
>  src/lib/core/port.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/lib/core/port.h b/src/lib/core/port.h
> index d61342287..bfdfa4656 100644
> --- a/src/lib/core/port.h
> +++ b/src/lib/core/port.h
> @@ -122,7 +122,7 @@ struct port {
>  	 * Implementation dependent content. Needed to declare
>  	 * an abstract port instance on stack.
>  	 */
> -	char pad[48];
> +	char pad[52];
>  };
>  
>  /** Is not inlined just to be exported. */
> -- 
> 2.15.1
> 

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 06/16] port: increase padding of struct port

[Nikita Pettik]

On 04 Dec 14:42, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> > We are going to extend context of struct port_sql. One already inherits
> > struct port_tuple, which makes it size barely fits into 48 bytes of
> > padding of basic structure (struct port). Hence, let's increase padding
> > a bit to be able to add at least one more member to struct port_sql.
> 
> This is very verbose but still unclear :) What makes you certain 4
> bytes is enough?  Be it a pointer or integer, it may be 4 or 8
> bytes depending on the platform.
> Please explain.

Because I'm adding two 1-byte members (uint8_t). Two additional
bytes just to avoid another one size patching in case smb needs
more space.

> Otherwise lgtm.
> 
> > ---
> >  src/lib/core/port.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/lib/core/port.h b/src/lib/core/port.h
> > index d61342287..bfdfa4656 100644
> > --- a/src/lib/core/port.h
> > +++ b/src/lib/core/port.h
> > @@ -122,7 +122,7 @@ struct port {
> >  	 * Implementation dependent content. Needed to declare
> >  	 * an abstract port instance on stack.
> >  	 */
> > -	char pad[48];
> > +	char pad[52];
> >  };
> >  
> >  /** Is not inlined just to be exported. */
> > -- 
> > 2.15.1
> > 
> 
> -- 
> Konstantin Osipov, Moscow, Russia

[Tarantool-patches] [PATCH v2 07/16] port: add dump format and request type to port_sql

[Nikita Pettik]

Dump formats of DQL and DML queries are different: the last one contains
number of affected rows and optionally list of autoincremented ids; the
first one comprises all meta-information including column names of
resulting set and their types. What is more, dump format is going to be
different for execute and prepare requests. So let's introduce separate
member to struct port_sql responsible for dump format to be used.

What is more, prepared statement finalization is required only for
PREPARE-AND-EXECUTE requests. So let's keep request type in port as well.

Note that C standard specifies that enums are integers, but it does not
specify the size. Hence, let's use simple uint8 - mentioned enums are
small enough to fit into it.

Needed for #2592
---
 src/box/execute.c     | 32 +++++++++++++++++++++++---------
 src/box/execute.h     | 24 ++++++++++++++++++++++++
 src/box/lua/execute.c | 20 +++++++++++++-------
 3 files changed, 60 insertions(+), 16 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index 83680b70f..d2a999099 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -100,7 +100,9 @@ static void
 port_sql_destroy(struct port *base)
 {
 	port_tuple_vtab.destroy(base);
-	sql_finalize(((struct port_sql *)base)->stmt);
+	struct port_sql *port_sql = (struct port_sql *) base;
+	if (port_sql->request == PREPARE_AND_EXECUTE)
+		sql_finalize(((struct port_sql *)base)->stmt);
 }
 
 const struct port_vtab port_sql_vtab = {
@@ -114,11 +116,15 @@ const struct port_vtab port_sql_vtab = {
 };
 
 static void
-port_sql_create(struct port *port, struct sql_stmt *stmt)
+port_sql_create(struct port *port, struct sql_stmt *stmt,
+		enum sql_dump_format dump_format, enum sql_request_type request)
 {
 	port_tuple_create(port);
-	((struct port_sql *)port)->stmt = stmt;
 	port->vtab = &port_sql_vtab;
+	struct port_sql *port_sql = (struct port_sql *) port;
+	port_sql->stmt = stmt;
+	port_sql->dump_format = dump_format;
+	port_sql->request = request;
 }
 
 /**
@@ -324,9 +330,10 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
 {
 	assert(port->vtab == &port_sql_vtab);
 	sql *db = sql_get();
-	struct sql_stmt *stmt = ((struct port_sql *)port)->stmt;
-	int column_count = sql_column_count(stmt);
-	if (column_count > 0) {
+	struct port_sql *sql_port = (struct port_sql *)port;
+	struct sql_stmt *stmt = sql_port->stmt;
+	switch (sql_port->dump_format) {
+	case DQL_EXECUTE: {
 		int keys = 2;
 		int size = mp_sizeof_map(keys);
 		char *pos = (char *) obuf_alloc(out, size);
@@ -335,7 +342,7 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
 			return -1;
 		}
 		pos = mp_encode_map(pos, keys);
-		if (sql_get_metadata(stmt, out, column_count) != 0)
+		if (sql_get_metadata(stmt, out, sql_column_count(stmt)) != 0)
 			return -1;
 		size = mp_sizeof_uint(IPROTO_DATA);
 		pos = (char *) obuf_alloc(out, size);
@@ -346,7 +353,9 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
 		pos = mp_encode_uint(pos, IPROTO_DATA);
 		if (port_tuple_vtab.dump_msgpack(port, out) < 0)
 			return -1;
-	} else {
+		break;
+	}
+	case DML_EXECUTE: {
 		int keys = 1;
 		assert(((struct port_tuple *)port)->size == 0);
 		struct stailq *autoinc_id_list =
@@ -395,6 +404,9 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
 				      mp_encode_int(buf, id_entry->id);
 			}
 		}
+		break;
+	}
+	default: unreachable();
 	}
 	return 0;
 }
@@ -445,7 +457,9 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
 	if (sql_compile(sql, len, NULL, &stmt, NULL) != 0)
 		return -1;
 	assert(stmt != NULL);
-	port_sql_create(port, stmt);
+	enum sql_dump_format dump_format = sql_column_count(stmt) > 0 ?
+					   DQL_EXECUTE : DML_EXECUTE;
+	port_sql_create(port, stmt, dump_format, PREPARE_AND_EXECUTE);
 	if (sql_bind(stmt, bind, bind_count) == 0 &&
 	    sql_execute(stmt, port, region) == 0)
 		return 0;
diff --git a/src/box/execute.h b/src/box/execute.h
index a85fca5fc..6702a18cc 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -46,6 +46,23 @@ enum sql_info_key {
 	sql_info_key_MAX,
 };
 
+/**
+ * One of possible formats used to dump msgpack/Lua.
+ * For details see port_sql_dump_msgpack() and port_sql_dump_lua().
+ */
+enum sql_dump_format {
+	DQL_EXECUTE = 0,
+	DML_EXECUTE = 1,
+	DQL_PREPARE = 2,
+	DML_PREPARE = 3,
+};
+
+enum sql_request_type {
+	PREPARE_AND_EXECUTE = 0,
+	PREPARE = 1,
+	EXECUTE_PREPARED = 2
+};
+
 extern const char *sql_info_key_strs[];
 
 struct region;
@@ -85,6 +102,13 @@ struct port_sql {
 	struct port_tuple port_tuple;
 	/* Prepared SQL statement. */
 	struct sql_stmt *stmt;
+	/**
+	 * Dump format depends on type of SQL query: DML or DQL;
+	 * and on type of SQL request: execute or prepare.
+	 */
+	uint8_t dump_format;
+	/** enum sql_request_type */
+	uint8_t request;
 };
 
 extern const struct port_vtab port_sql_vtab;
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index 76ecdd541..1b2f8d235 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -45,18 +45,21 @@ port_sql_dump_lua(struct port *port, struct lua_State *L, bool is_flat)
 	assert(is_flat == false);
 	assert(port->vtab == &port_sql_vtab);
 	struct sql *db = sql_get();
-	struct sql_stmt *stmt = ((struct port_sql *)port)->stmt;
-	int column_count = sql_column_count(stmt);
-	if (column_count > 0) {
+	struct port_sql *port_sql = (struct port_sql *)port;
+	struct sql_stmt *stmt = port_sql->stmt;
+	switch (port_sql->dump_format) {
+	case DQL_EXECUTE: {
 		lua_createtable(L, 0, 2);
-		lua_sql_get_metadata(stmt, L, column_count);
+		lua_sql_get_metadata(stmt, L, sql_column_count(stmt));
 		lua_setfield(L, -2, "metadata");
 		port_tuple_vtab.dump_lua(port, L, false);
 		lua_setfield(L, -2, "rows");
-	} else {
-		assert(((struct port_tuple *)port)->size == 0);
+		break;
+	}
+	case DML_EXECUTE: {
+		assert(((struct port_tuple *) port)->size == 0);
 		struct stailq *autoinc_id_list =
-			vdbe_autoinc_id_list((struct Vdbe *)stmt);
+			vdbe_autoinc_id_list((struct Vdbe *) stmt);
 		lua_createtable(L, 0, stailq_empty(autoinc_id_list) ? 1 : 2);
 
 		luaL_pushuint64(L, db->nChange);
@@ -77,6 +80,9 @@ port_sql_dump_lua(struct port *port, struct lua_State *L, bool is_flat)
 				sql_info_key_strs[SQL_INFO_AUTOINCREMENT_IDS];
 			lua_setfield(L, -2, field_name);
 		}
+		break;
+	}
+	default: unreachable();
 	}
 }
 
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 07/16] port: add dump format and request type to port_sql

[Vladislav Shpilevoy]

Thanks for the patch!

See 4 comments below.

On 20/11/2019 22:28, Nikita Pettik wrote:
> Dump formats of DQL and DML queries are different: the last one contains
> number of affected rows and optionally list of autoincremented ids; the
> first one comprises all meta-information including column names of
> resulting set and their types. What is more, dump format is going to be
> different for execute and prepare requests. So let's introduce separate
> member to struct port_sql responsible for dump format to be used.
> 
> What is more, prepared statement finalization is required only for
> PREPARE-AND-EXECUTE requests. So let's keep request type in port as well.

1. I don't think it is a good idea to rely on certain
meta being returned by only certain requests. There is
a ticket, which will remove the border between different
request types:
https://github.com/tarantool/tarantool/issues/3649.

Yes, now this is either sql_info or metadata, but that
will change.

> 
> Note that C standard specifies that enums are integers, but it does not
> specify the size. Hence, let's use simple uint8 - mentioned enums are
> small enough to fit into it.

2. Try this:

    enum ... <member> : 8;

> 
> Needed for #2592
> ---
>  src/box/execute.c     | 32 +++++++++++++++++++++++---------
>  src/box/execute.h     | 24 ++++++++++++++++++++++++
>  src/box/lua/execute.c | 20 +++++++++++++-------
>  3 files changed, 60 insertions(+), 16 deletions(-)
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 83680b70f..d2a999099 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -100,7 +100,9 @@ static void
>  port_sql_destroy(struct port *base)
>  {
>  	port_tuple_vtab.destroy(base);
> -	sql_finalize(((struct port_sql *)base)->stmt);
> +	struct port_sql *port_sql = (struct port_sql *) base;
> +	if (port_sql->request == PREPARE_AND_EXECUTE)

3. You could turn the types into a bitmask to drop the
dependency on a certain request type:

    bool do_finalize : 1;
    bool do_omit_data : 1;

> +		sql_finalize(((struct port_sql *)base)->stmt);
>  }
>  
>  const struct port_vtab port_sql_vtab = {
> @@ -395,6 +404,9 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
>  				      mp_encode_int(buf, id_entry->id);
>  			}
>  		}
> +		break;
> +	}
> +	default: unreachable();

4. Please, wrap on a new line, and include into {}. I know,
this is shorter, but this just does not match the code
style. In the other place too.

>  	}
>  	return 0;
>  }

Re: [Tarantool-patches] [PATCH v2 07/16] port: add dump format and request type to port_sql

[Nikita Pettik]

On 03 Dec 23:51, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 4 comments below.
> 
> On 20/11/2019 22:28, Nikita Pettik wrote:
> > Dump formats of DQL and DML queries are different: the last one contains
> > number of affected rows and optionally list of autoincremented ids; the
> > first one comprises all meta-information including column names of
> > resulting set and their types. What is more, dump format is going to be
> > different for execute and prepare requests. So let's introduce separate
> > member to struct port_sql responsible for dump format to be used.
> > 
> > What is more, prepared statement finalization is required only for
> > PREPARE-AND-EXECUTE requests. So let's keep request type in port as well.
> 
> 1. I don't think it is a good idea to rely on certain
> meta being returned by only certain requests. There is
> a ticket, which will remove the border between different
> request types:
> https://github.com/tarantool/tarantool/issues/3649.

It's in the wishlist, so I doubt that it will be implemented
in observed feature.

As an alternative to your proposal below I can suggest to add ref count
to prepared statements. It is assumed to be increased once it is added
to prepared statement cache and before execution; after execution it is
decremented.

> Yes, now this is either sql_info or metadata, but that
> will change.
> 
> > 
> > Note that C standard specifies that enums are integers, but it does not
> > specify the size. Hence, let's use simple uint8 - mentioned enums are
> > small enough to fit into it.
> 
> 2. Try this:
> 
>     enum ... <member> : 8;

Hm, doesn't it allocate one whole enum member anyway? If so,
still undefined size is implied.

> > --- a/src/box/execute.c
> > +++ b/src/box/execute.c
> > @@ -100,7 +100,9 @@ static void
> >  port_sql_destroy(struct port *base)
> >  {
> >  	port_tuple_vtab.destroy(base);
> > -	sql_finalize(((struct port_sql *)base)->stmt);
> > +	struct port_sql *port_sql = (struct port_sql *) base;
> > +	if (port_sql->request == PREPARE_AND_EXECUTE)
> 
> 3. You could turn the types into a bitmask to drop the
> dependency on a certain request type:
> 
>     bool do_finalize : 1;

Well, seems to be quite suitable, let's use it:

diff --git a/src/box/execute.c b/src/box/execute.c
index 0e731672c..dfe586334 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -101,7 +101,7 @@ port_sql_destroy(struct port *base)
 {
        port_tuple_vtab.destroy(base);
        struct port_sql *port_sql = (struct port_sql *) base;
-       if (port_sql->request == PREPARE_AND_EXECUTE)
+       if (port_sql->do_finalize)
                sql_stmt_finalize(((struct port_sql *)base)->stmt);
 }
 
@@ -117,15 +117,14 @@ const struct port_vtab port_sql_vtab = {
 
 static void
 port_sql_create(struct port *port, struct sql_stmt *stmt,
-               enum sql_serialization_format format,
-               enum sql_request_type request)
+               enum sql_serialization_format format, bool do_finalize)
 {
        port_tuple_create(port);
        port->vtab = &port_sql_vtab;
        struct port_sql *port_sql = (struct port_sql *) port;
        port_sql->stmt = stmt;
        port_sql->serialization_format = format;
-       port_sql->request = request;
+       port_sql->do_finalize = do_finalize;
 }
 
 /**
@@ -460,7 +459,7 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
        assert(stmt != NULL);
        enum sql_serialization_format format = sql_column_count(stmt) > 0 ?
                                           DQL_EXECUTE : DML_EXECUTE;
-       port_sql_create(port, stmt, format, PREPARE_AND_EXECUTE);
+       port_sql_create(port, stmt, format, true);
        if (sql_bind(stmt, bind, bind_count) == 0 &&
            sql_execute(stmt, port, region) == 0)
                return 0;
diff --git a/src/box/execute.h b/src/box/execute.h
index e9b74f7c8..c87e765cf 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -57,12 +57,6 @@ enum sql_serialization_format {
        DML_PREPARE = 3,
 };
 
-enum sql_request_type {
-       PREPARE_AND_EXECUTE = 0,
-       PREPARE = 1,
-       EXECUTE_PREPARED = 2
-};
-
 extern const char *sql_info_key_strs[];
 
 struct region;
@@ -107,8 +101,11 @@ struct port_sql {
         * DQL; and on type of SQL request: execute or prepare.
         */
        uint8_t serialization_format;
-       /** enum sql_request_type */
-       uint8_t request;
+       /**
+        * There's no need in clean-up in case of PREPARE request:
+        * statement remains in cache and will be deleted later.
+        */
+       bool do_finalize;
 };

>     bool do_omit_data : 1;

do_omit_data is not enough: serialize formats are different for all
four cases. 
 
> > +		sql_finalize(((struct port_sql *)base)->stmt);
> >  }
> >  
> >  const struct port_vtab port_sql_vtab = {
> > @@ -395,6 +404,9 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
> >  				      mp_encode_int(buf, id_entry->id);
> >  			}
> >  		}
> > +		break;
> > +	}
> > +	default: unreachable();
> 
> 4. Please, wrap on a new line, and include into {}. I know,
> this is shorter, but this just does not match the code
> style. In the other place too.

Okay:

diff --git a/src/box/execute.c b/src/box/execute.c
index 0e731672c..3bc4988b7 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -407,7 +406,9 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
                }
                break;
        }
-       default: unreachable();
+       default: {
+               unreachable();
+       }
        }
        return 0;
 }

diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index 2cbc5dfab..b164ffcaf 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -82,7 +82,9 @@ port_sql_dump_lua(struct port *port, struct lua_State *L, bool is_flat)
                }
                break;
        }
-       default: unreachable();
+       default: {
+               unreachable();
+       }
        }
 }

 
> >  	}
> >  	return 0;
> >  }

Re: [Tarantool-patches] [PATCH v2 07/16] port: add dump format and request type to port_sql

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> Dump formats of DQL and DML queries are different: the last one contains

Ergh, this is not dump, but result set/serialization format. Dump
in SQL is usually used for logical or physical database
backup.

> number of affected rows and optionally list of autoincremented ids; the
> first one comprises all meta-information including column names of
> resulting set and their types. What is more, dump format is going to be
> different for execute and prepare requests. So let's introduce separate
> member to struct port_sql responsible for dump format to be used.
> 
> What is more, prepared statement finalization is required only for
> PREPARE-AND-EXECUTE requests. So let's keep request type in port as well.

> 
> Note that C standard specifies that enums are integers, but it does not
> specify the size. Hence, let's use simple uint8 - mentioned enums are
> small enough to fit into it.

enum sizeof in C and older C++ is implementation dependent.

what do you mean here?

> +	struct port_sql *port_sql = (struct port_sql *) base;
> +	if (port_sql->request == PREPARE_AND_EXECUTE)
> +		sql_finalize(((struct port_sql *)base)->stmt);

Does this work with the statement object only or with the cache as
well? 

I suggest we introduce a clear naming for API calls:

sql_stmt_* - for statement object api
sql_stmt_cache_* - for prepared statement api

sql_finalize, sql_prepare, sql_execute - for SQL high level API
which manipulates both statements and the cache.

What do you think?

> -port_sql_create(struct port *port, struct sql_stmt *stmt)
> +port_sql_create(struct port *port, struct sql_stmt *stmt,
> +		enum sql_dump_format dump_format, enum sql_request_type request)
>  {
>  	port_tuple_create(port);
> -	((struct port_sql *)port)->stmt = stmt;
>  	port->vtab = &port_sql_vtab;
> +	struct port_sql *port_sql = (struct port_sql *) port;
> +	port_sql->stmt = stmt;
> +	port_sql->dump_format = dump_format;

Let's use sql_result_set_format? Do you have to introduce this
enum ? This information can be derived from sql_request_type, no +
statement type, no? If yes, I suggest to have a function, which
returns it, rather than store.

> +/**
> + * One of possible formats used to dump msgpack/Lua.
> + * For details see port_sql_dump_msgpack() and port_sql_dump_lua().


> + */
> +enum sql_dump_format {
> +	DQL_EXECUTE = 0,
> +	DML_EXECUTE = 1,
> +	DQL_PREPARE = 2,
> +	DML_PREPARE = 3,
> +};

Neither the names of the members nor comments convey what is going
on here - looks like a simple matrix dml/dql * prepare/execute.
Having a couple of if statments is not such a big deal IMHO.

> +	/**
> +	 * Dump format depends on type of SQL query: DML or DQL;
> +	 * and on type of SQL request: execute or prepare.
> +	 */
> +	uint8_t dump_format;
> +	/** enum sql_request_type */
> +	uint8_t request;

If I had to add these constants, I would not use type-erasing
uint8 here, but use max value for enum in C/C++, which is 64 bits
and the original type. 

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 07/16] port: add dump format and request type to port_sql

[Nikita Pettik]

On 04 Dec 14:52, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> > Dump formats of DQL and DML queries are different: the last one contains
> 
> Ergh, this is not dump, but result set/serialization format. Dump
> in SQL is usually used for logical or physical database
> backup.

Okay, let's rename to sql_serialization_format
 
> > number of affected rows and optionally list of autoincremented ids; the
> > first one comprises all meta-information including column names of
> > resulting set and their types. What is more, dump format is going to be
> > different for execute and prepare requests. So let's introduce separate
> > member to struct port_sql responsible for dump format to be used.
> > 
> > What is more, prepared statement finalization is required only for
> > PREPARE-AND-EXECUTE requests. So let's keep request type in port as well.
> 
> > 
> > Note that C standard specifies that enums are integers, but it does not
> > specify the size. Hence, let's use simple uint8 - mentioned enums are
> > small enough to fit into it.
> 
> enum sizeof in C and older C++ is implementation dependent.
> 
> what do you mean here?

To store uint8 numbers instead of enums (due to their implementation
dependent sizes).

> > +	struct port_sql *port_sql = (struct port_sql *) base;
> > +	if (port_sql->request == PREPARE_AND_EXECUTE)
> > +		sql_finalize(((struct port_sql *)base)->stmt);
> 
> Does this work with the statement object only or with the cache as
> well? 

Only with prepared statement objects.
 
> I suggest we introduce a clear naming for API calls:
> 
> sql_stmt_* - for statement object api
> sql_stmt_cache_* - for prepared statement api

> sql_finalize, sql_prepare, sql_execute - for SQL high level API
> which manipulates both statements and the cache.
> 
> What do you think?

Ok, will move renaming sql_finalize -> sql_stmt_finalize etc to
a separate patch. I'm okay with suggested naming and I'm going
to use it in the next patches.
 
> > -port_sql_create(struct port *port, struct sql_stmt *stmt)
> > +port_sql_create(struct port *port, struct sql_stmt *stmt,
> > +		enum sql_dump_format dump_format, enum sql_request_type request)
> >  {
> >  	port_tuple_create(port);
> > -	((struct port_sql *)port)->stmt = stmt;
> >  	port->vtab = &port_sql_vtab;
> > +	struct port_sql *port_sql = (struct port_sql *) port;
> > +	port_sql->stmt = stmt;
> > +	port_sql->dump_format = dump_format;
> 
> Let's use sql_result_set_format? Do you have to introduce this
> enum ? This information can be derived from sql_request_type, no +
> statement type, no? If yes, I suggest to have a function, which
> returns it, rather than store.

To save 1 byte? :) Doesn't seem to be reasonable tho.
Current implementation looks quite suitable in code.
 
> > +/**
> > + * One of possible formats used to dump msgpack/Lua.
> > + * For details see port_sql_dump_msgpack() and port_sql_dump_lua().
> 
> 
> > + */
> > +enum sql_dump_format {
> > +	DQL_EXECUTE = 0,
> > +	DML_EXECUTE = 1,
> > +	DQL_PREPARE = 2,
> > +	DML_PREPARE = 3,
> > +};
> 
> Neither the names of the members nor comments convey what is going
> on here - looks like a simple matrix dml/dql * prepare/execute.

That's it.

> Having a couple of if statments is not such a big deal IMHO.

With this enum code looks way much better.
 
> > +	/**
> > +	 * Dump format depends on type of SQL query: DML or DQL;
> > +	 * and on type of SQL request: execute or prepare.
> > +	 */
> > +	uint8_t dump_format;
> > +	/** enum sql_request_type */
> > +	uint8_t request;
> 
> If I had to add these constants, I would not use type-erasing
> uint8 here, but use max value for enum in C/C++, which is 64 bits
> and the original type. 

Now one is replaced with do_finalize as Vlad suggested.

[Tarantool-patches] [PATCH v2 08/16] sql: resurrect sql_bind_parameter_count() function

[Nikita Pettik]

This function is present in sql/vdbeapi.c source file, its prototype is
missing in any header file. It makes impossible to use it. Let's add
prototype declaration to sql/sqlInt.h (as other parameter
setters/getters) and refactor a bit in accordance with our codestyle.

Need for #2592
---
 src/box/sql/sqlInt.h  |  6 ++++++
 src/box/sql/vdbeapi.c | 10 +++-------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index bd0dca703..875efd6e3 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -688,6 +688,12 @@ int
 sql_bind_zeroblob64(sql_stmt *, int,
 			sql_uint64);
 
+/**
+ * Return the number of wildcards that should be bound to.
+ */
+int
+sql_bind_parameter_count(sql_stmt *stmt);
+
 /**
  * Perform pointer parameter binding for the prepared sql
  * statement.
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index db7936e78..11f05786c 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -1051,15 +1051,11 @@ sql_bind_zeroblob64(sql_stmt * pStmt, int i, sql_uint64 n)
 	return sql_bind_zeroblob(pStmt, i, n);
 }
 
-/*
- * Return the number of wildcards that can be potentially bound to.
- * This routine is added to support DBD::sql.
- */
 int
-sql_bind_parameter_count(sql_stmt * pStmt)
+sql_bind_parameter_count(sql_stmt *stmt)
 {
-	Vdbe *p = (Vdbe *) pStmt;
-	return p ? p->nVar : 0;
+	struct Vdbe *p = (struct Vdbe *) stmt;
+	return p->nVar;
 }
 
 /*
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 08/16] sql: resurrect sql_bind_parameter_count() function

[Vladislav Shpilevoy]

Thanks for the patch!

On 20/11/2019 22:28, Nikita Pettik wrote:
> This function is present in sql/vdbeapi.c source file, its prototype is
> missing in any header file. It makes impossible to use it. Let's add
> prototype declaration to sql/sqlInt.h (as other parameter
> setters/getters) and refactor a bit in accordance with our codestyle.
> 
> Need for #2592
> ---
>  src/box/sql/sqlInt.h  |  6 ++++++
>  src/box/sql/vdbeapi.c | 10 +++-------
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index bd0dca703..875efd6e3 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -688,6 +688,12 @@ int
>  sql_bind_zeroblob64(sql_stmt *, int,
>  			sql_uint64);
>  
> +/**
> + * Return the number of wildcards that should be bound to.
> + */
> +int
> +sql_bind_parameter_count(sql_stmt *stmt);

Why we are here, lets make it 'const struct sql_stmt *'.
In sql_stmt_schema_version() too.

> +
>  /**
>   * Perform pointer parameter binding for the prepared sql
>   * statement.

Re: [Tarantool-patches] [PATCH v2 08/16] sql: resurrect sql_bind_parameter_count() function

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> This function is present in sql/vdbeapi.c source file, its prototype is
> missing in any header file. It makes impossible to use it. Let's add
> prototype declaration to sql/sqlInt.h (as other parameter
> setters/getters) and refactor a bit in accordance with our codestyle.

lgtm


-- 
Konstantin Osipov, Moscow, Russia

[Tarantool-patches] [PATCH v2 09/16] sql: resurrect sql_bind_parameter_name()

[Nikita Pettik]

We may need to get name of parameter to be bound by its index position.
So let's resurrect sql_bind_parameter_name() - put its prototype to
sql/sqlInt.h header and update codestyle.

Need for #2592
---
 src/box/sql/sqlInt.h  |  7 +++++++
 src/box/sql/vdbeapi.c | 14 ++++----------
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 875efd6e3..8fc4b79db 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -694,6 +694,13 @@ sql_bind_zeroblob64(sql_stmt *, int,
 int
 sql_bind_parameter_count(sql_stmt *stmt);
 
+/**
+ * Return the name of a wildcard parameter. Return NULL if the index
+ * is out of range or if the wildcard is unnamed.
+ */
+const char *
+sql_bind_parameter_name(sql_stmt *stmt, int i);
+
 /**
  * Perform pointer parameter binding for the prepared sql
  * statement.
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 11f05786c..c090bd4bb 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -1058,18 +1058,12 @@ sql_bind_parameter_count(sql_stmt *stmt)
 	return p->nVar;
 }
 
-/*
- * Return the name of a wildcard parameter.  Return NULL if the index
- * is out of range or if the wildcard is unnamed.
- *
- * The result is always UTF-8.
- */
 const char *
-sql_bind_parameter_name(sql_stmt * pStmt, int i)
+sql_bind_parameter_name(sql_stmt *stmt, int i)
 {
-	Vdbe *p = (Vdbe *) pStmt;
-	if (p == 0)
-		return 0;
+	struct Vdbe *p = (struct Vdbe *) stmt;
+	if (p == NULL)
+		return NULL;
 	return sqlVListNumToName(p->pVList, i);
 }
 
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 09/16] sql: resurrect sql_bind_parameter_name()

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> +/**
> + * Return the name of a wildcard parameter. Return NULL if the index
> + * is out of range or if the wildcard is unnamed.
> + */

Please document the valid range of the index (0-based I assume)
> +const char *
> +sql_bind_parameter_name(sql_stmt *stmt, int i);
> 

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 09/16] sql: resurrect sql_bind_parameter_name()

[Konstantin Osipov]

* Konstantin Osipov <kostja.osipov@gmail.com> [19/12/04 14:55]:
> * Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> > +/**
> > + * Return the name of a wildcard parameter. Return NULL if the index
> > + * is out of range or if the wildcard is unnamed.
> > + */
> 
> Please document the valid range of the index (0-based I assume)

Otherwise lgtm

> > +const char *
> > +sql_bind_parameter_name(sql_stmt *stmt, int i);
> > 
> 
> -- 
> Konstantin Osipov, Moscow, Russia

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 09/16] sql: resurrect sql_bind_parameter_name()

[Nikita Pettik]

On 04 Dec 14:55, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> > +/**
> > + * Return the name of a wildcard parameter. Return NULL if the index
> > + * is out of range or if the wildcard is unnamed.
> > + */
> 
> Please document the valid range of the index (0-based I assume)
> > +const char *
> > +sql_bind_parameter_name(sql_stmt *stmt, int i);
> > 

Fixed:

diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index e5092b7a4..a5c564b8a 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -697,7 +697,8 @@ sql_bind_parameter_count(sql_stmt *stmt);
 
 /**
  * Return the name of a wildcard parameter. Return NULL if the index
- * is out of range or if the wildcard is unnamed.
+ * is out of range or if the wildcard is unnamed. Parameter's index
+ * is 0-based.
  */
 const char *
 sql_bind_parameter_name(sql_stmt *stmt, int i);

> -- 
> Konstantin Osipov, Moscow, Russia

[Tarantool-patches] [PATCH v2 10/16] sql: add sql_stmt_schema_version()

[Nikita Pettik]

Let's introduce interface function to get schema version of prepared
statement. It is required since sturct sql_stmt (i.e. prepared
statement) is an opaque object and in fact is an alias to struct Vdbe.
Statements with schema version different from the current one are
considered to be expired and should be re-compiled.

Needed for #2592
---
 src/box/sql/sqlInt.h  | 3 +++
 src/box/sql/vdbeapi.c | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 8fc4b79db..22cb9a5e4 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -570,6 +570,9 @@ sql_column_name(sql_stmt *, int N);
 const char *
 sql_column_datatype(sql_stmt *, int N);
 
+uint32_t
+sql_stmt_schema_version(sql_stmt *stmt);
+
 int
 sql_initialize(void);
 
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index c090bd4bb..da528a4dc 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -798,6 +798,13 @@ sql_column_decltype(sql_stmt * pStmt, int N)
 			  COLNAME_DECLTYPE);
 }
 
+uint32_t
+sql_stmt_schema_version(sql_stmt *stmt)
+{
+	struct Vdbe *v = (struct Vdbe *) stmt;
+	return v->schema_ver;
+}
+
 /******************************* sql_bind_  **************************
  *
  * Routines used to attach values to wildcards in a compiled SQL statement.
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 10/16] sql: add sql_stmt_schema_version()

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> Let's introduce interface function to get schema version of prepared
> statement. It is required since sturct sql_stmt (i.e. prepared
> statement) is an opaque object and in fact is an alias to struct Vdbe.
> Statements with schema version different from the current one are
> considered to be expired and should be re-compiled.

lgtm


-- 
Konstantin Osipov, Moscow, Russia

[Tarantool-patches] [PATCH v2 11/16] sql: introduce sql_stmt_sizeof() function

[Nikita Pettik]

To implement memory quota of prepared statement cache, we have to
estimate size of prepared statement. This function attempts at that.

Part of #2592
---
 src/box/execute.h     |  8 ++++++++
 src/box/sql/vdbeapi.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/src/box/execute.h b/src/box/execute.h
index 6702a18cc..d5b4d8421 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -116,6 +116,14 @@ extern const struct port_vtab port_sql_vtab;
 int
 sql_finalize(struct sql_stmt *stmt);
 
+/**
+ * Calculate estimated size of memory occupied by VM.
+ * See sqlVdbeMakeReady() for details concerning allocated
+ * memory.
+ */
+size_t
+sql_stmt_sizeof(const struct sql_stmt *stmt);
+
 /**
  * Prepare (compile into VDBE byte-code) statement.
  *
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index da528a4dc..10135bb68 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -805,6 +805,59 @@ sql_stmt_schema_version(sql_stmt *stmt)
 	return v->schema_ver;
 }
 
+size_t
+sql_stmt_sizeof(const sql_stmt *stmt)
+{
+	struct Vdbe *v = (struct Vdbe *) stmt;
+	size_t size = sizeof(*v);
+	/* Resulting set */
+	size += sizeof(struct Mem) * v->nResColumn * COLNAME_N;
+	/* Opcodes */
+	size += sizeof(struct VdbeOp) * v->nOp;
+	/* Memory cells */
+	size += sizeof(struct Mem) * v->nMem;
+	/* Bindings */
+	size += sizeof(struct Mem) * v->nVar;
+	/* Bindings included in the result set */
+	size += sizeof(uint32_t) * v->res_var_count;
+	/* Cursors */
+	size += sizeof(struct VdbeCursor *) * v->nCursor;
+
+	for (int i = 0; i < v->nOp; ++i) {
+		/* Estimate size of p4 operand. */
+		if (v->aOp[i].p4type != P4_NOTUSED) {
+			switch (v->aOp[i].p4type) {
+				case P4_DYNAMIC:
+				case P4_STATIC:
+					if (v->aOp[i].opcode == OP_Blob ||
+					    v->aOp[i].opcode == OP_String)
+						size += v->aOp[i].p1;
+					else if (v->aOp[i].opcode == OP_String8)
+						size += strlen(v->aOp[i].p4.z);
+					break;
+				case P4_BOOL:
+					size += sizeof(v->aOp[i].p4.b);
+					break;
+				case P4_INT32:
+					size += sizeof(v->aOp[i].p4.i);
+					break;
+				case P4_UINT64:
+				case P4_INT64:
+					size += sizeof(*v->aOp[i].p4.pI64);
+					break;
+				case P4_REAL:
+					size += sizeof(*v->aOp[i].p4.pReal);
+					break;
+				default:
+					size += sizeof(v->aOp[i].p4.p);
+					break;
+			}
+		}
+	}
+	size += strlen(v->zSql);
+	return size;
+}
+
 /******************************* sql_bind_  **************************
  *
  * Routines used to attach values to wildcards in a compiled SQL statement.
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 11/16] sql: introduce sql_stmt_sizeof() function

[Vladislav Shpilevoy]

Thanks for the patch!

See 4 comments below.

On 20/11/2019 22:28, Nikita Pettik wrote:
> To implement memory quota of prepared statement cache, we have to
> estimate size of prepared statement. This function attempts at that.
> 
> Part of #2592
> ---
>  src/box/execute.h     |  8 ++++++++
>  src/box/sql/vdbeapi.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/src/box/execute.h b/src/box/execute.h
> index 6702a18cc..d5b4d8421 100644
> --- a/src/box/execute.h
> +++ b/src/box/execute.h
> @@ -116,6 +116,14 @@ extern const struct port_vtab port_sql_vtab;
>  int
>  sql_finalize(struct sql_stmt *stmt);
>  
> +/**
> + * Calculate estimated size of memory occupied by VM.
> + * See sqlVdbeMakeReady() for details concerning allocated
> + * memory.
> + */
> +size_t
> +sql_stmt_sizeof(const struct sql_stmt *stmt);
> +

1. Lets call it 'sql_stmt_sizeof_estimated()'. To emphasize that
the size is not exact.

>  /**
>   * Prepare (compile into VDBE byte-code) statement.
>   *
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index da528a4dc..10135bb68 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -805,6 +805,59 @@ sql_stmt_schema_version(sql_stmt *stmt)
>  	return v->schema_ver;
>  }
>  
> +size_t
> +sql_stmt_sizeof(const sql_stmt *stmt)

2. You have 'struct' in the declaration, but not
here. I think struct should be added there too.

> +{
> +	struct Vdbe *v = (struct Vdbe *) stmt;
> +	size_t size = sizeof(*v);
> +	/* Resulting set */
> +	size += sizeof(struct Mem) * v->nResColumn * COLNAME_N;

3. Could we account column names too? aColName member.

> +	/* Opcodes */
> +	size += sizeof(struct VdbeOp) * v->nOp;
> +	/* Memory cells */
> +	size += sizeof(struct Mem) * v->nMem;
> +	/* Bindings */
> +	size += sizeof(struct Mem) * v->nVar;
> +	/* Bindings included in the result set */
> +	size += sizeof(uint32_t) * v->res_var_count;
> +	/* Cursors */
> +	size += sizeof(struct VdbeCursor *) * v->nCursor;
> +
> +	for (int i = 0; i < v->nOp; ++i) {
> +		/* Estimate size of p4 operand. */
> +		if (v->aOp[i].p4type != P4_NOTUSED) {

4. You can reduce the indentation:

- Try to invert the check and do 'continue' when P4 is
  unused;

- 'Case' should be aligned under 'switch' according to
  the code style.

> +			switch (v->aOp[i].p4type) {
> +				case P4_DYNAMIC:
> +				case P4_STATIC:
> +					if (v->aOp[i].opcode == OP_Blob ||
> +					    v->aOp[i].opcode == OP_String)
> +						size += v->aOp[i].p1;
> +					else if (v->aOp[i].opcode == OP_String8)
> +						size += strlen(v->aOp[i].p4.z);
> +					break;
> +				case P4_BOOL:
> +					size += sizeof(v->aOp[i].p4.b);
> +					break;
> +				case P4_INT32:
> +					size += sizeof(v->aOp[i].p4.i);
> +					break;
> +				case P4_UINT64:
> +				case P4_INT64:
> +					size += sizeof(*v->aOp[i].p4.pI64);
> +					break;
> +				case P4_REAL:
> +					size += sizeof(*v->aOp[i].p4.pReal);
> +					break;
> +				default:
> +					size += sizeof(v->aOp[i].p4.p);
> +					break;
> +			}
> +		}
> +	}
> +	size += strlen(v->zSql);
> +	return size;
> +}
> +
>  /******************************* sql_bind_  **************************
>   *
>   * Routines used to attach values to wildcards in a compiled SQL statement.
> 

Re: [Tarantool-patches] [PATCH v2 11/16] sql: introduce sql_stmt_sizeof() function

[Nikita Pettik]

On 03 Dec 23:51, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 4 comments below.
> 
> On 20/11/2019 22:28, Nikita Pettik wrote:
> > To implement memory quota of prepared statement cache, we have to
> > estimate size of prepared statement. This function attempts at that.
> > 
> > Part of #2592
> > ---
> >  src/box/execute.h     |  8 ++++++++
> >  src/box/sql/vdbeapi.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 61 insertions(+)
> > 
> > diff --git a/src/box/execute.h b/src/box/execute.h
> > index 6702a18cc..d5b4d8421 100644
> > --- a/src/box/execute.h
> > +++ b/src/box/execute.h
> > @@ -116,6 +116,14 @@ extern const struct port_vtab port_sql_vtab;
> >  int
> >  sql_finalize(struct sql_stmt *stmt);
> >  
> > +/**
> > + * Calculate estimated size of memory occupied by VM.
> > + * See sqlVdbeMakeReady() for details concerning allocated
> > + * memory.
> > + */
> > +size_t
> > +sql_stmt_sizeof(const struct sql_stmt *stmt);
> > +
> 
> 1. Lets call it 'sql_stmt_sizeof_estimated()'. To emphasize that
> the size is not exact.

TBO it seems to be too long. I'd better use Konstantin's version:
sql_stmt_est_size().
 
> >  /**
> >   * Prepare (compile into VDBE byte-code) statement.
> >   *
> > diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> > index da528a4dc..10135bb68 100644
> > --- a/src/box/sql/vdbeapi.c
> > +++ b/src/box/sql/vdbeapi.c
> > @@ -805,6 +805,59 @@ sql_stmt_schema_version(sql_stmt *stmt)
> >  	return v->schema_ver;
> >  }
> >  
> > +size_t
> > +sql_stmt_sizeof(const sql_stmt *stmt)
> 
> 2. You have 'struct' in the declaration, but not
> here. I think struct should be added there too.

Fixed.

> > +{
> > +	struct Vdbe *v = (struct Vdbe *) stmt;
> > +	size_t size = sizeof(*v);
> > +	/* Resulting set */
> > +	size += sizeof(struct Mem) * v->nResColumn * COLNAME_N;
> 
> 3. Could we account column names too? aColName member.

It is exactly what is accounted here. Fixed a bit comment:

-       /* Resulting set */
+       /* Names and types of result set columns */

Memory cells containing values of result set are already accounted
in v->nMem.

> > +	/* Opcodes */
> > +	size += sizeof(struct VdbeOp) * v->nOp;
> > +	/* Memory cells */
> > +	size += sizeof(struct Mem) * v->nMem;
> > +	/* Bindings */
> > +	size += sizeof(struct Mem) * v->nVar;
> > +	/* Bindings included in the result set */
> > +	size += sizeof(uint32_t) * v->res_var_count;
> > +	/* Cursors */
> > +	size += sizeof(struct VdbeCursor *) * v->nCursor;
> > +
> > +	for (int i = 0; i < v->nOp; ++i) {
> > +		/* Estimate size of p4 operand. */
> > +		if (v->aOp[i].p4type != P4_NOTUSED) {
> 
> 4. You can reduce the indentation:
> 
> - Try to invert the check and do 'continue' when P4 is
>   unused;
> 
> - 'Case' should be aligned under 'switch' according to
>   the code style.

Thanks, refactord:

@@ -825,33 +825,33 @@ sql_stmt_sizeof(const sql_stmt *stmt)
 
        for (int i = 0; i < v->nOp; ++i) {
                /* Estimate size of p4 operand. */
-               if (v->aOp[i].p4type != P4_NOTUSED) {
-                       switch (v->aOp[i].p4type) {
-                               case P4_DYNAMIC:
-                               case P4_STATIC:
-                                       if (v->aOp[i].opcode == OP_Blob ||
-                                           v->aOp[i].opcode == OP_String)
-                                               size += v->aOp[i].p1;
-                                       else if (v->aOp[i].opcode == OP_String8)
-                                               size += strlen(v->aOp[i].p4.z);
-                                       break;
-                               case P4_BOOL:
-                                       size += sizeof(v->aOp[i].p4.b);
-                                       break;
-                               case P4_INT32:
-                                       size += sizeof(v->aOp[i].p4.i);
-                                       break;
-                               case P4_UINT64:
-                               case P4_INT64:
-                                       size += sizeof(*v->aOp[i].p4.pI64);
-                                       break;
-                               case P4_REAL:
-                                       size += sizeof(*v->aOp[i].p4.pReal);
-                                       break;
-                               default:
-                                       size += sizeof(v->aOp[i].p4.p);
-                                       break;
-                       }
+               if (v->aOp[i].p4type == P4_NOTUSED)
+                       continue;
+               switch (v->aOp[i].p4type) {
+               case P4_DYNAMIC:
+               case P4_STATIC:
+                       if (v->aOp[i].opcode == OP_Blob ||
+                           v->aOp[i].opcode == OP_String)
+                               size += v->aOp[i].p1;
+                       else if (v->aOp[i].opcode == OP_String8)
+                               size += strlen(v->aOp[i].p4.z);
+                       break;
+               case P4_BOOL:
+                       size += sizeof(v->aOp[i].p4.b);
+                       break;
+               case P4_INT32:
+                       size += sizeof(v->aOp[i].p4.i);
+                       break;
+               case P4_UINT64:
+               case P4_INT64:
+                       size += sizeof(*v->aOp[i].p4.pI64);
+                       break;
+               case P4_REAL:
+                       size += sizeof(*v->aOp[i].p4.pReal);
+                       break;
+               default:
+                       size += sizeof(v->aOp[i].p4.p);
+                       break;
                }
        }
        size += strlen(v->zSql);
 
> > +			switch (v->aOp[i].p4type) {
> > +				case P4_DYNAMIC:
> > +				case P4_STATIC:
> > +					if (v->aOp[i].opcode == OP_Blob ||
> > +					    v->aOp[i].opcode == OP_String)
> > +						size += v->aOp[i].p1;
> > +					else if (v->aOp[i].opcode == OP_String8)
> > +						size += strlen(v->aOp[i].p4.z);
> > +					break;
> > +				case P4_BOOL:
> > +					size += sizeof(v->aOp[i].p4.b);
> > +					break;
> > +				case P4_INT32:
> > +					size += sizeof(v->aOp[i].p4.i);
> > +					break;
> > +				case P4_UINT64:
> > +				case P4_INT64:
> > +					size += sizeof(*v->aOp[i].p4.pI64);
> > +					break;
> > +				case P4_REAL:
> > +					size += sizeof(*v->aOp[i].p4.pReal);
> > +					break;
> > +				default:
> > +					size += sizeof(v->aOp[i].p4.p);
> > +					break;
> > +			}
> > +		}

Re: [Tarantool-patches] [PATCH v2 11/16] sql: introduce sql_stmt_sizeof() function

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> To implement memory quota of prepared statement cache, we have to
> estimate size of prepared statement. This function attempts at that.

I suggest sql_stmt_est_size() as the name.

I would add size-caching right away, to make this function cheap.

if (size != 0)
    return size;

I wonder if we could use another region for the bytecode to 
make this quick & cheap to maintain...

I don't want to sidetrack this work, though.

So lgtm.


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 11/16] sql: introduce sql_stmt_sizeof() function

[Nikita Pettik]

On 04 Dec 14:59, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> > To implement memory quota of prepared statement cache, we have to
> > estimate size of prepared statement. This function attempts at that.
> 
> I suggest sql_stmt_est_size() as the name.

OK, renamed.
 
> I would add size-caching right away, to make this function cheap.
> 
> if (size != 0)
>     return size;

Does it make any sense? Now for one VM instance it can be called at
most three times: twice during insertion and once during deletion.

> I wonder if we could use another region for the bytecode to 
> make this quick & cheap to maintain...
> 
> I don't want to sidetrack this work, though.
> 
> So lgtm.
> 
> 
> -- 
> Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 11/16] sql: introduce sql_stmt_sizeof() function

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/12/13 16:59]:
> > I would add size-caching right away, to make this function cheap.
> > 
> > if (size != 0)
> >     return size;
> 
> Does it make any sense? Now for one VM instance it can be called at
> most three times: twice during insertion and once during deletion.

This will make the contract this function provides more reliable.
Please keep in mind that whoever is going to use it in the future
will not read its code and not want to think about its complexity.
And the added cost will be hard to notice during a code review.

So it's less about performance and more about maintenance costs.


-- 
Konstantin Osipov, Moscow, Russia

[Tarantool-patches] [PATCH v2 12/16] box: increment schema_version on ddl operations

[Nikita Pettik]

Some DDL operations such as SQL trigger alter, check and foreign
constraint alter don't result in schema version change. On the other
hand, we are going to rely on schema version to determine expired
prepared statements: for instance, if FK constraint has been created
after DML statement preparation, the latter may ignore FK constraint
(instead of proper "statement has expired" error). Let's fix it and
account schema change on each DDL operation.

Need for #2592
---
 src/box/alter.cc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 4a3241a79..921e76725 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -4728,6 +4728,7 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 
 	txn_stmt_on_rollback(stmt, on_rollback);
 	txn_stmt_on_commit(stmt, on_commit);
+	++schema_version;
 	return 0;
 }
 
@@ -5236,6 +5237,7 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
 		space_reset_fk_constraint_mask(child_space);
 		space_reset_fk_constraint_mask(parent_space);
 	}
+	++schema_version;
 	return 0;
 }
 
@@ -5480,6 +5482,7 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
 
 	if (trigger_run(&on_alter_space, space) != 0)
 		return -1;
+	++schema_version;
 	return 0;
 }
 
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 12/16] box: increment schema_version on ddl operations

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> Some DDL operations such as SQL trigger alter, check and foreign
> constraint alter don't result in schema version change. On the other
> hand, we are going to rely on schema version to determine expired
> prepared statements: for instance, if FK constraint has been created
> after DML statement preparation, the latter may ignore FK constraint
> (instead of proper "statement has expired" error). Let's fix it and
> account schema change on each DDL operation.

This is very crude:

- I can't imagine that changing a trigger affects any existing
  statement. It will have its own vdbe, and it doesn't change
  table schema. 

  Constraints need more thinking, but the idea is the same:
  not all schema changes are created equal.

- flushing entire cache on any such change is a huge inefficiency.
  It was not an issue before, but time has come to introduce
  per-object versioning, in addition to global versioning.

This is subject for future work.

So lgtm.


-- 
Konstantin Osipov, Moscow, Russia

[Tarantool-patches] [PATCH v2 13/16] sql: introduce sql_stmt_query_str() method

[Nikita Pettik]

It is getter to fetch string of SQL query from prepared statement.

Needed for #2592
---
 src/box/execute.h     | 6 ++++++
 src/box/sql/vdbeapi.c | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/src/box/execute.h b/src/box/execute.h
index d5b4d8421..16b424d36 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -124,6 +124,12 @@ sql_finalize(struct sql_stmt *stmt);
 size_t
 sql_stmt_sizeof(const struct sql_stmt *stmt);
 
+/**
+ * Return string of SQL query.
+ */
+const char *
+sql_stmt_query_str(const struct sql_stmt *stmt);
+
 /**
  * Prepare (compile into VDBE byte-code) statement.
  *
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 10135bb68..0978e12fd 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -858,6 +858,13 @@ sql_stmt_sizeof(const sql_stmt *stmt)
 	return size;
 }
 
+const char *
+sql_stmt_query_str(const sql_stmt *stmt)
+{
+	struct Vdbe *v = (struct Vdbe *) stmt;
+	return v->zSql;
+}
+
 /******************************* sql_bind_  **************************
  *
  * Routines used to attach values to wildcards in a compiled SQL statement.
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 13/16] sql: introduce sql_stmt_query_str() method

[Vladislav Shpilevoy]

Thanks for the patch!

> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index 10135bb68..0978e12fd 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -858,6 +858,13 @@ sql_stmt_sizeof(const sql_stmt *stmt)
> +const char *
> +sql_stmt_query_str(const sql_stmt *stmt)
> +{
> +	struct Vdbe *v = (struct Vdbe *) stmt;

'const struct Vdbe *'.

> +	return v->zSql;
> +}
> +
>  /******************************* sql_bind_  **************************
>   *
>   * Routines used to attach values to wildcards in a compiled SQL statement.
> 

Re: [Tarantool-patches] [PATCH v2 13/16] sql: introduce sql_stmt_query_str() method

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> It is getter to fetch string of SQL query from prepared statement.

lgtm


-- 
Konstantin Osipov, Moscow, Russia

[Tarantool-patches] [PATCH v2 14/16] sql: introduce cache for prepared statemets

[Nikita Pettik]

This patch introduces cache (as data structure) to handle prepared
statements and a set of interface functions (insert, delete, find) to
operate on it. Cache under the hood uses LRU eviction policy. It is
implemented as a hash table with string keys (which contain original SQL
statements) and prepared statements (struct sql_stmt which is an alias
for struct Vdbe) as values. To realise LRU strategy we maintain list of
nodes: head is the newest prepared statement, tail a candidate to be
evicted. Size of cache is regulated via box.cfg{sql_cache_size}
parameter. Cache is global to all sessions. To erase session manually,
one can set its size to 0. Default cache size is assumed to be 5 Mb.

Part of #2592
---
 src/box/CMakeLists.txt          |   1 +
 src/box/box.cc                  |  20 +++++
 src/box/box.h                   |   1 +
 src/box/errcode.h               |   1 +
 src/box/lua/cfg.cc              |  12 +++
 src/box/lua/load_cfg.lua        |   3 +
 src/box/prep_stmt.c             | 186 ++++++++++++++++++++++++++++++++++++++++
 src/box/prep_stmt.h             | 112 ++++++++++++++++++++++++
 src/box/session.cc              |   1 +
 src/box/sql.c                   |   3 +
 test/app-tap/init_script.result |  37 ++++----
 test/box/admin.result           |   2 +
 test/box/cfg.result             |   7 ++
 test/box/cfg.test.lua           |   1 +
 test/box/misc.result            |   1 +
 15 files changed, 370 insertions(+), 18 deletions(-)
 create mode 100644 src/box/prep_stmt.c
 create mode 100644 src/box/prep_stmt.h

diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 5cd5cba81..8be3d982d 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -126,6 +126,7 @@ add_library(box STATIC
     sql.c
     bind.c
     execute.c
+    prep_stmt.c
     wal.c
     call.c
     merger.c
diff --git a/src/box/box.cc b/src/box/box.cc
index b119c927b..7d0b36f13 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -74,6 +74,7 @@
 #include "call.h"
 #include "func.h"
 #include "sequence.h"
+#include "prep_stmt.h"
 
 static char status[64] = "unknown";
 
@@ -599,6 +600,15 @@ box_check_vinyl_options(void)
 	}
 }
 
+static void
+box_check_sql_cache_size(int size)
+{
+	if (size < 0) {
+		tnt_raise(ClientError, ER_CFG, "sql_cache_size",
+			  "must be non-negative");
+	}
+}
+
 void
 box_check_config()
 {
@@ -620,6 +630,7 @@ box_check_config()
 	box_check_memtx_memory(cfg_geti64("memtx_memory"));
 	box_check_memtx_min_tuple_size(cfg_geti64("memtx_min_tuple_size"));
 	box_check_vinyl_options();
+	box_check_sql_cache_size(cfg_geti("sql_cache_size"));
 }
 
 /*
@@ -886,6 +897,14 @@ box_set_net_msg_max(void)
 				IPROTO_FIBER_POOL_SIZE_FACTOR);
 }
 
+void
+box_set_prepared_stmt_cache_size(void)
+{
+	int cache_sz = cfg_geti("sql_cache_size");
+	box_check_sql_cache_size(cache_sz);
+	sql_prepared_stmt_cache_set_size(cache_sz);
+}
+
 /* }}} configuration bindings */
 
 /**
@@ -2096,6 +2115,7 @@ box_cfg_xc(void)
 	box_check_instance_uuid(&instance_uuid);
 	box_check_replicaset_uuid(&replicaset_uuid);
 
+	box_set_prepared_stmt_cache_size();
 	box_set_net_msg_max();
 	box_set_readahead();
 	box_set_too_long_threshold();
diff --git a/src/box/box.h b/src/box/box.h
index ccd527bd5..f2e88c8a9 100644
--- a/src/box/box.h
+++ b/src/box/box.h
@@ -235,6 +235,7 @@ void box_set_replication_sync_lag(void);
 void box_set_replication_sync_timeout(void);
 void box_set_replication_skip_conflict(void);
 void box_set_net_msg_max(void);
+void box_set_prepared_stmt_cache_size(void);
 
 extern "C" {
 #endif /* defined(__cplusplus) */
diff --git a/src/box/errcode.h b/src/box/errcode.h
index c660b1c70..ee44f61b3 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -258,6 +258,7 @@ struct errcode_record {
 	/*203 */_(ER_BOOTSTRAP_READONLY,	"Trying to bootstrap a local read-only instance as master") \
 	/*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") \
 
 /*
  * !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/lua/cfg.cc b/src/box/lua/cfg.cc
index 4884ce013..42070fe49 100644
--- a/src/box/lua/cfg.cc
+++ b/src/box/lua/cfg.cc
@@ -274,6 +274,17 @@ lbox_cfg_set_net_msg_max(struct lua_State *L)
 	return 0;
 }
 
+static int
+lbox_set_prepared_stmt_cache_size(struct lua_State *L)
+{
+	try {
+		box_set_prepared_stmt_cache_size();
+	} catch (Exception *) {
+		luaT_error(L);
+	}
+	return 0;
+}
+
 static int
 lbox_cfg_set_worker_pool_threads(struct lua_State *L)
 {
@@ -378,6 +389,7 @@ box_lua_cfg_init(struct lua_State *L)
 		{"cfg_set_replication_sync_timeout", lbox_cfg_set_replication_sync_timeout},
 		{"cfg_set_replication_skip_conflict", lbox_cfg_set_replication_skip_conflict},
 		{"cfg_set_net_msg_max", lbox_cfg_set_net_msg_max},
+		{"cfg_set_sql_cache_size", lbox_set_prepared_stmt_cache_size},
 		{NULL, NULL}
 	};
 
diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 85617c8f0..4463f989c 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -81,6 +81,7 @@ local default_cfg = {
     feedback_host         = "https://feedback.tarantool.io",
     feedback_interval     = 3600,
     net_msg_max           = 768,
+    sql_cache_size        = 5 * 1024 * 1024,
 }
 
 -- types of available options
@@ -144,6 +145,7 @@ local template_cfg = {
     feedback_host         = 'string',
     feedback_interval     = 'number',
     net_msg_max           = 'number',
+    sql_cache_size        = 'number',
 }
 
 local function normalize_uri(port)
@@ -250,6 +252,7 @@ local dynamic_cfg = {
     instance_uuid           = check_instance_uuid,
     replicaset_uuid         = check_replicaset_uuid,
     net_msg_max             = private.cfg_set_net_msg_max,
+    sql_cache_size          = private.cfg_set_sql_cache_size,
 }
 
 --
diff --git a/src/box/prep_stmt.c b/src/box/prep_stmt.c
new file mode 100644
index 000000000..fe3b8244e
--- /dev/null
+++ b/src/box/prep_stmt.c
@@ -0,0 +1,186 @@
+/*
+ * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#include "prep_stmt.h"
+
+#include "assoc.h"
+#include "error.h"
+#include "execute.h"
+#include "session.h"
+
+/** Default cache size is 5 Mb. */
+size_t prep_stmt_cache_size = 5 * 1024 * 1024;
+
+static struct prep_stmt_cache prep_stmt_cache;
+
+void
+sql_prepared_stmt_cache_init()
+{
+	prep_stmt_cache.hash = mh_strnptr_new();
+	if (prep_stmt_cache.hash == NULL)
+		panic("out of memory");
+	prep_stmt_cache.mem_quota = prep_stmt_cache_size;
+	prep_stmt_cache.mem_used = 0;
+	rlist_create(&prep_stmt_cache.cache_lru);
+}
+
+static size_t
+sql_cache_node_sizeof(struct sql_stmt *stmt)
+{
+	return sql_stmt_sizeof(stmt) + sizeof(struct stmt_cache_node *);
+}
+
+static void
+sql_cache_node_delete(struct prep_stmt_cache *cache,
+		      struct stmt_cache_node *node)
+{
+	cache->mem_used -= sql_cache_node_sizeof(node->stmt);
+	rlist_del(&node->in_lru);
+	sql_finalize(node->stmt);
+	TRASH(node);
+	free(node);
+}
+
+void
+sql_prepared_stmt_cache_delete(struct stmt_cache_node *node)
+{
+	struct prep_stmt_cache *cache = &prep_stmt_cache;
+	const char *sql_str = sql_stmt_query_str(node->stmt);
+	mh_int_t hash_id =
+		mh_strnptr_find_inp(cache->hash, sql_str, strlen(sql_str));
+	assert(hash_id != mh_end(cache->hash));
+	mh_strnptr_del(cache->hash, hash_id, NULL);
+	sql_cache_node_delete(cache, node);
+}
+
+void
+sql_cache_stmt_refresh(struct stmt_cache_node *node)
+{
+	rlist_move_entry(&prep_stmt_cache.cache_lru, node, in_lru);
+}
+
+static void
+sql_prepared_stmt_cache_gc()
+{
+	if (rlist_empty(&prep_stmt_cache.cache_lru)) {
+		assert(prep_stmt_cache.mem_used == 0);
+		return;
+	}
+	struct stmt_cache_node *node =
+		rlist_last_entry(&prep_stmt_cache.cache_lru, struct stmt_cache_node,
+				 in_lru);
+	/*
+	 * TODO: instead of following simple LRU rule it could turn
+	 * out to be reasonable to also account value of reference
+	 * counters.
+	 */
+	sql_prepared_stmt_cache_delete(node);
+}
+
+/**
+ * Allocate new cache node containing given prepared statement.
+ * Add it to the LRU cache list. Account cache size enlargement.
+ */
+static struct stmt_cache_node *
+sql_cache_node_new(struct sql_stmt *stmt)
+{
+	struct stmt_cache_node *node = malloc(sizeof(*node));
+	if (node == NULL) {
+		diag_set(OutOfMemory, sizeof(*node), "malloc",
+			 "struct stmt_cache_node");
+		return NULL;
+	}
+	node->stmt = stmt;
+	rlist_add(&prep_stmt_cache.cache_lru, &node->in_lru);
+	prep_stmt_cache.mem_used += sql_cache_node_sizeof(stmt);
+	return node;
+}
+
+/**
+ * Return true if used memory (accounting new node) for SQL
+ * prepared statement cache does not exceed the limit.
+ */
+static bool
+sql_cache_check_new_node_size(size_t size)
+{
+	return prep_stmt_cache.mem_used + size <= prep_stmt_cache.mem_quota;
+}
+
+int
+sql_prepared_stmt_cache_insert(struct sql_stmt *stmt)
+{
+	assert(stmt != NULL);
+	struct prep_stmt_cache *cache = &prep_stmt_cache;
+	size_t new_node_size = sql_cache_node_sizeof(stmt);
+	if (new_node_size > prep_stmt_cache.mem_quota) {
+		diag_set(ClientError, ER_SQL_PREPARE, "size of statement "\
+			"exceeds cache memory limit. Please, increase SQL "\
+			"cache size");
+		return -1;
+	}
+	while (! sql_cache_check_new_node_size(new_node_size))
+		sql_prepared_stmt_cache_gc();
+	struct mh_strnptr_t *hash = cache->hash;
+	const char *sql_str = sql_stmt_query_str(stmt);
+	assert(sql_prepared_stmt_cache_find(sql_str) == NULL);
+	struct stmt_cache_node *cache_node = sql_cache_node_new(stmt);
+	if (cache_node == NULL)
+		return -1;
+	uint32_t str_hash = mh_strn_hash(sql_str, strlen(sql_str));
+	const struct mh_strnptr_node_t hash_node = { sql_str, strlen(sql_str),
+						     str_hash, cache_node };
+	struct mh_strnptr_node_t *old_node = NULL;
+	mh_int_t i = mh_strnptr_put(hash, &hash_node, &old_node, NULL);
+	if (i == mh_end(hash)) {
+		sql_cache_node_delete(cache, cache_node);
+		diag_set(OutOfMemory, 0, "mh_strnptr_put", "mh_strnptr_node");
+		return -1;
+	}
+	assert(old_node == NULL);
+	return 0;
+}
+
+struct stmt_cache_node *
+sql_prepared_stmt_cache_find(const char *sql_str)
+{
+	struct mh_strnptr_t *hash = prep_stmt_cache.hash;
+	mh_int_t stmt = mh_strnptr_find_inp(hash, sql_str, strlen(sql_str));
+	if (stmt == mh_end(hash))
+		return NULL;
+	return mh_strnptr_node(hash, stmt)->val;
+}
+
+void
+sql_prepared_stmt_cache_set_size(size_t size)
+{
+	prep_stmt_cache.mem_quota = size;
+	while (prep_stmt_cache.mem_used > size)
+		sql_prepared_stmt_cache_gc();
+}
diff --git a/src/box/prep_stmt.h b/src/box/prep_stmt.h
new file mode 100644
index 000000000..31f3ff52b
--- /dev/null
+++ b/src/box/prep_stmt.h
@@ -0,0 +1,112 @@
+#ifndef INCLUDES_PREP_STMT_H
+#define INCLUDES_PREP_STMT_H
+/*
+ * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#include <stdint.h>
+#include <stdio.h>
+
+#include "small/rlist.h"
+
+#if defined(__cplusplus)
+extern "C" {
+#endif
+
+extern size_t prep_stmt_cache_size;
+
+/**
+ * Global prepared statement cache which follows LRU
+ * eviction policy. Implemented as hash <str : stmt>
+ * and double linked list.
+ */
+struct prep_stmt_cache {
+	/** Size of memory currently occupied by prepared statements. */
+	size_t mem_used;
+	/**
+	 * Max memory size that can be used for cache.
+	 */
+	size_t mem_quota;
+	/** Query hash -> struct prepared_stmt hash.*/
+	struct mh_strnptr_t *hash;
+	struct rlist cache_lru;
+};
+
+struct stmt_cache_node {
+	/** Prepared statement itself. */
+	struct sql_stmt *stmt;
+	/**
+	 * Link to the next node. Head is the newest, tail is
+	 * a candidate to be evicted.
+	 */
+	struct rlist in_lru;
+};
+
+struct sql_stmt;
+
+/**
+ * Initialize global cache for prepared statements. Called once
+ * in sql_init().
+ */
+void
+sql_prepared_stmt_cache_init();
+
+/** Remove statement node from cache and release all resources. */
+void
+sql_prepared_stmt_cache_delete(struct stmt_cache_node *node);
+
+/**
+ * Account LRU cache node as the newest one (i.e. move to the HEAD
+ * of LRU list).
+ */
+void
+sql_cache_stmt_refresh(struct stmt_cache_node *node);
+
+/**
+ * Save prepared statement to the prepared statement cache.
+ * Account cache size change. If the cache is full (i.e. memory
+ * quota is exceeded) diag error is raised. In case of success
+ * return id of prepared statement via output parameter @id.
+ */
+int
+sql_prepared_stmt_cache_insert(struct sql_stmt *stmt);
+
+/** Find entry by SQL string. In case of search fails it returns NULL. */
+struct stmt_cache_node *
+sql_prepared_stmt_cache_find(const char *sql_str);
+
+/** Set @prep_stmt_cache_size value. */
+void
+sql_prepared_stmt_cache_set_size(size_t size);
+
+#if defined(__cplusplus)
+} /* extern "C" { */
+#endif
+
+#endif
diff --git a/src/box/session.cc b/src/box/session.cc
index 461d1cf25..fe33ae6b6 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -36,6 +36,7 @@
 #include "user.h"
 #include "error.h"
 #include "tt_static.h"
+#include "execute.h"
 
 const char *session_type_strs[] = {
 	"background",
diff --git a/src/box/sql.c b/src/box/sql.c
index f1df55571..3a991ccd5 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -54,6 +54,7 @@
 #include "iproto_constants.h"
 #include "fk_constraint.h"
 #include "mpstream.h"
+#include "prep_stmt.h"
 
 static sql *db = NULL;
 
@@ -74,6 +75,8 @@ sql_init()
 	if (sql_init_db(&db) != 0)
 		panic("failed to initialize SQL subsystem");
 
+	sql_prepared_stmt_cache_init();
+
 	assert(db != NULL);
 }
 
diff --git a/test/app-tap/init_script.result b/test/app-tap/init_script.result
index 799297ba0..551a0bbeb 100644
--- a/test/app-tap/init_script.result
+++ b/test/app-tap/init_script.result
@@ -31,24 +31,25 @@ box.cfg
 26	replication_sync_timeout:300
 27	replication_timeout:1
 28	slab_alloc_factor:1.05
-29	strip_core:true
-30	too_long_threshold:0.5
-31	vinyl_bloom_fpr:0.05
-32	vinyl_cache:134217728
-33	vinyl_dir:.
-34	vinyl_max_tuple_size:1048576
-35	vinyl_memory:134217728
-36	vinyl_page_size:8192
-37	vinyl_read_threads:1
-38	vinyl_run_count_per_level:2
-39	vinyl_run_size_ratio:3.5
-40	vinyl_timeout:60
-41	vinyl_write_threads:4
-42	wal_dir:.
-43	wal_dir_rescan_delay:2
-44	wal_max_size:268435456
-45	wal_mode:write
-46	worker_pool_threads:4
+29	sql_cache_size:5242880
+30	strip_core:true
+31	too_long_threshold:0.5
+32	vinyl_bloom_fpr:0.05
+33	vinyl_cache:134217728
+34	vinyl_dir:.
+35	vinyl_max_tuple_size:1048576
+36	vinyl_memory:134217728
+37	vinyl_page_size:8192
+38	vinyl_read_threads:1
+39	vinyl_run_count_per_level:2
+40	vinyl_run_size_ratio:3.5
+41	vinyl_timeout:60
+42	vinyl_write_threads:4
+43	wal_dir:.
+44	wal_dir_rescan_delay:2
+45	wal_max_size:268435456
+46	wal_mode:write
+47	worker_pool_threads:4
 --
 -- Test insert from detached fiber
 --
diff --git a/test/box/admin.result b/test/box/admin.result
index 6126f3a97..852c1cde8 100644
--- a/test/box/admin.result
+++ b/test/box/admin.result
@@ -83,6 +83,8 @@ cfg_filter(box.cfg)
     - 1
   - - slab_alloc_factor
     - 1.05
+  - - sql_cache_size
+    - 5242880
   - - strip_core
     - true
   - - too_long_threshold
diff --git a/test/box/cfg.result b/test/box/cfg.result
index 5370bb870..9542e6375 100644
--- a/test/box/cfg.result
+++ b/test/box/cfg.result
@@ -71,6 +71,8 @@ cfg_filter(box.cfg)
  |     - 1
  |   - - slab_alloc_factor
  |     - 1.05
+ |   - - sql_cache_size
+ |     - 5242880
  |   - - strip_core
  |     - true
  |   - - too_long_threshold
@@ -170,6 +172,8 @@ cfg_filter(box.cfg)
  |     - 1
  |   - - slab_alloc_factor
  |     - 1.05
+ |   - - sql_cache_size
+ |     - 5242880
  |   - - strip_core
  |     - true
  |   - - too_long_threshold
@@ -315,6 +319,9 @@ box.cfg{memtx_memory = box.cfg.memtx_memory}
 box.cfg{vinyl_memory = box.cfg.vinyl_memory}
  | ---
  | ...
+box.cfg{sql_cache_size = 1024}
+ | ---
+ | ...
 
 --------------------------------------------------------------------------------
 -- Test of default cfg options
diff --git a/test/box/cfg.test.lua b/test/box/cfg.test.lua
index 56ccb6767..e129568e6 100644
--- a/test/box/cfg.test.lua
+++ b/test/box/cfg.test.lua
@@ -51,6 +51,7 @@ box.cfg{replicaset_uuid = '12345678-0123-5678-1234-abcdefabcdef'}
 
 box.cfg{memtx_memory = box.cfg.memtx_memory}
 box.cfg{vinyl_memory = box.cfg.vinyl_memory}
+box.cfg{sql_cache_size = 1024}
 
 --------------------------------------------------------------------------------
 -- Test of default cfg options
diff --git a/test/box/misc.result b/test/box/misc.result
index b2930515b..78ffbf1dc 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -535,6 +535,7 @@ t;
   203: box.error.BOOTSTRAP_READONLY
   204: box.error.SQL_FUNC_WRONG_RET_COUNT
   205: box.error.FUNC_INVALID_RETURN_TYPE
+  206: box.error.SQL_PREPARE
 ...
 test_run:cmd("setopt delimiter ''");
 ---
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 14/16] sql: introduce cache for prepared statemets

[Vladislav Shpilevoy]

Thanks for the patch!

See 13 comments below.

On 20/11/2019 22:28, Nikita Pettik wrote:
> This patch introduces cache (as data structure) to handle prepared
> statements and a set of interface functions (insert, delete, find) to
> operate on it. Cache under the hood uses LRU eviction policy. It is
> implemented as a hash table with string keys (which contain original SQL
> statements) and prepared statements (struct sql_stmt which is an alias
> for struct Vdbe) as values. To realise LRU strategy we maintain list of
> nodes: head is the newest prepared statement, tail a candidate to be
> evicted. Size of cache is regulated via box.cfg{sql_cache_size}
> parameter. Cache is global to all sessions. To erase session manually,

1. Maybe 'erase cache', not session?

> one can set its size to 0. Default cache size is assumed to be 5 Mb.

2. In the cover letter you said, that it is not LRU. So what is it?
From the code looks like a normal LRU.

> 
> Part of #2592
> ---
>  src/box/CMakeLists.txt          |   1 +
>  src/box/box.cc                  |  20 +++++
>  src/box/box.h                   |   1 +
>  src/box/errcode.h               |   1 +
>  src/box/lua/cfg.cc              |  12 +++
>  src/box/lua/load_cfg.lua        |   3 +
>  src/box/prep_stmt.c             | 186 ++++++++++++++++++++++++++++++++++++++++
>  src/box/prep_stmt.h             | 112 ++++++++++++++++++++++++
>  src/box/session.cc              |   1 +
>  src/box/sql.c                   |   3 +
>  test/app-tap/init_script.result |  37 ++++----
>  test/box/admin.result           |   2 +
>  test/box/cfg.result             |   7 ++
>  test/box/cfg.test.lua           |   1 +
>  test/box/misc.result            |   1 +
>  15 files changed, 370 insertions(+), 18 deletions(-)
>  create mode 100644 src/box/prep_stmt.c
>  create mode 100644 src/box/prep_stmt.h
> 
> diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
> index 5cd5cba81..8be3d982d 100644
> --- a/src/box/CMakeLists.txt
> +++ b/src/box/CMakeLists.txt
> @@ -126,6 +126,7 @@ add_library(box STATIC
>      sql.c
>      bind.c
>      execute.c
> +    prep_stmt.c

3. Could you please rename it to sql_stmt_cache.c/.h?

>      wal.c
>      call.c
>      merger.c
> diff --git a/src/box/lua/cfg.cc b/src/box/lua/cfg.cc
> index 4884ce013..42070fe49 100644
> --- a/src/box/lua/cfg.cc
> +++ b/src/box/lua/cfg.cc
> @@ -274,6 +274,17 @@ lbox_cfg_set_net_msg_max(struct lua_State *L)
>  	return 0;
>  }
>  
> +static int
> +lbox_set_prepared_stmt_cache_size(struct lua_State *L)
> +{
> +	try {
> +		box_set_prepared_stmt_cache_size();
> +	} catch (Exception *) {

4. Lets avoid exceptions in new code to reduce size of
future patches converting everything to C. Just make diag_set +
int return.

> +		luaT_error(L);
> +	}
> +	return 0;
> +}
> +
>  static int
>  lbox_cfg_set_worker_pool_threads(struct lua_State *L)
>  {
> diff --git a/src/box/prep_stmt.c b/src/box/prep_stmt.c
> new file mode 100644
> index 000000000..fe3b8244e
> --- /dev/null
> +++ b/src/box/prep_stmt.c
> @@ -0,0 +1,186 @@
> +#include "prep_stmt.h"
> +
> +#include "assoc.h"
> +#include "error.h"
> +#include "execute.h"
> +#include "session.h"
> +
> +/** Default cache size is 5 Mb. */
> +size_t prep_stmt_cache_size = 5 * 1024 * 1024;

5. This value is 'extern' in the header, but is never
used out of prep_stmt.c. And is not changed even here.

I think you can drop it. And make the initial size 0,
because box.cfg will call lbox_set_prepared_stmt_cache_size().
Even if a user didn't specify that option, it is called
anyway with the default value from load_cfg.lua.

> +
> +static struct prep_stmt_cache prep_stmt_cache;
> +
> +void
> +sql_prepared_stmt_cache_init()

6. Consider name 'sql_stmt_cache'. 'Prepared' is assumed.
Because sql_stmt by definition is a prepared statement.

> +{
> +	prep_stmt_cache.hash = mh_strnptr_new();
> +	if (prep_stmt_cache.hash == NULL)
> +		panic("out of memory");
> +	prep_stmt_cache.mem_quota = prep_stmt_cache_size;
> +	prep_stmt_cache.mem_used = 0;
> +	rlist_create(&prep_stmt_cache.cache_lru);
> +}
> +
> +static size_t
> +sql_cache_node_sizeof(struct sql_stmt *stmt)
> +{
> +	return sql_stmt_sizeof(stmt) + sizeof(struct stmt_cache_node *);

7. Maybe sizeof(struct stmt_cache_node)? Without '*'.

> +}
> +
> +/**
> + * Allocate new cache node containing given prepared statement.
> + * Add it to the LRU cache list. Account cache size enlargement.
> + */
> +static struct stmt_cache_node *
> +sql_cache_node_new(struct sql_stmt *stmt)

8. This is strange. You not just created the node,
but also added to the cache. But not in the hash table,
only in the lru list. How about to make this function
do only creation of the node, and add to lru + update
the size in sql_prepared_stmt_cache_insert?

The same about node_delete. Or make new/delete() add
to the hash table/remove from it too?

> +{
> +	struct stmt_cache_node *node = malloc(sizeof(*node));
> +	if (node == NULL) {
> +		diag_set(OutOfMemory, sizeof(*node), "malloc",
> +			 "struct stmt_cache_node");
> +		return NULL;
> +	}
> +	node->stmt = stmt;
> +	rlist_add(&prep_stmt_cache.cache_lru, &node->in_lru);
> +	prep_stmt_cache.mem_used += sql_cache_node_sizeof(stmt);
> +	return node;
> +}
> diff --git a/src/box/prep_stmt.h b/src/box/prep_stmt.h
> new file mode 100644
> index 000000000..31f3ff52b
> --- /dev/null
> +++ b/src/box/prep_stmt.h
> @@ -0,0 +1,112 @@
> +#include <stdint.h>
> +#include <stdio.h>
> +
> +#include "small/rlist.h"
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif
> +
> +extern size_t prep_stmt_cache_size;
> +
> +/**
> + * Global prepared statement cache which follows LRU
> + * eviction policy. Implemented as hash <str : stmt>
> + * and double linked list.
> + */
> +struct prep_stmt_cache {

9. Lets call it 'sql_stmt_cache'. To be consistent in having
this widespread alias of struct Vdbe.

> +	/** Size of memory currently occupied by prepared statements. */
> +	size_t mem_used;
> +	/**
> +	 * Max memory size that can be used for cache.
> +	 */
> +	size_t mem_quota;
> +	/** Query hash -> struct prepared_stmt hash.*/

10. A typo? Struct prepared_stmt does not exist, and
you store a statement as a value, not its hash.

> +	struct mh_strnptr_t *hash;

11. Please, rename 'hash' to 'cache', and add a comment,
that cache_lru and cache contain the same records. The
lru list is maintained only for the eviction policy.

> +	struct rlist cache_lru;
> +};
> +
> +struct stmt_cache_node {

12. Please, try to encapsulate this inside prep_stmt.c.
The API should become simpler, when you operate by
struct sql_stmt only, and don't need to worry about
cache nodes.

Maybe that is not possible, I don't know. I didn't
looked at the last two patches yet.

> +	/** Prepared statement itself. */
> +	struct sql_stmt *stmt;
> +	/**
> +	 * Link to the next node. Head is the newest, tail is
> +	 * a candidate to be evicted.
> +	 */
> +	struct rlist in_lru;
> +};
> +
> +struct sql_stmt;

13. This probably should go before first usage of struct sql_stmt,
which is above. As well as declaration of struct mh_strnptr_t.

Re: [Tarantool-patches] [PATCH v2 14/16] sql: introduce cache for prepared statemets

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> +    prep_stmt.c

sql_stmt_cache.{h,c}
> +static void
> +box_check_sql_cache_size(int size)
> +{
> +	if (size < 0) {
> +		tnt_raise(ClientError, ER_CFG, "sql_cache_size",
> +			  "must be non-negative");
> +	}
> +}

sql_stmt_cache_size?

> +void
> +box_set_prepared_stmt_cache_size(void)

Please use consistent naming. box_get_{sql_stmt_cache}_size(),
box_set_{sql_stmt_cache*}_size

> +{
> +	int cache_sz = cfg_geti("sql_cache_size");

Same here. sql_stmt_cache_size, or simply sz or s. Please
either use a shorthand like s or sz, or the same name
as the configuration variable name, to make the code simple to
grep, rename, refactor, etc.

> +	}
> +	while (! sql_cache_check_new_node_size(new_node_size))
> +		sql_prepared_stmt_cache_gc();

'Entry' is a more common name for cache entries than a 'node'.

> +	uint32_t str_hash = mh_strn_hash(sql_str, strlen(sql_str));
> +	const struct mh_strnptr_node_t hash_node = { sql_str, strlen(sql_str),
> +						     str_hash, cache_node };
> +	struct mh_strnptr_node_t *old_node = NULL;
> +	mh_int_t i = mh_strnptr_put(hash, &hash_node, &old_node, NULL);
> +	if (i == mh_end(hash)) {
> +		sql_cache_node_delete(cache, cache_node);
> +		diag_set(OutOfMemory, 0, "mh_strnptr_put", "mh_strnptr_node");
> +		return -1;
> +	}

> +sql_prepared_stmt_cache_set_size(size_t size)
> +{
> +	prep_stmt_cache.mem_quota = size;
> +	while (prep_stmt_cache.mem_used > size)
> +		sql_prepared_stmt_cache_gc();
> +}

How does this work if there are active statements which are
referenced/in use?


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 14/16] sql: introduce cache for prepared statemets

[Kirill Yukhin]

Hello Nikita,
On 21 ноя 00:28, Nikita Pettik wrote:
> This patch introduces cache (as data structure) to handle prepared
> statements and a set of interface functions (insert, delete, find) to
> operate on it. Cache under the hood uses LRU eviction policy. It is
> implemented as a hash table with string keys (which contain original SQL
> statements) and prepared statements (struct sql_stmt which is an alias
> for struct Vdbe) as values. To realise LRU strategy we maintain list of
> nodes: head is the newest prepared statement, tail a candidate to be
> evicted. Size of cache is regulated via box.cfg{sql_cache_size}
> parameter. Cache is global to all sessions. To erase session manually,
> one can set its size to 0. Default cache size is assumed to be 5 Mb.
> 
> Part of #2592

Could you please implement prepared statements storage
machinery according to proposal posted in `discussions`.
Here's the quote:

    Date: Tue, 22 Oct 2019 15:57:09 +0300
    From: Nikita Pettik <korablev@tarantool.org>
    To: dev@tarantool.org
    Cc: kostja.osipov@gmail.com, v.shpilevoy@tarantool.org, georgy@tarantool.org, sergos@tarantool.org
    Subject: Re: [dev] rfc: prepared statements
    User-Agent: Mutt/1.9.2 (2017-12-15)

    On 01 Oct 00:16, Nikita Pettik wrote:
    > Hi everyone,
    >
    > RFC in human-readable format is available here:
    > https://github.com/tarantool/tarantool/blob/np/sql-prepared-stmt-rfc/doc/rfc/2592-prepared-statement.md

    Yesterday we had discussion concerning prepared statements identifiers.
    It was decided to use sequential numeric ids instead of strings since
    strings are likely to add too much overhead on binary protocol payload.
    All problems which may appear due to cluster configuration were decided
    to delegate to proxy. Proxy is assumed to take responsibility of mapping
    query id to appropriate Tarantool instance.


--
Regards, Kirill Yukhin

[Tarantool-patches] [PATCH v2 15/16] box: introduce prepared statements

[Nikita Pettik]

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 string, 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 method
:execute(). It corresponds to box.execute(stmt.sql_str), i.e. automatically
substitutes 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 are re-prepared automatically on demand.
It is worth noting that box.execute() always attempts at finding
statement to be executed in prepared statement cache. Thus, once statement
is prepared, both box.execute() and :execute() methods will execute
already compiled statement.

SQL cache memory limit is regulated by box{sql_cache_size} which can be
set dynamically. Setting it to 0 completely erases cache.

Part of #2592
---
 src/box/execute.c          |  90 +++++++
 src/box/execute.h          |   8 +-
 src/box/lua/execute.c      | 141 ++++++++++-
 src/box/lua/execute.h      |   2 +-
 src/box/lua/init.c         |   2 +-
 src/box/sql/prepare.c      |   9 -
 test/box/misc.result       |   1 +
 test/sql/engine.cfg        |   3 +
 test/sql/prepared.result   | 574 +++++++++++++++++++++++++++++++++++++++++++++
 test/sql/prepared.test.lua | 196 ++++++++++++++++
 10 files changed, 1009 insertions(+), 17 deletions(-)
 create mode 100644 test/sql/prepared.result
 create mode 100644 test/sql/prepared.test.lua

diff --git a/src/box/execute.c b/src/box/execute.c
index d2a999099..2b5a9ba90 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -45,6 +45,7 @@
 #include "tuple.h"
 #include "sql/vdbe.h"
 #include "box/lua/execute.h"
+#include "box/prep_stmt.h"
 
 const char *sql_info_key_strs[] = {
 	"row_count",
@@ -411,6 +412,59 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
 	return 0;
 }
 
+static bool
+sql_stmt_check_schema_version(struct sql_stmt *stmt)
+{
+	return sql_stmt_schema_version(stmt) == box_schema_version();
+}
+
+static int
+sql_reprepare(struct stmt_cache_node **node)
+{
+	struct sql_stmt *stmt = (*node)->stmt;
+	const char *sql_str = sql_stmt_query_str(stmt);
+	struct sql_stmt *fresh_stmt;
+	if (sql_compile(sql_str, strlen(sql_str), NULL,
+			&fresh_stmt, NULL) != 0)
+		return -1;
+	sql_prepared_stmt_cache_delete(*node);
+	if (sql_prepared_stmt_cache_insert(fresh_stmt) != 0) {
+		sql_finalize(fresh_stmt);
+		return -1;
+	}
+	sql_str = sql_stmt_query_str(fresh_stmt);
+	*node = sql_prepared_stmt_cache_find(sql_str);
+	return 0;
+}
+
+int
+sql_prepare(const char *sql, int len, struct port *port)
+{
+	struct stmt_cache_node *stmt_node = sql_prepared_stmt_cache_find(sql);
+	struct sql_stmt *stmt;
+	if (stmt_node == NULL) {
+		if (sql_compile(sql, len, NULL, &stmt, NULL) != 0)
+			return -1;
+		if (sql_prepared_stmt_cache_insert(stmt) != 0) {
+			sql_finalize(stmt);
+			return -1;
+		}
+	} else {
+		if (! sql_stmt_check_schema_version(stmt_node->stmt)) {
+			if (sql_reprepare(&stmt_node) != 0)
+				return -1;
+		} else {
+			sql_cache_stmt_refresh(stmt_node);
+		}
+		stmt = stmt_node->stmt;
+	}
+	enum sql_dump_format dump_format = sql_column_count(stmt) > 0 ?
+					   DQL_PREPARE : DML_PREPARE;
+	port_sql_create(port, stmt, dump_format, PREPARE);
+
+	return 0;
+}
+
 /**
  * Execute prepared SQL statement.
  *
@@ -448,11 +502,47 @@ sql_execute(struct sql_stmt *stmt, struct port *port, struct region *region)
 	return 0;
 }
 
+static int
+sql_execute_prepared(struct sql_stmt *stmt, const struct sql_bind *bind,
+		     uint32_t bind_count, struct port *port,
+		     struct region *region)
+{
+	if (sql_bind(stmt, bind, bind_count) != 0)
+		return -1;
+	enum sql_dump_format dump_format = sql_column_count(stmt) > 0 ?
+					   DQL_EXECUTE : DML_EXECUTE;
+	port_sql_create(port, stmt, dump_format, EXECUTE_PREPARED);
+	if (sql_execute(stmt, port, region) != 0) {
+		port_destroy(port);
+		sql_reset(stmt);
+		return -1;
+	}
+	sql_reset(stmt);
+
+	return 0;
+}
+
 int
 sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
 			uint32_t bind_count, struct port *port,
 			struct region *region)
 {
+	struct stmt_cache_node *stmt_node = sql_prepared_stmt_cache_find(sql);
+	if (stmt_node != NULL) {
+		if (! sql_stmt_check_schema_version(stmt_node->stmt)) {
+			if (sql_reprepare(&stmt_node) != 0)
+				return -1;
+		}
+		if (! sql_stmt_busy(stmt_node->stmt)) {
+			return sql_execute_prepared(stmt_node->stmt, bind,
+						    bind_count, port, region);
+		}
+	}
+	/*
+	 * In case statement is evicted from cache or it is executed
+	 * right now by another fiber, EXECUTE_PREPARED request results
+	 * in casual PREPARE + EXECUTE.
+	 */
 	struct sql_stmt *stmt;
 	if (sql_compile(sql, len, NULL, &stmt, NULL) != 0)
 		return -1;
diff --git a/src/box/execute.h b/src/box/execute.h
index 16b424d36..176966716 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -134,13 +134,11 @@ sql_stmt_query_str(const struct sql_stmt *stmt);
  * Prepare (compile into VDBE byte-code) statement.
  *
  * @param sql UTF-8 encoded SQL statement.
- * @param length Length of @param sql in bytes.
- * @param[out] stmt A pointer to the prepared statement.
- * @param[out] sql_tail End of parsed string.
+ * @param len Length of @param sql in bytes.
+ * @param port Port to store request response.
  */
 int
-sql_prepare(const char *sql, int length, struct sql_stmt **stmt,
-	    const char **sql_tail);
+sql_prepare(const char *sql, int len, struct port *port);
 
 #if defined(__cplusplus)
 } /* extern "C" { */
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index 1b2f8d235..8d46399d4 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -5,6 +5,8 @@
 #include "box/port.h"
 #include "box/execute.h"
 #include "box/bind.h"
+#include "box/prep_stmt.h"
+#include "box/schema.h"
 
 /**
  * Serialize a description of the prepared statement.
@@ -38,6 +40,68 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
 	}
 }
 
+static inline void
+lua_sql_get_params_metadata(struct sql_stmt *stmt, struct lua_State *L)
+{
+	int bind_count = sql_bind_parameter_count(stmt);
+	lua_createtable(L, bind_count, 0);
+	for (int i = 0; i < bind_count; ++i) {
+		lua_createtable(L, 0, 2);
+		const char *name = sql_bind_parameter_name(stmt, i);
+		if (name == NULL)
+			name = "?";
+		const char *type = "ANY";
+		lua_pushstring(L, name);
+		lua_setfield(L, -2, "name");
+		lua_pushstring(L, type);
+		lua_setfield(L, -2, "type");
+		lua_rawseti(L, -2, i + 1);
+	}
+}
+
+/** Forward declaration to avoid code movement. */
+static int
+lbox_execute(struct lua_State *L);
+
+/**
+ * Prepare SQL statement: compile it and save to the cache.
+ * In fact it is wrapper around box.execute() which unfolds
+ * it to box.execute(stmt.query_id).
+ */
+static int
+lbox_execute_prepared(struct lua_State *L)
+{
+	int top = lua_gettop(L);
+
+	if ((top != 1 && top != 2) || ! lua_istable(L, 1))
+		return luaL_error(L, "Usage: statement:execute([, params])");
+	lua_getfield(L, 1, "sql_str");
+	if (!lua_isstring(L, -1))
+		return luaL_error(L, "Query is expected to be string");
+	lua_remove(L, 1);
+	if (top == 2) {
+		/*
+		 * Stack state (before remove operation):
+		 * 1 Prepared statement object (Lua table)
+		 * 2 Bindings (Lua table)
+		 * 3 SQL string (fetched from PS table) - top of stack
+		 *
+		 * We should make it suitable to pass arguments to
+		 * lbox_execute(), i.e. after manipulations stack
+		 * should look like:
+		 * 1 SQL string
+		 * 2 Bindings - top of stack
+		 * Since there's no swap operation, we firstly remove
+		 * PS object, then copy table of values to be bound to
+		 * the top of stack (push), and finally remove original
+		 * bindings from stack.
+		 */
+		lua_pushvalue(L, 1);
+		lua_remove(L, 1);
+	}
+	return lbox_execute(L);
+}
+
 void
 port_sql_dump_lua(struct port *port, struct lua_State *L, bool is_flat)
 {
@@ -82,6 +146,55 @@ port_sql_dump_lua(struct port *port, struct lua_State *L, bool is_flat)
 		}
 		break;
 	}
+	case DQL_PREPARE: {
+		/* Format is following:
+		 * query_id,
+		 * param_count,
+		 * params {name, type},
+		 * metadata {name, type}
+		 * execute(), unprepare()
+		 */
+		lua_createtable(L, 0, 6);
+		/* query_id */
+		lua_pushstring(L, sql_stmt_query_str(port_sql->stmt));
+		lua_setfield(L, -2, "sql_str");
+		/* param_count */
+		luaL_pushuint64(L, sql_bind_parameter_count(stmt));
+		lua_setfield(L, -2, "param_count");
+		/* params map */
+		lua_sql_get_params_metadata(stmt, L);
+		lua_setfield(L, -2, "params");
+		/* metadata */
+		lua_sql_get_metadata(stmt, L, sql_column_count(stmt));
+		lua_setfield(L, -2, "metadata");
+		/* execute function */
+		lua_pushcfunction(L, lbox_execute_prepared);
+		lua_setfield(L, -2, "execute");
+		break;
+	}
+	case DML_PREPARE : {
+		assert(((struct port_tuple *) port)->size == 0);
+		/* Format is following:
+		 * query_id,
+		 * param_count,
+		 * params {name, type},
+		 * execute(), unprepare()
+		 */
+		lua_createtable(L, 0, 5);
+		/* query_id */
+		lua_pushstring(L, sql_stmt_query_str(port_sql->stmt));
+		lua_setfield(L, -2, "sql_str");
+		/* param_count */
+		luaL_pushuint64(L, sql_bind_parameter_count(stmt));
+		lua_setfield(L, -2, "param_count");
+		/* params map */
+		lua_sql_get_params_metadata(stmt, L);
+		lua_setfield(L, -2, "params");
+		/* execute function */
+		lua_pushcfunction(L, lbox_execute_prepared);
+		lua_setfield(L, -2, "execute");
+		break;
+	}
 	default: unreachable();
 	}
 }
@@ -272,12 +385,38 @@ lbox_execute(struct lua_State *L)
 	return 1;
 }
 
+/**
+ * Prepare SQL statement: compile it and save to the cache.
+ */
+static int
+lbox_prepare(struct lua_State *L)
+{
+	size_t length;
+	struct port port;
+	int top = lua_gettop(L);
+
+	if ((top != 1 && top != 2) || ! lua_isstring(L, 1))
+		return luaL_error(L, "Usage: box.prepare(sqlstring)");
+
+	const char *sql = lua_tolstring(L, 1, &length);
+	if (sql_prepare(sql, length, &port) != 0)
+		return luaT_push_nil_and_error(L);
+	port_dump_lua(&port, L, false);
+	port_destroy(&port);
+	return 1;
+}
+
 void
-box_lua_execute_init(struct lua_State *L)
+box_lua_sql_init(struct lua_State *L)
 {
 	lua_getfield(L, LUA_GLOBALSINDEX, "box");
 	lua_pushstring(L, "execute");
 	lua_pushcfunction(L, lbox_execute);
 	lua_settable(L, -3);
+
+	lua_pushstring(L, "prepare");
+	lua_pushcfunction(L, lbox_prepare);
+	lua_settable(L, -3);
+
 	lua_pop(L, 1);
 }
diff --git a/src/box/lua/execute.h b/src/box/lua/execute.h
index 23e193fa4..bafd67615 100644
--- a/src/box/lua/execute.h
+++ b/src/box/lua/execute.h
@@ -66,6 +66,6 @@ lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind,
 			 int idx);
 
 void
-box_lua_execute_init(struct lua_State *L);
+box_lua_sql_init(struct lua_State *L);
 
 #endif /* INCLUDES_TARANTOOL_LUA_EXECUTE_H */
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index 7ffed409d..7be520e09 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -314,7 +314,7 @@ box_lua_init(struct lua_State *L)
 	box_lua_ctl_init(L);
 	box_lua_session_init(L);
 	box_lua_xlog_init(L);
-	box_lua_execute_init(L);
+	box_lua_sql_init(L);
 	luaopen_net_box(L);
 	lua_pop(L, 1);
 	tarantool_lua_console_init(L);
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 47e40223d..182c154d5 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -193,15 +193,6 @@ sqlReprepare(Vdbe * p)
 	return 0;
 }
 
-int
-sql_prepare(const char *sql, int length, struct sql_stmt **stmt,
-	    const char **sql_tail)
-{
-	int rc = sql_compile(sql, length, 0, stmt, sql_tail);
-	assert(rc == 0 || stmt == NULL || *stmt == NULL);
-	return rc;
-}
-
 void
 sql_parser_create(struct Parse *parser, struct sql *db, uint32_t sql_flags)
 {
diff --git a/test/box/misc.result b/test/box/misc.result
index 78ffbf1dc..ce66e1bfe 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -73,6 +73,7 @@ t
   - on_commit
   - on_rollback
   - once
+  - prepare
   - priv
   - rollback
   - rollback_to_savepoint
diff --git a/test/sql/engine.cfg b/test/sql/engine.cfg
index 284c42082..a1b4b0fc5 100644
--- a/test/sql/engine.cfg
+++ b/test/sql/engine.cfg
@@ -9,6 +9,9 @@
         "remote": {"remote": "true"},
         "local": {"remote": "false"}
     },
+    "prepared.test.lua": {
+        "local": {"remote": "false"}
+    },
     "*": {
         "memtx": {"engine": "memtx"},
         "vinyl": {"engine": "vinyl"}
diff --git a/test/sql/prepared.result b/test/sql/prepared.result
new file mode 100644
index 000000000..3321f41f3
--- /dev/null
+++ b/test/sql/prepared.result
@@ -0,0 +1,574 @@
+-- test-run result file version 2
+remote = require('net.box')
+ | ---
+ | ...
+test_run = require('test_run').new()
+ | ---
+ | ...
+fiber = require('fiber')
+ | ---
+ | ...
+
+-- Wrappers to make remote and local execution interface return
+-- same result pattern.
+--
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+execute = function(...)
+    local res, err = box.execute(...)
+    if err ~= nil then
+        error(err)
+    end
+    return res
+end;
+ | ---
+ | ...
+prepare = function(...)
+    local res, err = box.prepare(...)
+    if err ~= nil then
+        error(err)
+    end
+    return res
+end;
+ | ---
+ | ...
+
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+-- Test local interface and basic capabilities of prepared statements.
+--
+execute('CREATE TABLE test (id INT PRIMARY KEY, a NUMBER, b TEXT)')
+ | ---
+ | - row_count: 1
+ | ...
+space = box.space.TEST
+ | ---
+ | ...
+space:replace{1, 2, '3'}
+ | ---
+ | - [1, 2, '3']
+ | ...
+space:replace{4, 5, '6'}
+ | ---
+ | - [4, 5, '6']
+ | ...
+space:replace{7, 8.5, '9'}
+ | ---
+ | - [7, 8.5, '9']
+ | ...
+s, e = prepare("SELECT * FROM test WHERE id = ? AND a = ?;")
+ | ---
+ | ...
+assert(e == nil)
+ | ---
+ | - true
+ | ...
+assert(s ~= nil)
+ | ---
+ | - true
+ | ...
+s.sql_str
+ | ---
+ | - SELECT * FROM test WHERE id = ? AND a = ?;
+ | ...
+s.metadata
+ | ---
+ | - - name: ID
+ |     type: integer
+ |   - name: A
+ |     type: number
+ |   - name: B
+ |     type: string
+ | ...
+s.params
+ | ---
+ | - - name: '?'
+ |     type: ANY
+ |   - name: '?'
+ |     type: ANY
+ | ...
+s.param_count
+ | ---
+ | - 2
+ | ...
+execute(s.sql_str, {1, 2})
+ | ---
+ | - metadata:
+ |   - name: ID
+ |     type: integer
+ |   - name: A
+ |     type: number
+ |   - name: B
+ |     type: string
+ |   rows:
+ |   - [1, 2, '3']
+ | ...
+execute(s.sql_str, {1, 3})
+ | ---
+ | - metadata:
+ |   - name: ID
+ |     type: integer
+ |   - name: A
+ |     type: number
+ |   - name: B
+ |     type: string
+ |   rows: []
+ | ...
+s:execute({1, 2})
+ | ---
+ | - metadata:
+ |   - name: ID
+ |     type: integer
+ |   - name: A
+ |     type: number
+ |   - name: B
+ |     type: string
+ |   rows:
+ |   - [1, 2, '3']
+ | ...
+s:execute({1, 3})
+ | ---
+ | - metadata:
+ |   - name: ID
+ |     type: integer
+ |   - name: A
+ |     type: number
+ |   - name: B
+ |     type: string
+ |   rows: []
+ | ...
+
+-- Test preparation of different types of queries.
+-- Let's start from DDL. It doesn't make much sense since
+-- any prepared DDL statement can be executed once, but
+-- anyway make sure that no crashes occur.
+--
+s = prepare("CREATE INDEX i1 ON test(a)")
+ | ---
+ | ...
+execute(s.sql_str)
+ | ---
+ | - row_count: 1
+ | ...
+execute(s.sql_str)
+ | ---
+ | - error: Index 'I1' already exists in space 'TEST'
+ | ...
+
+s = prepare("DROP INDEX i1 ON test;")
+ | ---
+ | ...
+execute(s.sql_str)
+ | ---
+ | - row_count: 1
+ | ...
+execute(s.sql_str)
+ | ---
+ | - error: No index 'I1' is defined in space 'TEST'
+ | ...
+
+s = prepare("CREATE VIEW v AS SELECT * FROM test;")
+ | ---
+ | ...
+execute(s.sql_str)
+ | ---
+ | - row_count: 1
+ | ...
+execute(s.sql_str)
+ | ---
+ | - error: Space 'V' already exists
+ | ...
+
+s = prepare("DROP VIEW v;")
+ | ---
+ | ...
+execute(s.sql_str)
+ | ---
+ | - row_count: 1
+ | ...
+execute(s.sql_str)
+ | ---
+ | - error: Space 'V' does not exist
+ | ...
+
+s = prepare("ALTER TABLE test RENAME TO test1")
+ | ---
+ | ...
+execute(s.sql_str)
+ | ---
+ | - row_count: 0
+ | ...
+execute(s.sql_str)
+ | ---
+ | - error: Space 'TEST1' already exists
+ | ...
+
+box.execute("CREATE TABLE test2 (id INT PRIMARY KEY);")
+ | ---
+ | - row_count: 1
+ | ...
+s = prepare("ALTER TABLE test2 ADD CONSTRAINT fk1 FOREIGN KEY (id) REFERENCES test2")
+ | ---
+ | ...
+execute(s.sql_str)
+ | ---
+ | - row_count: 1
+ | ...
+execute(s.sql_str)
+ | ---
+ | - error: Constraint FK1 already exists
+ | ...
+box.space.TEST2:drop()
+ | ---
+ | ...
+
+s = prepare("CREATE TRIGGER tr1 INSERT ON test1 FOR EACH ROW BEGIN DELETE FROM test1; END;")
+ | ---
+ | ...
+execute(s.sql_str)
+ | ---
+ | - row_count: 1
+ | ...
+execute(s.sql_str)
+ | ---
+ | - error: Trigger 'TR1' already exists
+ | ...
+
+s = prepare("DROP TRIGGER tr1;")
+ | ---
+ | ...
+execute(s.sql_str)
+ | ---
+ | - row_count: 1
+ | ...
+execute(s.sql_str)
+ | ---
+ | - error: Trigger 'TR1' doesn't exist
+ | ...
+
+s = prepare("DROP TABLE test1;")
+ | ---
+ | ...
+execute(s.sql_str)
+ | ---
+ | - row_count: 1
+ | ...
+execute(s.sql_str)
+ | ---
+ | - error: Space 'TEST1' does not exist
+ | ...
+
+-- DQL
+--
+execute('CREATE TABLE test (id INT PRIMARY KEY, a NUMBER, b TEXT)')
+ | ---
+ | - row_count: 1
+ | ...
+space = box.space.TEST
+ | ---
+ | ...
+space:replace{1, 2, '3'}
+ | ---
+ | - [1, 2, '3']
+ | ...
+space:replace{4, 5, '6'}
+ | ---
+ | - [4, 5, '6']
+ | ...
+space:replace{7, 8.5, '9'}
+ | ---
+ | - [7, 8.5, '9']
+ | ...
+s = prepare("SELECT a FROM test WHERE b = '3';")
+ | ---
+ | ...
+execute(s.sql_str)
+ | ---
+ | - metadata:
+ |   - name: A
+ |     type: number
+ |   rows:
+ |   - [2]
+ | ...
+execute(s.sql_str)
+ | ---
+ | - metadata:
+ |   - name: A
+ |     type: number
+ |   rows:
+ |   - [2]
+ | ...
+s:execute()
+ | ---
+ | - metadata:
+ |   - name: A
+ |     type: number
+ |   rows:
+ |   - [2]
+ | ...
+s:execute()
+ | ---
+ | - metadata:
+ |   - name: A
+ |     type: number
+ |   rows:
+ |   - [2]
+ | ...
+
+s = prepare("SELECT count(*), count(a - 3), max(b), abs(id) FROM test WHERE b = '3';")
+ | ---
+ | ...
+execute(s.sql_str)
+ | ---
+ | - metadata:
+ |   - name: count(*)
+ |     type: integer
+ |   - name: count(a - 3)
+ |     type: integer
+ |   - name: max(b)
+ |     type: scalar
+ |   - name: abs(id)
+ |     type: number
+ |   rows:
+ |   - [1, 1, '3', 1]
+ | ...
+execute(s.sql_str)
+ | ---
+ | - metadata:
+ |   - name: count(*)
+ |     type: integer
+ |   - name: count(a - 3)
+ |     type: integer
+ |   - name: max(b)
+ |     type: scalar
+ |   - name: abs(id)
+ |     type: number
+ |   rows:
+ |   - [1, 1, '3', 1]
+ | ...
+
+-- Let's try something a bit more complicated. For instance recursive
+-- query displaying Mandelbrot set.
+--
+s = prepare([[WITH RECURSIVE \
+                  xaxis(x) AS (VALUES(-2.0) UNION ALL SELECT x+0.05 FROM xaxis WHERE x<1.2), \
+                  yaxis(y) AS (VALUES(-1.0) UNION ALL SELECT y+0.1 FROM yaxis WHERE y<1.0), \
+                  m(iter, cx, cy, x, y) AS ( \
+                      SELECT 0, x, y, 0.0, 0.0 FROM xaxis, yaxis \
+                      UNION ALL \
+                      SELECT iter+1, cx, cy, x*x-y*y + cx, 2.0*x*y + cy FROM m \
+                          WHERE (x*x + y*y) < 4.0 AND iter<28), \
+                      m2(iter, cx, cy) AS ( \
+                          SELECT max(iter), cx, cy FROM m GROUP BY cx, cy), \
+                      a(t) AS ( \
+                          SELECT group_concat( substr(' .+*#', 1+LEAST(iter/7,4), 1), '') \
+                              FROM m2 GROUP BY cy) \
+                  SELECT group_concat(TRIM(TRAILING FROM t),x'0a') FROM a;]])
+ | ---
+ | ...
+
+res = execute(s.sql_str)
+ | ---
+ | ...
+res.metadata
+ | ---
+ | - - name: group_concat(TRIM(TRAILING FROM t),x'0a')
+ |     type: string
+ | ...
+
+-- Workflow with bindings is still the same.
+--
+s = prepare("SELECT a FROM test WHERE b = ?;")
+ | ---
+ | ...
+execute(s.sql_str, {'6'})
+ | ---
+ | - metadata:
+ |   - name: A
+ |     type: number
+ |   rows:
+ |   - [5]
+ | ...
+execute(s.sql_str, {'9'})
+ | ---
+ | - metadata:
+ |   - name: A
+ |     type: number
+ |   rows:
+ |   - [8.5]
+ | ...
+
+-- DML
+s = prepare("INSERT INTO test VALUES (?, ?, ?);")
+ | ---
+ | ...
+execute(s.sql_str, {5, 6, '7'})
+ | ---
+ | - row_count: 1
+ | ...
+execute(s.sql_str, {6, 10, '7'})
+ | ---
+ | - row_count: 1
+ | ...
+execute(s.sql_str, {9, 11, '7'})
+ | ---
+ | - row_count: 1
+ | ...
+
+-- EXPLAIN and PRAGMA work fine as well.
+--
+s1 = prepare("EXPLAIN SELECT a FROM test WHERE b = '3';")
+ | ---
+ | ...
+res = execute(s1.sql_str)
+ | ---
+ | ...
+res.metadata
+ | ---
+ | - - name: addr
+ |     type: integer
+ |   - name: opcode
+ |     type: text
+ |   - name: p1
+ |     type: integer
+ |   - name: p2
+ |     type: integer
+ |   - name: p3
+ |     type: integer
+ |   - name: p4
+ |     type: text
+ |   - name: p5
+ |     type: text
+ |   - name: comment
+ |     type: text
+ | ...
+assert(res.rows ~= nil)
+ | ---
+ | - true
+ | ...
+
+s2 = prepare("EXPLAIN QUERY PLAN SELECT a FROM test WHERE b = '3';")
+ | ---
+ | ...
+res = execute(s2.sql_str)
+ | ---
+ | ...
+res.metadata
+ | ---
+ | - - name: selectid
+ |     type: integer
+ |   - name: order
+ |     type: integer
+ |   - name: from
+ |     type: integer
+ |   - name: detail
+ |     type: text
+ | ...
+assert(res.rows ~= nil)
+ | ---
+ | - true
+ | ...
+
+s3 = prepare("PRAGMA count_changes;")
+ | ---
+ | ...
+execute(s3.sql_str)
+ | ---
+ | - metadata:
+ |   - name: defer_foreign_keys
+ |     type: integer
+ |   rows:
+ |   - [0]
+ | ...
+
+-- Setting cache size to 0 erases all content from it.
+--
+box.cfg{sql_cache_size = 0}
+ | ---
+ | ...
+s = prepare("SELECT a FROM test;")
+ | ---
+ | - error: 'Failed to prepare SQL statement: size of statement exceeds cache memory
+ |     limit. Please, increase SQL cache size'
+ | ...
+assert(s ~= nil)
+ | ---
+ | - true
+ | ...
+
+-- Still with small size everything should work.
+--
+box.cfg{sql_cache_size = 1500}
+ | ---
+ | ...
+
+test_run:cmd("setopt delimiter ';'");
+ | ---
+ | - true
+ | ...
+for i = 1, 5 do
+    pcall(prepare, string.format("SELECT * FROM test WHERE id = %d;", i))
+end;
+ | ---
+ | ...
+s = prepare("SELECT a FROM test");
+ | ---
+ | ...
+assert(s ~= nil);
+ | ---
+ | - true
+ | ...
+
+-- Make sure that if prepared statement is busy (is executed
+-- right now), prepared statement is not used, i.e. statement
+-- is compiled from scratch, executed and finilized.
+--
+box.schema.func.create('SLEEP', {language = 'Lua',
+    body = 'function () fiber.sleep(0.1) return 1 end',
+    exports = {'LUA', 'SQL'}});
+ | ---
+ | ...
+
+s = prepare("SELECT id, SLEEP() FROM test");
+ | ---
+ | ...
+assert(s ~= nil);
+ | ---
+ | - true
+ | ...
+
+function implicit_yield()
+    execute("SELECT id, SLEEP() FROM test")
+end;
+ | ---
+ | ...
+
+f1 = fiber.new(implicit_yield)
+f2 = fiber.new(implicit_yield)
+f1:set_joinable(true)
+f2:set_joinable(true)
+
+f1:join();
+ | ---
+ | ...
+f2:join();
+ | ---
+ | - true
+ | ...
+
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+box.space.TEST:drop()
+ | ---
+ | ...
+box.schema.func.drop('SLEEP')
+ | ---
+ | ...
diff --git a/test/sql/prepared.test.lua b/test/sql/prepared.test.lua
new file mode 100644
index 000000000..24e49832f
--- /dev/null
+++ b/test/sql/prepared.test.lua
@@ -0,0 +1,196 @@
+remote = require('net.box')
+test_run = require('test_run').new()
+fiber = require('fiber')
+
+-- Wrappers to make remote and local execution interface return
+-- same result pattern.
+--
+test_run:cmd("setopt delimiter ';'")
+execute = function(...)
+    local res, err = box.execute(...)
+    if err ~= nil then
+        error(err)
+    end
+    return res
+end;
+prepare = function(...)
+    local res, err = box.prepare(...)
+    if err ~= nil then
+        error(err)
+    end
+    return res
+end;
+
+test_run:cmd("setopt delimiter ''");
+
+-- Test local interface and basic capabilities of prepared statements.
+--
+execute('CREATE TABLE test (id INT PRIMARY KEY, a NUMBER, b TEXT)')
+space = box.space.TEST
+space:replace{1, 2, '3'}
+space:replace{4, 5, '6'}
+space:replace{7, 8.5, '9'}
+s, e = prepare("SELECT * FROM test WHERE id = ? AND a = ?;")
+assert(e == nil)
+assert(s ~= nil)
+s.sql_str
+s.metadata
+s.params
+s.param_count
+execute(s.sql_str, {1, 2})
+execute(s.sql_str, {1, 3})
+s:execute({1, 2})
+s:execute({1, 3})
+
+-- Test preparation of different types of queries.
+-- Let's start from DDL. It doesn't make much sense since
+-- any prepared DDL statement can be executed once, but
+-- anyway make sure that no crashes occur.
+--
+s = prepare("CREATE INDEX i1 ON test(a)")
+execute(s.sql_str)
+execute(s.sql_str)
+
+s = prepare("DROP INDEX i1 ON test;")
+execute(s.sql_str)
+execute(s.sql_str)
+
+s = prepare("CREATE VIEW v AS SELECT * FROM test;")
+execute(s.sql_str)
+execute(s.sql_str)
+
+s = prepare("DROP VIEW v;")
+execute(s.sql_str)
+execute(s.sql_str)
+
+s = prepare("ALTER TABLE test RENAME TO test1")
+execute(s.sql_str)
+execute(s.sql_str)
+
+box.execute("CREATE TABLE test2 (id INT PRIMARY KEY);")
+s = prepare("ALTER TABLE test2 ADD CONSTRAINT fk1 FOREIGN KEY (id) REFERENCES test2")
+execute(s.sql_str)
+execute(s.sql_str)
+box.space.TEST2:drop()
+
+s = prepare("CREATE TRIGGER tr1 INSERT ON test1 FOR EACH ROW BEGIN DELETE FROM test1; END;")
+execute(s.sql_str)
+execute(s.sql_str)
+
+s = prepare("DROP TRIGGER tr1;")
+execute(s.sql_str)
+execute(s.sql_str)
+
+s = prepare("DROP TABLE test1;")
+execute(s.sql_str)
+execute(s.sql_str)
+
+-- DQL
+--
+execute('CREATE TABLE test (id INT PRIMARY KEY, a NUMBER, b TEXT)')
+space = box.space.TEST
+space:replace{1, 2, '3'}
+space:replace{4, 5, '6'}
+space:replace{7, 8.5, '9'}
+s = prepare("SELECT a FROM test WHERE b = '3';")
+execute(s.sql_str)
+execute(s.sql_str)
+s:execute()
+s:execute()
+
+s = prepare("SELECT count(*), count(a - 3), max(b), abs(id) FROM test WHERE b = '3';")
+execute(s.sql_str)
+execute(s.sql_str)
+
+-- Let's try something a bit more complicated. For instance recursive
+-- query displaying Mandelbrot set.
+--
+s = prepare([[WITH RECURSIVE \
+                  xaxis(x) AS (VALUES(-2.0) UNION ALL SELECT x+0.05 FROM xaxis WHERE x<1.2), \
+                  yaxis(y) AS (VALUES(-1.0) UNION ALL SELECT y+0.1 FROM yaxis WHERE y<1.0), \
+                  m(iter, cx, cy, x, y) AS ( \
+                      SELECT 0, x, y, 0.0, 0.0 FROM xaxis, yaxis \
+                      UNION ALL \
+                      SELECT iter+1, cx, cy, x*x-y*y + cx, 2.0*x*y + cy FROM m \
+                          WHERE (x*x + y*y) < 4.0 AND iter<28), \
+                      m2(iter, cx, cy) AS ( \
+                          SELECT max(iter), cx, cy FROM m GROUP BY cx, cy), \
+                      a(t) AS ( \
+                          SELECT group_concat( substr(' .+*#', 1+LEAST(iter/7,4), 1), '') \
+                              FROM m2 GROUP BY cy) \
+                  SELECT group_concat(TRIM(TRAILING FROM t),x'0a') FROM a;]])
+
+res = execute(s.sql_str)
+res.metadata
+
+-- Workflow with bindings is still the same.
+--
+s = prepare("SELECT a FROM test WHERE b = ?;")
+execute(s.sql_str, {'6'})
+execute(s.sql_str, {'9'})
+
+-- DML
+s = prepare("INSERT INTO test VALUES (?, ?, ?);")
+execute(s.sql_str, {5, 6, '7'})
+execute(s.sql_str, {6, 10, '7'})
+execute(s.sql_str, {9, 11, '7'})
+
+-- EXPLAIN and PRAGMA work fine as well.
+--
+s1 = prepare("EXPLAIN SELECT a FROM test WHERE b = '3';")
+res = execute(s1.sql_str)
+res.metadata
+assert(res.rows ~= nil)
+
+s2 = prepare("EXPLAIN QUERY PLAN SELECT a FROM test WHERE b = '3';")
+res = execute(s2.sql_str)
+res.metadata
+assert(res.rows ~= nil)
+
+s3 = prepare("PRAGMA count_changes;")
+execute(s3.sql_str)
+
+-- Setting cache size to 0 erases all content from it.
+--
+box.cfg{sql_cache_size = 0}
+s = prepare("SELECT a FROM test;")
+assert(s ~= nil)
+
+-- Still with small size everything should work.
+--
+box.cfg{sql_cache_size = 1500}
+
+test_run:cmd("setopt delimiter ';'");
+for i = 1, 5 do
+    pcall(prepare, string.format("SELECT * FROM test WHERE id = %d;", i))
+end;
+s = prepare("SELECT a FROM test");
+assert(s ~= nil);
+
+-- Make sure that if prepared statement is busy (is executed
+-- right now), prepared statement is not used, i.e. statement
+-- is compiled from scratch, executed and finilized.
+--
+box.schema.func.create('SLEEP', {language = 'Lua',
+    body = 'function () fiber.sleep(0.1) return 1 end',
+    exports = {'LUA', 'SQL'}});
+
+s = prepare("SELECT id, SLEEP() FROM test");
+assert(s ~= nil);
+
+function implicit_yield()
+    execute("SELECT id, SLEEP() FROM test")
+end;
+
+f1 = fiber.new(implicit_yield)
+f2 = fiber.new(implicit_yield)
+f1:set_joinable(true)
+f2:set_joinable(true)
+
+f1:join();
+f2:join();
+
+test_run:cmd("setopt delimiter ''");
+
+box.space.TEST:drop()
+box.schema.func.drop('SLEEP')
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 15/16] box: introduce prepared statements

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/21 10:00]:
> 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 string, 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 method
> :execute(). It corresponds to box.execute(stmt.sql_str), i.e. automatically
> substitutes 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 are re-prepared automatically on demand.
> It is worth noting that box.execute() always attempts at finding
> statement to be executed in prepared statement cache. Thus, once statement
> is prepared, both box.execute() and :execute() methods will execute
> already compiled statement.
> 
> SQL cache memory limit is regulated by box{sql_cache_size} which can be
> set dynamically. Setting it to 0 completely erases cache.
> 
> Part of #2592

Turned out to be not complex at all, eh?

Generally, LGTM (will give it another look).

The biggest issue so far I see with the entire series is effects
of schema version changes and cache size changes on
performance/correctness. I'll need to give it some more thought.


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 15/16] box: introduce prepared statements

[Vladislav Shpilevoy]

Thanks for the patch!

See 7 comments below.

On 20/11/2019 22:28, 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 string, 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 method
> :execute(). It corresponds to box.execute(stmt.sql_str), i.e. automatically
> substitutes 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 are re-prepared automatically on demand.
> It is worth noting that box.execute() always attempts at finding
> statement to be executed in prepared statement cache. Thus, once statement
> is prepared, both box.execute() and :execute() methods will execute
> already compiled statement.
> 
> SQL cache memory limit is regulated by box{sql_cache_size} which can be
> set dynamically. Setting it to 0 completely erases cache.

1. The problem with cache size is that you gave users a way to
limit it, but didn't give a method to calculate how much memory
do they need for their statements. For example, a user knows
what statements he wants in the cache. How can he estimate the
cache size for the these statements? Does he need 100 bytes per
statement, 1KB, 100KB?

We either need a theoretical description how to calculate it,
even very rough, or maybe return a statement estimated size
on box.prepare(). Then a user at least can do some experiments
to estimate cache size.

Or you can add statistics to box.info about how many SQL
statements are prepared, and how much memory is occupied in
the cache. That is even better I think. In box.info.sql.

Maybe that is a good thing to do in a separate ticket. Don't
know.

> diff --git a/src/box/execute.c b/src/box/execute.c
> index d2a999099..2b5a9ba90 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c> @@ -411,6 +412,59 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
>  	return 0;
>  }
>  
> +static bool
> +sql_stmt_check_schema_version(struct sql_stmt *stmt)

2. 'Const' struct sql_stmt, please.

> +{
> +	return sql_stmt_schema_version(stmt) == box_schema_version();
> +}
> +
> +static int
> +sql_reprepare(struct stmt_cache_node **node)
> +{
> +	struct sql_stmt *stmt = (*node)->stmt;
> +	const char *sql_str = sql_stmt_query_str(stmt);
> +	struct sql_stmt *fresh_stmt;
> +	if (sql_compile(sql_str, strlen(sql_str), NULL,
> +			&fresh_stmt, NULL) != 0)
> +		return -1;
> +	sql_prepared_stmt_cache_delete(*node);
> +	if (sql_prepared_stmt_cache_insert(fresh_stmt) != 0) {
> +		sql_finalize(fresh_stmt);
> +		return -1;
> +	}
> +	sql_str = sql_stmt_query_str(fresh_stmt);
> +	*node = sql_prepared_stmt_cache_find(sql_str);
> +	return 0;
> +}
> +
> +int
> +sql_prepare(const char *sql, int len, struct port *port)
> +{
> +	struct stmt_cache_node *stmt_node = sql_prepared_stmt_cache_find(sql);
> +	struct sql_stmt *stmt;
> +	if (stmt_node == NULL) {
> +		if (sql_compile(sql, len, NULL, &stmt, NULL) != 0)
> +			return -1;
> +		if (sql_prepared_stmt_cache_insert(stmt) != 0) {
> +			sql_finalize(stmt);
> +			return -1;
> +		}
> +	} else {
> +		if (! sql_stmt_check_schema_version(stmt_node->stmt)) {
> +			if (sql_reprepare(&stmt_node) != 0)
> +				return -1;
> +		} else {
> +			sql_cache_stmt_refresh(stmt_node);
> +		}
> +		stmt = stmt_node->stmt;
> +	}
> +	enum sql_dump_format dump_format = sql_column_count(stmt) > 0 ?
> +					   DQL_PREPARE : DML_PREPARE;
> +	port_sql_create(port, stmt, dump_format, PREPARE);
> +

3. So, there is no any reference counting as I see?
Do you plan to add it? Because as you fairly mentioned,
in that implementation a statement, prepared in 100
different sessions, gets evicted the same as a statement
prepared only once.

However with reference counting there would be another
problem: how to increment it, actually? Each time
prepare() is called explicitly? And do nothing when only
execute() is called? And what to do when something needs
to be evicted?

> +	return 0;
> +}
> +
>  /**
>   * Execute prepared SQL statement.
>   *
> @@ -448,11 +502,47 @@ sql_execute(struct sql_stmt *stmt, struct port *port, struct region *region)
>  int
>  sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
>  			uint32_t bind_count, struct port *port,
>  			struct region *region)
>  {
> +	struct stmt_cache_node *stmt_node = sql_prepared_stmt_cache_find(sql);
> +	if (stmt_node != NULL) {

4. In case the node is NULL, and it was prepared sometime before
that, and was not unprepared, then a user does not have a way how
to understand, that a new PREPARE is needed. That makes impossible
to rely on any long living 'prepare' handle on a client side.

A client either needs to call prepare() before doing multiple
execute() just in case, or we need to return something to signal
whether the execute() has hit the cache. For example, a flag
IPROTO_SQL_CACHE_HIT: boolean. Or something like that.

Or send a flag which you lead to autoreprepare. I don't know. But
certainly it should be left as is. Probably that is a subject for
a separate ticket.

> +		if (! sql_stmt_check_schema_version(stmt_node->stmt)) {
> +			if (sql_reprepare(&stmt_node) != 0)
> +				return -1;
> +		}
> +		if (! sql_stmt_busy(stmt_node->stmt)) {
> +			return sql_execute_prepared(stmt_node->stmt, bind,
> +						    bind_count, port, region);
> +		}
> +	}
> diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
> index 1b2f8d235..8d46399d4 100644
> --- a/src/box/lua/execute.c
> +++ b/src/box/lua/execute.c
> @@ -82,6 +146,55 @@ port_sql_dump_lua(struct port *port, struct lua_State *L, bool is_flat)
>  		}
>  		break;
>  	}
> +	case DQL_PREPARE: {
> +		/* Format is following:

5. Lets better stick to the one approved code
style: '/*' is on a separate line always. Here
and in the next 'case:'.

And now I see, why did you introduce dump format
enum. But I don't know how to comment it yet.
I think, on a next iteration after the other
fixes I will say more.

> diff --git a/test/sql/prepared.test.lua b/test/sql/prepared.test.lua
> new file mode 100644
> index 000000000..24e49832f
> --- /dev/null
> +++ b/test/sql/prepared.test.lua
> @@ -0,0 +1,196 @@
> +s = prepare("CREATE TRIGGER tr1 INSERT ON test1 FOR EACH ROW BEGIN DELETE FROM test1; END;")
> +execute(s.sql_str)
> +execute(s.sql_str)
> +
> +s = prepare("DROP TRIGGER tr1;")
> +execute(s.sql_str)
> +execute(s.sql_str)
> +
> +s = prepare("DROP TABLE test1;")
> +execute(s.sql_str)
> +execute(s.sql_str)

6. I saw Kostja asked about why do you increment
schema version on some new system space updates,
such as _trigger.

There is why - we generate DROP TABLE as manual
deletion of all space-related objects, and then of
the space itself. Space related objects include
triggers, ck, fk. And they are taken from struct
space during compilation. That is, if you won't
increment schema version on trigger/ck/fk update,
a prepared DROP TABLE won't take the changes into
account and won't be regenerated.

This test fails in case you don't update schema
version on _trigger update, right?

Talking of all the 'generate DROP TABLE by struct
space content' idea - I don't like it. But this
is how it works now. I future we will need to
open an iterator on the needed spaces at runtime.
Because lookup in struct space won't be available
on client side anyway.

> +f1 = fiber.new(implicit_yield)
> +f2 = fiber.new(implicit_yield)
> +f1:set_joinable(true)
> +f2:set_joinable(true)
> +
> +f1:join();
> +f2:join();
> +
> +test_run:cmd("setopt delimiter ''");
> +
> +box.space.TEST:drop()
> +box.schema.func.drop('SLEEP')
> 

7. On the whole, that is probably the best thing happened to SQL
in Tarantool for a very long time.

[Tarantool-patches] [PATCH v2 16/16] netbox: introduce prepared statements

[Nikita Pettik]

This patch introduces support of prepared statements in IProto
protocol. To achieve this new IProto command is added - IPROTO_PREPARE.
It is sent with IPROTO_SQL_TEXT key and it means to prepare SQL statement
(for details see previous commit). Also to reply on PREPARE request a few
response keys are added: IPROTO_BIND_METADATA (contains parameters
metadata) and IPROTO_BIND_COUNT (count of parameters to be bound).

Closes #2592

@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 string
containing original SQL request. Prepared statement cache is global and
follows LRU eviction policy: when there's no place for another one
statement, the statement that is least recently used (prepared) is
removed. Cache size is adjusted by box.cfg{sql_cache_size} variable
(can be set dynamically; in case size is reduced some statements may be
erased from cache). Note that any DDL operation leads to expiration of all
prepared statements: they are recompiled on demand. To erase whole cache
one can set its size to 0. Prepared statements are available in local mode
(i.e. via box.prepare() function) and are supported in IProto protocol.
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})

In terms of remote connection:

cn = netbox:connect(addr)
s = cn:prepare("SELECT * FROM t WHERE id = ?;")
cn:execute(s.sql_str, {1})
---
 src/box/execute.c          |  88 +++++++++++++++++++++++++++++++++++++
 src/box/iproto.cc          |  25 ++++++++---
 src/box/iproto_constants.c |   6 ++-
 src/box/iproto_constants.h |   4 ++
 src/box/lua/net_box.c      |  79 +++++++++++++++++++++++++++++++++
 src/box/lua/net_box.lua    |  13 ++++++
 src/box/xrow.c             |   2 +-
 src/box/xrow.h             |   2 +-
 test/box/misc.result       |   1 +
 test/sql/engine.cfg        |   1 +
 test/sql/prepared.result   | 106 ++++++++++++++++++++++++++-------------------
 test/sql/prepared.test.lua |  61 +++++++++++++++++++-------
 12 files changed, 320 insertions(+), 68 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index 2b5a9ba90..13e5a6ba1 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -326,6 +326,67 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
 	return 0;
 }
 
+static inline int
+sql_get_params_metadata(struct sql_stmt *stmt, struct obuf *out)
+{
+	int bind_count = sql_bind_parameter_count(stmt);
+	int size = mp_sizeof_uint(IPROTO_BIND_METADATA) +
+		   mp_sizeof_array(bind_count);
+	char *pos = (char *) obuf_alloc(out, size);
+	if (pos == NULL) {
+		diag_set(OutOfMemory, size, "obuf_alloc", "pos");
+		return -1;
+	}
+	pos = mp_encode_uint(pos, IPROTO_BIND_METADATA);
+	pos = mp_encode_array(pos, bind_count);
+	for (int i = 0; i < bind_count; ++i) {
+		size_t size = mp_sizeof_map(2) +
+			      mp_sizeof_uint(IPROTO_FIELD_NAME) +
+			      mp_sizeof_uint(IPROTO_FIELD_TYPE);
+		const char *name = sql_bind_parameter_name(stmt, i);
+		if (name == NULL)
+			name = "?";
+		const char *type = "ANY";
+		size += mp_sizeof_str(strlen(name));
+		size += mp_sizeof_str(strlen(type));
+		char *pos = (char *) obuf_alloc(out, size);
+		if (pos == NULL) {
+			diag_set(OutOfMemory, size, "obuf_alloc", "pos");
+			return -1;
+		}
+		pos = mp_encode_map(pos, 2);
+		pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
+		pos = mp_encode_str(pos, name, strlen(name));
+		pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
+		pos = mp_encode_str(pos, type, strlen(type));
+	}
+	return 0;
+}
+
+static int
+sql_get_prepare_common_keys(struct sql_stmt *stmt, struct obuf *out, int keys,
+			    const char *sql_str)
+{
+	int size = mp_sizeof_map(keys) +
+		   mp_sizeof_uint(IPROTO_SQL_TEXT) +
+		   mp_sizeof_str(strlen(sql_str)) +
+		   mp_sizeof_uint(IPROTO_BIND_COUNT) +
+		   mp_sizeof_uint(sql_bind_parameter_count(stmt));
+	char *pos = (char *) obuf_alloc(out, size);
+	if (pos == NULL) {
+		diag_set(OutOfMemory, size, "obuf_alloc", "pos");
+		return -1;
+	}
+	pos = mp_encode_map(pos, keys);
+	pos = mp_encode_uint(pos, IPROTO_SQL_TEXT);
+	pos = mp_encode_str(pos, sql_str, strlen(sql_str));
+	pos = mp_encode_uint(pos, IPROTO_BIND_COUNT);
+	pos = mp_encode_uint(pos, sql_bind_parameter_count(stmt));
+	if (sql_get_params_metadata(stmt, out) != 0)
+		return -1;
+	return 0;
+}
+
 static int
 port_sql_dump_msgpack(struct port *port, struct obuf *out)
 {
@@ -407,6 +468,33 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
 		}
 		break;
 	}
+	case DQL_PREPARE: {
+		/* Format is following:
+		 * query_id,
+		 * param_count,
+		 * params {name, type},
+		 * metadata {name, type}
+		 */
+		int keys = 4;
+		if (sql_get_prepare_common_keys(stmt, out, keys,
+						sql_stmt_query_str(stmt)) != 0)
+			return -1;
+		if (sql_get_metadata(stmt, out, sql_column_count(stmt)) != 0)
+			return -1;
+		break;
+	}
+	case DML_PREPARE: {
+		/* Format is following:
+		 * query_id,
+		 * param_count,
+		 * params {name, type},
+		 */
+		int keys = 3;
+		if (sql_get_prepare_common_keys(stmt, out, keys,
+						sql_stmt_query_str(stmt)) != 0)
+			return -1;
+		break;
+		}
 	default: unreachable();
 	}
 	return 0;
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 34c8f469a..a52c76961 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -178,7 +178,7 @@ struct iproto_msg
 		struct call_request call;
 		/** Authentication request. */
 		struct auth_request auth;
-		/* SQL request, if this is the EXECUTE request. */
+		/* SQL request, if this is the EXECUTE/PREPARE request. */
 		struct sql_request sql;
 		/** In case of iproto parse error, saved diagnostics. */
 		struct diag diag;
@@ -1155,6 +1155,7 @@ static const struct cmsg_hop *dml_route[IPROTO_TYPE_STAT_MAX] = {
 	call_route,                             /* IPROTO_CALL */
 	sql_route,                              /* IPROTO_EXECUTE */
 	NULL,                                   /* IPROTO_NOP */
+	sql_route,                              /* IPROTO_PREPARE */
 };
 
 static const struct cmsg_hop join_route[] = {
@@ -1210,6 +1211,7 @@ iproto_msg_decode(struct iproto_msg *msg, const char **pos, const char *reqend,
 		cmsg_init(&msg->base, call_route);
 		break;
 	case IPROTO_EXECUTE:
+	case IPROTO_PREPARE:
 		if (xrow_decode_sql(&msg->header, &msg->sql) != 0)
 			goto error;
 		cmsg_init(&msg->base, sql_route);
@@ -1653,7 +1655,8 @@ tx_process_sql(struct cmsg *m)
 
 	if (tx_check_schema(msg->header.schema_version))
 		goto error;
-	assert(msg->header.type == IPROTO_EXECUTE);
+	assert(msg->header.type == IPROTO_EXECUTE ||
+	       msg->header.type == IPROTO_PREPARE);
 	tx_inject_delay();
 	if (msg->sql.bind != NULL) {
 		bind_count = sql_bind_list_decode(msg->sql.bind, &bind);
@@ -1662,9 +1665,20 @@ tx_process_sql(struct cmsg *m)
 	}
 	sql = msg->sql.sql_text;
 	sql = mp_decode_str(&sql, &len);
-	if (sql_prepare_and_execute(sql, len, bind, bind_count, &port,
-				    &fiber()->gc) != 0)
-		goto error;
+	if (msg->header.type == IPROTO_EXECUTE) {
+		sql = msg->sql.sql_text;
+		sql = mp_decode_str(&sql, &len);
+		if (sql_prepare_and_execute(sql, len, bind, bind_count,
+					    &port, &fiber()->gc) != 0)
+			goto error;
+	} else {
+		/* IPROTO_PREPARE */
+		sql = msg->sql.sql_text;
+		sql = mp_decode_str(&sql, &len);
+		if (sql_prepare(sql, len, &port) != 0)
+			goto error;
+	}
+
 	/*
 	 * Take an obuf only after execute(). Else the buffer can
 	 * become out of date during yield.
@@ -1676,6 +1690,7 @@ tx_process_sql(struct cmsg *m)
 		port_destroy(&port);
 		goto error;
 	}
+	/* Nothing to dump in case of UNPREPARE request. */
 	if (port_dump_msgpack(&port, out) != 0) {
 		port_destroy(&port);
 		obuf_rollback_to_svp(out, &header_svp);
diff --git a/src/box/iproto_constants.c b/src/box/iproto_constants.c
index 09ded1ecb..6c4dc4f8c 100644
--- a/src/box/iproto_constants.c
+++ b/src/box/iproto_constants.c
@@ -107,6 +107,7 @@ const char *iproto_type_strs[] =
 	"CALL",
 	"EXECUTE",
 	NULL, /* NOP */
+	"PREPARE",
 };
 
 #define bit(c) (1ULL<<IPROTO_##c)
@@ -124,6 +125,7 @@ const uint64_t iproto_body_key_map[IPROTO_TYPE_STAT_MAX] = {
 	0,                                                     /* CALL */
 	0,                                                     /* EXECUTE */
 	0,                                                     /* NOP */
+	0,                                                     /* PREPARE */
 };
 #undef bit
 
@@ -179,8 +181,8 @@ const char *iproto_key_strs[IPROTO_KEY_MAX] = {
 	"data",             /* 0x30 */
 	"error",            /* 0x31 */
 	"metadata",         /* 0x32 */
-	NULL,               /* 0x33 */
-	NULL,               /* 0x34 */
+	"bind meta",        /* 0x33 */
+	"bind count",       /* 0x34 */
 	NULL,               /* 0x35 */
 	NULL,               /* 0x36 */
 	NULL,               /* 0x37 */
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index 5e8a7d483..080886ab0 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -110,6 +110,8 @@ enum iproto_key {
 	 * ]
 	 */
 	IPROTO_METADATA = 0x32,
+	IPROTO_BIND_METADATA = 0x33,
+	IPROTO_BIND_COUNT = 0x34,
 
 	/* Leave a gap between response keys and SQL keys. */
 	IPROTO_SQL_TEXT = 0x40,
@@ -203,6 +205,8 @@ enum iproto_type {
 	IPROTO_EXECUTE = 11,
 	/** No operation. Treated as DML, used to bump LSN. */
 	IPROTO_NOP = 12,
+	/** Prepare SQL statement. */
+	IPROTO_PREPARE = 13,
 	/** The maximum typecode used for box.stat() */
 	IPROTO_TYPE_STAT_MAX,
 
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 001af95dc..03b585f36 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -585,6 +585,26 @@ netbox_encode_execute(lua_State *L)
 	return 0;
 }
 
+static int
+netbox_encode_prepare(lua_State *L)
+{
+	if (lua_gettop(L) < 3)
+		return luaL_error(L, "Usage: netbox.encode_prepare(ibuf, "\
+				     "sync, query)");
+	struct mpstream stream;
+	size_t svp = netbox_prepare_request(L, &stream, IPROTO_PREPARE);
+
+	mpstream_encode_map(&stream, 1);
+
+	size_t len;
+	const char *query = lua_tolstring(L, 3, &len);
+	mpstream_encode_uint(&stream, IPROTO_SQL_TEXT);
+	mpstream_encode_strn(&stream, query, len);
+
+	netbox_encode_request(&stream, svp);
+	return 0;
+}
+
 /**
  * Decode IPROTO_DATA into tuples array.
  * @param L Lua stack to push result on.
@@ -752,6 +772,63 @@ netbox_decode_execute(struct lua_State *L)
 	return 2;
 }
 
+static int
+netbox_decode_prepare(struct lua_State *L)
+{
+	uint32_t ctypeid;
+	const char *data = *(const char **)luaL_checkcdata(L, 1, &ctypeid);
+	assert(mp_typeof(*data) == MP_MAP);
+	uint32_t map_size = mp_decode_map(&data);
+	int query_id_idx = 0, meta_idx = 0, bind_meta_idx = 0,
+	    bind_count_idx = 0;
+	const char *sql_str = 0;
+	for (uint32_t i = 0; i < map_size; ++i) {
+		uint32_t key = mp_decode_uint(&data);
+		switch(key) {
+		case IPROTO_SQL_TEXT: {
+			uint32_t len = 0;
+			sql_str = mp_decode_str(&data, &len);
+			lua_pushlstring(L, sql_str, len);
+			query_id_idx = i - map_size;
+			break;
+		}
+		case IPROTO_METADATA: {
+			netbox_decode_metadata(L, &data);
+			meta_idx = i - map_size;
+			break;
+		}
+		case IPROTO_BIND_METADATA: {
+			netbox_decode_metadata(L, &data);
+			bind_meta_idx = i - map_size;
+			break;
+		}
+		default: {
+			assert(key == IPROTO_BIND_COUNT);
+			uint32_t bind_count = mp_decode_uint(&data);
+			luaL_pushuint64(L, bind_count);
+			bind_count_idx = i - map_size;
+			break;
+		}}
+	}
+	/* These fields must be present in response. */
+	assert(query_id_idx * bind_meta_idx * bind_count_idx != 0);
+	/* General meta is presented only in DQL responses. */
+	lua_createtable(L, 0, meta_idx != 0 ? 4 : 3);
+	lua_pushvalue(L, query_id_idx - 1);
+	lua_setfield(L, -2, "sql_str");
+	lua_pushvalue(L, bind_count_idx - 1);
+	lua_setfield(L, -2, "param_count");
+	lua_pushvalue(L, bind_meta_idx - 1);
+	lua_setfield(L, -2, "params");
+	if (meta_idx != 0) {
+		lua_pushvalue(L, meta_idx - 1);
+		lua_setfield(L, -2, "metadata");
+	}
+
+	*(const char **)luaL_pushcdata(L, ctypeid) = data;
+	return 2;
+}
+
 int
 luaopen_net_box(struct lua_State *L)
 {
@@ -767,11 +844,13 @@ luaopen_net_box(struct lua_State *L)
 		{ "encode_update",  netbox_encode_update },
 		{ "encode_upsert",  netbox_encode_upsert },
 		{ "encode_execute", netbox_encode_execute},
+		{ "encode_prepare", netbox_encode_prepare},
 		{ "encode_auth",    netbox_encode_auth },
 		{ "decode_greeting",netbox_decode_greeting },
 		{ "communicate",    netbox_communicate },
 		{ "decode_select",  netbox_decode_select },
 		{ "decode_execute", netbox_decode_execute },
+		{ "decode_prepare", netbox_decode_prepare },
 		{ NULL, NULL}
 	};
 	/* luaL_register_module polutes _G */
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index c2e1bb9c4..b1301cd96 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -104,6 +104,7 @@ local method_encoder = {
     upsert  = internal.encode_upsert,
     select  = internal.encode_select,
     execute = internal.encode_execute,
+    prepare = internal.encode_prepare,
     get     = internal.encode_select,
     min     = internal.encode_select,
     max     = internal.encode_select,
@@ -128,6 +129,7 @@ local method_decoder = {
     upsert  = decode_nil,
     select  = internal.decode_select,
     execute = internal.decode_execute,
+    prepare = internal.decode_prepare,
     get     = decode_get,
     min     = decode_get,
     max     = decode_get,
@@ -1197,6 +1199,17 @@ function remote_methods:execute(query, parameters, sql_opts, netbox_opts)
                          sql_opts or {})
 end
 
+function remote_methods:prepare(query, parameters, sql_opts, netbox_opts)
+    check_remote_arg(self, "prepare")
+    if type(query) ~= "string" then
+        box.error(box.error.SQL_PREPARE, "expected string as SQL statement")
+    end
+    if sql_opts ~= nil then
+        box.error(box.error.UNSUPPORTED, "prepare", "options")
+    end
+    return self:_request('prepare', netbox_opts, nil, query)
+end
+
 function remote_methods:wait_state(state, timeout)
     check_remote_arg(self, 'wait_state')
     if timeout == nil then
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 18bf08971..c27632617 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -591,7 +591,7 @@ error:
 		else
 			request->sql_text = value;
 	}
-	if (request->sql_text == NULL) {
+	if (request->sql_text == NULL ) {
 		xrow_on_decode_err(row->body[0].iov_base, end, ER_MISSING_REQUEST_FIELD,
 			 iproto_key_name(IPROTO_SQL_TEXT));
 		return -1;
diff --git a/src/box/xrow.h b/src/box/xrow.h
index 60def2d3c..bcb96f079 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -526,7 +526,7 @@ int
 iproto_reply_error(struct obuf *out, const struct error *e, uint64_t sync,
 		   uint32_t schema_version);
 
-/** EXECUTE request. */
+/** EXECUTE/PREPARE request. */
 struct sql_request {
 	/** SQL statement text. */
 	const char *sql_text;
diff --git a/test/box/misc.result b/test/box/misc.result
index ce66e1bfe..8bb93b16c 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -230,6 +230,7 @@ t;
   - EVAL
   - CALL
   - ERROR
+  - PREPARE
   - REPLACE
   - UPSERT
   - AUTH
diff --git a/test/sql/engine.cfg b/test/sql/engine.cfg
index a1b4b0fc5..e38bec24e 100644
--- a/test/sql/engine.cfg
+++ b/test/sql/engine.cfg
@@ -10,6 +10,7 @@
         "local": {"remote": "false"}
     },
     "prepared.test.lua": {
+        "remote": {"remote": "true"},
         "local": {"remote": "false"}
     },
     "*": {
diff --git a/test/sql/prepared.result b/test/sql/prepared.result
index 3321f41f3..4a92f4d97 100644
--- a/test/sql/prepared.result
+++ b/test/sql/prepared.result
@@ -12,25 +12,41 @@ fiber = require('fiber')
 -- Wrappers to make remote and local execution interface return
 -- same result pattern.
 --
+is_remote = test_run:get_cfg('remote') == 'true'
+ | ---
+ | ...
+execute = nil
+ | ---
+ | ...
+prepare = nil
+ | ---
+ | ...
+
 test_run:cmd("setopt delimiter ';'")
  | ---
  | - true
  | ...
-execute = function(...)
-    local res, err = box.execute(...)
-    if err ~= nil then
-        error(err)
+if is_remote then
+    box.schema.user.grant('guest','read, write, execute', 'universe')
+    box.schema.user.grant('guest', 'create', 'space')
+    cn = remote.connect(box.cfg.listen)
+    execute = function(...) return cn:execute(...) end
+    prepare = function(...) return cn:prepare(...) end
+else
+    execute = function(...)
+        local res, err = box.execute(...)
+        if err ~= nil then
+            error(err)
+        end
+        return res
     end
-    return res
-end;
- | ---
- | ...
-prepare = function(...)
-    local res, err = box.prepare(...)
-    if err ~= nil then
-        error(err)
+    prepare = function(...)
+        local res, err = box.prepare(...)
+        if err ~= nil then
+            error(err)
+        end
+        return res
     end
-    return res
 end;
  | ---
  | ...
@@ -119,28 +135,22 @@ execute(s.sql_str, {1, 3})
  |     type: string
  |   rows: []
  | ...
-s:execute({1, 2})
+
+test_run:cmd("setopt delimiter ';'")
  | ---
- | - metadata:
- |   - name: ID
- |     type: integer
- |   - name: A
- |     type: number
- |   - name: B
- |     type: string
- |   rows:
- |   - [1, 2, '3']
+ | - true
  | ...
-s:execute({1, 3})
+if not is_remote then
+    res = s:execute({1, 2})
+    assert(res ~= nil)
+    res = s:execute({1, 3})
+    assert(res ~= nil)
+end;
  | ---
- | - metadata:
- |   - name: ID
- |     type: integer
- |   - name: A
- |     type: number
- |   - name: B
- |     type: string
- |   rows: []
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
  | ...
 
 -- Test preparation of different types of queries.
@@ -287,6 +297,7 @@ space:replace{7, 8.5, '9'}
 s = prepare("SELECT a FROM test WHERE b = '3';")
  | ---
  | ...
+
 execute(s.sql_str)
  | ---
  | - metadata:
@@ -303,21 +314,21 @@ execute(s.sql_str)
  |   rows:
  |   - [2]
  | ...
-s:execute()
+test_run:cmd("setopt delimiter ';'")
  | ---
- | - metadata:
- |   - name: A
- |     type: number
- |   rows:
- |   - [2]
+ | - true
  | ...
-s:execute()
+if not is_remote then
+    res = s:execute()
+    assert(res ~= nil)
+    res = s:execute()
+    assert(res ~= nil)
+end;
  | ---
- | - metadata:
- |   - name: A
- |     type: number
- |   rows:
- |   - [2]
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
  | ...
 
 s = prepare("SELECT count(*), count(a - 3), max(b), abs(id) FROM test WHERE b = '3';")
@@ -561,6 +572,13 @@ f2:join();
  | - true
  | ...
 
+if is_remote then
+    cn:close()
+    box.schema.user.revoke('guest', 'read, write, execute', 'universe')
+    box.schema.user.revoke('guest', 'create', 'space')
+end;
+ | ---
+ | ...
 test_run:cmd("setopt delimiter ''");
  | ---
  | - true
diff --git a/test/sql/prepared.test.lua b/test/sql/prepared.test.lua
index 24e49832f..de667d6d5 100644
--- a/test/sql/prepared.test.lua
+++ b/test/sql/prepared.test.lua
@@ -5,20 +5,32 @@ fiber = require('fiber')
 -- Wrappers to make remote and local execution interface return
 -- same result pattern.
 --
+is_remote = test_run:get_cfg('remote') == 'true'
+execute = nil
+prepare = nil
+
 test_run:cmd("setopt delimiter ';'")
-execute = function(...)
-    local res, err = box.execute(...)
-    if err ~= nil then
-        error(err)
+if is_remote then
+    box.schema.user.grant('guest','read, write, execute', 'universe')
+    box.schema.user.grant('guest', 'create', 'space')
+    cn = remote.connect(box.cfg.listen)
+    execute = function(...) return cn:execute(...) end
+    prepare = function(...) return cn:prepare(...) end
+else
+    execute = function(...)
+        local res, err = box.execute(...)
+        if err ~= nil then
+            error(err)
+        end
+        return res
     end
-    return res
-end;
-prepare = function(...)
-    local res, err = box.prepare(...)
-    if err ~= nil then
-        error(err)
+    prepare = function(...)
+        local res, err = box.prepare(...)
+        if err ~= nil then
+            error(err)
+        end
+        return res
     end
-    return res
 end;
 
 test_run:cmd("setopt delimiter ''");
@@ -39,8 +51,15 @@ s.params
 s.param_count
 execute(s.sql_str, {1, 2})
 execute(s.sql_str, {1, 3})
-s:execute({1, 2})
-s:execute({1, 3})
+
+test_run:cmd("setopt delimiter ';'")
+if not is_remote then
+    res = s:execute({1, 2})
+    assert(res ~= nil)
+    res = s:execute({1, 3})
+    assert(res ~= nil)
+end;
+test_run:cmd("setopt delimiter ''");
 
 -- Test preparation of different types of queries.
 -- Let's start from DDL. It doesn't make much sense since
@@ -93,10 +112,17 @@ space:replace{1, 2, '3'}
 space:replace{4, 5, '6'}
 space:replace{7, 8.5, '9'}
 s = prepare("SELECT a FROM test WHERE b = '3';")
+
 execute(s.sql_str)
 execute(s.sql_str)
-s:execute()
-s:execute()
+test_run:cmd("setopt delimiter ';'")
+if not is_remote then
+    res = s:execute()
+    assert(res ~= nil)
+    res = s:execute()
+    assert(res ~= nil)
+end;
+test_run:cmd("setopt delimiter ''");
 
 s = prepare("SELECT count(*), count(a - 3), max(b), abs(id) FROM test WHERE b = '3';")
 execute(s.sql_str)
@@ -190,6 +216,11 @@ f2:set_joinable(true)
 f1:join();
 f2:join();
 
+if is_remote then
+    cn:close()
+    box.schema.user.revoke('guest', 'read, write, execute', 'universe')
+    box.schema.user.revoke('guest', 'create', 'space')
+end;
 test_run:cmd("setopt delimiter ''");
 
 box.space.TEST:drop()
-- 
2.15.1

Re: [Tarantool-patches] [PATCH v2 16/16] netbox: introduce prepared statements

[Vladislav Shpilevoy]

Thanks for the patch!

See 5 comments below.

On 20/11/2019 22:28, Nikita Pettik wrote:
> This patch introduces support of prepared statements in IProto
> protocol. To achieve this new IProto command is added - IPROTO_PREPARE.
> It is sent with IPROTO_SQL_TEXT key and it means to prepare SQL statement
> (for details see previous commit). Also to reply on PREPARE request a few
> response keys are added: IPROTO_BIND_METADATA (contains parameters
> metadata) and IPROTO_BIND_COUNT (count of parameters to be bound).

1. Please, remind me, why do we need these two new keys?

> 
> Closes #2592
> 
> @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 string
> containing original SQL request. Prepared statement cache is global and
> follows LRU eviction policy: when there's no place for another one
> statement, the statement that is least recently used (prepared) is
> removed. Cache size is adjusted by box.cfg{sql_cache_size} variable
> (can be set dynamically; in case size is reduced some statements may be
> erased from cache). Note that any DDL operation leads to expiration of all
> prepared statements: they are recompiled on demand. To erase whole cache
> one can set its size to 0. Prepared statements are available in local mode
> (i.e. via box.prepare() function) and are supported in IProto protocol.
> 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})
> 
> In terms of remote connection:
> 
> cn = netbox:connect(addr)
> s = cn:prepare("SELECT * FROM t WHERE id = ?;")
> cn:execute(s.sql_str, {1})

2. Each time when binary protocol is changed the doc
request should describe these changes in detail: where
the new keys are added, what are their names, what are
their values and types, are they optional, what to
assume when they are omitted? This is needed so as our
users could update their connectors.

3. That would be cool to give user an advice here,
that the cache eviction is not signaled anyhow. So
if they want the prepared statements really stay
prepared, then should recall prepare(), when a sequence
of execute() calls is expected soon.

4. Please, document new 'prepared statement' object -
what attributes does it have, with what types.

> diff --git a/src/box/execute.c b/src/box/execute.c
> index 2b5a9ba90..13e5a6ba1 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -326,6 +326,67 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
> +static int
> +sql_get_prepare_common_keys(struct sql_stmt *stmt, struct obuf *out, int keys,
> +			    const char *sql_str)
> +{
> +	int size = mp_sizeof_map(keys) +
> +		   mp_sizeof_uint(IPROTO_SQL_TEXT) +
> +		   mp_sizeof_str(strlen(sql_str)) +
> +		   mp_sizeof_uint(IPROTO_BIND_COUNT) +
> +		   mp_sizeof_uint(sql_bind_parameter_count(stmt));
> +	char *pos = (char *) obuf_alloc(out, size);
> +	if (pos == NULL) {
> +		diag_set(OutOfMemory, size, "obuf_alloc", "pos");
> +		return -1;
> +	}
> +	pos = mp_encode_map(pos, keys);
> +	pos = mp_encode_uint(pos, IPROTO_SQL_TEXT);
> +	pos = mp_encode_str(pos, sql_str, strlen(sql_str));
> +	pos = mp_encode_uint(pos, IPROTO_BIND_COUNT);
> +	pos = mp_encode_uint(pos, sql_bind_parameter_count(stmt));
> +	if (sql_get_params_metadata(stmt, out) != 0)
> +		return -1;
> +	return 0;

5. Just make 'return sql_get_params_metadata(...)'.

6. Have you done any benchmarks? Just out of curiosity.
How much a prepared execution is faster then an execute
from the scratch? Just a microbenchmark with lua-land
time measurement would be fine.

Re: [Tarantool-patches] [PATCH v2 00/16] sql: prepared statements

[Vladislav Shpilevoy]

Hi! Thanks for the patchset!

This is really cool.

Re: [Tarantool-patches] [PATCH v2 00/16] sql: prepared statements

[Georgy Kirichenko]

[-- Attachment #1: Type: text/plain, Size: 5450 bytes --]

I did a quick overview of the patchset and there are some important questions.

1. The statement cache is a great feature but it is completely orthogonal with 
prepared statement feature. If we consider non-prepared statement going
through the cache (I do not see any reason to not to do statements caching) 
then there is no differences  between them and prepared statements except
caching aggressiveness and parameter substitution.

2. There is an interference between statements prepared in different session. A 
session which prepares huge loads of one-time-used prepared statements evicts
prepared statements from all other session even if this statements are reused 
a lot. And we have not got any clear explanation how you will prevent this 
behavior.
As a database user I have strong opinion that if I have prepared a statement 
then this statement should be alive for the statement lifetime. And I 
definitely do not want any unpredictability from a database.

3. If you will insist on statement cache then I will ask you for any 
justification regarding the chosen caching algorithm.


On Thursday, November 21, 2019 12:27:59 AM MSK Nikita Pettik wrote:
> Branch:
> https://github.com/tarantool/tarantool/tree/np/gh-2592-prepared-statemets-v
> 2 Issue: https://github.com/tarantool/tarantool/issues/2592
> 
> Patch v1:
> https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/01227
> 4.html
> 
> Changes in this iteration:
> 
> - Now cache is global and statements are shared among all session
>   (main reason for this change is to increase performance);
> - Cache follows next eviction policy: when there's no space for
>   another one statement, statement which was prepared earlier than others
>   is removed from cache (strickly speaking it's not LRU policy, but rather
>   FIFO; however, the same approach is already used for tuple cache and it
>   is called LRU anyway);
> - Owing to the previous point, there's no need in 'unprepare' invocation
>   at all (it concerns both manual request and on_disconnect clean-up);
> - Also due to using replacement policy eviction from cache should
>   not be "visible" to user, i.e. statement should still be capable of being
>   executed. Keeping only numeric ids doesn't allow this, so now we use
>   string ids (which consist of original SQL request);
> - Statement is automatically re-prepared (on demand) if it is expired
>   as a result of DDL request;
> - In case statement is busy (is executed right now by other session), it
>   is compiled from scratch and then executed.
> 
> Nikita Pettik (16):
>   sql: remove sql_prepare_v2()
>   sql: refactor sql_prepare() and sqlPrepare()
>   sql: move sql_prepare() declaration to box/execute.h
>   sql: rename sqlPrepare() to sql_compile()
>   sql: move sql_finalize() to execute.h
>   port: increase padding of struct port
>   port: add dump format and request type to port_sql
>   sql: resurrect sql_bind_parameter_count() function
>   sql: resurrect sql_bind_parameter_name()
>   sql: add sql_stmt_schema_version()
>   sql: introduce sql_stmt_sizeof() function
>   box: increment schema_version on ddl operations
>   sql: introduce sql_stmt_query_str() method
>   sql: introduce cache for prepared statemets
>   box: introduce prepared statements
>   netbox: introduce prepared statements
> 
>  src/box/CMakeLists.txt          |   1 +
>  src/box/alter.cc                |   3 +
>  src/box/box.cc                  |  20 ++
>  src/box/box.h                   |   1 +
>  src/box/ck_constraint.c         |   1 +
>  src/box/errcode.h               |   1 +
>  src/box/execute.c               | 213 ++++++++++++++-
>  src/box/execute.h               |  51 ++++
>  src/box/iproto.cc               |  25 +-
>  src/box/iproto_constants.c      |   6 +-
>  src/box/iproto_constants.h      |   4 +
>  src/box/lua/cfg.cc              |  12 +
>  src/box/lua/execute.c           | 161 ++++++++++-
>  src/box/lua/execute.h           |   2 +-
>  src/box/lua/init.c              |   2 +-
>  src/box/lua/load_cfg.lua        |   3 +
>  src/box/lua/net_box.c           |  79 ++++++
>  src/box/lua/net_box.lua         |  13 +
>  src/box/prep_stmt.c             | 186 +++++++++++++
>  src/box/prep_stmt.h             | 112 ++++++++
>  src/box/session.cc              |   1 +
>  src/box/sql.c                   |   3 +
>  src/box/sql/analyze.c           |  16 +-
>  src/box/sql/legacy.c            |   3 +-
>  src/box/sql/prepare.c           |  56 +---
>  src/box/sql/sqlInt.h            |  44 +--
>  src/box/sql/vdbe.h              |   2 +-
>  src/box/sql/vdbeInt.h           |   1 -
>  src/box/sql/vdbeapi.c           |  95 +++++--
>  src/box/sql/vdbeaux.c           |   5 +-
>  src/box/xrow.c                  |   2 +-
>  src/box/xrow.h                  |   2 +-
>  src/lib/core/port.h             |   2 +-
>  test/app-tap/init_script.result |  37 +--
>  test/box/admin.result           |   2 +
>  test/box/cfg.result             |   7 +
>  test/box/cfg.test.lua           |   1 +
>  test/box/misc.result            |   3 +
>  test/sql/engine.cfg             |   4 +
>  test/sql/prepared.result        | 592
> ++++++++++++++++++++++++++++++++++++++++ test/sql/prepared.test.lua      |
> 227 +++++++++++++++
>  41 files changed, 1851 insertions(+), 150 deletions(-)
>  create mode 100644 src/box/prep_stmt.c
>  create mode 100644 src/box/prep_stmt.h
>  create mode 100644 test/sql/prepared.result
>  create mode 100644 test/sql/prepared.test.lua


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]