[tarantool-patches] Re: [PATCH v6 2/5] iproto: create port_sql

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch! See 11 comments below.

On 28/12/2018 21:11, imeevma@tarantool.org wrote:
> Hi! Thank you for review! My answers, diff between versions and
> new version below.
> 
> On 12/25/18 5:57 PM, Vladislav Shpilevoy wrote:
>> Hi! Thanks for the patch! See 4 comments below.
> 
>> 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++.
> Added. Not sure that it is enough.

Than add more explanations? Hardly it is possible to 'overexplain'
something, but it is as easy as nothing to 'underexplain'.

> 
>> 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.
> Done.
> 
>> 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.
> Done.
> 
>> 4. Do not forget about comments. It still describes 'response' parameter.
> Fixed.
> 
> 
> Diff between versions:
> 
> commit 414bf16c094fbb7b9e0ecb6b4b56f069f109ef86
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Wed Dec 26 22:00:35 2018 +0300
> 
>      Temporary: review fixes
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index b07de28..c6fcb50 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -83,6 +83,36 @@ struct sql_bind {
>   };
>   
>   /**
> + * Port implementation used for dump tuples, stored in port_tuple,
> + * to obuf or Lua.

1. Not only tuples, but the whole response, including headers,
meta, info.

> + *
> + * All port_tuple methods can be used with port_sql, since
> + * port_sql is a structure inherited from port_tuple. Calling
> + * port_tuple methods we are going via port_tuple_vtab.

2. Not sure if the sentence is correct. 'Calling we are going'
looks flawed. Maybe better 'Port_tuple methods are called via
explicit access to port_tuple_vtab just like C++ does with
BaseClass::method, when it is called in a child method'. ?
Or something like this.

> + */
> +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)");
> +
> +static int
> +port_sql_dump_msgpack(struct port *port, struct obuf *out);

3. Please, add empty line between function declarations. Also, we
usually write a comment above a first function declaration appearance,
so the whole comment about port_sql_dump_msgpack() should be put here.

> +static void
> +port_sql_destroy(struct port *base);

4. Correct me if I am wrong, but as I see, port_sql_destroy can be
implemented right here, without pre-announcement.

Also, please, move port_sql_create here to gather all methods
in one place.

> +
> +const struct port_vtab port_sql_vtab = {

5. 'static'. It is not used anywhere outside execute.c now.

> +	.dump_msgpack = port_sql_dump_msgpack,
> +	.dump_msgpack_16 = NULL,
> +	.dump_lua = NULL,
> +	.dump_plain = NULL,
> +	.destroy = port_sql_destroy,

6. Not sure why, but in new code we do not explicitly
write attribute names. Instead, we use

	/* .method_name = */ func,

not

	.method_name = func,

To be honest I like your way and how it was done in old
code, but Vladimir once forced me to use /* ... = */, so
lets follow.

> +};
> +
> +/**
>    * Return a string name of a parameter marker.
>    * @param Bind to get name.
>    * @retval Zero terminated name.
> @@ -356,6 +386,11 @@ sql_row_to_port(struct sqlite3_stmt *stmt, int column_count,
>   	if (tuple == NULL)
>   		goto error;
>   	region_truncate(region, svp);
> +	/*
> +	 * The port_tuple_add function can be used with port_sql,
> +	 * since it does not call any port_tuple methods and works
> +	 * only with fields.

7. It does not matter even if it would call port_tuple_methods.
As we stated in port_sql comment, we derived from port_tuple and
can use it in-place of base. I do not think this particular place
should be commented at all.

> +	 */
>   	return port_tuple_add(port, tuple);
>   
>   error:
> commit 8390b09d95aa5bcaa6ef89924d9a334dff746f19
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Fri Dec 21 14:52:03 2018 +0300
> 
>      iproto: create port_sql
>      
>      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
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 38b6cbc..c6fcb50 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -575,26 +606,19 @@ 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. */
> +		/* Calling BaseStruct::methods via its vtab. */

8. Lets do not repeat this comment on each port_tuple_vtab usage. It
it direct repetition of what is said in port_sql comment.

> +		if (port_tuple_vtab.dump_msgpack(port, out) < 0)
>   			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 +667,63 @@ err:
> +
> +static inline int
> +sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
> +	    struct region *region)

9. The function is not so trivial to omit a comment. Please, write it.
In particular what a one should do with region after sql_execute() is
finished. Should it be cleared?

> +{
> +	int rc, column_count = sqlite3_column_count(stmt);
> +	if (column_count > 0) {
> +		/* Either ROW or DONE or ERROR. */
> +		while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) {
> +			if (sql_row_to_port(stmt, column_count, region,
> +					    port) != 0)
> +				return -1;
> +		}
> +		assert(rc == SQLITE_DONE || rc != SQLITE_OK);
> +	} else {
> +		/* No rows. Either DONE or ERROR. */
> +		rc = sqlite3_step(stmt);
> +		assert(rc != SQLITE_ROW && rc != SQLITE_OK);
> +	}
> +	if (rc != SQLITE_DONE) {
> +		diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +int
> +sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
> +			uint32_t bind_count, struct port *port,
> +			struct region *region)
> +{
> +	struct sqlite3_stmt *stmt;
> +	sqlite3 *db = sql_get();
> +	if (db == NULL) {
> +		diag_set(ClientError, ER_LOADING);
> +		return -1;

10. Not sure if it is possible. Is it?

> +	}
> +	if (sqlite3_prepare_v2(db, sql, len, &stmt, NULL) != SQLITE_OK) {
> +		diag_set(ClientError, ER_SQL_EXECUTE, sqlite3_errmsg(db));
> +		return -1;
> +	}
> +	assert(stmt != NULL);
> +	port_sql_create(port, stmt);
> +	if (sql_bind(stmt, bind, bind_count) == 0 &&
> +	    sql_execute(db, stmt, port, region) == 0)
> +		return 0;
> +	port_destroy(port);
> +	return -1;
> +}
> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index a08c8c5..9dc0462 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -1645,7 +1645,7 @@ tx_process_sql(struct cmsg *m)
>   	/* Prepare memory for the iproto header. */
>   	if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0)
>   		goto error;
> -	if (sql_response_dump(&response, out) != 0) {
> +	if (port_dump_msgpack(&port, out) != 0) {
>   		obuf_rollback_to_svp(out, &header_svp);
>   		goto error;

11. Port leaks if an error occurred in iproto_prepare_header.

>   	}

Sorry, most of comments are minor and I could fix them
myself, but I was asked to do not rewrite patches if they
are not urgent.