From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id AAA8D250F6 for ; Tue, 15 Jan 2019 11:10:10 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id vSBe_WySrcJW for ; Tue, 15 Jan 2019 11:10:10 -0500 (EST) Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 0293E250AF for ; Tue, 15 Jan 2019 11:10:09 -0500 (EST) From: imeevma@tarantool.org Subject: [tarantool-patches] [PATCH v7 3/6] iproto: create port_sql Date: Tue, 15 Jan 2019 19:10:08 +0300 Message-Id: <1a3007030ea54fa81533acbf7421f27a2399c941.1547565887.git.imeevma@gmail.com> In-Reply-To: References: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, v.shpilevoy@tarantool.org Hi! Thank you for review! My answers, diff between versions and new patch below. On 12/29/18 4:19 PM, Vladislav Shpilevoy wrote: > 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'. > Changed comment. I think that should be 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 >> 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. > Fixed. >> + * >> + * 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. > Fixed. >> + */ >> +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. > Fixed. >> +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. You are right, fixed. > > Also, please, move port_sql_create here to gather all methods > in one place. > Fixed. >> + >> +const struct port_vtab port_sql_vtab = { > > 5. 'static'. It is not used anywhere outside execute.c now. > Fixed. >> + .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. > Fixed. >> +}; >> + >> +/** >> * 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. > Removed all such comments with exception of one before definition of struct port_sql. >> + */ >> return port_tuple_add(port, tuple); >> error: >> commit 8390b09d95aa5bcaa6ef89924d9a334dff746f19 >> Author: Mergen Imeev >> 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. > Fixed. >> + 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? > Added description. >> +{ >> + 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? > Removed. >> + } >> + 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. > Fixed. >> } > > 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. Diff between versions: commit 20864423db67a3322e203cb69d910d3979d63c2b Author: Mergen Imeev Date: Wed Jan 9 17:36:15 2019 +0300 Temporary: review fix diff --git a/src/box/execute.c b/src/box/execute.c index c6fcb50..a81f482 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -83,12 +83,15 @@ struct sql_bind { }; /** - * Port implementation used for dump tuples, stored in port_tuple, - * to obuf or Lua. + * 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. * - * 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. + * 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. */ @@ -96,22 +99,76 @@ struct port_sql { /* 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)"); +/** + * Dump data from port to buffer. Data in port contains tuples, + * metadata, or information obtained from an executed SQL query. + * 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 that contains SQL response. + * @param[out] out Output buffer. + * + * @retval 0 Success. + * @retval -1 Memory error. + */ static int port_sql_dump_msgpack(struct port *port, struct obuf *out); + static void -port_sql_destroy(struct port *base); +port_sql_destroy(struct port *base) +{ + port_tuple_vtab.destroy(base); + sqlite3_finalize(((struct port_sql *)base)->stmt); +} -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, +static 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, }; +static void +port_sql_create(struct port *port, struct sqlite3_stmt *stmt) +{ + port_tuple_create(port); + ((struct port_sql *)port)->stmt = stmt; + port->vtab = &port_sql_vtab; +} + /** * Return a string name of a parameter marker. * @param Bind to get name. @@ -386,11 +443,6 @@ 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: @@ -522,6 +574,7 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out, * column_name simply returns them. */ assert(name != NULL); + assert(type != NULL); size += mp_sizeof_str(strlen(name)); size += mp_sizeof_str(strlen(type)); char *pos = (char *) obuf_alloc(out, size); @@ -538,53 +591,6 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out, return 0; } -static void -port_sql_create(struct port *port, struct sqlite3_stmt *stmt) -{ - port_tuple_create(port); - ((struct port_sql *)port)->stmt = stmt; - port->vtab = &port_sql_vtab; -} - -/** - * 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) { @@ -613,7 +619,6 @@ 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 { @@ -671,14 +676,21 @@ finish: 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); -} - +/** + * 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[out] port Port to store SQL response. + * @param region Region to allocate temporary objects. + * + * @retval 0 Success. + * @retval -1 Error. + */ static inline int sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port, struct region *region) @@ -711,10 +723,6 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind, { 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; diff --git a/src/box/execute.h b/src/box/execute.h index c90789f..1ed9ab5 100644 --- a/src/box/execute.h +++ b/src/box/execute.h @@ -74,7 +74,7 @@ sql_bind_list_decode(const char *data, struct sql_bind **out_bind); * @param len Length of @a sql. * @param bind Array of parameters. * @param bind_count Length of @a bind. - * @param[out] response Response to store result. + * @param[out] port Port to store SQL response. * @param region Runtime allocator for temporary objects * (columns, tuples ...). * diff --git a/src/box/iproto.cc b/src/box/iproto.cc index 9dc0462..09e8914 100644 --- a/src/box/iproto.cc +++ b/src/box/iproto.cc @@ -1643,8 +1643,10 @@ tx_process_sql(struct cmsg *m) out = msg->connection->tx.p_obuf; struct obuf_svp header_svp; /* Prepare memory for the iproto header. */ - if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0) + if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0) { + port_destroy(&port); goto error; + } if (port_dump_msgpack(&port, out) != 0) { obuf_rollback_to_svp(out, &header_svp); goto error; New version: commit 1a3007030ea54fa81533acbf7421f27a2399c941 Author: Mergen Imeev 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..a81f482 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -83,6 +83,93 @@ struct sql_bind { }; /** + * 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 sqlite3_stmt *stmt; +}; + +static_assert(sizeof(struct port_sql) <= sizeof(struct port), + "sizeof(struct port_sql) must be <= sizeof(struct port)"); + +/** + * Dump data from port to buffer. Data in port contains tuples, + * metadata, or information obtained from an executed SQL query. + * 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 that contains SQL response. + * @param[out] out Output buffer. + * + * @retval 0 Success. + * @retval -1 Memory error. + */ +static int +port_sql_dump_msgpack(struct port *port, struct obuf *out); + +static void +port_sql_destroy(struct port *base) +{ + port_tuple_vtab.destroy(base); + sqlite3_finalize(((struct port_sql *)base)->stmt); +} + +static 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, +}; + +static void +port_sql_create(struct port *port, struct sqlite3_stmt *stmt) +{ + port_tuple_create(port); + ((struct port_sql *)port)->stmt = stmt; + port->vtab = &port_sql_vtab; +} + +/** * Return a string name of a parameter marker. * @param Bind to get name. * @retval Zero terminated name. @@ -487,6 +574,7 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out, * column_name simply returns them. */ assert(name != NULL); + assert(type != NULL); size += mp_sizeof_str(strlen(name)); size += mp_sizeof_str(strlen(type)); char *pos = (char *) obuf_alloc(out, size); @@ -503,63 +591,12 @@ 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 int +port_sql_dump_msgpack(struct port *port, struct obuf *out) { - 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 sql_response *response, - struct region *region) -{ - struct sqlite3_stmt *stmt; + assert(port->vtab == &port_sql_vtab); 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) -{ - 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 +612,18 @@ 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. */ + 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 +672,66 @@ err: } } finish: - port_destroy(&response->port); - sqlite3_finalize(stmt); + port_destroy(port); return rc; } + +/** + * 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[out] port Port to store SQL response. + * @param region Region to allocate temporary objects. + * + * @retval 0 Success. + * @retval -1 Error. + */ +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 (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..1ed9ab5 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,48 +69,12 @@ 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. * @param bind Array of parameters. * @param bind_count Length of @a bind. - * @param[out] response Response to store result. + * @param[out] port Port to store SQL response. * @param region Runtime allocator for temporary objects * (columns, tuples ...). * @@ -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..09e8914 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; /* @@ -1643,9 +1643,11 @@ tx_process_sql(struct cmsg *m) out = msg->connection->tx.p_obuf; struct obuf_svp header_svp; /* Prepare memory for the iproto header. */ - if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0) + if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0) { + port_destroy(&port); 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