[tarantool-patches] Re: [PATCH v6 2/5] iproto: create port_sql
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Dec 29 16:19:33 MSK 2018
Thanks for the patch! See 11 comments below.
On 28/12/2018 21:11, imeevma at 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 at 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 at 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.
More information about the Tarantool-patches
mailing list