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

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

[Alexander Turenko]

The documentation [1] says this field is optional. I don't know which
commit lead to the regression, only that 2.1.1-7-gd381a45b6 is good.

[1]: https://tarantool.io/en/doc/2.1/dev_guide/internals/sql_protocol/

Fixes #4077.
---

https://github.com/tarantool/tarantool/issues/4077
https://github.com/tarantool/tarantool/tree/Totktonada/gh-4077-iproto-execute-no-bind

 src/box/iproto.cc                             |  7 +-
 .../gh-4077-iproto-execute-no-bind.test.lua   | 69 +++++++++++++++++++
 2 files changed, 73 insertions(+), 3 deletions(-)
 create mode 100755 test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua

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);
 	if (bind_count < 0)
 		goto error;
 	sql = msg->sql.sql_text;
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
@@ -0,0 +1,69 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local net_box = require('net.box')
+local urilib = require('uri')
+local msgpack = require('msgpack')
+
+local IPROTO_REQUEST_TYPE       = 0x00
+local IPROTO_EXECUTE            = 0x0b
+local IPROTO_SYNC               = 0x01
+local IPROTO_SQL_TEXT           = 0x40
+local IPROTO_SQL_INFO           = 0x42
+local IPROTO_SQL_INFO_ROW_COUNT = 0x00
+local IPROTO_OK                 = 0x00
+local IPROTO_SCHEMA_VERSION     = 0x05
+local IPROTO_STATUS_KEY         = 0x00
+
+box.cfg({
+    listen = os.getenv('LISTEN') or 'localhost:3301',
+})
+
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
+box.sql.execute('create table T(ID int primary key)')
+
+local test = tap.test('gh-4077-iproto-execute-no-bind')
+test:plan(3)
+
+local uri = urilib.parse(box.cfg.listen)
+local sock = net_box.establish_connection(uri.host, uri.service)
+
+-- Send request w/o SQL_BIND field in body.
+local next_request_id = 16
+local header = msgpack.encode({
+    [IPROTO_REQUEST_TYPE] = IPROTO_EXECUTE,
+    [IPROTO_SYNC] = next_request_id,
+})
+local body = msgpack.encode({
+    [IPROTO_SQL_TEXT] = 'insert into T values (1)',
+})
+local size = msgpack.encode(header:len() + body:len())
+sock:write(size .. header .. body)
+
+-- Read response.
+local size = msgpack.decode(sock:read(5))
+local header_body = sock:read(size)
+local header, header_len = msgpack.decode(header_body)
+local body = msgpack.decode(header_body:sub(header_len))
+sock:close()
+
+-- Verify response.
+header[IPROTO_SCHEMA_VERSION] = nil -- expect any
+local exp_header = {
+    [IPROTO_STATUS_KEY] = IPROTO_OK,
+    [IPROTO_SYNC] = next_request_id,
+}
+local exp_body = {
+    [IPROTO_SQL_INFO] = {
+        [IPROTO_SQL_INFO_ROW_COUNT] = 1,
+    }
+}
+test:is_deeply(exp_header, header, 'verify response header')
+test:is_deeply(exp_body, body, 'verify response body')
+
+-- Verify space data.
+local exp_res = {{1}}
+local res = box.space.T:pairs():map(box.tuple.totable):totable()
+test:is_deeply(res, exp_res, 'verify inserted data')
+
+os.exit(test:check() == true and 0 or 1)
-- 
2.20.1

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

[n.pettik]

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,

> 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 ?

[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.

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

[Kirill Yukhin]

Hello,

On 26 Mar 02:20, Alexander Turenko wrote:
> The documentation [1] says this field is optional. I don't know which
> commit lead to the regression, only that 2.1.1-7-gd381a45b6 is good.
> 
> [1]: https://tarantool.io/en/doc/2.1/dev_guide/internals/sql_protocol/
> 
> Fixes #4077.

I've checked yout patch into master and 2.1 branches.

--
Regards, Kirill Yukhin