Re: [Tarantool-patches] [PATCH v2 16/16] netbox: introduce prepared statements

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

Thanks for the patch!

See 5 comments below.

On 20/11/2019 22:28, Nikita Pettik wrote:
> This patch introduces support of prepared statements in IProto
> protocol. To achieve this new IProto command is added - IPROTO_PREPARE.
> It is sent with IPROTO_SQL_TEXT key and it means to prepare SQL statement
> (for details see previous commit). Also to reply on PREPARE request a few
> response keys are added: IPROTO_BIND_METADATA (contains parameters
> metadata) and IPROTO_BIND_COUNT (count of parameters to be bound).

1. Please, remind me, why do we need these two new keys?

> 
> Closes #2592
> 
> @TarantoolBot document
> Title: Prepared statements in SQL
> 
> Now it is possible to 'prepare' (i.e. compile into byte-code and save to
> the cache) statement and execute it several times. Mechanism is similar
> to ones in other DBs. Prepared statement is identified by string
> containing original SQL request. Prepared statement cache is global and
> follows LRU eviction policy: when there's no place for another one
> statement, the statement that is least recently used (prepared) is
> removed. Cache size is adjusted by box.cfg{sql_cache_size} variable
> (can be set dynamically; in case size is reduced some statements may be
> erased from cache). Note that any DDL operation leads to expiration of all
> prepared statements: they are recompiled on demand. To erase whole cache
> one can set its size to 0. Prepared statements are available in local mode
> (i.e. via box.prepare() function) and are supported in IProto protocol.
> Typical workflow with prepared statements is following:
> 
> s = box.prepare("SELECT * FROM t WHERE id = ?;")
> s:execute({1}) or box.execute(s.sql_str, {1})
> s:execute({2}) or box.execute(s.sql_str, {2})
> 
> In terms of remote connection:
> 
> cn = netbox:connect(addr)
> s = cn:prepare("SELECT * FROM t WHERE id = ?;")
> cn:execute(s.sql_str, {1})

2. Each time when binary protocol is changed the doc
request should describe these changes in detail: where
the new keys are added, what are their names, what are
their values and types, are they optional, what to
assume when they are omitted? This is needed so as our
users could update their connectors.

3. That would be cool to give user an advice here,
that the cache eviction is not signaled anyhow. So
if they want the prepared statements really stay
prepared, then should recall prepare(), when a sequence
of execute() calls is expected soon.

4. Please, document new 'prepared statement' object -
what attributes does it have, with what types.

> diff --git a/src/box/execute.c b/src/box/execute.c
> index 2b5a9ba90..13e5a6ba1 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -326,6 +326,67 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
> +static int
> +sql_get_prepare_common_keys(struct sql_stmt *stmt, struct obuf *out, int keys,
> +			    const char *sql_str)
> +{
> +	int size = mp_sizeof_map(keys) +
> +		   mp_sizeof_uint(IPROTO_SQL_TEXT) +
> +		   mp_sizeof_str(strlen(sql_str)) +
> +		   mp_sizeof_uint(IPROTO_BIND_COUNT) +
> +		   mp_sizeof_uint(sql_bind_parameter_count(stmt));
> +	char *pos = (char *) obuf_alloc(out, size);
> +	if (pos == NULL) {
> +		diag_set(OutOfMemory, size, "obuf_alloc", "pos");
> +		return -1;
> +	}
> +	pos = mp_encode_map(pos, keys);
> +	pos = mp_encode_uint(pos, IPROTO_SQL_TEXT);
> +	pos = mp_encode_str(pos, sql_str, strlen(sql_str));
> +	pos = mp_encode_uint(pos, IPROTO_BIND_COUNT);
> +	pos = mp_encode_uint(pos, sql_bind_parameter_count(stmt));
> +	if (sql_get_params_metadata(stmt, out) != 0)
> +		return -1;
> +	return 0;

5. Just make 'return sql_get_params_metadata(...)'.

6. Have you done any benchmarks? Just out of curiosity.
How much a prepared execution is faster then an execute
from the scratch? Just a microbenchmark with lua-land
time measurement would be fine.