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 D298222146 for ; Sat, 29 Dec 2018 08:19:36 -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 2g-d5euYAiLC for ; Sat, 29 Dec 2018 08:19:36 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 DEBE122134 for ; Sat, 29 Dec 2018 08:19:35 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v6 2/5] iproto: create port_sql References: <8390b09d95aa5bcaa6ef89924d9a334dff746f19.1546017932.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Sat, 29 Dec 2018 16:19:33 +0300 MIME-Version: 1.0 In-Reply-To: <8390b09d95aa5bcaa6ef89924d9a334dff746f19.1546017932.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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, imeevma@tarantool.org 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 > 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 > 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.