[tarantool-patches] [PATCH 0/8] rfc: introduce dry-run execution of SQL queries

[tarantool-patches] [PATCH 0/8] rfc: introduce dry-run execution of SQL queries

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/tree/np/sql-dry-run-rfc
Issue: https://github.com/tarantool/tarantool/issues/3292

In a nutshell dry-run is a way to get auxiliary (meta-) information of query
execution without direct execution itself. For instance, every DQL query
returns column's name and type alongside with tuples forming the resulting set.
Meta-information can be extended with column's nullability, collation and other
properties. This feature is required to develop SQL drivers. 

A bit of details.

There are three forms of dumped msgpack structure (and corresponding IPROTO
response format). First one relates to DQL (i.e. SELECT) queries:
METADATA : [{name, type}, {name, type}, ...], DATA : [tuple, tuple ...]

In case of dry-run, only metadata should be dumped. As a consequence, DATA
section becomes optional and depends on port settings (see new 'meta_only'
option in struct port_sql). To pass flag indicating dry-run execution,
:execute() method has been allowed to accept 'options' argument. Options can
form a map of named parameters ({'dry_run' = true}) or simply a list of
unnamed ordered values. Options are encoded with IPROTO_OPTIONS key. The only
available option now is 'dry_run'.

To execute query in dry-run mode locally separate method of box has been added:
box.dry_run("sql_string"). Current implementation does not modify box.execute()
making it accept third optional argument (except for array of bindings) - this
is to be discussed. 

Other open (or dubious) questions.

1. How to handle DML queries. Meanwhile getting meta-information of DDL queries
is likely to be useless, calculating number of affected rows in case of DML
query can be sensible. On the other hand, it can turn out to be quite
non-trivial task.  In its current state, box.dry_run("DML_Query") always
returns 0 as row count. IMHO it can be done but as a follow-up

2. Should dry_run() except for parsing also bind variables? I guess no, since
it results in byte-code (i.e. prepared statement) modification. On the other
hand, it allows setting correct type for bindings (see sql_bind_type()). By
default, all binding parameters have boolean type.

3. Interface for local call. In current implementation I've added box.dry_run()
in addition to traditional box.execute() method. Instead, we can make
box.execute() accept third argument as it is handled in net.box.

Nikita Pettik (8):
  port: increase padding of struct port
  port: move struct port_sql to box/port.h
  sql: remove sql_prepare_v2()
  sql: refactor sql_prepare() and sqlPrepare()
  sql: move sql_prepare() declaration to box/execute.h
  refactoring: use sql_prepare() and sql_execute() in tx_process_sql()
  netbox: allow passing options to :execute()
  sql: introduce dry-run execution

 src/box/errcode.h        |   1 +
 src/box/execute.c        | 104 ++++++++++++++++++++++++++++++++++++-----------
 src/box/execute.h        |  61 ++++++++++++++++++++-------
 src/box/iproto.cc        |  22 +++++++++-
 src/box/lua/execute.c    |  37 ++++++++++++++++-
 src/box/lua/net_box.c    |   7 ++--
 src/box/lua/net_box.lua  |   3 --
 src/box/port.h           |  26 ++++++++++++
 src/box/sql/analyze.c    |  16 ++++----
 src/box/sql/legacy.c     |   3 +-
 src/box/sql/prepare.c    |  38 +++--------------
 src/box/sql/sqlInt.h     |  16 --------
 src/box/sql/vdbe.h       |   2 +-
 src/box/sql/vdbeInt.h    |   1 -
 src/box/sql/vdbeapi.c    |   9 ++--
 src/box/sql/vdbeaux.c    |   5 +--
 src/box/xrow.c           |  19 +++++++--
 src/box/xrow.h           |   5 +++
 src/lib/core/port.h      |   3 +-
 test/box/misc.result     |   2 +
 test/sql/iproto.result   |  93 +++++++++++++++++++++++++++++++++++++++++-
 test/sql/iproto.test.lua |  19 +++++++++
 22 files changed, 371 insertions(+), 121 deletions(-)

-- 
2.15.1

[tarantool-patches] [PATCH 1/8] 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 09a026df5..abe21aa53 100644
--- a/src/lib/core/port.h
+++ b/src/lib/core/port.h
@@ -113,7 +113,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

[tarantool-patches] Re: [PATCH 1/8] port: increase padding of struct port

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/08/27 16:34]:
> 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.

lgtm.


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 1/8] port: increase padding of struct port

[Vladislav Shpilevoy]

Thanks for the patch!

Currently sizeof(struct port) is 64, sizeof(struct port_sql) is 56.
It means, that one new flag port_sql fits easily. I don't think we
need that patch now.

On 27/08/2019 15:34, Nikita Pettik wrote:
> 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 09a026df5..abe21aa53 100644
> --- a/src/lib/core/port.h
> +++ b/src/lib/core/port.h
> @@ -113,7 +113,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. */
> 

[tarantool-patches] [PATCH 2/8] port: move struct port_sql to box/port.h

[Nikita Pettik]

We are going to use port_sql_create() in box/lua/execute.c so to be able
to call it let's make port_sql_create() non-static and move its
declaration to box/port.h alongside with struct port_sql (since
box/port.h already contains struct port_tuple and struct port_lua).

No functional changes are provided.

Needed for #3292
---
 src/box/execute.c   |  2 +-
 src/box/execute.h   | 20 --------------------
 src/box/port.h      | 24 ++++++++++++++++++++++++
 src/lib/core/port.h |  1 +
 4 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index 68e94e442..a6454d5bb 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -112,7 +112,7 @@ const struct port_vtab port_sql_vtab = {
 	/* .destroy = */ port_sql_destroy,
 };
 
-static void
+void
 port_sql_create(struct port *port, struct sql_stmt *stmt)
 {
 	port_tuple_create(port);
diff --git a/src/box/execute.h b/src/box/execute.h
index a2fd4d1b7..1995c43d5 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -69,26 +69,6 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
 			uint32_t bind_count, struct port *port,
 			struct region *region);
 
-/**
- * Port implementation that is used to store SQL responses and
- * output them to obuf or Lua. This port implementation is
- * inherited from the port_tuple structure. This allows us to use
- * this structure in the port_tuple methods instead of port_tuple
- * itself.
- *
- * The methods of port_tuple are called via explicit access to
- * port_tuple_vtab just like C++ does with BaseClass::method, when
- * it is called in a child method.
- */
-struct port_sql {
-	/* port_tuple to inherit from. */
-	struct port_tuple port_tuple;
-	/* Prepared SQL statement. */
-	struct sql_stmt *stmt;
-};
-
-extern const struct port_vtab port_sql_vtab;
-
 #if defined(__cplusplus)
 } /* extern "C" { */
 #endif
