* [tarantool-patches] [PATCH v6 0/5] sql: remove box.sql.execute
@ 2018-12-28 18:11 imeevma
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 1/5] iproto: move map creation to sql_response_dump() imeevma
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: imeevma @ 2018-12-28 18:11 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
The goal of this patch-set is to make functions from execute.c
the only way to execute SQL statements. This goal includes
similar output for executed SQL statements no matter how they
were executed: through net.box or through box.
This is the sixth version of patch-set. It is not complete. It
still has no last part, which is replacing box.sql.execute by
box.execute, because it will lead to massive test editing.
For now this patch-set blocked by #3832. Small temporary fix added
to temporary patch of patch-set.
https://github.com/tarantool/tarantool/issues/3505
https://github.com/tarantool/tarantool/tree/imeevma/gh-3505-replace-box_sql_execute-by-box_execute
General information of difference from previous version of
patch-set:
- port_sql completely removed from execute.h.
- Fixed some bugs in new binding.
- Added new and fixed old comments and descriptions.
- Refactoring.
A bit about patches of the patch-set:
Patch 1 moves map creation from xrow functions to
sql_response_dump(). It allows us to use sql_response_dump() as
method of port.
Patch 2 creates port_sql and two its methods: dump_msgpack() and
destroy().
Patch 3 creates dump_lua() method for port_sql.
Patch 4 adds binding to new_execute().
Patch 5 is temporary patch. It was created to check that
new_execute() is able to pass through tests created for execute().
v1:
https://www.freelists.org/post/tarantool-patches/PATCH-v1-0010-sql-remove-boxsqlexecute
v2:
https://www.freelists.org/post/tarantool-patches/PATCH-v2-07-Remove-boxsqlexecute
v3:
https://www.freelists.org/post/tarantool-patches/PATCH-v3-07-Remove-boxsqlexecute
v4:
https://www.freelists.org/post/tarantool-patches/PATCH-v4-05-Remove-boxsqlexecute
v5:
https://www.freelists.org/post/tarantool-patches/PATCH-v5-05-sql-remove-boxsqlexecute
Mergen Imeev (5):
iproto: move map creation to sql_response_dump()
iproto: create port_sql
lua: create method dump_lua for port_sql
lua: parameter binding for new execute()
sql: check new box.sql.execute()
src/box/execute.c | 506 ++++++++++++++++++++++++++++++++++++++++++-------
src/box/execute.h | 61 ++----
src/box/iproto.cc | 14 +-
src/box/lua/schema.lua | 23 +++
src/box/lua/sql.c | 39 +++-
src/box/port.h | 1 -
src/box/xrow.c | 8 +-
src/box/xrow.h | 9 +-
8 files changed, 524 insertions(+), 137 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] [PATCH v6 1/5] iproto: move map creation to sql_response_dump()
2018-12-28 18:11 [tarantool-patches] [PATCH v6 0/5] sql: remove box.sql.execute imeevma
@ 2018-12-28 18:11 ` imeevma
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 2/5] iproto: create port_sql imeevma
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: imeevma @ 2018-12-28 18:11 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Currently, function sql_response_dump() puts data into an already
created map. Moving the map creation to sql_response_dump()
simplifies the code and allows us to use sql_response_dump() as
one of the port_sql methods.
Needed for #3505
---
src/box/execute.c | 23 ++++++++++++++++-------
src/box/execute.h | 3 +--
src/box/iproto.cc | 8 +++-----
src/box/xrow.c | 8 +-------
src/box/xrow.h | 9 +--------
5 files changed, 22 insertions(+), 29 deletions(-)
diff --git a/src/box/execute.c b/src/box/execute.c
index 7fff5fd..38b6cbc 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -555,22 +555,29 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
}
int
-sql_response_dump(struct sql_response *response, int *keys, struct obuf *out)
+sql_response_dump(struct sql_response *response, struct obuf *out)
{
sqlite3 *db = sql_get();
struct sqlite3_stmt *stmt = (struct sqlite3_stmt *) response->prep_stmt;
struct port_tuple *port_tuple = (struct port_tuple *) &response->port;
int rc = 0, column_count = sqlite3_column_count(stmt);
if (column_count > 0) {
+ int keys = 2;
+ int size = mp_sizeof_map(keys);
+ char *pos = (char *) obuf_alloc(out, size);
+ if (pos == NULL) {
+ diag_set(OutOfMemory, size, "obuf_alloc", "pos");
+ goto err;
+ }
+ pos = mp_encode_map(pos, keys);
if (sql_get_description(stmt, out, column_count) != 0) {
err:
rc = -1;
goto finish;
}
- *keys = 2;
- int size = mp_sizeof_uint(IPROTO_DATA) +
- mp_sizeof_array(port_tuple->size);
- char *pos = (char *) obuf_alloc(out, size);
+ size = mp_sizeof_uint(IPROTO_DATA) +
+ mp_sizeof_array(port_tuple->size);
+ pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
goto err;
@@ -586,18 +593,20 @@ err:
goto err;
}
} else {
- *keys = 1;
+ int keys = 1;
assert(port_tuple->size == 0);
struct stailq *autoinc_id_list =
vdbe_autoinc_id_list((struct Vdbe *)stmt);
uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
- int size = mp_sizeof_uint(IPROTO_SQL_INFO) +
+ int size = mp_sizeof_map(keys) +
+ mp_sizeof_uint(IPROTO_SQL_INFO) +
mp_sizeof_map(map_size);
char *pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
goto err;
}
+ pos = mp_encode_map(pos, keys);
pos = mp_encode_uint(pos, IPROTO_SQL_INFO);
pos = mp_encode_map(pos, map_size);
uint64_t id_count = 0;
diff --git a/src/box/execute.h b/src/box/execute.h
index 9c1bc4f..60b8f31 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -105,14 +105,13 @@ sql_bind_list_decode(const char *data, struct sql_bind **out_bind);
* | } |
* +----------------------------------------------+
* @param response EXECUTE response.
- * @param[out] keys number of keys in dumped map.
* @param out Output buffer.
*
* @retval 0 Success.
* @retval -1 Memory error.
*/
int
-sql_response_dump(struct sql_response *response, int *keys, struct obuf *out);
+sql_response_dump(struct sql_response *response, struct obuf *out);
/**
* Prepare and execute an SQL statement.
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 1debc3c..a08c8c5 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1641,17 +1641,15 @@ tx_process_sql(struct cmsg *m)
* become out of date during yield.
*/
out = msg->connection->tx.p_obuf;
- int keys;
struct obuf_svp header_svp;
/* Prepare memory for the iproto header. */
- if (iproto_prepare_header(out, &header_svp, IPROTO_SQL_HEADER_LEN) != 0)
+ if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0)
goto error;
- if (sql_response_dump(&response, &keys, out) != 0) {
+ if (sql_response_dump(&response, out) != 0) {
obuf_rollback_to_svp(out, &header_svp);
goto error;
}
- iproto_reply_sql(out, &header_svp, msg->header.sync, schema_version,
- keys);
+ iproto_reply_sql(out, &header_svp, msg->header.sync, schema_version);
iproto_wpos_create(&msg->wpos, out);
return;
error:
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 67019a6..c4e3073 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -508,17 +508,11 @@ error:
void
iproto_reply_sql(struct obuf *buf, struct obuf_svp *svp, uint64_t sync,
- uint32_t schema_version, int keys)
+ uint32_t schema_version)
{
char *pos = (char *) obuf_svp_to_ptr(buf, svp);
iproto_header_encode(pos, IPROTO_OK, sync, schema_version,
obuf_size(buf) - svp->used - IPROTO_HEADER_LEN);
- /*
- * MessagePack encodes value <= 15 as
- * bitwise OR with 0x80.
- */
- assert(keys <= 15);
- *(pos + IPROTO_HEADER_LEN) = 0x80 | keys;
}
void
diff --git a/src/box/xrow.h b/src/box/xrow.h
index 6bab0a1..2654e35 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -52,12 +52,6 @@ enum {
IPROTO_HEADER_LEN = 28,
/** 7 = sizeof(iproto_body_bin). */
IPROTO_SELECT_HEADER_LEN = IPROTO_HEADER_LEN + 7,
- /**
- * Header of message + header of body with one or two
- * keys: IPROTO_DATA and IPROTO_METADATA or
- * IPROTO_SQL_INFO. 1 == mp_sizeof_map(<=15).
- */
- IPROTO_SQL_HEADER_LEN = IPROTO_HEADER_LEN + 1,
};
struct xrow_header {
@@ -491,11 +485,10 @@ xrow_decode_sql(const struct xrow_header *row, struct sql_request *request);
* @param svp Savepoint of the header beginning.
* @param sync Request sync.
* @param schema_version Schema version.
- * @param keys Count of keys in the body.
*/
void
iproto_reply_sql(struct obuf *buf, struct obuf_svp *svp, uint64_t sync,
- uint32_t schema_version, int keys);
+ uint32_t schema_version);
/**
* Write an IPROTO_CHUNK header from a specified position in a
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] [PATCH v6 2/5] iproto: create port_sql
2018-12-28 18:11 [tarantool-patches] [PATCH v6 0/5] sql: remove box.sql.execute imeevma
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 1/5] iproto: move map creation to sql_response_dump() imeevma
@ 2018-12-28 18:11 ` imeevma
2018-12-29 13:19 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 3/5] lua: create method dump_lua for port_sql imeevma
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: imeevma @ 2018-12-28 18:11 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Hi! Thank you for review! My answers, diff between versions and
new version below.
On 12/25/18 5:57 PM, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch! See 4 comments below.
> 1. Please, write a comment about how could you use port_tuple_vtab on
> port_sql. Here I expected to see words about calling methods of a base
> structure, like BaseClass::method in C++.
Added. Not sure that it is enough.
> 2. Please, rename this method to port_sql_create and call here port_tuple_create()
> explicitly. It is just a constructor. port_tuple_add, used in sql_execute(), will
> work anyway. Since port_sql is derived from port_tuple, it can be used wherever the
> base.
Done.
> 3. I think, we should not expose port_sql to the header. It is used in execute.c
> only. Same about port_tuple_to_port_sql.
Done.
> 4. Do not forget about comments. It still describes 'response' parameter.
Fixed.
Diff between versions:
commit 414bf16c094fbb7b9e0ecb6b4b56f069f109ef86
Author: Mergen Imeev <imeevma@gmail.com>
Date: Wed Dec 26 22:00:35 2018 +0300
Temporary: review fixes
diff --git a/src/box/execute.c b/src/box/execute.c
index b07de28..c6fcb50 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -83,6 +83,36 @@ struct sql_bind {
};
/**
+ * Port implementation used for dump tuples, stored in port_tuple,
+ * to obuf or Lua.
+ *
+ * All port_tuple methods can be used with port_sql, since
+ * port_sql is a structure inherited from port_tuple. Calling
+ * port_tuple methods we are going via port_tuple_vtab.
+ */
+struct port_sql {
+ /* port_tuple to inherit from. */
+ struct port_tuple port_tuple;
+ /* Prepared SQL statement. */
+ struct sqlite3_stmt *stmt;
+};
+static_assert(sizeof(struct port_sql) <= sizeof(struct port),
+ "sizeof(struct port_sql) must be <= sizeof(struct port)");
+
+static int
+port_sql_dump_msgpack(struct port *port, struct obuf *out);
+static void
+port_sql_destroy(struct port *base);
+
+const struct port_vtab port_sql_vtab = {
+ .dump_msgpack = port_sql_dump_msgpack,
+ .dump_msgpack_16 = NULL,
+ .dump_lua = NULL,
+ .dump_plain = NULL,
+ .destroy = port_sql_destroy,
+};
+
+/**
* Return a string name of a parameter marker.
* @param Bind to get name.
* @retval Zero terminated name.
@@ -356,6 +386,11 @@ sql_row_to_port(struct sqlite3_stmt *stmt, int column_count,
if (tuple == NULL)
goto error;
region_truncate(region, svp);
+ /*
+ * The port_tuple_add function can be used with port_sql,
+ * since it does not call any port_tuple methods and works
+ * only with fields.
+ */
return port_tuple_add(port, tuple);
error:
@@ -503,71 +538,24 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
return 0;
}
-static inline int
-sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
- struct region *region)
-{
- int rc, column_count = sqlite3_column_count(stmt);
- if (column_count > 0) {
- /* Either ROW or DONE or ERROR. */
- while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) {
- if (sql_row_to_port(stmt, column_count, region,
- port) != 0)
- return -1;
- }
- assert(rc == SQLITE_DONE || rc != SQLITE_OK);
- } else {
- /* No rows. Either DONE or ERROR. */
- rc = sqlite3_step(stmt);
- assert(rc != SQLITE_ROW && rc != SQLITE_OK);
- }
- if (rc != SQLITE_DONE) {
- diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
- return -1;
- }
- 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)
+static void
+port_sql_create(struct port *port, struct sqlite3_stmt *stmt)
{
- struct sqlite3_stmt *stmt;
- sqlite3 *db = sql_get();
- if (db == NULL) {
- diag_set(ClientError, ER_LOADING);
- return -1;
- }
- if (sqlite3_prepare_v2(db, sql, len, &stmt, NULL) != SQLITE_OK) {
- diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
- return -1;
- }
- assert(stmt != NULL);
port_tuple_create(port);
- if (sql_bind(stmt, bind, bind_count) == 0 &&
- sql_execute(db, stmt, port, region) == 0) {
- port_tuple_to_port_sql(port, stmt);
- return 0;
- }
- port_destroy(port);
- sqlite3_finalize(stmt);
- return -1;
+ ((struct port_sql *)port)->stmt = stmt;
+ port->vtab = &port_sql_vtab;
}
/**
- * Dump a built response into @an out buffer. The response is
- * destroyed.
- * Response structure:
+ * Dump tuples, metadata, or information obtained from an excuted
+ * SQL query and saved to the port in obuf. The port is destroyed.
+ *
+ * Dumped msgpack structure:
* +----------------------------------------------+
- * | IPROTO_OK, sync, schema_version ... | iproto_header
- * +----------------------------------------------+---------------
- * | Body - a map with one or two keys. |
- * | |
* | IPROTO_BODY: { |
* | IPROTO_METADATA: [ |
* | {IPROTO_FIELD_NAME: column name1}, |
- * | {IPROTO_FIELD_NAME: column name2}, | iproto_body
+ * | {IPROTO_FIELD_NAME: column name2}, |
* | ... |
* | ], |
* | |
@@ -579,10 +567,19 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
* | IPROTO_BODY: { |
* | IPROTO_SQL_INFO: { |
* | SQL_INFO_ROW_COUNT: number |
+ * | SQL_INFO_AUTOINCREMENT_IDS: [ |
+ * | id, id, id, ... |
+ * | ] |
+ * | } |
+ * | } |
+ * +-------------------- OR ----------------------+
+ * | IPROTO_BODY: { |
+ * | IPROTO_SQL_INFO: { |
+ * | SQL_INFO_ROW_COUNT: number |
* | } |
* | } |
* +----------------------------------------------+
- * @param port port with EXECUTE response.
+ * @param port port with tuples, metadata or info.
* @param out Output buffer.
*
* @retval 0 Success.
@@ -616,6 +613,7 @@ err:
goto err;
}
pos = mp_encode_uint(pos, IPROTO_DATA);
+ /* Calling BaseStruct::methods via its vtab. */
if (port_tuple_vtab.dump_msgpack(port, out) < 0)
goto err;
} else {
@@ -670,28 +668,62 @@ err:
}
finish:
port_destroy(port);
- sqlite3_finalize(stmt);
return rc;
}
static void
port_sql_destroy(struct port *base)
{
+ /* Calling BaseStruct::methods via its vtab. */
port_tuple_vtab.destroy(base);
+ sqlite3_finalize(((struct port_sql *)base)->stmt);
}
-void
-port_tuple_to_port_sql(struct port *port, struct sqlite3_stmt *stmt)
+static inline int
+sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
+ struct region *region)
{
- assert(port->vtab == &port_tuple_vtab);
- ((struct port_sql *)port)->stmt = stmt;
- port->vtab = &port_sql_vtab;
+ int rc, column_count = sqlite3_column_count(stmt);
+ if (column_count > 0) {
+ /* Either ROW or DONE or ERROR. */
+ while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) {
+ if (sql_row_to_port(stmt, column_count, region,
+ port) != 0)
+ return -1;
+ }
+ assert(rc == SQLITE_DONE || rc != SQLITE_OK);
+ } else {
+ /* No rows. Either DONE or ERROR. */
+ rc = sqlite3_step(stmt);
+ assert(rc != SQLITE_ROW && rc != SQLITE_OK);
+ }
+ if (rc != SQLITE_DONE) {
+ diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
+ return -1;
+ }
+ return 0;
}
-const struct port_vtab port_sql_vtab = {
- .dump_msgpack = port_sql_dump_msgpack,
- .dump_msgpack_16 = NULL,
- .dump_lua = NULL,
- .dump_plain = NULL,
- .destroy = port_sql_destroy,
-};
+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 sqlite3_stmt *stmt;
+ sqlite3 *db = sql_get();
+ if (db == NULL) {
+ diag_set(ClientError, ER_LOADING);
+ return -1;
+ }
+ if (sqlite3_prepare_v2(db, sql, len, &stmt, NULL) != SQLITE_OK) {
+ diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
+ return -1;
+ }
+ assert(stmt != NULL);
+ port_sql_create(port, stmt);
+ if (sql_bind(stmt, bind, bind_count) == 0 &&
+ sql_execute(db, stmt, port, region) == 0)
+ return 0;
+ port_destroy(port);
+ return -1;
+}
diff --git a/src/box/execute.h b/src/box/execute.h
index 5aef546..c90789f 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -48,34 +48,8 @@ enum sql_info_key {
extern const char *sql_info_key_strs[];
-struct obuf;
struct region;
struct sql_bind;
-struct sqlite3_stmt;
-
-/**
- * Port implementation used for dump tuples, stored in port_tuple,
- * to obuf or Lua.
- */
-struct port_sql {
- /* port_tuple to inherit from. */
- struct port_tuple port_tuple;
- /* Prepared SQL statement. */
- struct sqlite3_stmt *stmt;
-};
-static_assert(sizeof(struct port_sql) <= sizeof(struct port),
- "sizeof(struct port_sql) must be <= sizeof(struct port)");
-
-extern const struct port_vtab port_sql_vtab;
-
-/**
- * Transform port_tuple with already stored tuples to port_sql
- * that will dump these tuples into obut or Lua.
- *
- * @param port port_tuple to transform into port_sql.
- */
-void
-port_tuple_to_port_sql(struct port *port, struct sqlite3_stmt *stmt);
/**
* Parse MessagePack array of SQL parameters.
New version:
commit 8390b09d95aa5bcaa6ef89924d9a334dff746f19
Author: Mergen Imeev <imeevma@gmail.com>
Date: Fri Dec 21 14:52:03 2018 +0300
iproto: create port_sql
This patch creates port_sql implementation for the port. This will
allow us to dump sql responses to obuf or to Lua. Also this patch
defines methods dump_msgpack() and destroy() of port_sql.
Part of #3505
diff --git a/src/box/execute.c b/src/box/execute.c
index 38b6cbc..c6fcb50 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -83,6 +83,36 @@ struct sql_bind {
};
/**
+ * Port implementation used for dump tuples, stored in port_tuple,
+ * to obuf or Lua.
+ *
+ * All port_tuple methods can be used with port_sql, since
+ * port_sql is a structure inherited from port_tuple. Calling
+ * port_tuple methods we are going via port_tuple_vtab.
+ */
+struct port_sql {
+ /* port_tuple to inherit from. */
+ struct port_tuple port_tuple;
+ /* Prepared SQL statement. */
+ struct sqlite3_stmt *stmt;
+};
+static_assert(sizeof(struct port_sql) <= sizeof(struct port),
+ "sizeof(struct port_sql) must be <= sizeof(struct port)");
+
+static int
+port_sql_dump_msgpack(struct port *port, struct obuf *out);
+static void
+port_sql_destroy(struct port *base);
+
+const struct port_vtab port_sql_vtab = {
+ .dump_msgpack = port_sql_dump_msgpack,
+ .dump_msgpack_16 = NULL,
+ .dump_lua = NULL,
+ .dump_plain = NULL,
+ .destroy = port_sql_destroy,
+};
+
+/**
* Return a string name of a parameter marker.
* @param Bind to get name.
* @retval Zero terminated name.
@@ -356,6 +386,11 @@ sql_row_to_port(struct sqlite3_stmt *stmt, int column_count,
if (tuple == NULL)
goto error;
region_truncate(region, svp);
+ /*
+ * The port_tuple_add function can be used with port_sql,
+ * since it does not call any port_tuple methods and works
+ * only with fields.
+ */
return port_tuple_add(port, tuple);
error:
@@ -503,63 +538,59 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
return 0;
}
-static inline int
-sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
- struct region *region)
+static void
+port_sql_create(struct port *port, struct sqlite3_stmt *stmt)
{
- int rc, column_count = sqlite3_column_count(stmt);
- if (column_count > 0) {
- /* Either ROW or DONE or ERROR. */
- while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) {
- if (sql_row_to_port(stmt, column_count, region,
- port) != 0)
- return -1;
- }
- assert(rc == SQLITE_DONE || rc != SQLITE_OK);
- } else {
- /* No rows. Either DONE or ERROR. */
- rc = sqlite3_step(stmt);
- assert(rc != SQLITE_ROW && rc != SQLITE_OK);
- }
- if (rc != SQLITE_DONE) {
- diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
- return -1;
- }
- return 0;
+ port_tuple_create(port);
+ ((struct port_sql *)port)->stmt = stmt;
+ port->vtab = &port_sql_vtab;
}
-int
-sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
- uint32_t bind_count, struct sql_response *response,
- struct region *region)
-{
- struct sqlite3_stmt *stmt;
- sqlite3 *db = sql_get();
- if (db == NULL) {
- diag_set(ClientError, ER_LOADING);
- return -1;
- }
- if (sqlite3_prepare_v2(db, sql, len, &stmt, NULL) != SQLITE_OK) {
- diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
- return -1;
- }
- assert(stmt != NULL);
- port_tuple_create(&response->port);
- response->prep_stmt = stmt;
- if (sql_bind(stmt, bind, bind_count) == 0 &&
- sql_execute(db, stmt, &response->port, region) == 0)
- return 0;
- port_destroy(&response->port);
- sqlite3_finalize(stmt);
- return -1;
-}
-
-int
-sql_response_dump(struct sql_response *response, struct obuf *out)
+/**
+ * Dump tuples, metadata, or information obtained from an excuted
+ * SQL query and saved to the port in obuf. The port is destroyed.
+ *
+ * Dumped msgpack structure:
+ * +----------------------------------------------+
+ * | IPROTO_BODY: { |
+ * | IPROTO_METADATA: [ |
+ * | {IPROTO_FIELD_NAME: column name1}, |
+ * | {IPROTO_FIELD_NAME: column name2}, |
+ * | ... |
+ * | ], |
+ * | |
+ * | IPROTO_DATA: [ |
+ * | tuple, tuple, tuple, ... |
+ * | ] |
+ * | } |
+ * +-------------------- OR ----------------------+
+ * | IPROTO_BODY: { |
+ * | IPROTO_SQL_INFO: { |
+ * | SQL_INFO_ROW_COUNT: number |
+ * | SQL_INFO_AUTOINCREMENT_IDS: [ |
+ * | id, id, id, ... |
+ * | ] |
+ * | } |
+ * | } |
+ * +-------------------- OR ----------------------+
+ * | IPROTO_BODY: { |
+ * | IPROTO_SQL_INFO: { |
+ * | SQL_INFO_ROW_COUNT: number |
+ * | } |
+ * | } |
+ * +----------------------------------------------+
+ * @param port port with tuples, metadata or info.
+ * @param out Output buffer.
+ *
+ * @retval 0 Success.
+ * @retval -1 Memory error.
+ */
+static int
+port_sql_dump_msgpack(struct port *port, struct obuf *out)
{
+ assert(port->vtab == &port_sql_vtab);
sqlite3 *db = sql_get();
- struct sqlite3_stmt *stmt = (struct sqlite3_stmt *) response->prep_stmt;
- struct port_tuple *port_tuple = (struct port_tuple *) &response->port;
+ struct sqlite3_stmt *stmt = ((struct port_sql *)port)->stmt;
int rc = 0, column_count = sqlite3_column_count(stmt);
if (column_count > 0) {
int keys = 2;
@@ -575,26 +606,19 @@ err:
rc = -1;
goto finish;
}
- size = mp_sizeof_uint(IPROTO_DATA) +
- mp_sizeof_array(port_tuple->size);
+ size = mp_sizeof_uint(IPROTO_DATA);
pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
goto err;
}
pos = mp_encode_uint(pos, IPROTO_DATA);
- pos = mp_encode_array(pos, port_tuple->size);
- /*
- * Just like SELECT, SQL uses output format compatible
- * with Tarantool 1.6
- */
- if (port_dump_msgpack_16(&response->port, out) < 0) {
- /* Failed port dump destroyes the port. */
+ /* Calling BaseStruct::methods via its vtab. */
+ if (port_tuple_vtab.dump_msgpack(port, out) < 0)
goto err;
- }
} else {
int keys = 1;
- assert(port_tuple->size == 0);
+ assert(((struct port_tuple *)port)->size == 0);
struct stailq *autoinc_id_list =
vdbe_autoinc_id_list((struct Vdbe *)stmt);
uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
@@ -643,7 +667,63 @@ err:
}
}
finish:
- port_destroy(&response->port);
- sqlite3_finalize(stmt);
+ port_destroy(port);
return rc;
}
+
+static void
+port_sql_destroy(struct port *base)
+{
+ /* Calling BaseStruct::methods via its vtab. */
+ port_tuple_vtab.destroy(base);
+ sqlite3_finalize(((struct port_sql *)base)->stmt);
+}
+
+static inline int
+sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
+ struct region *region)
+{
+ int rc, column_count = sqlite3_column_count(stmt);
+ if (column_count > 0) {
+ /* Either ROW or DONE or ERROR. */
+ while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) {
+ if (sql_row_to_port(stmt, column_count, region,
+ port) != 0)
+ return -1;
+ }
+ assert(rc == SQLITE_DONE || rc != SQLITE_OK);
+ } else {
+ /* No rows. Either DONE or ERROR. */
+ rc = sqlite3_step(stmt);
+ assert(rc != SQLITE_ROW && rc != SQLITE_OK);
+ }
+ if (rc != SQLITE_DONE) {
+ diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
+ return -1;
+ }
+ 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 sqlite3_stmt *stmt;
+ sqlite3 *db = sql_get();
+ if (db == NULL) {
+ diag_set(ClientError, ER_LOADING);
+ return -1;
+ }
+ if (sqlite3_prepare_v2(db, sql, len, &stmt, NULL) != SQLITE_OK) {
+ diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
+ return -1;
+ }
+ assert(stmt != NULL);
+ port_sql_create(port, stmt);
+ if (sql_bind(stmt, bind, bind_count) == 0 &&
+ sql_execute(db, stmt, port, region) == 0)
+ return 0;
+ port_destroy(port);
+ return -1;
+}
diff --git a/src/box/execute.h b/src/box/execute.h
index 60b8f31..c90789f 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -48,18 +48,9 @@ enum sql_info_key {
extern const char *sql_info_key_strs[];
-struct obuf;
struct region;
struct sql_bind;
-/** Response on EXECUTE request. */
-struct sql_response {
- /** Port with response data if any. */
- struct port port;
- /** Prepared SQL statement with metadata. */
- void *prep_stmt;
-};
-
/**
* Parse MessagePack array of SQL parameters.
* @param data MessagePack array of parameters. Each parameter
@@ -78,42 +69,6 @@ int
sql_bind_list_decode(const char *data, struct sql_bind **out_bind);
/**
- * Dump a built response into @an out buffer. The response is
- * destroyed.
- * Response structure:
- * +----------------------------------------------+
- * | IPROTO_OK, sync, schema_version ... | iproto_header
- * +----------------------------------------------+---------------
- * | Body - a map with one or two keys. |
- * | |
- * | IPROTO_BODY: { |
- * | IPROTO_METADATA: [ |
- * | {IPROTO_FIELD_NAME: column name1}, |
- * | {IPROTO_FIELD_NAME: column name2}, | iproto_body
- * | ... |
- * | ], |
- * | |
- * | IPROTO_DATA: [ |
- * | tuple, tuple, tuple, ... |
- * | ] |
- * | } |
- * +-------------------- OR ----------------------+
- * | IPROTO_BODY: { |
- * | IPROTO_SQL_INFO: { |
- * | SQL_INFO_ROW_COUNT: number |
- * | } |
- * | } |
- * +----------------------------------------------+
- * @param response EXECUTE response.
- * @param out Output buffer.
- *
- * @retval 0 Success.
- * @retval -1 Memory error.
- */
-int
-sql_response_dump(struct sql_response *response, struct obuf *out);
-
-/**
* Prepare and execute an SQL statement.
* @param sql SQL statement.
* @param len Length of @a sql.
@@ -128,7 +83,7 @@ sql_response_dump(struct sql_response *response, struct obuf *out);
*/
int
sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
- uint32_t bind_count, struct sql_response *response,
+ uint32_t bind_count, struct port *port,
struct region *region);
#if defined(__cplusplus)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index a08c8c5..9dc0462 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1616,7 +1616,7 @@ tx_process_sql(struct cmsg *m)
{
struct iproto_msg *msg = tx_accept_msg(m);
struct obuf *out;
- struct sql_response response;
+ struct port port;
struct sql_bind *bind;
int bind_count;
const char *sql;
@@ -1633,7 +1633,7 @@ tx_process_sql(struct cmsg *m)
goto error;
sql = msg->sql.sql_text;
sql = mp_decode_str(&sql, &len);
- if (sql_prepare_and_execute(sql, len, bind, bind_count, &response,
+ if (sql_prepare_and_execute(sql, len, bind, bind_count, &port,
&fiber()->gc) != 0)
goto error;
/*
@@ -1645,7 +1645,7 @@ tx_process_sql(struct cmsg *m)
/* Prepare memory for the iproto header. */
if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0)
goto error;
- if (sql_response_dump(&response, out) != 0) {
+ if (port_dump_msgpack(&port, out) != 0) {
obuf_rollback_to_svp(out, &header_svp);
goto error;
}
diff --git a/src/box/port.h b/src/box/port.h
index ad1b349..f188036 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -65,7 +65,6 @@ extern const struct port_vtab port_tuple_vtab;
static inline struct port_tuple *
port_tuple(struct port *port)
{
- assert(port->vtab == &port_tuple_vtab);
return (struct port_tuple *)port;
}
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] [PATCH v6 3/5] lua: create method dump_lua for port_sql
2018-12-28 18:11 [tarantool-patches] [PATCH v6 0/5] sql: remove box.sql.execute imeevma
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 1/5] iproto: move map creation to sql_response_dump() imeevma
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 2/5] iproto: create port_sql imeevma
@ 2018-12-28 18:11 ` imeevma
2018-12-29 13:19 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 4/5] lua: parameter binding for new execute() imeevma
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 5/5] sql: check new box.sql.execute() imeevma
4 siblings, 1 reply; 9+ messages in thread
From: imeevma @ 2018-12-28 18:11 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Hi! Thank you for review! My answers, diff between versions and
new version below.
On 12/25/18 5:57 PM, Vladislav Shpilevoy wrote:
> Thanks for the patch! See 7 comments below.
> 1. Use lua_pushstring.
Fixed.
> 2. It is strange, that this function creates a table and sets it
> as a member of some externally created table. Please, move this line
> to a caller function.
Fixed.
> 3. Just inline keys 2.
Fixed.
> 4. Why 1 array element? And why do you separate fixed/optional keys?
> lua_createtable does not take statis/dynamic key count. It takes
> array/map key count.
Fixed. Now it takes 1 or 2 as map key count.
> 5. I think, port_sql_destroy should call sqlite3_finalize.
Fixed in patch #2.
> 6. Impossible. It isn't?
Removed.
> 7. Please, start message with a capital letter.
Fixed.
Diff between versions:
commit 2d1f7aca7dc5a8526f6916bf4d782d0e4ff5f5da
Author: Mergen Imeev <imeevma@gmail.com>
Date: Thu Dec 27 16:26:25 2018 +0300
Temporary: review fixes
diff --git a/src/box/execute.c b/src/box/execute.c
index bab0048..4606239 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -697,13 +697,12 @@ lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
* column_name simply returns them.
*/
assert(name != NULL);
- lua_pushlstring(L, name, strlen(name));
+ lua_pushstring(L, name);
lua_setfield(L, -2, "name");
- lua_pushlstring(L, type, strlen(type));
+ lua_pushstring(L, type);
lua_setfield(L, -2, "type");
lua_rawseti(L, -2, i + 1);
}
- lua_setfield(L, -2, "metadata");
}
/**
@@ -721,18 +720,20 @@ port_sql_dump_lua(struct port *port, struct lua_State *L)
struct sqlite3_stmt *stmt = ((struct port_sql *)port)->stmt;
int column_count = sqlite3_column_count(stmt);
if (column_count > 0) {
- int keys = 2;
- lua_createtable(L, 0, keys);
+ lua_createtable(L, 0, 2);
lua_sql_get_description(stmt, L, column_count);
+ lua_setfield(L, -2, "metadata");
+ /*
+ * We can use the port_tuple methods, since
+ * port_sql was inherited from it.
+ */
port_tuple_vtab.dump_lua(port, L);
lua_setfield(L, -2, "rows");
} else {
assert(((struct port_tuple *)port)->size == 0);
struct stailq *autoinc_id_list =
vdbe_autoinc_id_list((struct Vdbe *)stmt);
-
- int dynamic_keys = stailq_empty(autoinc_id_list) ? 0 : 1;
- lua_createtable(L, 1, dynamic_keys);
+ lua_createtable(L, 0, stailq_empty(autoinc_id_list) ? 1 : 2);
luaL_pushuint64(L, db->nChange);
lua_setfield(L, -2, "rowcount");
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index 318f331..a55566e 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -119,13 +119,12 @@ lbox_execute(struct lua_State *L)
size_t length;
struct port port;
- struct sqlite3 *db = sql_get();
- if (db == NULL)
- return luaL_error(L, "not ready");
+ if (lua_type(L, 1) != LUA_TSTRING)
+ return luaL_error(L, "Usage: box.execute(sqlstring)");
const char *sql = lua_tolstring(L, 1, &length);
if (sql == NULL)
- return luaL_error(L, "usage: box.execute(sqlstring)");
+ return luaL_error(L, "Usage: box.execute(sqlstring)");
if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
&fiber()->gc) != 0)
New version:
commit 74f30d133b34ea86dc145f6ffee347abfd489cf2
Author: Mergen Imeev <imeevma@gmail.com>
Date: Fri Dec 21 16:48:37 2018 +0300
lua: create method dump_lua for port_sql
This patch creates the dump_lua method for port_sql. It also
defines a new function box.sql.new_execute(), which will be
converted to box.sql.execute().
Part of #3505
diff --git a/src/box/execute.c b/src/box/execute.c
index c6fcb50..4606239 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -43,6 +43,7 @@
#include "port.h"
#include "tuple.h"
#include "sql/vdbe.h"
+#include "lua/utils.h"
const char *sql_type_strs[] = {
NULL,
@@ -102,12 +103,14 @@ static_assert(sizeof(struct port_sql) <= sizeof(struct port),
static int
port_sql_dump_msgpack(struct port *port, struct obuf *out);
static void
+port_sql_dump_lua(struct port *port, struct lua_State *L);
+static void
port_sql_destroy(struct port *base);
const struct port_vtab port_sql_vtab = {
.dump_msgpack = port_sql_dump_msgpack,
.dump_msgpack_16 = NULL,
- .dump_lua = NULL,
+ .dump_lua = port_sql_dump_lua,
.dump_plain = NULL,
.destroy = port_sql_destroy,
};
@@ -671,6 +674,87 @@ finish:
return rc;
}
+/**
+ * Push a metadata of the prepared statement to Lua stack.
+ *
+ * @param stmt Prepared statement.
+ * @param L Lua stack.
+ * @param column_count Statement's column count.
+ */
+static inline void
+lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
+ int column_count)
+{
+ assert(column_count > 0);
+ lua_createtable(L, column_count, 0);
+ for (int i = 0; i < column_count; ++i) {
+ lua_createtable(L, 2, 0);
+ const char *name = sqlite3_column_name(stmt, i);
+ const char *type = sqlite3_column_datatype(stmt, i);
+ /*
+ * Can not fail, since all column names are
+ * preallocated during prepare phase and the
+ * column_name simply returns them.
+ */
+ assert(name != NULL);
+ lua_pushstring(L, name);
+ lua_setfield(L, -2, "name");
+ lua_pushstring(L, type);
+ lua_setfield(L, -2, "type");
+ lua_rawseti(L, -2, i + 1);
+ }
+}
+
+/**
+ * Dump a built response into Lua stack. The response is
+ * destroyed.
+ *
+ * @param port port with EXECUTE response.
+ * @param L Lua stack.
+ */
+static void
+port_sql_dump_lua(struct port *port, struct lua_State *L)
+{
+ assert(port->vtab == &port_sql_vtab);
+ sqlite3 *db = sql_get();
+ struct sqlite3_stmt *stmt = ((struct port_sql *)port)->stmt;
+ int column_count = sqlite3_column_count(stmt);
+ if (column_count > 0) {
+ lua_createtable(L, 0, 2);
+ lua_sql_get_description(stmt, L, column_count);
+ lua_setfield(L, -2, "metadata");
+ /*
+ * We can use the port_tuple methods, since
+ * port_sql was inherited from it.
+ */
+ port_tuple_vtab.dump_lua(port, L);
+ lua_setfield(L, -2, "rows");
+ } else {
+ assert(((struct port_tuple *)port)->size == 0);
+ struct stailq *autoinc_id_list =
+ vdbe_autoinc_id_list((struct Vdbe *)stmt);
+ lua_createtable(L, 0, stailq_empty(autoinc_id_list) ? 1 : 2);
+
+ luaL_pushuint64(L, db->nChange);
+ lua_setfield(L, -2, "rowcount");
+
+ if (!stailq_empty(autoinc_id_list)) {
+ lua_newtable(L);
+ int i = 1;
+ struct autoinc_id_entry *id_entry;
+ stailq_foreach_entry(id_entry, autoinc_id_list, link) {
+ if (id_entry->id >= 0)
+ luaL_pushuint64(L, id_entry->id);
+ else
+ luaL_pushint64(L, id_entry->id);
+ lua_rawseti(L, -2, i++);
+ }
+ lua_setfield(L, -2, "autoincrement_ids");
+ }
+ }
+ port_destroy(port);
+}
+
static void
port_sql_destroy(struct port *base)
{
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index 6923666..a55566e 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -6,6 +6,7 @@
#include <info.h>
#include "lua/info.h"
#include "lua/utils.h"
+#include "box/execute.h"
static void
lua_push_column_names(struct lua_State *L, struct sqlite3_stmt *stmt)
@@ -111,6 +112,28 @@ sqlerror:
}
static int
+lbox_execute(struct lua_State *L)
+{
+ struct sql_bind *bind = NULL;
+ int bind_count = 0;
+ size_t length;
+ struct port port;
+
+ if (lua_type(L, 1) != LUA_TSTRING)
+ return luaL_error(L, "Usage: box.execute(sqlstring)");
+
+ const char *sql = lua_tolstring(L, 1, &length);
+ if (sql == NULL)
+ return luaL_error(L, "Usage: box.execute(sqlstring)");
+
+ if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
+ &fiber()->gc) != 0)
+ return luaT_error(L);
+ port_dump_lua(&port, L);
+ return 1;
+}
+
+static int
lua_sql_debug(struct lua_State *L)
{
struct info_handler info;
@@ -124,6 +147,7 @@ box_lua_sqlite_init(struct lua_State *L)
{
static const struct luaL_Reg module_funcs [] = {
{"execute", lua_sql_execute},
+ {"new_execute", lbox_execute},
{"debug", lua_sql_debug},
{NULL, NULL}
};
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] [PATCH v6 4/5] lua: parameter binding for new execute()
2018-12-28 18:11 [tarantool-patches] [PATCH v6 0/5] sql: remove box.sql.execute imeevma
` (2 preceding siblings ...)
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 3/5] lua: create method dump_lua for port_sql imeevma
@ 2018-12-28 18:11 ` imeevma
2018-12-29 13:19 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 5/5] sql: check new box.sql.execute() imeevma
4 siblings, 1 reply; 9+ messages in thread
From: imeevma @ 2018-12-28 18:11 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Hi! Thank you for review! My answers, diff between versions and
new version below.
On 12/25/18 5:57 PM, Vladislav Shpilevoy wrote:
> Thanks for the patch! See 9 comments below.
> 1. It is not msgpack.
Fixed.
> 2. In Lua syntax {'name' : value} is incorrect.
Fixed.
> 3. Just -1. Stack indexes can be negative.
lua_next() throws an assert when I change arg value to -1:
tarantool: lj_api.c:892: lua_next: Assertion `(((t)->it) == (~11u))' failed.
> 4. Please, do not pop the value stored in a luaL_field. Pop 2
> values at one time below, on line 294.
Fixed. Actually, I found out that my popping policy was quite a
mess in previous version. Now everything will be popped at the
end of the function.
> 5. As I know, luaL_checklstring never returns NULL. It throws. Please,
> check for string type explicitly and throw normal usage error.
Fixed.
> 6. As I said above, you've already popped it.
Fixed.
> 7. Please, check for overflow.
Fixed.
> 8. To be honest, I feel utterly uncomfortable with luaL_field. IMO,
> this vast structure is too overloaded. Can you please avoid its usage?
> Almost nothing will change as I see. For switch on types you can use
> lua_type().
After some discussion it was decided to left it as it is now. The
main reason for this is that if we remove luaL_field() than this
function become downgraded version of luaL_field().
> 9. Not msgpack. Please, do not blindly copy-paste code. Check out the
> commit more accurately. I will not point out other places.
Fixed. Moved this check to sql.c.
Diff between versions:
commit 93e4ea0ffcc195c4a904dab99c5cc516db33fa44
Author: Mergen Imeev <imeevma@gmail.com>
Date: Thu Dec 27 17:00:51 2018 +0300
Temporary: review fixes
diff --git a/src/box/execute.c b/src/box/execute.c
index 457403b..ade6e6c 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -278,43 +278,43 @@ static inline int
lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
{
struct luaL_field field;
+ struct region *region = &fiber()->gc;
char *buf;
lua_rawgeti(L, idx, i + 1);
luaL_tofield(L, luaL_msgpack_default, -1, &field);
bind->pos = i + 1;
if (field.type == MP_MAP) {
/*
- * A named parameter is an MP_MAP with
- * one key - {'name': value}.
- * Report parse error otherwise.
+ * A named parameter is a single-row table that
+ * contains one value associated with a key. The
+ * key must be a string. This key is the name of
+ * the parameter, and the value associated with
+ * the key is the value of the parameter.
*/
if (field.size != 1) {
diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
- "parameter should be {'name': value}");
+ "named parameter should be a single-row "\
+ "table with one key-value pair");
return -1;
}
/*
- * Get key and value of the only map element to
+ * Get key and value of the only table element to
* lua stack.
*/
lua_pushnil(L);
lua_next(L, lua_gettop(L) - 1);
- /* At first we should deal with the value. */
- luaL_tofield(L, luaL_msgpack_default, -1, &field);
- lua_pop(L, 1);
- /* Now key is on the top of Lua stack. */
- size_t name_len = 0;
- bind->name = luaL_checklstring(L, -1, &name_len);
- if (bind->name == NULL) {
- diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
- "parameter should be {'name': value}");
+ if (lua_type(L, -2) != LUA_TSTRING) {
+ diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\
+ "parameter should be a string.");
return -1;
}
+ size_t name_len;
+ bind->name = lua_tolstring(L, -2, &name_len);
/*
* Name should be saved in allocated memory as it
* will be poped from Lua stack.
*/
- buf = region_alloc(&fiber()->gc, name_len + 1);
+ buf = region_alloc(region, name_len + 1);
if (buf == NULL) {
diag_set(OutOfMemory, name_len + 1, "region_alloc",
"buf");
@@ -323,13 +323,18 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
memcpy(buf, bind->name, name_len + 1);
bind->name = buf;
bind->name_len = name_len;
- lua_pop(L, 1);
+ luaL_tofield(L, luaL_msgpack_default, -1, &field);
} else {
bind->name = NULL;
bind->name_len = 0;
}
switch (field.type) {
case MP_UINT: {
+ if (field.ival < 0) {
+ diag_set(ClientError, ER_SQL_BIND_VALUE,
+ sql_bind_name(bind), "INTEGER");
+ return -1;
+ }
bind->i64 = field.ival;
bind->type = SQLITE_INTEGER;
bind->bytes = sizeof(bind->i64);
@@ -345,7 +350,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
* Data should be saved in allocated memory as it
* will be poped from Lua stack.
*/
- buf = region_alloc(&fiber()->gc, field.sval.len + 1);
+ buf = region_alloc(region, field.sval.len + 1);
if (buf == NULL) {
diag_set(OutOfMemory, field.sval.len + 1,
"region_alloc", "buf");
@@ -385,7 +390,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
* Data should be saved in allocated memory as it
* will be poped from Lua stack.
*/
- buf = region_alloc(&fiber()->gc, sizeof(field));
+ buf = region_alloc(region, sizeof(field));
if (buf == NULL) {
diag_set(OutOfMemory, sizeof(field), "region_alloc",
"buf");
@@ -407,7 +412,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
default:
unreachable();
}
- lua_pop(L, 1);
+ lua_pop(L, lua_gettop(L) - idx);
return 0;
}
@@ -416,10 +421,6 @@ lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind,
int idx)
{
assert(out_bind != NULL);
- if (! lua_istable(L, idx)) {
- diag_set(ClientError, ER_INVALID_MSGPACK, "SQL parameter list");
- return -1;
- }
uint32_t bind_count = lua_objlen(L, idx);
if (bind_count == 0)
return 0;
diff --git a/src/box/execute.h b/src/box/execute.h
index 0625e2a..cb149a0 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -69,10 +69,16 @@ int
sql_bind_list_decode(const char *data, struct sql_bind **out_bind);
/**
- * Parse Lua table of SQL parameters and store a result
- * into the @request->bind, bind_count.
+ * Parse Lua table of SQL parameters.
*
- * @param L Lua stack to get data from.
+ * @param L Lua stack contains table with parameters. Each
+ * parameter either must have scalar type, or must be a
+ * single-row table with the following format:
+ * table[name] = value. Name - string name of the named
+ * parameter, value - scalar value of the parameter.
+ * Named and positioned parameters can be mixed.
+ * For more details
+ * @sa https://www.sqlite.org/lang_expr.html#varparam.
* @param[out] out_bind Pointer to save decoded parameters.
* @param idx Position of table with parameters on Lua stack.
*
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index a27e0c5..20ba43b 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -118,17 +118,22 @@ lbox_execute(struct lua_State *L)
int bind_count = 0;
size_t length;
struct port port;
+ int top = lua_gettop(L);
- if (lua_type(L, 1) != LUA_TSTRING)
- return luaL_error(L, "Usage: box.execute(sqlstring)");
+ if ((top != 1 && top != 2) || lua_type(L, 1) != LUA_TSTRING)
+ return luaL_error(L, "Usage: box.execute(sqlstring[, params])");
const char *sql = lua_tolstring(L, 1, &length);
if (sql == NULL)
- return luaL_error(L, "Usage: box.execute(sqlstring)");
-
- if (lua_gettop(L) == 2 &&
- (bind_count = lua_sql_bind_list_decode(L, &bind, 2)) < 0)
- return luaT_error(L);
+ return luaL_error(L, "Usage: box.execute(sqlstring[, params])");
+
+ if (top == 2) {
+ if (! lua_istable(L, 2))
+ return luaL_error(L, "Second argument must be a table");
+ bind_count = lua_sql_bind_list_decode(L, &bind, 2);
+ if (bind_count < 0)
+ return luaT_error(L);
+ }
if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
&fiber()->gc) != 0)
New version:
commit a1fe2aa7edc82097b2fcf63a41fb7fa2145b9d3c
Author: Mergen Imeev <imeevma@gmail.com>
Date: Fri Dec 21 17:54:16 2018 +0300
lua: parameter binding for new execute()
This patch defines parameters binding for SQL statements executed
through box.
Part of #3505
Closes #3401
diff --git a/src/box/execute.c b/src/box/execute.c
index 4606239..ade6e6c 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -44,6 +44,7 @@
#include "tuple.h"
#include "sql/vdbe.h"
#include "lua/utils.h"
+#include "lua/msgpack.h"
const char *sql_type_strs[] = {
NULL,
@@ -261,6 +262,196 @@ sql_bind_list_decode(const char *data, struct sql_bind **out_bind)
return bind_count;
}
+
+/**
+ * Decode a single bind column from Lua stack.
+ *
+ * @param L Lua stack.
+ * @param[out] bind Bind to decode to.
+ * @param idx Position of table with bind columns on Lua stack.
+ * @param i Ordinal bind number.
+ *
+ * @retval 0 Success.
+ * @retval -1 Memory or client error.
+ */
+static inline int
+lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
+{
+ struct luaL_field field;
+ struct region *region = &fiber()->gc;
+ char *buf;
+ lua_rawgeti(L, idx, i + 1);
+ luaL_tofield(L, luaL_msgpack_default, -1, &field);
+ bind->pos = i + 1;
+ if (field.type == MP_MAP) {
+ /*
+ * A named parameter is a single-row table that
+ * contains one value associated with a key. The
+ * key must be a string. This key is the name of
+ * the parameter, and the value associated with
+ * the key is the value of the parameter.
+ */
+ if (field.size != 1) {
+ diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
+ "named parameter should be a single-row "\
+ "table with one key-value pair");
+ return -1;
+ }
+ /*
+ * Get key and value of the only table element to
+ * lua stack.
+ */
+ lua_pushnil(L);
+ lua_next(L, lua_gettop(L) - 1);
+ if (lua_type(L, -2) != LUA_TSTRING) {
+ diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\
+ "parameter should be a string.");
+ return -1;
+ }
+ size_t name_len;
+ bind->name = lua_tolstring(L, -2, &name_len);
+ /*
+ * Name should be saved in allocated memory as it
+ * will be poped from Lua stack.
+ */
+ buf = region_alloc(region, name_len + 1);
+ if (buf == NULL) {
+ diag_set(OutOfMemory, name_len + 1, "region_alloc",
+ "buf");
+ return -1;
+ }
+ memcpy(buf, bind->name, name_len + 1);
+ bind->name = buf;
+ bind->name_len = name_len;
+ luaL_tofield(L, luaL_msgpack_default, -1, &field);
+ } else {
+ bind->name = NULL;
+ bind->name_len = 0;
+ }
+ switch (field.type) {
+ case MP_UINT: {
+ if (field.ival < 0) {
+ diag_set(ClientError, ER_SQL_BIND_VALUE,
+ sql_bind_name(bind), "INTEGER");
+ return -1;
+ }
+ bind->i64 = field.ival;
+ bind->type = SQLITE_INTEGER;
+ bind->bytes = sizeof(bind->i64);
+ break;
+ }
+ case MP_INT:
+ bind->i64 = field.ival;
+ bind->type = SQLITE_INTEGER;
+ bind->bytes = sizeof(bind->i64);
+ break;
+ case MP_STR:
+ /*
+ * Data should be saved in allocated memory as it
+ * will be poped from Lua stack.
+ */
+ buf = region_alloc(region, field.sval.len + 1);
+ if (buf == NULL) {
+ diag_set(OutOfMemory, field.sval.len + 1,
+ "region_alloc", "buf");
+ return -1;
+ }
+ memcpy(buf, field.sval.data, field.sval.len + 1);
+ bind->s = buf;
+ bind->type = SQLITE_TEXT;
+ bind->bytes = field.sval.len;
+ break;
+ case MP_DOUBLE:
+ bind->d = field.dval;
+ bind->type = SQLITE_FLOAT;
+ bind->bytes = sizeof(bind->d);
+ break;
+ case MP_FLOAT:
+ bind->d = field.dval;
+ bind->type = SQLITE_FLOAT;
+ bind->bytes = sizeof(bind->d);
+ break;
+ case MP_NIL:
+ bind->type = SQLITE_NULL;
+ bind->bytes = 1;
+ break;
+ case MP_BOOL:
+ /* SQLite doesn't support boolean. Use int instead. */
+ bind->i64 = field.bval ? 1 : 0;
+ bind->type = SQLITE_INTEGER;
+ bind->bytes = sizeof(bind->i64);
+ break;
+ case MP_BIN:
+ bind->s = mp_decode_bin(&field.sval.data, &bind->bytes);
+ bind->type = SQLITE_BLOB;
+ break;
+ case MP_EXT:
+ /*
+ * Data should be saved in allocated memory as it
+ * will be poped from Lua stack.
+ */
+ buf = region_alloc(region, sizeof(field));
+ if (buf == NULL) {
+ diag_set(OutOfMemory, sizeof(field), "region_alloc",
+ "buf");
+ return -1;
+ }
+ memcpy(buf, &field, sizeof(field));
+ bind->s = buf;
+ bind->bytes = sizeof(field);
+ bind->type = SQLITE_BLOB;
+ break;
+ case MP_ARRAY:
+ diag_set(ClientError, ER_SQL_BIND_TYPE, "ARRAY",
+ sql_bind_name(bind));
+ return -1;
+ case MP_MAP:
+ diag_set(ClientError, ER_SQL_BIND_TYPE, "MAP",
+ sql_bind_name(bind));
+ return -1;
+ default:
+ unreachable();
+ }
+ lua_pop(L, lua_gettop(L) - idx);
+ return 0;
+}
+
+int
+lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind,
+ int idx)
+{
+ assert(out_bind != NULL);
+ uint32_t bind_count = lua_objlen(L, idx);
+ if (bind_count == 0)
+ return 0;
+ if (bind_count > SQL_BIND_PARAMETER_MAX) {
+ diag_set(ClientError, ER_SQL_BIND_PARAMETER_MAX,
+ (int) bind_count);
+ return -1;
+ }
+ struct region *region = &fiber()->gc;
+ uint32_t used = region_used(region);
+ size_t size = sizeof(struct sql_bind) * bind_count;
+ /*
+ * Memory allocated here will be freed in
+ * sqlite3_finalize() or in txn_commit()/txn_rollback() if
+ * there is an active transaction.
+ */
+ struct sql_bind *bind = (struct sql_bind *) region_alloc(region, size);
+ if (bind == NULL) {
+ diag_set(OutOfMemory, size, "region_alloc", "bind");
+ return -1;
+ }
+ for (uint32_t i = 0; i < bind_count; ++i) {
+ if (lua_sql_bind_decode(L, &bind[i], idx, i) != 0) {
+ region_truncate(region, used);
+ return -1;
+ }
+ }
+ *out_bind = bind;
+ return bind_count;
+}
+
/**
* Serialize a single column of a result set row.
* @param stmt Prepared and started statement. At least one
diff --git a/src/box/execute.h b/src/box/execute.h
index c90789f..cb149a0 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -69,6 +69,27 @@ int
sql_bind_list_decode(const char *data, struct sql_bind **out_bind);
/**
+ * Parse Lua table of SQL parameters.
+ *
+ * @param L Lua stack contains table with parameters. Each
+ * parameter either must have scalar type, or must be a
+ * single-row table with the following format:
+ * table[name] = value. Name - string name of the named
+ * parameter, value - scalar value of the parameter.
+ * Named and positioned parameters can be mixed.
+ * For more details
+ * @sa https://www.sqlite.org/lang_expr.html#varparam.
+ * @param[out] out_bind Pointer to save decoded parameters.
+ * @param idx Position of table with parameters on Lua stack.
+ *
+ * @retval >= 0 Number of decoded parameters.
+ * @retval -1 Client or memory error.
+ */
+int
+lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind,
+ int idx);
+
+/**
* Prepare and execute an SQL statement.
* @param sql SQL statement.
* @param len Length of @a sql.
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index a55566e..20ba43b 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -118,13 +118,22 @@ lbox_execute(struct lua_State *L)
int bind_count = 0;
size_t length;
struct port port;
+ int top = lua_gettop(L);
- if (lua_type(L, 1) != LUA_TSTRING)
- return luaL_error(L, "Usage: box.execute(sqlstring)");
+ if ((top != 1 && top != 2) || lua_type(L, 1) != LUA_TSTRING)
+ return luaL_error(L, "Usage: box.execute(sqlstring[, params])");
const char *sql = lua_tolstring(L, 1, &length);
if (sql == NULL)
- return luaL_error(L, "Usage: box.execute(sqlstring)");
+ return luaL_error(L, "Usage: box.execute(sqlstring[, params])");
+
+ if (top == 2) {
+ if (! lua_istable(L, 2))
+ return luaL_error(L, "Second argument must be a table");
+ bind_count = lua_sql_bind_list_decode(L, &bind, 2);
+ if (bind_count < 0)
+ return luaT_error(L);
+ }
if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
&fiber()->gc) != 0)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] [PATCH v6 5/5] sql: check new box.sql.execute()
2018-12-28 18:11 [tarantool-patches] [PATCH v6 0/5] sql: remove box.sql.execute imeevma
` (3 preceding siblings ...)
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 4/5] lua: parameter binding for new execute() imeevma
@ 2018-12-28 18:11 ` imeevma
4 siblings, 0 replies; 9+ messages in thread
From: imeevma @ 2018-12-28 18:11 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
This commit checks that new implementation of box.sql.execute() is
able to pass all tests. This is temporary commit and should be
dropped later. Even though this commit is temporary it shows that
this patch-set should be pushed after patch for issue #3832.
Needed for #3505
---
src/box/execute.c | 14 +++++++++-----
src/box/execute.h | 2 +-
src/box/iproto.cc | 2 +-
src/box/lua/schema.lua | 23 +++++++++++++++++++++++
src/box/lua/sql.c | 10 +++++++---
5 files changed, 41 insertions(+), 10 deletions(-)
diff --git a/src/box/execute.c b/src/box/execute.c
index ade6e6c..0a75dd6 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -716,6 +716,8 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
* column_name simply returns them.
*/
assert(name != NULL);
+ if (type == NULL)
+ type = "UNKNOWN";
size += mp_sizeof_str(strlen(name));
size += mp_sizeof_str(strlen(type));
char *pos = (char *) obuf_alloc(out, size);
@@ -888,6 +890,8 @@ lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
* column_name simply returns them.
*/
assert(name != NULL);
+ if (type == NULL)
+ type = "UNKNOWN";
lua_pushstring(L, name);
lua_setfield(L, -2, "name");
lua_pushstring(L, type);
@@ -956,7 +960,7 @@ port_sql_destroy(struct port *base)
static inline int
sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
- struct region *region)
+ struct region *region, int error_id)
{
int rc, column_count = sqlite3_column_count(stmt);
if (column_count > 0) {
@@ -973,7 +977,7 @@ sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
assert(rc != SQLITE_ROW && rc != SQLITE_OK);
}
if (rc != SQLITE_DONE) {
- diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
+ diag_set(ClientError, error_id, sqlite3_errmsg(db));
return -1;
}
return 0;
@@ -982,7 +986,7 @@ sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
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 region *region, int error_id)
{
struct sqlite3_stmt *stmt;
sqlite3 *db = sql_get();
@@ -991,13 +995,13 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
return -1;
}
if (sqlite3_prepare_v2(db, sql, len, &stmt, NULL) != SQLITE_OK) {
- diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
+ diag_set(ClientError, error_id, sqlite3_errmsg(db));
return -1;
}
assert(stmt != NULL);
port_sql_create(port, stmt);
if (sql_bind(stmt, bind, bind_count) == 0 &&
- sql_execute(db, stmt, port, region) == 0)
+ sql_execute(db, stmt, port, region, error_id) == 0)
return 0;
port_destroy(port);
return -1;
diff --git a/src/box/execute.h b/src/box/execute.h
index cb149a0..ce8f20f 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -105,7 +105,7 @@ lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind,
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 region *region, int error_id);
#if defined(__cplusplus)
} /* extern "C" { */
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 9dc0462..a6bd354 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1634,7 +1634,7 @@ 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)
+ &fiber()->gc, ER_SQL_EXECUTE) != 0)
goto error;
/*
* Take an obuf only after execute(). Else the buffer can
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 8a804f0..02ec2fd 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -2456,3 +2456,26 @@ box.feedback.save = function(file_name)
end
box.NULL = msgpack.NULL
+
+box.sql.execute = function(sql)
+ local result = box.sql.new_execute(sql)
+ if result == nil then return end
+ local ret = nil
+ if result.rows ~= nil then
+ ret = {}
+ for key, row in pairs(result.rows) do
+ if type(row) == 'cdata' then
+ table.insert(ret, row:totable())
+ end
+ end
+ end
+ if result.metadata ~= nil then
+ if ret == nil then ret = {} end
+ ret[0] = {}
+ for key, row in pairs(result.metadata) do
+ table.insert(ret[0], row['name'])
+ end
+ setmetatable(ret, {__serialize = 'sequence'})
+ end
+ if ret ~= nil then return ret end
+end
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index 20ba43b..e1c67d8 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -136,10 +136,14 @@ lbox_execute(struct lua_State *L)
}
if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
- &fiber()->gc) != 0)
- return luaT_error(L);
+ &fiber()->gc, ER_SYSTEM) != 0)
+ goto sqlerror;
port_dump_lua(&port, L);
return 1;
+
+sqlerror:
+ lua_pushstring(L, sqlite3_errmsg(sql_get()));
+ return lua_error(L);
}
static int
@@ -155,7 +159,7 @@ void
box_lua_sqlite_init(struct lua_State *L)
{
static const struct luaL_Reg module_funcs [] = {
- {"execute", lua_sql_execute},
+ {"old_execute", lua_sql_execute},
{"new_execute", lbox_execute},
{"debug", lua_sql_debug},
{NULL, NULL}
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] Re: [PATCH v6 2/5] iproto: create port_sql
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 2/5] iproto: create port_sql imeevma
@ 2018-12-29 13:19 ` Vladislav Shpilevoy
0 siblings, 0 replies; 9+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-29 13:19 UTC (permalink / raw)
To: tarantool-patches, imeevma
Thanks for the patch! See 11 comments below.
On 28/12/2018 21:11, imeevma@tarantool.org wrote:
> Hi! Thank you for review! My answers, diff between versions and
> new version below.
>
> On 12/25/18 5:57 PM, Vladislav Shpilevoy wrote:
>> Hi! Thanks for the patch! See 4 comments below.
>
>> 1. Please, write a comment about how could you use port_tuple_vtab on
>> port_sql. Here I expected to see words about calling methods of a base
>> structure, like BaseClass::method in C++.
> Added. Not sure that it is enough.
Than add more explanations? Hardly it is possible to 'overexplain'
something, but it is as easy as nothing to 'underexplain'.
>
>> 2. Please, rename this method to port_sql_create and call here port_tuple_create()
>> explicitly. It is just a constructor. port_tuple_add, used in sql_execute(), will
>> work anyway. Since port_sql is derived from port_tuple, it can be used wherever the
>> base.
> Done.
>
>> 3. I think, we should not expose port_sql to the header. It is used in execute.c
>> only. Same about port_tuple_to_port_sql.
> Done.
>
>> 4. Do not forget about comments. It still describes 'response' parameter.
> Fixed.
>
>
> Diff between versions:
>
> commit 414bf16c094fbb7b9e0ecb6b4b56f069f109ef86
> Author: Mergen Imeev <imeevma@gmail.com>
> Date: Wed Dec 26 22:00:35 2018 +0300
>
> Temporary: review fixes
>
> diff --git a/src/box/execute.c b/src/box/execute.c
> index b07de28..c6fcb50 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -83,6 +83,36 @@ struct sql_bind {
> };
>
> /**
> + * Port implementation used for dump tuples, stored in port_tuple,
> + * to obuf or Lua.
1. Not only tuples, but the whole response, including headers,
meta, info.
> + *
> + * All port_tuple methods can be used with port_sql, since
> + * port_sql is a structure inherited from port_tuple. Calling
> + * port_tuple methods we are going via port_tuple_vtab.
2. Not sure if the sentence is correct. 'Calling we are going'
looks flawed. Maybe better 'Port_tuple methods are called via
explicit access to port_tuple_vtab just like C++ does with
BaseClass::method, when it is called in a child method'. ?
Or something like this.
> + */
> +struct port_sql {
> + /* port_tuple to inherit from. */
> + struct port_tuple port_tuple;
> + /* Prepared SQL statement. */
> + struct sqlite3_stmt *stmt;
> +};
> +static_assert(sizeof(struct port_sql) <= sizeof(struct port),
> + "sizeof(struct port_sql) must be <= sizeof(struct port)");
> +
> +static int
> +port_sql_dump_msgpack(struct port *port, struct obuf *out);
3. Please, add empty line between function declarations. Also, we
usually write a comment above a first function declaration appearance,
so the whole comment about port_sql_dump_msgpack() should be put here.
> +static void
> +port_sql_destroy(struct port *base);
4. Correct me if I am wrong, but as I see, port_sql_destroy can be
implemented right here, without pre-announcement.
Also, please, move port_sql_create here to gather all methods
in one place.
> +
> +const struct port_vtab port_sql_vtab = {
5. 'static'. It is not used anywhere outside execute.c now.
> + .dump_msgpack = port_sql_dump_msgpack,
> + .dump_msgpack_16 = NULL,
> + .dump_lua = NULL,
> + .dump_plain = NULL,
> + .destroy = port_sql_destroy,
6. Not sure why, but in new code we do not explicitly
write attribute names. Instead, we use
/* .method_name = */ func,
not
.method_name = func,
To be honest I like your way and how it was done in old
code, but Vladimir once forced me to use /* ... = */, so
lets follow.
> +};
> +
> +/**
> * Return a string name of a parameter marker.
> * @param Bind to get name.
> * @retval Zero terminated name.
> @@ -356,6 +386,11 @@ sql_row_to_port(struct sqlite3_stmt *stmt, int column_count,
> if (tuple == NULL)
> goto error;
> region_truncate(region, svp);
> + /*
> + * The port_tuple_add function can be used with port_sql,
> + * since it does not call any port_tuple methods and works
> + * only with fields.
7. It does not matter even if it would call port_tuple_methods.
As we stated in port_sql comment, we derived from port_tuple and
can use it in-place of base. I do not think this particular place
should be commented at all.
> + */
> return port_tuple_add(port, tuple);
>
> error:
> commit 8390b09d95aa5bcaa6ef89924d9a334dff746f19
> Author: Mergen Imeev <imeevma@gmail.com>
> Date: Fri Dec 21 14:52:03 2018 +0300
>
> iproto: create port_sql
>
> This patch creates port_sql implementation for the port. This will
> allow us to dump sql responses to obuf or to Lua. Also this patch
> defines methods dump_msgpack() and destroy() of port_sql.
>
> Part of #3505
>
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 38b6cbc..c6fcb50 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -575,26 +606,19 @@ err:
> rc = -1;
> goto finish;
> }
> - size = mp_sizeof_uint(IPROTO_DATA) +
> - mp_sizeof_array(port_tuple->size);
> + size = mp_sizeof_uint(IPROTO_DATA);
> pos = (char *) obuf_alloc(out, size);
> if (pos == NULL) {
> diag_set(OutOfMemory, size, "obuf_alloc", "pos");
> goto err;
> }
> pos = mp_encode_uint(pos, IPROTO_DATA);
> - pos = mp_encode_array(pos, port_tuple->size);
> - /*
> - * Just like SELECT, SQL uses output format compatible
> - * with Tarantool 1.6
> - */
> - if (port_dump_msgpack_16(&response->port, out) < 0) {
> - /* Failed port dump destroyes the port. */
> + /* Calling BaseStruct::methods via its vtab. */
8. Lets do not repeat this comment on each port_tuple_vtab usage. It
it direct repetition of what is said in port_sql comment.
> + if (port_tuple_vtab.dump_msgpack(port, out) < 0)
> goto err;
> - }
> } else {
> int keys = 1;
> - assert(port_tuple->size == 0);
> + assert(((struct port_tuple *)port)->size == 0);
> struct stailq *autoinc_id_list =
> vdbe_autoinc_id_list((struct Vdbe *)stmt);
> uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
> @@ -643,7 +667,63 @@ err:
> +
> +static inline int
> +sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
> + struct region *region)
9. The function is not so trivial to omit a comment. Please, write it.
In particular what a one should do with region after sql_execute() is
finished. Should it be cleared?
> +{
> + int rc, column_count = sqlite3_column_count(stmt);
> + if (column_count > 0) {
> + /* Either ROW or DONE or ERROR. */
> + while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) {
> + if (sql_row_to_port(stmt, column_count, region,
> + port) != 0)
> + return -1;
> + }
> + assert(rc == SQLITE_DONE || rc != SQLITE_OK);
> + } else {
> + /* No rows. Either DONE or ERROR. */
> + rc = sqlite3_step(stmt);
> + assert(rc != SQLITE_ROW && rc != SQLITE_OK);
> + }
> + if (rc != SQLITE_DONE) {
> + diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
> + return -1;
> + }
> + 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 sqlite3_stmt *stmt;
> + sqlite3 *db = sql_get();
> + if (db == NULL) {
> + diag_set(ClientError, ER_LOADING);
> + return -1;
10. Not sure if it is possible. Is it?
> + }
> + if (sqlite3_prepare_v2(db, sql, len, &stmt, NULL) != SQLITE_OK) {
> + diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
> + return -1;
> + }
> + assert(stmt != NULL);
> + port_sql_create(port, stmt);
> + if (sql_bind(stmt, bind, bind_count) == 0 &&
> + sql_execute(db, stmt, port, region) == 0)
> + return 0;
> + port_destroy(port);
> + return -1;
> +}
> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index a08c8c5..9dc0462 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -1645,7 +1645,7 @@ tx_process_sql(struct cmsg *m)
> /* Prepare memory for the iproto header. */
> if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0)
> goto error;
> - if (sql_response_dump(&response, out) != 0) {
> + if (port_dump_msgpack(&port, out) != 0) {
> obuf_rollback_to_svp(out, &header_svp);
> goto error;
11. Port leaks if an error occurred in iproto_prepare_header.
> }
Sorry, most of comments are minor and I could fix them
myself, but I was asked to do not rewrite patches if they
are not urgent.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] Re: [PATCH v6 3/5] lua: create method dump_lua for port_sql
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 3/5] lua: create method dump_lua for port_sql imeevma
@ 2018-12-29 13:19 ` Vladislav Shpilevoy
0 siblings, 0 replies; 9+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-29 13:19 UTC (permalink / raw)
To: tarantool-patches, imeevma
Thanks for the patch! See 6 comments below.
On 28/12/2018 21:11, imeevma@tarantool.org wrote:
> Hi! Thank you for review! My answers, diff between versions and
> new version below.
>
> On 12/25/18 5:57 PM, Vladislav Shpilevoy wrote:
>> Thanks for the patch! See 7 comments below.
>
>> 1. Use lua_pushstring.
> Fixed.
>
>> 2. It is strange, that this function creates a table and sets it
>> as a member of some externally created table. Please, move this line
>> to a caller function.
> Fixed.
>
>> 3. Just inline keys 2.
> Fixed.
>
>> 4. Why 1 array element? And why do you separate fixed/optional keys?
>> lua_createtable does not take statis/dynamic key count. It takes
>> array/map key count.
> Fixed. Now it takes 1 or 2 as map key count.
>
>> 5. I think, port_sql_destroy should call sqlite3_finalize.
> Fixed in patch #2.
>
>> 6. Impossible. It isn't?
> Removed.
>
>> 7. Please, start message with a capital letter.
> Fixed.
Please, next time write answered inlined into an original
email. With bare comments/answered without a context code it
is hard to remember what was wrong.
> commit 74f30d133b34ea86dc145f6ffee347abfd489cf2
> Author: Mergen Imeev <imeevma@gmail.com>
> Date: Fri Dec 21 16:48:37 2018 +0300
>
> lua: create method dump_lua for port_sql
>
> This patch creates the dump_lua method for port_sql. It also
> defines a new function box.sql.new_execute(), which will be
> converted to box.sql.execute().
>
> Part of #3505
>
> diff --git a/src/box/execute.c b/src/box/execute.c
> index c6fcb50..4606239 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -102,12 +103,14 @@ static_assert(sizeof(struct port_sql) <= sizeof(struct port),
> static int
> port_sql_dump_msgpack(struct port *port, struct obuf *out);
> static void
> +port_sql_dump_lua(struct port *port, struct lua_State *L);
1. Please, add empty lines between declarations, and put
a comment above a first function appearance.
> +static void
> port_sql_destroy(struct port *base);
>
> const struct port_vtab port_sql_vtab = {
> .dump_msgpack = port_sql_dump_msgpack,
> .dump_msgpack_16 = NULL,
> - .dump_lua = NULL,
> + .dump_lua = port_sql_dump_lua,
> .dump_plain = NULL,
> .destroy = port_sql_destroy,
> };
> @@ -671,6 +674,87 @@ finish:
> return rc;
> }
>
> +/**
> + * Push a metadata of the prepared statement to Lua stack.
2. Not sure if 'metadata' word is enumerable and can be
accompanied by article 'a'.
> + *
> + * @param stmt Prepared statement.
> + * @param L Lua stack.
> + * @param column_count Statement's column count.
> + */
> +static inline void
> +lua_sql_get_description(struct sqlite3_stmt *stmt, struct lua_State *L,
> + int column_count)
> +{
> + assert(column_count > 0);
> + lua_createtable(L, column_count, 0);
> + for (int i = 0; i < column_count; ++i) {
> + lua_createtable(L, 2, 0);
3. Read carefully what parameters of lua_createtable mean.
First parameter is how many indexed elements preallocate
for indexes [1, N]. Second parameter is how many dictionary
key/value pairs preallocate.
Here table is going to be {name = ..., type = ...} = 2 dictionary
values. So it is lua_createtable(L, 0, 2), not (L, 2, 0).
> + const char *name = sqlite3_column_name(stmt, i);
> + const char *type = sqlite3_column_datatype(stmt, i);
> + /*
> + * Can not fail, since all column names are
> + * preallocated during prepare phase and the
> + * column_name simply returns them.
> + */
> + assert(name != NULL);
4. I guess, type != NULL as well.
> + lua_pushstring(L, name);
> + lua_setfield(L, -2, "name");
> + lua_pushstring(L, type);
> + lua_setfield(L, -2, "type");
> + lua_rawseti(L, -2, i + 1);
> + }
> +}
> +
> +/**
> + * Dump a built response into Lua stack. The response is
> + * destroyed.
> + *
> + * @param port port with EXECUTE response.
> + * @param L Lua stack.
> + */
> +static void
> +port_sql_dump_lua(struct port *port, struct lua_State *L)
> +{
> + assert(port->vtab == &port_sql_vtab);
> + sqlite3 *db = sql_get();
> + struct sqlite3_stmt *stmt = ((struct port_sql *)port)->stmt;
> + int column_count = sqlite3_column_count(stmt);
> + if (column_count > 0) {
> + lua_createtable(L, 0, 2);
> + lua_sql_get_description(stmt, L, column_count);
> + lua_setfield(L, -2, "metadata");
> + /*
> + * We can use the port_tuple methods, since
> + * port_sql was inherited from it.
5. Drop this comment please, everywhere.
> + */
> + port_tuple_vtab.dump_lua(port, L);
> + lua_setfield(L, -2, "rows");
> + } else {
> + assert(((struct port_tuple *)port)->size == 0);
> + struct stailq *autoinc_id_list =
> + vdbe_autoinc_id_list((struct Vdbe *)stmt);
> + lua_createtable(L, 0, stailq_empty(autoinc_id_list) ? 1 : 2);
> +
> + luaL_pushuint64(L, db->nChange);
> + lua_setfield(L, -2, "rowcount");
> +
> + if (!stailq_empty(autoinc_id_list)) {
> + lua_newtable(L);
> + int i = 1;
> + struct autoinc_id_entry *id_entry;
> + stailq_foreach_entry(id_entry, autoinc_id_list, link) {
> + if (id_entry->id >= 0)
> + luaL_pushuint64(L, id_entry->id);
> + else
> + luaL_pushint64(L, id_entry->id);
> + lua_rawseti(L, -2, i++);
> + }
> + lua_setfield(L, -2, "autoincrement_ids");
> + }
> + }
> + port_destroy(port);
> +}
> +
> static void
> port_sql_destroy(struct port *base)
> {
> diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
> index 6923666..a55566e 100644
> --- a/src/box/lua/sql.c
> +++ b/src/box/lua/sql.c
> @@ -111,6 +112,28 @@ sqlerror:
> }
>
> static int
> +lbox_execute(struct lua_State *L)
> +{
> + struct sql_bind *bind = NULL;
> + int bind_count = 0;
> + size_t length;
> + struct port port;
> +
> + if (lua_type(L, 1) != LUA_TSTRING)
> + return luaL_error(L, "Usage: box.execute(sqlstring)");
6. Maybe just add here ' || ! lua_isstring(L, 1)' and remove the
check for 'sql == NULL' below?
> +
> + const char *sql = lua_tolstring(L, 1, &length);
> + if (sql == NULL)
> + return luaL_error(L, "Usage: box.execute(sqlstring)");
> +
> + if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
> + &fiber()->gc) != 0)
> + return luaT_error(L);
> + port_dump_lua(&port, L);
> + return 1;
> +}
> +
> +static int
> lua_sql_debug(struct lua_State *L)
> {
> struct info_handler info;
^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] Re: [PATCH v6 4/5] lua: parameter binding for new execute()
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 4/5] lua: parameter binding for new execute() imeevma
@ 2018-12-29 13:19 ` Vladislav Shpilevoy
0 siblings, 0 replies; 9+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-29 13:19 UTC (permalink / raw)
To: tarantool-patches, imeevma
Thanks for the patch! See 10 comments below.
On 28/12/2018 21:11, imeevma@tarantool.org wrote:
> Hi! Thank you for review! My answers, diff between versions and
> new version below.
>
>
> On 12/25/18 5:57 PM, Vladislav Shpilevoy wrote:
>> Thanks for the patch! See 9 comments below.
>
>> 1. It is not msgpack.
> Fixed.
>
>> 2. In Lua syntax {'name' : value} is incorrect.
> Fixed.
>
>> 3. Just -1. Stack indexes can be negative.
> lua_next() throws an assert when I change arg value to -1:
>
> tarantool: lj_api.c:892: lua_next: Assertion `(((t)->it) == (~11u))' failed.
It is suspicious. I looked at lua_next source and it does
convert negative index to positive with ease. Also, it is used
already in cosole.c, space.cc, httpc. and other places.
I tried it myself, and -2 value worked for me. Yes, -1 was
incorrect, my fault.
>
>> 4. Please, do not pop the value stored in a luaL_field. Pop 2
>> values at one time below, on line 294.
> Fixed. Actually, I found out that my popping policy was quite a
> mess in previous version. Now everything will be popped at the
> end of the function.
>
>> 5. As I know, luaL_checklstring never returns NULL. It throws. Please,
>> check for string type explicitly and throw normal usage error.
> Fixed.
>
>> 6. As I said above, you've already popped it.
> Fixed.
>
>> 7. Please, check for overflow.
> Fixed.
>
>> 8. To be honest, I feel utterly uncomfortable with luaL_field. IMO,
>> this vast structure is too overloaded. Can you please avoid its usage?
>> Almost nothing will change as I see. For switch on types you can use
>> lua_type().
> After some discussion it was decided to left it as it is now. The
> main reason for this is that if we remove luaL_field() than this
> function become downgraded version of luaL_field().
>
>> 9. Not msgpack. Please, do not blindly copy-paste code. Check out the
>> commit more accurately. I will not point out other places.
> Fixed. Moved this check to sql.c.
>
>
> Diff between versions:
>
> commit 93e4ea0ffcc195c4a904dab99c5cc516db33fa44
> Author: Mergen Imeev <imeevma@gmail.com>
> Date: Thu Dec 27 17:00:51 2018 +0300
>
> Temporary: review fixes
>
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 457403b..ade6e6c 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -278,43 +278,43 @@ static inline int
> lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
> {
> struct luaL_field field;
> + struct region *region = &fiber()->gc;
> char *buf;
> lua_rawgeti(L, idx, i + 1);
> luaL_tofield(L, luaL_msgpack_default, -1, &field);
> bind->pos = i + 1;
> if (field.type == MP_MAP) {
> /*
> - * A named parameter is an MP_MAP with
> - * one key - {'name': value}.
> - * Report parse error otherwise.
> + * A named parameter is a single-row table that
> + * contains one value associated with a key. The
> + * key must be a string. This key is the name of
> + * the parameter, and the value associated with
> + * the key is the value of the parameter.
1. Or just change MP_MAP to table, and {'name': value} to {name = value},
and in other places too. But it is up to you. Your comment is ok as well.
> */
> if (field.size != 1) {
> diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
> - "parameter should be {'name': value}");
> + "named parameter should be a single-row "\
> + "table with one key-value pair");
> return -1;
> }
> /*
> - * Get key and value of the only map element to
> + * Get key and value of the only table element to
> * lua stack.
> */
> lua_pushnil(L);
> lua_next(L, lua_gettop(L) - 1);
> - /* At first we should deal with the value. */
> - luaL_tofield(L, luaL_msgpack_default, -1, &field);
> - lua_pop(L, 1);
> - /* Now key is on the top of Lua stack. */
> - size_t name_len = 0;
> - bind->name = luaL_checklstring(L, -1, &name_len);
> - if (bind->name == NULL) {
> - diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
> - "parameter should be {'name': value}");
> + if (lua_type(L, -2) != LUA_TSTRING) {
2. Why so complex? Why not "! lua_isstring()" ?
> + diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\
> + "parameter should be a string.");
> return -1;
> }
> + size_t name_len;
> + bind->name = lua_tolstring(L, -2, &name_len);
> /*
> * Name should be saved in allocated memory as it
> * will be poped from Lua stack.
> */
> - buf = region_alloc(&fiber()->gc, name_len + 1);
> + buf = region_alloc(region, name_len + 1);
> if (buf == NULL) {
> diag_set(OutOfMemory, name_len + 1, "region_alloc",
> "buf");
> @@ -323,13 +323,18 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
> memcpy(buf, bind->name, name_len + 1);
> bind->name = buf;
> bind->name_len = name_len;
> - lua_pop(L, 1);
> + luaL_tofield(L, luaL_msgpack_default, -1, &field);
> } else {
> bind->name = NULL;
> bind->name_len = 0;
> }
> switch (field.type) {
> case MP_UINT: {
> + if (field.ival < 0) {
> + diag_set(ClientError, ER_SQL_BIND_VALUE,
> + sql_bind_name(bind), "INTEGER");
> + return -1;
> + }
3. How is it possible? Lua field reports that it is an unsigned
integer. If it is a way of checking for overflow, then please,
make it more straightforward. I looked into the source of
luaL_tofield and I saw that ival stores uint64_t just casting it
to int64_t. So here you should cast it back and check if it
is > INT64_MAX.
(uint64_t) field.ival > INT64_MAX
Also, you do not need braces {} for 'case MP_UINT'.
> bind->i64 = field.ival;
> bind->type = SQLITE_INTEGER;
> bind->bytes = sizeof(bind->i64);
> commit a1fe2aa7edc82097b2fcf63a41fb7fa2145b9d3c
> Author: Mergen Imeev <imeevma@gmail.com>
> Date: Fri Dec 21 17:54:16 2018 +0300
>
> lua: parameter binding for new execute()
>
> This patch defines parameters binding for SQL statements executed
> through box.
>
> Part of #3505
> Closes #3401
>
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 4606239..ade6e6c 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -261,6 +262,196 @@ sql_bind_list_decode(const char *data, struct sql_bind **out_bind)
> return bind_count;
> }
>
> +
> +/**
> + * Decode a single bind column from Lua stack.
> + *
> + * @param L Lua stack.
> + * @param[out] bind Bind to decode to.
> + * @param idx Position of table with bind columns on Lua stack.
> + * @param i Ordinal bind number.
> + *
> + * @retval 0 Success.
> + * @retval -1 Memory or client error.
> + */
> +static inline int
> +lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
> +{
> + struct luaL_field field;
> + struct region *region = &fiber()->gc;
> + char *buf;
> + lua_rawgeti(L, idx, i + 1);
> + luaL_tofield(L, luaL_msgpack_default, -1, &field);
4. I remembered why I do not like luaL_field. When it meets a
non-scalar value like map, it traverses it without necessity.
Lets refactor it a bit.
diff --git a/src/box/execute.c b/src/box/execute.c
index ade6e6c95..3a45fc524 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -281,33 +281,28 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
struct region *region = &fiber()->gc;
char *buf;
lua_rawgeti(L, idx, i + 1);
- luaL_tofield(L, luaL_msgpack_default, -1, &field);
bind->pos = i + 1;
- if (field.type == MP_MAP) {
- /*
- * A named parameter is a single-row table that
- * contains one value associated with a key. The
- * key must be a string. This key is the name of
- * the parameter, and the value associated with
- * the key is the value of the parameter.
- */
- if (field.size != 1) {
- diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
- "named parameter should be a single-row "\
- "table with one key-value pair");
- return -1;
- }
+ if (lua_istable(L, -1)) {
/*
* Get key and value of the only table element to
* lua stack.
*/
lua_pushnil(L);
- lua_next(L, lua_gettop(L) - 1);
+ lua_next(L, -2);
if (lua_type(L, -2) != LUA_TSTRING) {
diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\
"parameter should be a string.");
return -1;
}
+ /* Check that the table is one-row sized. */
+ lua_pushvalue(L, -2);
+ if (lua_next(L, -4) != 0) {
+ diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
+ "named parameter should be a single-row "\
+ "table with one key-value pair");
+ return -1;
+ }
+ lua_pop(L, 1);
size_t name_len;
bind->name = lua_tolstring(L, -2, &name_len);
/*
@@ -323,11 +318,11 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
memcpy(buf, bind->name, name_len + 1);
bind->name = buf;
bind->name_len = name_len;
- luaL_tofield(L, luaL_msgpack_default, -1, &field);
} else {
bind->name = NULL;
bind->name_len = 0;
}
+ luaL_tofield(L, luaL_msgpack_default, -1, &field);
switch (field.type) {
case MP_UINT: {
if (field.ival < 0) {
Here I removed common traversal and added an explicit
check that a table consists of one key-value pair.
Are you ok with these changes? If you are - squash. But
I did not test it though.
> + bind->pos = i + 1;
> + if (field.type == MP_MAP) {
> + /*
> + * A named parameter is a single-row table that
> + * contains one value associated with a key. The
> + * key must be a string. This key is the name of
> + * the parameter, and the value associated with
> + * the key is the value of the parameter.
> + */
5. Are you sure this comment is necessary? It just repeats the
error message below.
> + if (field.size != 1) {
> + diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\
> + "named parameter should be a single-row "\
> + "table with one key-value pair");
> + return -1;
> + }
> + /*
> + * Get key and value of the only table element to
> + * lua stack.
> + */
> + lua_pushnil(L);
> + lua_next(L, lua_gettop(L) - 1);
> + if (lua_type(L, -2) != LUA_TSTRING) {
> + diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\
> + "parameter should be a string.");
> + return -1;
> + }
> + size_t name_len;
> + bind->name = lua_tolstring(L, -2, &name_len);
> + /*
> + * Name should be saved in allocated memory as it
> + * will be poped from Lua stack.
> + */
> + buf = region_alloc(region, name_len + 1);
> + if (buf == NULL) {
> + diag_set(OutOfMemory, name_len + 1, "region_alloc",
> + "buf");
> + return -1;
> + }
> + memcpy(buf, bind->name, name_len + 1);
> + bind->name = buf;
> + bind->name_len = name_len;
> + luaL_tofield(L, luaL_msgpack_default, -1, &field);
> + } else {
> + bind->name = NULL;
> + bind->name_len = 0;
> + }
> + switch (field.type) {
> + case MP_UINT: {
> + if (field.ival < 0) {
> + diag_set(ClientError, ER_SQL_BIND_VALUE,
> + sql_bind_name(bind), "INTEGER");
> + return -1;
> + }
> + bind->i64 = field.ival;
> + bind->type = SQLITE_INTEGER;
> + bind->bytes = sizeof(bind->i64);
> + break;
> + }
> + case MP_INT:
> + bind->i64 = field.ival;
> + bind->type = SQLITE_INTEGER;
> + bind->bytes = sizeof(bind->i64);
> + break;
6. How about this?
diff --git a/src/box/execute.c b/src/box/execute.c
index ade6e6c95..c1b06b345 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -335,11 +335,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
sql_bind_name(bind), "INTEGER");
return -1;
}
- bind->i64 = field.ival;
- bind->type = SQLITE_INTEGER;
- bind->bytes = sizeof(bind->i64);
- break;
- }
+ FALLTHROUGH;
case MP_INT:
bind->i64 = field.ival;
bind->type = SQLITE_INTEGER;
> + case MP_STR:
> + /*
> + * Data should be saved in allocated memory as it
> + * will be poped from Lua stack.
> + */
> + buf = region_alloc(region, field.sval.len + 1);
> + if (buf == NULL) {
> + diag_set(OutOfMemory, field.sval.len + 1,
> + "region_alloc", "buf");
> + return -1;
> + }
> + memcpy(buf, field.sval.data, field.sval.len + 1);
> + bind->s = buf;
> + bind->type = SQLITE_TEXT;
> + bind->bytes = field.sval.len;
> + break;
> + case MP_DOUBLE:
> + bind->d = field.dval;
> + bind->type = SQLITE_FLOAT;
> + bind->bytes = sizeof(bind->d);
> + break;
> + case MP_FLOAT:
> + bind->d = field.dval;
> + bind->type = SQLITE_FLOAT;
> + bind->bytes = sizeof(bind->d);
> + break;
7.
diff --git a/src/box/execute.c b/src/box/execute.c
index ade6e6c95..fb2e5ea29 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -362,10 +362,6 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
bind->bytes = field.sval.len;
break;
case MP_DOUBLE:
- bind->d = field.dval;
- bind->type = SQLITE_FLOAT;
- bind->bytes = sizeof(bind->d);
- break;
case MP_FLOAT:
bind->d = field.dval;
bind->type = SQLITE_FLOAT;
> + case MP_NIL:
> + bind->type = SQLITE_NULL;
> + bind->bytes = 1;
> + break;
> + case MP_BOOL:
> + /* SQLite doesn't support boolean. Use int instead. */
> + bind->i64 = field.bval ? 1 : 0;
> + bind->type = SQLITE_INTEGER;
> + bind->bytes = sizeof(bind->i64);
> + break;
> + case MP_BIN:
> + bind->s = mp_decode_bin(&field.sval.data, &bind->bytes);
> + bind->type = SQLITE_BLOB;
> + break;
> + case MP_EXT:
> + /*
> + * Data should be saved in allocated memory as it
> + * will be poped from Lua stack.
> + */
> + buf = region_alloc(region, sizeof(field));
> + if (buf == NULL) {
> + diag_set(OutOfMemory, sizeof(field), "region_alloc",
> + "buf");
> + return -1;
> + }
> + memcpy(buf, &field, sizeof(field));
8. Why? What a point of copying struct luaL_field onto region? As I
see, MP_EXT is set for unknown cdata like just void *, and the same
for unknown userdata. When void * is met, we do not even know its
size, nor what a value it stores. When we meet userdata, we can learn
its size, but we do not know what a value is stored here and can not
decode it.
Please, set and error when such thing happens.
> + bind->s = buf;
> + bind->bytes = sizeof(field);
> + bind->type = SQLITE_BLOB;
> + break;
> + case MP_ARRAY:
> + diag_set(ClientError, ER_SQL_BIND_TYPE, "ARRAY",
> + sql_bind_name(bind));
> + return -1;
> + case MP_MAP:
> + diag_set(ClientError, ER_SQL_BIND_TYPE, "MAP",
> + sql_bind_name(bind));
> + return -1;
> + default:
> + unreachable();
> + }
> + lua_pop(L, lua_gettop(L) - idx);
> + return 0;
> +}
9. Also, I noticed one more problem with luaL_tofield. It can
deliberately throw during a number decoding, see at CHECK_NUMBER
macro. Moreover, it throws during table/map traversing in
lua_field_inspect_table. I once tried to remove these exceptions
but failed.
Here on a throw region leaks.
Also port_sql_dump_lua can throw during pushing data onto the stack
and the whole port_sql leaks.
Both problems can be solved using lua_cpcall, but please, ask in
the server team chat if it is worth doing. As I remember, Kostja
says 'let it crash' in such cases, but if a caller will use
pcall(box.execute)
then it will not crash, but just leak.
> diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
> index a55566e..20ba43b 100644
> --- a/src/box/lua/sql.c
> +++ b/src/box/lua/sql.c
> @@ -118,13 +118,22 @@ lbox_execute(struct lua_State *L)
> int bind_count = 0;
> size_t length;
> struct port port;
> + int top = lua_gettop(L);
>
> - if (lua_type(L, 1) != LUA_TSTRING)
> - return luaL_error(L, "Usage: box.execute(sqlstring)");
> + if ((top != 1 && top != 2) || lua_type(L, 1) != LUA_TSTRING)
> + return luaL_error(L, "Usage: box.execute(sqlstring[, params])");
>
> const char *sql = lua_tolstring(L, 1, &length);
> if (sql == NULL)
> - return luaL_error(L, "Usage: box.execute(sqlstring)");
> + return luaL_error(L, "Usage: box.execute(sqlstring[, params])");
10. How can it be NULL, if you have checked above that here a string
is stored?
> +
> + if (top == 2) {
> + if (! lua_istable(L, 2))
> + return luaL_error(L, "Second argument must be a table");
> + bind_count = lua_sql_bind_list_decode(L, &bind, 2);
> + if (bind_count < 0)
> + return luaT_error(L);
> + }
>
> if (sql_prepare_and_execute(sql, length, bind, bind_count, &port,
> &fiber()->gc) != 0)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-12-29 13:19 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-28 18:11 [tarantool-patches] [PATCH v6 0/5] sql: remove box.sql.execute imeevma
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 1/5] iproto: move map creation to sql_response_dump() imeevma
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 2/5] iproto: create port_sql imeevma
2018-12-29 13:19 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 3/5] lua: create method dump_lua for port_sql imeevma
2018-12-29 13:19 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 4/5] lua: parameter binding for new execute() imeevma
2018-12-29 13:19 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-28 18:11 ` [tarantool-patches] [PATCH v6 5/5] sql: check new box.sql.execute() imeevma
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox