[tarantool-patches] Re: [PATCH v5 1/1] sql: return all generated ids via IPROTO

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

Hi! Thanks for the patch!

I see that Kostja slightly reviewed it and you did not
answer him, so I inlined here some of his comments to
agree/disagree with them.

See 8 comments below.

On 28/09/2018 19:53, imeevma@tarantool.org wrote:
> According to documentation some JDBC functions have an ability to
> return all ids that were generated in executed INSERT statement.
> This patch gives a way to implement such functionality.
> 
> Closes #2618
> ---
> Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-2618-return-all-generated-ids
> Issue: https://github.com/tarantool/tarantool/issues/2618
> 
>   src/box/execute.c        | 24 ++++++++++++++++
>   src/box/execute.h        |  1 +
>   src/box/lua/net_box.c    | 21 ++++++++++++--
>   src/box/sequence.c       | 29 ++++++++++++++++++++
>   src/box/sql/vdbe.c       |  2 +-
>   src/box/sql/vdbe.h       |  3 +-
>   src/box/sql/vdbeInt.h    |  3 ++
>   src/box/sql/vdbeaux.c    | 15 ++++++++--
>   src/box/txn.h            | 13 +++++++++
>   test/sql/iproto.result   | 71 ++++++++++++++++++++++++++++++++++++++++++++++++
>   test/sql/iproto.test.lua | 15 ++++++++++
>   11 files changed, 190 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 24459b4..d9fe33f 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -42,6 +42,7 @@
>   #include "schema.h"
>   #include "port.h"
>   #include "tuple.h"
> +#include "sql/vdbeInt.h"

1. Kostja said:

>> Ugh. You're including a header which ends with Int. Have you
>> thought what this Int means? It's not Integer.

Strictly speaking, I agree with him. Src/box/execute is not
an SQL internal file. It even uses SQLite public API to
fetch names and types. I guess, you should implement a getter
for ids list like

     const struct stailq *
     vdbe_auto_ids_list(const struct Vdbe *v);

>   
>   const char *sql_type_strs[] = {
>   	NULL,
> @@ -639,14 +640,37 @@ err:
>   		assert(port_tuple->size == 0);
>   		if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0)
>   			goto err;
> +		struct stailq *id_list = &((struct Vdbe *)stmt)->id_list;
> +		uint64_t id_count = 0;
>   		int changes = sqlite3_changes(db);
>   		int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
>   			   mp_sizeof_uint(changes);
> +		if (stailq_empty(id_list) == 0) {

2. Kostja said:

>> What is the point of this == 0? Why are you comparing with an
>> integer here, not boolean? 

I partially agree and I guess it is my fault that I am too harsh
about code style. You used here == 0 because stailq_empty()
returns an integer, but its name looks like a flag getter, so you
can use it as a boolean here.

> +			struct id_entry *id_entry;
> +			stailq_foreach_entry(id_entry, id_list, link) {
> +				size += id_entry->id >= 0 ?
> +					mp_sizeof_uint(id_entry->id) :
> +					mp_sizeof_int(id_entry->id);
> +				id_count++;
> +			}
> +			size += mp_sizeof_uint(SQL_INFO_GENERATED_IDS) +
> +				mp_sizeof_array(id_count);
> +		}>   		char *buf = obuf_alloc(out, size);
>   		if (buf == NULL) {
>   			diag_set(OutOfMemory, size, "obuf_alloc", "buf");
>   			goto err;
>   		}
> +		if (id_count > 0) {

3. Lets be consistent: either you use !stailq_empty() in both
places, or id_count > 0.

> +			buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
> +			buf = mp_encode_array(buf, id_count);
> +			struct id_entry *id_entry;
> +			stailq_foreach_entry(id_entry, id_list, link) {
> +				buf = id_entry->id >= 0 ?
> +				      mp_encode_uint(buf, id_entry->id) :
> +				      mp_encode_int(buf, id_entry->id);
> +			}
> +		}
>   		buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
>   		buf = mp_encode_uint(buf, changes);
>   	}
> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index a928a4c..5d2ac78 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -671,11 +671,28 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
>   	/* Only SQL_INFO_ROW_COUNT is available. */
>   	assert(map_size == 1);

4. How does it work? map_size can be 2 now, and the comment is
outdated - not only SQL_INFO_ROW_COUNT is available.

