Re: [Tarantool-patches] [PATCH v2 07/16] port: add dump format and request type to port_sql

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

Thanks for the patch!

See 4 comments below.

On 20/11/2019 22:28, Nikita Pettik wrote:
> Dump formats of DQL and DML queries are different: the last one contains
> number of affected rows and optionally list of autoincremented ids; the
> first one comprises all meta-information including column names of
> resulting set and their types. What is more, dump format is going to be
> different for execute and prepare requests. So let's introduce separate
> member to struct port_sql responsible for dump format to be used.
> 
> What is more, prepared statement finalization is required only for
> PREPARE-AND-EXECUTE requests. So let's keep request type in port as well.

1. I don't think it is a good idea to rely on certain
meta being returned by only certain requests. There is
a ticket, which will remove the border between different
request types:
https://github.com/tarantool/tarantool/issues/3649.

Yes, now this is either sql_info or metadata, but that
will change.

> 
> Note that C standard specifies that enums are integers, but it does not
> specify the size. Hence, let's use simple uint8 - mentioned enums are
> small enough to fit into it.

2. Try this:

    enum ... <member> : 8;

> 
> Needed for #2592
> ---
>  src/box/execute.c     | 32 +++++++++++++++++++++++---------
>  src/box/execute.h     | 24 ++++++++++++++++++++++++
>  src/box/lua/execute.c | 20 +++++++++++++-------
>  3 files changed, 60 insertions(+), 16 deletions(-)
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 83680b70f..d2a999099 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -100,7 +100,9 @@ static void
>  port_sql_destroy(struct port *base)
>  {
>  	port_tuple_vtab.destroy(base);
> -	sql_finalize(((struct port_sql *)base)->stmt);
> +	struct port_sql *port_sql = (struct port_sql *) base;
> +	if (port_sql->request == PREPARE_AND_EXECUTE)

3. You could turn the types into a bitmask to drop the
dependency on a certain request type:

    bool do_finalize : 1;
    bool do_omit_data : 1;

> +		sql_finalize(((struct port_sql *)base)->stmt);
>  }
>  
>  const struct port_vtab port_sql_vtab = {
> @@ -395,6 +404,9 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
>  				      mp_encode_int(buf, id_entry->id);
>  			}
>  		}
> +		break;
> +	}
> +	default: unreachable();

4. Please, wrap on a new line, and include into {}. I know,
this is shorter, but this just does not match the code
style. In the other place too.

>  	}
>  	return 0;
>  }