diff --git a/src/box/port.h b/src/box/port.h
index a7f5d81bd..0d2cc7e84 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -119,6 +119,30 @@ port_init(void);
 void
 port_free(void);
 
+/**
+ * Port implementation that is used to store SQL responses and
+ * output them to obuf or Lua. This port implementation is
+ * inherited from the port_tuple structure. This allows us to use
+ * this structure in the port_tuple methods instead of port_tuple
+ * itself.
+ *
+ * The methods of port_tuple are called via explicit access to
+ * port_tuple_vtab just like C++ does with BaseClass::method, when
+ * it is called in a child method.
+ */
+struct port_sql {
+	/* port_tuple to inherit from. */
+	struct port_tuple port_tuple;
+	/* Prepared SQL statement. */
+	struct sql_stmt *stmt;
+};
+
+extern const struct port_vtab port_sql_vtab;
+
+/** Create instance of SQL port using given attributes. */
+void
+port_sql_create(struct port *port, struct sql_stmt *stmt);
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined __cplusplus */
diff --git a/src/lib/core/port.h b/src/lib/core/port.h
index abe21aa53..2b1ccaae3 100644
--- a/src/lib/core/port.h
+++ b/src/lib/core/port.h
@@ -39,6 +39,7 @@ extern "C" {
 
 struct obuf;
 struct lua_State;
+struct sql_stmt;
 struct port;
 
 /**
-- 
2.15.1

[tarantool-patches] Re: [PATCH 2/8] port: move struct port_sql to box/port.h

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/08/27 16:34]:
> We are going to use port_sql_create() in box/lua/execute.c so to be able
> to call it let's make port_sql_create() non-static and move its
> declaration to box/port.h alongside with struct port_sql (since
> box/port.h already contains struct port_tuple and struct port_lua).
> 
> No functional changes are provided.

lgtm


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 2/8] port: move struct port_sql to box/port.h

[Vladislav Shpilevoy]

Thanks for the patch!

On 27/08/2019 15:34, Nikita Pettik wrote:
> We are going to use port_sql_create() in box/lua/execute.c so to be able
> to call it let's make port_sql_create() non-static and move its
> declaration to box/port.h alongside with struct port_sql (since
> box/port.h already contains struct port_tuple and struct port_lua).
> 
> No functional changes are provided.

As I see, you did it to use port_sql like an in-out parameter for sql_execute.
Please, don't do this. Port is a result container. sql_execute should create
it from the scratch. If you want to pass options to sql_execute, then pass
sql_opts introduced in the last patch. port_sql_create should remain static
function of execute.c.

[tarantool-patches] [PATCH 3/8] 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 a6454d5bb..b8d161815 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 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 e077a8b5e..ba3b7d71f 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 f01fe0e30..2ad96b93a 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -447,21 +447,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 dfaff9365..0b54a9429 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -455,7 +455,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

[tarantool-patches] Re: [PATCH 3/8] sql: remove sql_prepare_v2()

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/08/27 16:34]:
> 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.

lgtm


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 3/8] sql: remove sql_prepare_v2()

[Vladislav Shpilevoy]

Thanks for the patch!

LGTM.

On 27/08/2019 15:34, Nikita Pettik wrote:
> 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

Seems like simple refactoring. And can be pushed out of order.

[tarantool-patches] [PATCH 4/8] 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 |  7 +++----
 src/box/sql/vdbeaux.c |  5 +----
 9 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index b8d161815..078753342 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -441,8 +441,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 bd52d12df..fa3d364e9 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1344,7 +1344,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;
@@ -1360,7 +1360,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;
@@ -1428,7 +1428,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;
@@ -1506,12 +1506,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) {
@@ -1697,7 +1696,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
@@ -1740,7 +1739,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 ba3b7d71f..f72f9aa23 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 2ad96b93a..d2fc76919 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -450,14 +450,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 06f258805..b5b13a269 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -246,7 +246,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 3a416aea5..0b6109234 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -414,7 +414,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 0b54a9429..a25f063cc 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -112,7 +112,7 @@ sql_clear_bindings(sql_stmt * pStmt)
 		sqlVdbeMemRelease(&p->aVar[i]);
 		p->aVar[i].flags = MEM_Null;
 	}
-	if (p->isPrepareV2 && p->expmask) {
+	if (p->expmask) {
 		p->expired = 1;
 	}
 	return rc;
@@ -454,7 +454,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.
 		 */
@@ -845,8 +845,7 @@ vdbeUnbind(Vdbe * p, int i)
 	 * as if there had been a schema change, on the first sql_step() call
 	 * following any change to the bindings of that parameter.
 	 */
-	if (p->isPrepareV2 &&
-	    ((i < 32 && p->expmask & ((u32) 1 << i))
+	if (((i < 32 && p->expmask & ((u32) 1 << i))
 	     || p->expmask == 0xffffffff)
 	    ) {
 		p->expired = 1;
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 61dc74dc1..e09c88fd1 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

[tarantool-patches] Re: [PATCH 4/8] sql: refactor sql_prepare() and sqlPrepare()

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/08/27 16:34]:
> -	if (p->isPrepareV2 &&
> -	    ((i < 32 && p->expmask & ((u32) 1 << i))
> +	if (((i < 32 && p->expmask & ((u32) 1 << i))

extra braces


lgtm

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 4/8] sql: refactor sql_prepare() and sqlPrepare()

[Vladislav Shpilevoy]

Thanks for the patch!

On 27/08/2019 15:34, Nikita Pettik wrote:
> - 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

Same as the previous patch - I think, it can be pushed out of
order.

See 2 comments below.

> ---
>  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 |  7 +++----
>  src/box/sql/vdbeaux.c |  5 +----
>  9 files changed, 19 insertions(+), 29 deletions(-)
> 
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index bd52d12df..fa3d364e9 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -1344,7 +1344,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)

1. Please, drop the comment about 'db'.

>  {
>  	struct index **indexes = NULL;
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index 0b54a9429..a25f063cc 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -845,8 +845,7 @@ vdbeUnbind(Vdbe * p, int i)
>  	 * as if there had been a schema change, on the first sql_step() call
>  	 * following any change to the bindings of that parameter.
>  	 */
> -	if (p->isPrepareV2 &&
> -	    ((i < 32 && p->expmask & ((u32) 1 << i))
> +	if (((i < 32 && p->expmask & ((u32) 1 << i))

2. Kostja is right, braces.

[tarantool-patches] [PATCH 5/8] 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 1995c43d5..1b4e14246 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -69,6 +69,18 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
 			uint32_t bind_count, struct port *port,
 			struct region *region);
 
+/**
+ * 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 fa3d364e9..7e3ad270e 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 d2fc76919..7dc0fb8b4 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -447,18 +447,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

[tarantool-patches] Re: [PATCH 5/8] sql: move sql_prepare() declaration to box/execute.h

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/08/27 16:34]:

lgtm

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] [PATCH 6/8] refactoring: use sql_prepare() and sql_execute() in tx_process_sql()

[Nikita Pettik]

In case of dry-run execution we don't need to substitute binding values
or execute statement. So now we split sql_prepare_and_execute() into
independent steps, so that we can omit some of them depending on
execution mode.

Needed for #3292
---
 src/box/execute.c | 17 +----------------
 src/box/execute.h | 17 +++++++++++++++++
 src/box/iproto.cc | 15 +++++++++++++--
 3 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index 078753342..a8a2e516b 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -398,22 +398,7 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
 	return 0;
 }
 
-/**
- * Execute prepared SQL statement.
- *
- * This function uses region to allocate memory for temporary
- * objects. After this function, region will be in the same state
- * in which it was before this function.
- *
- * @param db SQL handle.
- * @param stmt Prepared statement.
- * @param port Port to store SQL response.
- * @param region Region to allocate temporary objects.
- *
- * @retval  0 Success.
- * @retval -1 Error.
- */
-static inline int
+int
 sql_execute(struct sql_stmt *stmt, struct port *port, struct region *region)
 {
 	int rc, column_count = sql_column_count(stmt);
diff --git a/src/box/execute.h b/src/box/execute.h
index 1b4e14246..23366d65c 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -81,6 +81,23 @@ int
 sql_prepare(const char *sql, int length, struct sql_stmt **stmt,
 	    const char **sql_tail);
 
+/**
+ * Execute prepared SQL statement.
+ *
+ * This function uses region to allocate memory for temporary
+ * objects. After this function, region will be in the same state
+ * in which it was before this function.
+ *
+ * @param stmt Prepared statement.
+ * @param port Port to store SQL response.
+ * @param region Region to allocate temporary objects.
+ *
+ * @retval  0 Success.
+ * @retval -1 Error.
+ */
+int
+sql_execute(struct sql_stmt *stmt, struct port *port, struct region *region);
+
 #if defined(__cplusplus)
 } /* extern "C" { */
 #endif
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 8f899fed8..9b59e1af0 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1661,9 +1661,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)
+	/* Compile, bind and execute SQL statement. */
+	struct sql_stmt *stmt;
+	if (sql_prepare(sql, len, &stmt, NULL) != 0)
 		goto error;
+	assert(stmt != NULL);
+	port_sql_create(&port, stmt);
+	if (sql_bind(stmt, bind, bind_count) != 0) {
+		port_destroy(&port);
+		goto error;
+	}
+	if (sql_execute(stmt, &port, &fiber()->gc) != 0) {
+		port_destroy(&port);
+		goto error;
+	}
 	/*
 	 * Take an obuf only after execute(). Else the buffer can
 	 * become out of date during yield.
-- 
2.15.1

[tarantool-patches] Re: [PATCH 6/8] refactoring: use sql_prepare() and sql_execute() in tx_process_sql()

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/08/27 16:34]:
> In case of dry-run execution we don't need to substitute binding values
> or execute statement. So now we split sql_prepare_and_execute() into
> independent steps, so that we can omit some of them depending on
> execution mode.
> 
> Needed for #3292

lgtm. 


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 6/8] refactoring: use sql_prepare() and sql_execute() in tx_process_sql()

[Vladislav Shpilevoy]

Thanks for the patch!

I think, it is better to keep using subsystem names in commit
titles. I.e. not 'refactoring:', but 'sql:'.

On 27/08/2019 15:34, Nikita Pettik wrote:
> In case of dry-run execution we don't need to substitute binding values
> or execute statement. So now we split sql_prepare_and_execute() into
> independent steps, so that we can omit some of them depending on
> execution mode.

Not sure if it is worth doing. I think, you can keep prepare_and_execute,
pass sql_opts to it, and inside this function you fill port options, omit
execution etc. Anyway you will end up with the same, but in two places -
Lua and iproto. It is better to have one function for both.

[tarantool-patches] [PATCH 7/8] netbox: allow passing options to :execute()

[Nikita Pettik]

To implement dry-run execution, let's allow 'options' be valid argument
of :execute() method. SQL options are encoded with IPROTO_OPTIONS
request key. Options are supposed to be a map of named parameters or a
list of unnamed (considering their order). The only currently available
option is 'dry_run' with boolean values. Now it doesn't affect anything,
but will allow to get meta-information of query execution.

Part of #3292
---
 src/box/errcode.h        |  1 +
 src/box/execute.c        | 62 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/box/execute.h        | 22 +++++++++++++++++
 src/box/iproto.cc        |  5 ++++
 src/box/lua/net_box.lua  |  3 ---
 src/box/xrow.c           | 19 ++++++++++++---
 src/box/xrow.h           |  5 ++++
 test/box/misc.result     |  1 +
 test/sql/iproto.result   | 18 +++++++++++++-
 test/sql/iproto.test.lua |  4 ++++
 10 files changed, 133 insertions(+), 7 deletions(-)

diff --git a/src/box/errcode.h b/src/box/errcode.h
index 46b0b365a..3aedd3995 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -253,6 +253,7 @@ struct errcode_record {
 	/*198 */_(ER_FUNC_INDEX_FUNC,		"Failed to build a key for functional index '%s' of space '%s': %s") \
 	/*199 */_(ER_FUNC_INDEX_FORMAT,		"Key format doesn't match one defined in functional index '%s' of space '%s': %s") \
 	/*200 */_(ER_FUNC_INDEX_PARTS,		"Wrong functional index definition: %s") \
+	/*201 */_(ER_WRONG_SQL_EXECUTE_OPTIONS,	"Wrong SQL options passed to execute: %.*s") \
 
 /*
  * !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/execute.c b/src/box/execute.c
index a8a2e516b..2e3aeef2c 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -51,6 +51,15 @@ const char *sql_info_key_strs[] = {
 	"autoincrement_ids",
 };
 
+const struct sql_opts sql_opts_default = {
+	/* .dry_run = */ false,
+};
+
+const struct opt_def sql_opts_reg[] = {
+	OPT_DEF("dry_run", OPT_BOOL, struct sql_opts, dry_run),
+	OPT_END,
+};
+
 static_assert(sizeof(struct port_sql) <= sizeof(struct port),
 	      "sizeof(struct port_sql) must be <= sizeof(struct port)");
 
@@ -398,6 +407,59 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
 	return 0;
 }
 
+int
+sql_opts_decode(const char *data, struct sql_opts *opts)
+{
+	assert(data != NULL);
+	assert(opts != NULL);
+	if (mp_typeof(*data) != MP_ARRAY) {
+		diag_set(ClientError, ER_INVALID_MSGPACK,
+			 "SQL options are expected to be array");
+		return -1;
+	}
+	*opts = sql_opts_default;
+	uint32_t opts_count = mp_decode_array(&data);
+	if (opts_count == 0)
+		return 0;
+	if (mp_typeof(*data) == MP_MAP) {
+		/*
+		 * Options can be passed either as a map with
+		 * named arguments: ({'dry_run' = true}); or
+		 * as an array of unnamed values in the
+		 * documented order.
+		 *
+		 * FIXME: opts_decode() as a rule is used to
+		 * decode index or space opts. So to display
+		 * proper diag message number of field which is
+		 * supposed to hold options is passed as an
+		 * argument to opts_decode(). Here we don't really
+		 * have such field, so instead we can pass any
+		 * meaningful and large enough constant which
+		 * simply restricts length of error message
+		 * (see format string of error message).
+		 */
+		if (opts_decode(opts, sql_opts_reg, &data,
+				ER_WRONG_SQL_EXECUTE_OPTIONS, DIAG_ERRMSG_MAX,
+				&fiber()->gc) != 0)
+			return -1;
+		return 0;
+	}
+	if (opts_count > 1) {
+		diag_set(ClientError, ER_WRONG_SQL_EXECUTE_OPTIONS,
+			 DIAG_ERRMSG_MAX, "too many options are specified");
+		return -1;
+	}
+	for (uint32_t i = 0; i < opts_count; ++i) {
+		if (mp_typeof(*data) != MP_BOOL) {
+			diag_set(ClientError, ER_WRONG_SQL_EXECUTE_OPTIONS,
+				 DIAG_ERRMSG_MAX, "dry_run must be boolean");
+			return -1;
+		}
+		opts->dry_run = mp_decode_bool(&data);
+	}
+	return 0;
+}
+
 int
 sql_execute(struct sql_stmt *stmt, struct port *port, struct region *region)
 {
diff --git a/src/box/execute.h b/src/box/execute.h
index 23366d65c..68aff908e 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -33,6 +33,7 @@
 
 #include <stdint.h>
 #include <stdbool.h>
+#include "opt_def.h"
 #include "port.h"
 
 #if defined(__cplusplus)
@@ -48,6 +49,20 @@ enum sql_info_key {
 
 extern const char *sql_info_key_strs[];
 
+/** Options which can be passed to :execute. */
+struct sql_opts {
+	/**
+	 * In case of dry-run query is not really executed,
+	 * but only prepared (i.e. compiled into byte-code).
+	 * It allows to get query's metadata without execution
+	 * or update prepared statement cache.
+	 */
+	bool dry_run;
+};
+
+extern const struct sql_opts sql_opts_default;
+extern const struct opt_def sql_opts_reg[];
+
 struct region;
 struct sql_bind;
 
@@ -98,6 +113,13 @@ sql_prepare(const char *sql, int length, struct sql_stmt **stmt,
 int
 sql_execute(struct sql_stmt *stmt, struct port *port, struct region *region);
 
+/**
+ * Parse SQL execution options encoded in @param data and fill in
+ * corresponding fields in output @param opts.
+ */
+int
+sql_opts_decode(const char *data, struct sql_opts *opts);
+
 #if defined(__cplusplus)
 } /* extern "C" { */
 #endif
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 9b59e1af0..a92e66ace 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1647,6 +1647,7 @@ tx_process_sql(struct cmsg *m)
 	int bind_count = 0;
 	const char *sql;
 	uint32_t len;
+	struct sql_opts opts = sql_opts_default;
 
 	tx_fiber_init(msg->connection->session, msg->header.sync);
 
@@ -1659,6 +1660,10 @@ tx_process_sql(struct cmsg *m)
 		if (bind_count < 0)
 			goto error;
 	}
+	if (msg->sql.opts != NULL) {
+		if (sql_opts_decode(msg->sql.opts, &opts) != 0)
+			goto error;
+	}
 	sql = msg->sql.sql_text;
 	sql = mp_decode_str(&sql, &len);
 	/* Compile, bind and execute SQL statement. */
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 31a8c16b7..96d68af8d 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -1185,9 +1185,6 @@ end
 
 function remote_methods:execute(query, parameters, sql_opts, netbox_opts)
     check_remote_arg(self, "execute")
-    if sql_opts ~= nil then
-        box.error(box.error.UNSUPPORTED, "execute", "options")
-    end
     return self:_request('execute', netbox_opts, nil, query, parameters or {},
                          sql_opts or {})
 end
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 0ae5271c1..5d3dc8b09 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -573,9 +573,11 @@ error:
 	uint32_t map_size = mp_decode_map(&data);
 	request->sql_text = NULL;
 	request->bind = NULL;
+	request->opts = NULL;
 	for (uint32_t i = 0; i < map_size; ++i) {
 		uint8_t key = *data;
-		if (key != IPROTO_SQL_BIND && key != IPROTO_SQL_TEXT) {
+		if (key != IPROTO_SQL_BIND && key != IPROTO_SQL_TEXT &&
+		    key != IPROTO_OPTIONS) {
 			mp_check(&data, end);   /* skip the key */
 			mp_check(&data, end);   /* skip the value */
 			continue;
@@ -583,10 +585,21 @@ error:
 		const char *value = ++data;     /* skip the key */
 		if (mp_check(&data, end) != 0)  /* check the value */
 			goto error;
-		if (key == IPROTO_SQL_BIND)
+		switch (key) {
+		case IPROTO_SQL_BIND:
 			request->bind = value;
-		else
+			break;
+		case IPROTO_SQL_TEXT:
 			request->sql_text = value;
+			break;
+		case IPROTO_OPTIONS:
+			request->opts = value;
+			break;
+		default:
+			xrow_on_decode_err(data, end, ER_INVALID_MSGPACK,
+					   "unknown IPROTO request key");
+			return -1;
+		}
 	}
 	if (request->sql_text == NULL) {
 		xrow_on_decode_err(row->body[0].iov_base, end, ER_MISSING_REQUEST_FIELD,
diff --git a/src/box/xrow.h b/src/box/xrow.h
index 35ec06dc0..f0c1f29ed 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -527,6 +527,11 @@ struct sql_request {
 	const char *sql_text;
 	/** MessagePack array of parameters. */
 	const char *bind;
+	/**
+	 * Map containing SQL execution options.
+	 * See struct sql_options for details.
+	 */
+	const char *opts;
 };
 
 /**
diff --git a/test/box/misc.result b/test/box/misc.result
index 287d84e5b..4d25a9fe7 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -530,6 +530,7 @@ t;
   198: box.error.FUNC_INDEX_FUNC
   199: box.error.FUNC_INDEX_FORMAT
   200: box.error.FUNC_INDEX_PARTS
+  201: box.error.WRONG_SQL_EXECUTE_OPTIONS
 ...
 test_run:cmd("setopt delimiter ''");
 ---
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 1e5c30aec..ae5349546 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -123,7 +123,23 @@ cn:execute(100)
 ...
 cn:execute('select 1', nil, {dry_run = true})
 ---
-- error: execute does not support options
+- error: Tuple/Key must be MsgPack array
+...
+cn:execute('select 1', nil, {1})
+---
+- error: 'Wrong SQL options passed to execute: dry_run must be boolean'
+...
+cn:execute('select 1', nil, {true, false})
+---
+- error: 'Wrong SQL options passed to execute: too many options are specified'
+...
+cn:execute('select 1', nil, {{dri_run = true}})
+---
+- error: 'Wrong SQL options passed to execute: unexpected option ''dri_run'''
+...
+cn:execute('select 1', nil, {{dry_run = 1}})
+---
+- error: 'Wrong SQL options passed to execute: ''dry_run'' must be boolean'
 ...
 -- Empty request.
 cn:execute('')
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 5dfe95ccc..7dcea7c2e 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -39,6 +39,10 @@ cn:execute('select id as identifier from test where a = 5;')
 -- netbox API errors.
 cn:execute(100)
 cn:execute('select 1', nil, {dry_run = true})
+cn:execute('select 1', nil, {1})
+cn:execute('select 1', nil, {true, false})
+cn:execute('select 1', nil, {{dri_run = true}})
+cn:execute('select 1', nil, {{dry_run = 1}})
 
 -- Empty request.
 cn:execute('')
-- 
2.15.1

[tarantool-patches] Re: [PATCH 7/8] netbox: allow passing options to :execute()

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/08/27 16:34]:
> To implement dry-run execution, let's allow 'options' be valid argument
> of :execute() method. SQL options are encoded with IPROTO_OPTIONS
> request key. Options are supposed to be a map of named parameters or a
> list of unnamed (considering their order). The only currently available
> option is 'dry_run' with boolean values. Now it doesn't affect anything,
> but will allow to get meta-information of query execution.

Please don't bother with anything but a map.


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 7/8] netbox: allow passing options to :execute()

[Vladislav Shpilevoy]

Thanks for the patch!

On 27/08/2019 15:34, Nikita Pettik wrote:
> To implement dry-run execution, let's allow 'options' be valid argument
> of :execute() method. SQL options are encoded with IPROTO_OPTIONS
> request key. Options are supposed to be a map of named parameters or a
> list of unnamed (considering their order). The only currently available
> option is 'dry_run' with boolean values. Now it doesn't affect anything,
> but will allow to get meta-information of query execution.

Why do you need an array of unnamed options? Such thing will be impossible
to maintain. Each time you will add a new option, you will extend this array,
and to pass that option a user would need to pass all the previous ones.
Please, drop arrays support.

[tarantool-patches] [PATCH 8/8] sql: introduce dry-run execution

[Nikita Pettik]

To get result of dry-run execution locally, one can use
box.dry_run("sql_string") method, which returns query's
meta-information. Note this method does not support binding values
substitution.

To get result of dry-run execution using net.box facilities, one can
pass map containing dry_run options to :execute() method (leaving array
of bindings empty):

cn:execute("SELECT 1;", {}, {{dry_run = true}})

Or simply set options considering their order:

cn:execute("SELECT 1;", {}, {true})

Note that there's no binding substitution even if array of values to be
bound is not empty. Also, dry-run execution for DML and DDL request
doesn't make any sense - zero row_count is always returned.

Under the hood we add 'meta_only' flag to struct port_sql, which
regulates whether response contains only metadata or metadata and
tuples forming the result set of query.

Closes #3292
---
 src/box/execute.c        | 20 ++++++++++---
 src/box/iproto.cc        | 18 ++++++------
 src/box/lua/execute.c    | 37 ++++++++++++++++++++++--
 src/box/lua/net_box.c    |  7 +++--
 src/box/port.h           |  4 ++-
 test/box/misc.result     |  1 +
 test/sql/iproto.result   | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
 test/sql/iproto.test.lua | 15 ++++++++++
 8 files changed, 159 insertions(+), 18 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index 2e3aeef2c..d8245c72b 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -82,6 +82,14 @@ static_assert(sizeof(struct port_sql) <= sizeof(struct port),
  * | }                                            |
  * +-------------------- OR ----------------------+
  * | IPROTO_BODY: {                               |
+ * |     IPROTO_METADATA: [                       |
+ * |         {IPROTO_FIELD_NAME: column name1},   |
+ * |         {IPROTO_FIELD_NAME: column name2},   |
+ * |         ...                                  |
+ * |     ],                                       |
+ * | }                                            |
+ * +-------------------- OR ----------------------+
+ * | IPROTO_BODY: {                               |
  * |     IPROTO_SQL_INFO: {                       |
  * |         SQL_INFO_ROW_COUNT: number           |
  * |         SQL_INFO_AUTOINCREMENT_IDS: [        |
@@ -122,10 +130,11 @@ const struct port_vtab port_sql_vtab = {
 };
 
 void
-port_sql_create(struct port *port, struct sql_stmt *stmt)
+port_sql_create(struct port *port, struct sql_stmt *stmt, bool meta_only)
 {
 	port_tuple_create(port);
 	((struct port_sql *)port)->stmt = stmt;
+	((struct port_sql *)port)->meta_only = meta_only;
 	port->vtab = &port_sql_vtab;
 }
 
@@ -332,10 +341,11 @@ 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;
+	struct port_sql *port_sql = (struct port_sql *) port;
+	struct sql_stmt *stmt = port_sql->stmt;
 	int column_count = sql_column_count(stmt);
 	if (column_count > 0) {
-		int keys = 2;
+		int keys = port_sql->meta_only ? 1 : 2;
 		int size = mp_sizeof_map(keys);
 		char *pos = (char *) obuf_alloc(out, size);
 		if (pos == NULL) {
@@ -345,6 +355,8 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
 		pos = mp_encode_map(pos, keys);
 		if (sql_get_metadata(stmt, out, column_count) != 0)
 			return -1;
+		if (port_sql->meta_only)
+			return 0;
 		size = mp_sizeof_uint(IPROTO_DATA);
 		pos = (char *) obuf_alloc(out, size);
 		if (pos == NULL) {
@@ -491,7 +503,7 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
 	if (sql_prepare(sql, len, &stmt, NULL) != 0)
 		return -1;
 	assert(stmt != NULL);
-	port_sql_create(port, stmt);
+	port_sql_create(port, stmt, false);
 	if (sql_bind(stmt, bind, bind_count) == 0 &&
 	    sql_execute(stmt, port, region) == 0)
 		return 0;
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index a92e66ace..22019efaa 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1671,14 +1671,16 @@ tx_process_sql(struct cmsg *m)
 	if (sql_prepare(sql, len, &stmt, NULL) != 0)
 		goto error;
 	assert(stmt != NULL);
-	port_sql_create(&port, stmt);
-	if (sql_bind(stmt, bind, bind_count) != 0) {
-		port_destroy(&port);
-		goto error;
-	}
-	if (sql_execute(stmt, &port, &fiber()->gc) != 0) {
-		port_destroy(&port);
-		goto error;
+	port_sql_create(&port, stmt, opts.dry_run);
+	if (!opts.dry_run) {
+		if (sql_bind(stmt, bind, bind_count) != 0) {
+			port_destroy(&port);
+			goto error;
+		}
+		if (sql_execute(stmt, &port, &fiber()->gc) != 0) {
+			port_destroy(&port);
+			goto error;
+		}
 	}
 	/*
 	 * Take an obuf only after execute(). Else the buffer can
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index 76ecdd541..fd95e508e 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -45,12 +45,15 @@ 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;
+	struct port_sql *port_sql = (struct port_sql *) port;
+	struct sql_stmt *stmt = port_sql->stmt;
 	int column_count = sql_column_count(stmt);
 	if (column_count > 0) {
-		lua_createtable(L, 0, 2);
+		lua_createtable(L, 0, port_sql->meta_only ? 1 : 2);
 		lua_sql_get_metadata(stmt, L, column_count);
 		lua_setfield(L, -2, "metadata");
+		if (port_sql->meta_only)
+			return;
 		port_tuple_vtab.dump_lua(port, L, false);
 		lua_setfield(L, -2, "rows");
 	} else {
@@ -266,6 +269,31 @@ lbox_execute(struct lua_State *L)
 	return 1;
 }
 
+/**
+ * In contrast to ordinary "execute" method, this one only
+ * prepares (compiles) statement but not executes. It allows
+ * to get query's meta-information.
+ */
+static int
+lbox_dry_run(struct lua_State *L)
+{
+	size_t length;
+	struct port port;
+	int top = lua_gettop(L);
+
+	if ((top != 1) || ! lua_isstring(L, 1))
+		return luaL_error(L, "Usage: box.dry_run(sqlstring)");
+
+	const char *sql = lua_tolstring(L, 1, &length);
+	struct sql_stmt *stmt;
+	if (sql_prepare(sql, length, &stmt, NULL) != 0)
+		return luaT_push_nil_and_error(L);
+	port_sql_create(&port, stmt, true);
+	port_dump_lua(&port, L, false);
+	port_destroy(&port);
+	return 1;
+}
+
 void
 box_lua_execute_init(struct lua_State *L)
 {
@@ -273,5 +301,10 @@ box_lua_execute_init(struct lua_State *L)
 	lua_pushstring(L, "execute");
 	lua_pushcfunction(L, lbox_execute);
 	lua_settable(L, -3);
+
+	lua_pushstring(L, "dry_run");
+	lua_pushcfunction(L, lbox_dry_run);
+	lua_settable(L, -3);
+
 	lua_pop(L, 1);
 }
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 001af95dc..fe8492a52 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -738,12 +738,13 @@ netbox_decode_execute(struct lua_State *L)
 	}
 	if (info_index == 0) {
 		assert(meta_index != 0);
-		assert(rows_index != 0);
 		lua_createtable(L, 0, 2);
 		lua_pushvalue(L, meta_index - 1);
 		lua_setfield(L, -2, "metadata");
-		lua_pushvalue(L, rows_index - 1);
-		lua_setfield(L, -2, "rows");
+		if (rows_index != 0) {
+			lua_pushvalue(L, rows_index - 1);
+			lua_setfield(L, -2, "rows");
+		}
 	} else {
 		assert(meta_index == 0);
 		assert(rows_index == 0);
diff --git a/src/box/port.h b/src/box/port.h
index 0d2cc7e84..65b980e61 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -135,13 +135,15 @@ struct port_sql {
 	struct port_tuple port_tuple;
 	/* Prepared SQL statement. */
 	struct sql_stmt *stmt;
+	/** If true then dump only query's meta-info. */
+	bool meta_only;
 };
 
 extern const struct port_vtab port_sql_vtab;
 
 /** Create instance of SQL port using given attributes. */
 void
-port_sql_create(struct port *port, struct sql_stmt *stmt);
+port_sql_create(struct port *port, struct sql_stmt *stmt, bool meta_only);
 
 #if defined(__cplusplus)
 } /* extern "C" */
diff --git a/test/box/misc.result b/test/box/misc.result
index 4d25a9fe7..6e92d4778 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -62,6 +62,7 @@ t
   - cfg
   - commit
   - ctl
+  - dry_run
   - error
   - execute
   - feedback
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index ae5349546..bd4705157 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -820,6 +820,81 @@ cn:execute("SELECT GREATEST(1, 2, 3);")
   rows:
   - [3]
 ...
+-- gh-3292: introduce dry-run SQL query execution.
+-- Dry-run returns only meta-information, i.e. there's no
+-- execution of query, only its compilation.
+--
+cn:execute("SELECT * FROM t1;", {}, {true})
+---
+- metadata:
+  - name: ID
+    type: integer
+...
+cn:execute("SELECT * FROM t1;", {}, {{dry_run = true}})
+---
+- metadata:
+  - name: ID
+    type: integer
+...
+box.dry_run("SELECT * FROM t1;")
+---
+- metadata:
+  - name: ID
+    type: integer
+...
+cn:execute("SELECT 1, 'abc', 0.123, x'00', ?;", {1}, {{dry_run = true}})
+---
+- metadata:
+  - name: '1'
+    type: integer
+  - name: '''abc'''
+    type: string
+  - name: '0.123'
+    type: number
+  - name: x'00'
+    type: varbinary
+  - name: '?'
+    type: boolean
+...
+box.dry_run("SELECT 1, 'abc', 0.123, x'00', ?;")
+---
+- metadata:
+  - name: '1'
+    type: integer
+  - name: '''abc'''
+    type: string
+  - name: '0.123'
+    type: number
+  - name: x'00'
+    type: varbinary
+  - name: '?'
+    type: boolean
+...
+cn:execute("DELETE FROM t1;")
+---
+- row_count: 5
+...
+cn:execute("INSERT INTO t1 VALUES (6);", {}, {{dry_run = true}})
+---
+- metadata:
+  - name: rows inserted
+    type: INTEGER
+...
+cn:execute("SELECT * FROM t1;")
+---
+- metadata:
+  - name: ID
+    type: integer
+  rows: []
+...
+cn:execute("CREATE TABLE new_t (id INT PRIMARY KEY);", {}, {{dry_run = true}})
+---
+- row_count: 0
+...
+assert(box.space.NEW_T == nil)
+---
+- true
+...
 cn:close()
 ---
 ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 7dcea7c2e..bf81ae335 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -247,6 +247,21 @@ res.metadata
 cn:execute("SELECT LEAST(1, 2, 3);")
 cn:execute("SELECT GREATEST(1, 2, 3);")
 
+-- gh-3292: introduce dry-run SQL query execution.
+-- Dry-run returns only meta-information, i.e. there's no
+-- execution of query, only its compilation.
+--
+cn:execute("SELECT * FROM t1;", {}, {true})
+cn:execute("SELECT * FROM t1;", {}, {{dry_run = true}})
+box.dry_run("SELECT * FROM t1;")
+cn:execute("SELECT 1, 'abc', 0.123, x'00', ?;", {1}, {{dry_run = true}})
+box.dry_run("SELECT 1, 'abc', 0.123, x'00', ?;")
+cn:execute("DELETE FROM t1;")
+cn:execute("INSERT INTO t1 VALUES (6);", {}, {{dry_run = true}})
+cn:execute("SELECT * FROM t1;")
+cn:execute("CREATE TABLE new_t (id INT PRIMARY KEY);", {}, {{dry_run = true}})
+assert(box.space.NEW_T == nil)
+
 cn:close()
 box.execute('DROP TABLE t1')
 
-- 
2.15.1

[tarantool-patches] Re: [PATCH 8/8] sql: introduce dry-run execution

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/08/27 16:34]:

Please address the design review. Returning SQL bind metadata is
the most important request.


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 8/8] sql: introduce dry-run execution

[Vladislav Shpilevoy]

Thanks for the patch!

See 2 comments below.

On 27/08/2019 15:34, Nikita Pettik wrote:
> To get result of dry-run execution locally, one can use
> box.dry_run("sql_string") method, which returns query's
> meta-information. Note this method does not support binding values
> substitution.
> 
> To get result of dry-run execution using net.box facilities, one can
> pass map containing dry_run options to :execute() method (leaving array
> of bindings empty):
> 
> cn:execute("SELECT 1;", {}, {{dry_run = true}})
> 
> Or simply set options considering their order:
> 
> cn:execute("SELECT 1;", {}, {true})
> 
> Note that there's no binding substitution even if array of values to be
> bound is not empty. Also, dry-run execution for DML and DDL request
> doesn't make any sense - zero row_count is always returned.
> 
> Under the hood we add 'meta_only' flag to struct port_sql, which
> regulates whether response contains only metadata or metadata and
> tuples forming the result set of query.
> 
> Closes #3292

1. JFR, we will need a docbot request. About box, netbox, and the binary
protocol.

> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index a92e66ace..22019efaa 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -1671,14 +1671,16 @@ tx_process_sql(struct cmsg *m)
>  	if (sql_prepare(sql, len, &stmt, NULL) != 0)
>  		goto error;
>  	assert(stmt != NULL);
> -	port_sql_create(&port, stmt);
> -	if (sql_bind(stmt, bind, bind_count) != 0) {
> -		port_destroy(&port);
> -		goto error;
> -	}
> -	if (sql_execute(stmt, &port, &fiber()->gc) != 0) {
> -		port_destroy(&port);
> -		goto error;
> +	port_sql_create(&port, stmt, opts.dry_run);
> +	if (!opts.dry_run) {
> +		if (sql_bind(stmt, bind, bind_count) != 0) {
> +			port_destroy(&port);
> +			goto error;
> +		}
> +		if (sql_execute(stmt, &port, &fiber()->gc) != 0) {
> +			port_destroy(&port);
> +			goto error;
> +		}
>  	}

2. This should be done inside sql_prepare_and_execute(), which
will take sql_opts parameter. It will allow you to 1) keep
sql_port encapsulated, 2) don't duplicate this code for Lua and
iproto versions.

[tarantool-patches] Re: [PATCH 0/8] rfc: introduce dry-run execution of SQL queries

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/08/27 16:34]:
> To execute query in dry-run mode locally separate method of box has been added:
> box.dry_run("sql_string"). Current implementation does not modify box.execute()
> making it accept third optional argument (except for array of bindings) - this
> is to be discussed. 

If you have to use a separate method, call it "prepare", and
return a prepare handle. The handle may simply contain SQL text as
prepared state, + metadata. Let's not invent a new API, dry run is
an implementation detail.

> Other open (or dubious) questions.
> 
> 1. How to handle DML queries. Meanwhile getting meta-information of DDL queries
> is likely to be useless, calculating number of affected rows in case of DML
> query can be sensible. On the other hand, it can turn out to be quite
> non-trivial task.  In its current state, box.dry_run("DML_Query") always
> returns 0 as row count. IMHO it can be done but as a follow-up

The point of dry-run in particular is to find out on the driver
side whether the query returns a result set or not. So please
handle them.

> 2. Should dry_run() except for parsing also bind variables? I guess no, since
> it results in byte-code (i.e. prepared statement) modification. On the other
> hand, it allows setting correct type for bindings (see sql_bind_type()). By
> default, all binding parameters have boolean type.

No, dry-run is an alias to 'prepare', which doesn't return a
prepare handle only because we decided to use SQL text as a
prepared statement identifier, and hide an explicit prepared
statement handle from the client. The prepared statement cache
will be managed transparently by the server when it is added, for
two reasons:

- to not force this complication on the client
- to be able to use prepared statements with statements which were
  not explicitly prepared.

An important detail: for this reason, dry run should also return
the number of parameter markers, their type and count.


> 3. Interface for local call. In current implementation I've added box.dry_run()
> in addition to traditional box.execute() method. Instead, we can make
> box.execute() accept third argument as it is handled in net.box.

Replied.

I will look at the patch separately.


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 0/8] rfc: introduce dry-run execution of SQL queries

[Vladislav Shpilevoy]

Hi! Thanks for the patchset!

On 27/08/2019 15:34, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/tree/np/sql-dry-run-rfc
> Issue: https://github.com/tarantool/tarantool/issues/3292
> 
> In a nutshell dry-run is a way to get auxiliary (meta-) information of query
> execution without direct execution itself. For instance, every DQL query
> returns column's name and type alongside with tuples forming the resulting set.
> Meta-information can be extended with column's nullability, collation and other
> properties. This feature is required to develop SQL drivers. 
> 
> A bit of details.
> 
> There are three forms of dumped msgpack structure (and corresponding IPROTO
> response format). First one relates to DQL (i.e. SELECT) queries:
> METADATA : [{name, type}, {name, type}, ...], DATA : [tuple, tuple ...]
> 
> In case of dry-run, only metadata should be dumped. As a consequence, DATA
> section becomes optional and depends on port settings (see new 'meta_only'
> option in struct port_sql). To pass flag indicating dry-run execution,
> :execute() method has been allowed to accept 'options' argument. Options can
> form a map of named parameters ({'dry_run' = true}) or simply a list of
> unnamed ordered values. Options are encoded with IPROTO_OPTIONS key. The only
> available option now is 'dry_run'.

I see that you made the API different for netbox and box - it should not be
so. We either do options for both netbox and box, or a separate method for both.

What to choose - I don't know. Kostja contradicts himself in his email, where he
said that you can use 'prepare' and return a handle, but on the other hand he
proposed to auto-cache all prepared statements. I don't think that auto-cache is
a good idea. It is hard to support, hard to prevent eviction when necessary, etc.
Kirll Y. also sticks to explicit prepare as I know.

On a chosen option it depends whether we need to use 'prepare' and return a handle
with metadata, or we need to add an option, and return the meta without a handle.

> To execute query in dry-run mode locally separate method of box has been added:
> box.dry_run("sql_string"). Current implementation does not modify box.execute()
> making it accept third optional argument (except for array of bindings) - this
> is to be discussed. 
> 
> Other open (or dubious) questions.
> 
> 1. How to handle DML queries. Meanwhile getting meta-information of DDL queries
> is likely to be useless, calculating number of affected rows in case of DML
> query can be sensible. On the other hand, it can turn out to be quite
> non-trivial task.  In its current state, box.dry_run("DML_Query") always
> returns 0 as row count. IMHO it can be done but as a follow-up

First about whether it makes sense to calculate any results like row_count at a
dry-run - of course it does not. Assume that you do it and appeared that row_count
would be 1. User relies on that and sends a new real request. Before the request
is executed, another one is scheduled and makes such changes, that the prepared one
now returns row_count 0, or even an error.

I think constant row_count 0 is better.

But DML will be able to return more meta in future via RETURNING as I know. Also
DML already now has other metadata - autoincremented field values. Perhaps
will have more. It means, that not only 'row_count' is returned. What to do with
them?

Perhaps it is worth not to return anything except meta for DQL. For DML and DDL
return an error, or empty meta. Assuming that DML in future will be able to
return meta, perhaps an empty one is better for now.

> 2. Should dry_run() except for parsing also bind variables? I guess no, since
> it results in byte-code (i.e. prepared statement) modification. On the other
> hand, it allows setting correct type for bindings (see sql_bind_type()). By
> default, all binding parameters have boolean type.

Yes, it changes bytecode. But it is harmless anyway. What other DBs do?

> 
> 3. Interface for local call. In current implementation I've added box.dry_run()
> in addition to traditional box.execute() method. Instead, we can make
> box.execute() accept third argument as it is handled in net.box.

The only thing I am sure about is that box and netbox APIs should be the same.

> Nikita Pettik (8):
>   port: increase padding of struct port
>   port: move struct port_sql to box/port.h
>   sql: remove sql_prepare_v2()
>   sql: refactor sql_prepare() and sqlPrepare()
>   sql: move sql_prepare() declaration to box/execute.h
>   refactoring: use sql_prepare() and sql_execute() in tx_process_sql()
>   netbox: allow passing options to :execute()
>   sql: introduce dry-run execution
> 
>  src/box/errcode.h        |   1 +
>  src/box/execute.c        | 104 ++++++++++++++++++++++++++++++++++++-----------
>  src/box/execute.h        |  61 ++++++++++++++++++++-------
>  src/box/iproto.cc        |  22 +++++++++-
>  src/box/lua/execute.c    |  37 ++++++++++++++++-
>  src/box/lua/net_box.c    |   7 ++--
>  src/box/lua/net_box.lua  |   3 --
>  src/box/port.h           |  26 ++++++++++++
>  src/box/sql/analyze.c    |  16 ++++----
>  src/box/sql/legacy.c     |   3 +-
>  src/box/sql/prepare.c    |  38 +++--------------
>  src/box/sql/sqlInt.h     |  16 --------
>  src/box/sql/vdbe.h       |   2 +-
>  src/box/sql/vdbeInt.h    |   1 -
>  src/box/sql/vdbeapi.c    |   9 ++--
>  src/box/sql/vdbeaux.c    |   5 +--
>  src/box/xrow.c           |  19 +++++++--
>  src/box/xrow.h           |   5 +++
>  src/lib/core/port.h      |   3 +-
>  test/box/misc.result     |   2 +
>  test/sql/iproto.result   |  93 +++++++++++++++++++++++++++++++++++++++++-
>  test/sql/iproto.test.lua |  19 +++++++++
>  22 files changed, 371 insertions(+), 121 deletions(-)
>