From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6712A4696C2 for ; Wed, 4 Dec 2019 01:51:17 +0300 (MSK) References: <10842a572a8b14dd8c96c258ffe372dda45416e5.1574277369.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <527d98cf-be26-edcc-f615-c5b9a2f7dd7c@tarantool.org> Date: Tue, 3 Dec 2019 23:51:15 +0100 MIME-Version: 1.0 In-Reply-To: <10842a572a8b14dd8c96c258ffe372dda45416e5.1574277369.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 07/16] port: add dump format and request type to port_sql List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , tarantool-patches@dev.tarantool.org Thanks for the patch! See 4 comments below. On 20/11/2019 22:28, Nikita Pettik wrote: > Dump formats of DQL and DML queries are different: the last one contains > number of affected rows and optionally list of autoincremented ids; the > first one comprises all meta-information including column names of > resulting set and their types. What is more, dump format is going to be > different for execute and prepare requests. So let's introduce separate > member to struct port_sql responsible for dump format to be used. > > What is more, prepared statement finalization is required only for > PREPARE-AND-EXECUTE requests. So let's keep request type in port as well. 1. I don't think it is a good idea to rely on certain meta being returned by only certain requests. There is a ticket, which will remove the border between different request types: https://github.com/tarantool/tarantool/issues/3649. Yes, now this is either sql_info or metadata, but that will change. > > Note that C standard specifies that enums are integers, but it does not > specify the size. Hence, let's use simple uint8 - mentioned enums are > small enough to fit into it. 2. Try this: enum ... : 8; > > Needed for #2592 > --- > src/box/execute.c | 32 +++++++++++++++++++++++--------- > src/box/execute.h | 24 ++++++++++++++++++++++++ > src/box/lua/execute.c | 20 +++++++++++++------- > 3 files changed, 60 insertions(+), 16 deletions(-) > > diff --git a/src/box/execute.c b/src/box/execute.c > index 83680b70f..d2a999099 100644 > --- a/src/box/execute.c > +++ b/src/box/execute.c > @@ -100,7 +100,9 @@ static void > port_sql_destroy(struct port *base) > { > port_tuple_vtab.destroy(base); > - sql_finalize(((struct port_sql *)base)->stmt); > + struct port_sql *port_sql = (struct port_sql *) base; > + if (port_sql->request == PREPARE_AND_EXECUTE) 3. You could turn the types into a bitmask to drop the dependency on a certain request type: bool do_finalize : 1; bool do_omit_data : 1; > + sql_finalize(((struct port_sql *)base)->stmt); > } > > const struct port_vtab port_sql_vtab = { > @@ -395,6 +404,9 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out) > mp_encode_int(buf, id_entry->id); > } > } > + break; > + } > + default: unreachable(); 4. Please, wrap on a new line, and include into {}. I know, this is shorter, but this just does not match the code style. In the other place too. > } > return 0; > }