[tarantool-patches] Re: [PATCH v7 3/6] iproto: create port_sql

[Vladislav Shpilevoy]

Thanks for the patch! See 2 comments below.

> commit 1a3007030ea54fa81533acbf7421f27a2399c941
> 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..a81f482 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -643,7 +672,66 @@ err:
> +/**
> + * Execute prepared SQL statement.
> + *
> + * This function uses region to allocate memory for temporary
> + * objects. After this function, region will be in the same state
> + * in which it was before this function.
> + *
> + * @param db SQL handle.
> + * @param stmt Prepared statement.
> + * @param[out] port Port to store SQL response.

1. It is not [out]. Port should be created before calling this function,
so it is [in][out], that we usually omit.

> + * @param region Region to allocate temporary objects.
> + *
> + * @retval  0 Success.
> + * @retval -1 Error.
> + */
> +static inline int
> +sql_execute(sqlite3 *db, struct sqlite3_stmt *stmt, struct port *port,
> +     struct region *region)
> +{
> + 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;
> +}
> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index a08c8c5..09e8914 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -1643,9 +1643,11 @@ tx_process_sql(struct cmsg *m)
>    out = msg->connection->tx.p_obuf;
>    struct obuf_svp header_svp;
>    /* Prepare memory for the iproto header. */
> - if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0)
> + if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0) {
> +   port_destroy(&port);
>      goto error;
> - if (sql_response_dump(&response, out) != 0) {
> + }
> + if (port_dump_msgpack(&port, out) != 0) {

2. port_sql_dump_msgpack destroys port on an error, but port_tuple_dump_msgpack
does not. I think, they should work similar, so port_sql_dump_msgpack does
not destroy the port as well. Lets do it, and add here port_destroy() on an
error. The same about port_dump_lua.

>      obuf_rollback_to_svp(out, &header_svp);
>      goto error;
>    }