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

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 4 comments below.

On 22/12/2018 14:31, imeevma at tarantool.org wrote:
> 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
> ---
>   src/box/execute.c | 90 ++++++++++++++++++++++++++++++++++++++++++-------------
>   src/box/execute.h | 67 +++++++++++++++--------------------------
>   src/box/iproto.cc |  6 ++--
>   src/box/port.h    |  1 -
>   4 files changed, 96 insertions(+), 68 deletions(-)
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 38b6cbc..b07de28 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -575,26 +609,18 @@ 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. */
> +		if (port_tuple_vtab.dump_msgpack(port, out) < 0)

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

>   			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 +669,29 @@ err:
> +
> +void
> +port_tuple_to_port_sql(struct port *port, struct sqlite3_stmt *stmt)
> +{
> +	assert(port->vtab == &port_tuple_vtab);
> +	((struct port_sql *)port)->stmt = stmt;
> +	port->vtab = &port_sql_vtab;
> +}

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.

> +
> +const struct port_vtab port_sql_vtab = {
> +	.dump_msgpack = port_sql_dump_msgpack,
> +	.dump_msgpack_16 = NULL,
> +	.dump_lua = NULL,
> +	.dump_plain = NULL,
> +	.destroy = port_sql_destroy,
> +};
> diff --git a/src/box/execute.h b/src/box/execute.h
> index 60b8f31..5aef546 100644
> --- a/src/box/execute.h
> +++ b/src/box/execute.h
> @@ -51,14 +51,31 @@ extern const char *sql_info_key_strs[];
>   struct obuf;
>   struct region;
>   struct sql_bind;
> +struct sqlite3_stmt;
>   
> -/** Response on EXECUTE request. */
> -struct sql_response {
> -	/** Port with response data if any. */
> -	struct port port;
> -	/** Prepared SQL statement with metadata. */
> -	void *prep_stmt;
> +/**
> + * Port implementation used for dump tuples, stored in port_tuple,
> + * to obuf or Lua.
> + */
> +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)");
> +
> +extern const struct port_vtab port_sql_vtab;
> +
> +/**
> + * Transform port_tuple with already stored tuples to port_sql
> + * that will dump these tuples into obut or Lua.
> + *
> + * @param port port_tuple to transform into port_sql.
> + */
> +void
> +port_tuple_to_port_sql(struct port *port, struct sqlite3_stmt *stmt);

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.

>   
>   /**
>    * Parse MessagePack array of SQL parameters.
> @@ -128,7 +109,7 @@ sql_response_dump(struct sql_response *response, struct obuf *out);
>    */
>   int
>   sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
> -			uint32_t bind_count, struct sql_response *response,
> +			uint32_t bind_count, struct port *port,
>   			struct region *region);

4. Do not forget about comments. It still describes 'response' parameter.

>   
>   #if defined(__cplusplus)