[tarantool-patches] Re: [PATCH] sql: make SQL_BIND optional in an iproto request

[Alexander Turenko]

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.