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 0FF3E2071D for ; Tue, 25 Dec 2018 09:57:08 -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 AjKvdzcZl9ev for ; Tue, 25 Dec 2018 09:57:07 -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 B778520708 for ; Tue, 25 Dec 2018 09:57:07 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v5 2/5] iproto: create port_sql References: <9c825dd4f7e2c18c829553caeeca4f525916866c.1545477494.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Tue, 25 Dec 2018 17:57:05 +0300 MIME-Version: 1.0 In-Reply-To: <9c825dd4f7e2c18c829553caeeca4f525916866c.1545477494.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 Hi! Thanks for the patch! See 4 comments below. On 22/12/2018 14:31, imeevma@tarantool.org wrote: > 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 > --- > src/box/execute.c | 90 ++++++++++++++++++++++++++++++++++++++++++------------- > src/box/execute.h | 67 +++++++++++++++-------------------------- > src/box/iproto.cc | 6 ++-- > src/box/port.h | 1 - > 4 files changed, 96 insertions(+), 68 deletions(-) > > diff --git a/src/box/execute.c b/src/box/execute.c > index 38b6cbc..b07de28 100644 > --- a/src/box/execute.c > +++ b/src/box/execute.c > @@ -575,26 +609,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) 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++. > 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 +669,29 @@ err: > + > +void > +port_tuple_to_port_sql(struct port *port, struct sqlite3_stmt *stmt) > +{ > + assert(port->vtab == &port_tuple_vtab); > + ((struct port_sql *)port)->stmt = stmt; > + port->vtab = &port_sql_vtab; > +} 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. > + > +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, > +}; > diff --git a/src/box/execute.h b/src/box/execute.h > index 60b8f31..5aef546 100644 > --- a/src/box/execute.h > +++ b/src/box/execute.h > @@ -51,14 +51,31 @@ extern const char *sql_info_key_strs[]; > struct obuf; > struct region; > struct sql_bind; > +struct sqlite3_stmt; > > -/** Response on EXECUTE request. */ > -struct sql_response { > - /** Port with response data if any. */ > - struct port port; > - /** Prepared SQL statement with metadata. */ > - void *prep_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); 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. > > /** > * Parse MessagePack array of SQL parameters. > @@ -128,7 +109,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); 4. Do not forget about comments. It still describes 'response' parameter. > > #if defined(__cplusplus)