>   	(void) map_size;
> +	lua_newtable(L);
>   	uint32_t key = mp_decode_uint(data);
> +	/*
> +	 * If SQL_INFO_GENERATED_IDS in data then it should be
> +	 * just before SQL_INFO_ROW_COUNT.
> +	 */
> +	if (key == SQL_INFO_GENERATED_IDS) {
> +		uint64_t count = mp_decode_array(data);
> +		assert (count > 0);
> +		lua_createtable(L, 0, count);
> +		lua_setfield(L, -2, "generated_ids");
> +		lua_getfield(L, -1, "generated_ids");
> +		for (uint32_t j = 0; j < count; ++j) {
> +			int64_t value = mp_decode_uint(data);
> +			lua_pushinteger(L, value);
> +			lua_rawseti(L, -2, j + 1);
> +		}
> +		lua_pop(L, 1);
> +		key = mp_decode_uint(data);
> +	}
>   	assert(key == SQL_INFO_ROW_COUNT);
> -	(void) key;
>   	uint32_t row_count = mp_decode_uint(data);
> -	lua_createtable(L, 0, 1);
>   	lua_pushinteger(L, row_count);
>   	lua_setfield(L, -2, "rowcount");
>   }
> diff --git a/src/box/sequence.c b/src/box/sequence.c
> index 35b7605..ed6b2da 100644
> --- a/src/box/sequence.c
> +++ b/src/box/sequence.c
> @@ -38,6 +38,7 @@
>   #include <small/mempool.h>
>   #include <msgpuck/msgpuck.h>
>   
> +#include "txn.h"
>   #include "diag.h"
>   #include "error.h"
>   #include "errcode.h"
> @@ -178,6 +179,30 @@ sequence_update(struct sequence *seq, int64_t value)
>   	return 0;
>   }
>   
> +/**
> + * Save generated id in VDBE.
> + *
> + * @param value ID to save in VDBE.
> + * @retval 0 Success.
> + * @retval -1 Error.
> + */
> +static inline int
> +add_new_id_in_vdbe(int64_t value)

5. Kostja said:

>> subject-verb-object
I agree. He means 'vdbe_add_new_id'. And in such a
case it is evidently a part of Vdbe API, so put it into
vdbe.h with this signature:

     int
     vdbe_add_new_auto_id(struct Vdbe *vdbe, int64_t id);

> +{
> +	struct txn *txn = in_txn();
> +	if (txn == NULL || txn->psql_txn == NULL)
> +		return 0;
> +	assert(txn->psql_txn->vdbe != NULL);
> +	struct id_entry *id_entry = malloc(sizeof(*id_entry));
> +	if (id_entry == NULL) {
> +		diag_set(OutOfMemory, sizeof(*id_entry), "malloc", "id_entry");
> +		return -1;
> +	}
> +	id_entry->id = value;
> +	stailq_add_tail_entry(txn->psql_txn->id_list, id_entry, link);

6. If you have Vdbe, you do not need to store the list both in
Vdbe and sql_txn. Store it in Vdbe only.

7. Kostja said:

>> Why malloc?
It is understandable why malloc - txn after commit invalidates
the region memory. We neither can save ids on obuf directly
since while getting next id a yield is possible and the obuf
will be messed with another request.  But guess I thought up a
solution. It is a bit tricky, so maybe it should be accounted
as a separate issue and this solved with malloc.

I propose to split txn_commit() into commit and region reset.
Txn_commit() goal is to write data to disk, not to destroy a
region. It saves us when a transaction is started and committed
by Vdbe. But what about autocommit transactions? I think, we
should get rid of autocommit transactions for Vdbe. Vdbe always
starts a transaction for DML and commits it, but without region
reset - it is a part of vdbe finalize routine (which already
exists actually).

This split of txn_commit logic allows to store anything on
a region during Vdbe work and not being afraid about a reset.

> +	return 0;
> +}
> +
> diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
> index d5da5d5..9b94183 100644
> --- a/src/box/sql/vdbe.h
> +++ b/src/box/sql/vdbe.h
> @@ -179,10 +179,11 @@ Vdbe *sqlite3VdbeCreate(Parse *);
>    * Allocate and initialize SQL-specific struct which completes
>    * original Tarantool's txn struct using region allocator.
>    *
> + * @param vdbe VDBE that is being prepared or executed.
>    * @retval NULL on OOM, new psql_txn struct on success.
>    **/
>   struct sql_txn *
> -sql_alloc_txn();
> +sql_alloc_txn(struct Vdbe *vdbe);

8. I think, alloc should only alloc. sql_txn_begin should
put Vdbe into sql_txn.