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 871DE2A9C8 for ; Tue, 26 Mar 2019 12:16:20 -0400 (EDT) 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 i8BAjmfIRbQk for ; Tue, 26 Mar 2019 12:16:20 -0400 (EDT) Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 A485F2AA58 for ; Tue, 26 Mar 2019 12:16:19 -0400 (EDT) Date: Tue, 26 Mar 2019 19:16:12 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] sql: make SQL_BIND optional in an iproto request Message-ID: <20190326161612.swlkn5zeirbyc5vi@tkn_work_nb> References: <416B71F5-C59B-4C35-9C4B-818BB1F766CE@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <416B71F5-C59B-4C35-9C4B-818BB1F766CE@tarantool.org> 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: "n.pettik" Cc: tarantool-patches@freelists.org, Kirill Yukhin CCed Kirill to proceed further. WBR, Alexander Turenko. On Tue, Mar 26, 2019 at 05:51:35PM +0300, n.pettik wrote: > Patch is OK as obvious. A few nits below. You could skip them though. > > > diff --git a/src/box/iproto.cc b/src/box/iproto.cc > > index 3b0ba6234..3c054338b 100644 > > --- a/src/box/iproto.cc > > +++ b/src/box/iproto.cc > > @@ -1622,8 +1622,8 @@ tx_process_sql(struct cmsg *m) > > struct iproto_msg *msg = tx_accept_msg(m); > > struct obuf *out; > > struct sql_response response; > > - struct sql_bind *bind; > > - int bind_count; > > + struct sql_bind *bind = NULL; > > + int bind_count = 0; > > const char *sql; > > uint32_t len; > > > > @@ -1633,7 +1633,8 @@ tx_process_sql(struct cmsg *m) > > goto error; > > assert(msg->header.type == IPROTO_EXECUTE); > > tx_inject_delay(); > > - bind_count = sql_bind_list_decode(msg->sql.bind, &bind); > > + if (msg->sql.bind != NULL) > > + bind_count = sql_bind_list_decode(msg->sql.bind, &bind); > > You may apply this diff (that’s matter of taste): > > diff --git a/src/box/iproto.cc b/src/box/iproto.cc > index 3c054338b..8934b7488 100644 > --- a/src/box/iproto.cc > +++ b/src/box/iproto.cc > @@ -1633,10 +1633,11 @@ tx_process_sql(struct cmsg *m) > goto error; > assert(msg->header.type == IPROTO_EXECUTE); > tx_inject_delay(); > - if (msg->sql.bind != NULL) > + if (msg->sql.bind != NULL) { > bind_count = sql_bind_list_decode(msg->sql.bind, &bind); > - if (bind_count < 0) > - goto error; > + if (bind_count < 0) > + goto error; > + } > sql = msg->sql.sql_text; > sql = mp_decode_str(&sql, &len); > if (sql_prepare_and_execute(sql, len, bind, bind_count, &response, > Applied. > > diff --git a/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua b/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua > > new file mode 100755 > > index 000000000..55804768c > > --- /dev/null > > +++ b/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua > > Mb it’s better to move it to sql/iproto.test.lua ? There are two points of taste here: 1. I prefer to use 'core = app' tests with TAP13 output format, because of two reasons: - I'm able to run it w/o test-run. It makes debugging easier in some cases. - Expected output is fixed within a test, so I don't need to switch between two files to understand a test case. 2. I prefer to use split test cases between files: - It also simplifies debugging. - And also help with understanding a case. As I see a developer always was free to choose btw core = app and core = tarantool. Splitting test cases to files are not consistent too, but mostly we don't do that (however there are voices for that and against both). Anyway, I'll rewrite it as you wish if you think it will give us some gains. Now I leave it as is.