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

[Konstantin Osipov <kostja@tarantool.org>]

* imeevma@tarantool.org <imeevma@tarantool.org> [18/09/28 19:53]:
> 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"

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

>  
>  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;

The name is very general. autoinc_id_list would be more specific.
Why not simply call it generated_ids here and everywhere? This is
what these values are called in the standard.

>  		int changes = sqlite3_changes(db);
>  		int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
>  			   mp_sizeof_uint(changes);
> +		if (stailq_empty(id_list) == 0) {

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

> +			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);

How can you get a negative 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) {
> +			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/execute.h b/src/box/execute.h
> index f21393b..614d3d0 100644
> --- a/src/box/execute.h
> +++ b/src/box/execute.h
> @@ -42,6 +42,7 @@ extern "C" {
>  /** Keys of IPROTO_SQL_INFO map. */
>  enum sql_info_key {
>  	SQL_INFO_ROW_COUNT = 0,
> +	SQL_INFO_GENERATED_IDS = 1,
>  	sql_info_key_MAX,
>  };
>  
> 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);
>  	(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)

subject-verb-object

> +{
> +	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));

Why malloc?

> +	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);
> +	return 0;
> +}
> +
>  int
>  sequence_next(struct sequence *seq, int64_t *result)
>  {
> @@ -194,6 +219,8 @@ sequence_next(struct sequence *seq, int64_t *result)
>  					  new_data) == light_sequence_end)
>  			return -1;
>  		*result = def->start;
> +		if(add_new_id_in_vdbe(*result) != 0)
> +			return -1;
>  		return 0;
>  	}
>  	old_data = light_sequence_get(&sequence_data_index, pos);
> @@ -228,6 +255,8 @@ done:
>  				   new_data, &old_data) == light_sequence_end)
>  		unreachable();
>  	*result = value;
> +	if(add_new_id_in_vdbe(*result) != 0)
> +		return -1;
You continue to miss a space after if.

>  	return 0;
>  overflow:
>  	if (!def->cycle) {
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 00ffb0c..cbec9a2 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -3040,7 +3040,7 @@ case OP_TransactionRollback: {
>   */
>  case OP_TTransaction: {
>  	if (!box_txn()) {
> -		if (box_txn_begin() != 0) {
> +		if (sql_txn_begin(p) != 0) {
>  			rc = SQL_TARANTOOL_ERROR;
>  			goto abort_due_to_error;
>  		}
> 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);
>  
>  /**
>   * Prepare given VDBE to execution: initialize structs connected
> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index ce97f49..ca1794c 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -368,6 +368,9 @@ struct Vdbe {
>  	/** The auto-commit flag. */
>  	bool auto_commit;
>  
> +	/** List of id generated in current VDBE. */
> +	struct stailq id_list;
> +
>  	/* When allocating a new Vdbe object, all of the fields below should be
>  	 * initialized to zero or NULL
>  	 */
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index 54edf1b..98ddef7 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -57,6 +57,8 @@ sqlite3VdbeCreate(Parse * pParse)
>  		return 0;
>  	memset(p, 0, sizeof(Vdbe));
>  	p->db = db;
> +	/* Init list of generated ids. */
> +	stailq_create(&p->id_list);
>  	if (db->pVdbe) {
>  		db->pVdbe->pPrev = p;
>  	}
> @@ -75,7 +77,7 @@ sqlite3VdbeCreate(Parse * pParse)
>  }
>  
>  struct sql_txn *
> -sql_alloc_txn()
> +sql_alloc_txn(struct Vdbe *vdbe)
>  {
>  	struct sql_txn *txn = region_alloc_object(&fiber()->gc,
>  						  struct sql_txn);
> @@ -86,6 +88,8 @@ sql_alloc_txn()
>  	}
>  	txn->pSavepoint = NULL;
>  	txn->fk_deferred_count = 0;
> +	txn->vdbe = vdbe;
> +	txn->id_list = &vdbe->id_list;

Why do you need both?

>  	return txn;
>  }
>  
> @@ -103,7 +107,7 @@ sql_vdbe_prepare(struct Vdbe *vdbe)
>  		 * check FK violations, at least now.
>  		 */
>  		if (txn->psql_txn == NULL) {
> -			txn->psql_txn = sql_alloc_txn();
> +			txn->psql_txn = sql_alloc_txn(vdbe);
>  			if (txn->psql_txn == NULL)
>  				return -1;
>  		}
> @@ -2261,7 +2265,7 @@ sql_txn_begin(Vdbe *p)
>  	struct txn *ptxn = txn_begin(false);
>  	if (ptxn == NULL)
>  		return -1;
> -	ptxn->psql_txn = sql_alloc_txn();
> +	ptxn->psql_txn = sql_alloc_txn(p);
>  	if (ptxn->psql_txn == NULL) {
>  		box_txn_rollback();
>  		return -1;
> @@ -2746,6 +2750,11 @@ sqlite3VdbeClearObject(sqlite3 * db, Vdbe * p)
>  	vdbeFreeOpArray(db, p->aOp, p->nOp);
>  	sqlite3DbFree(db, p->aColName);
>  	sqlite3DbFree(db, p->zSql);
> +	while (stailq_empty(&p->id_list) == 0) {
> +		struct id_entry *id_entry =
> +			stailq_shift_entry(&p->id_list, struct id_entry, link);
> +		free(id_entry);
> +	}
>  #ifdef SQLITE_ENABLE_STMT_SCANSTATUS
>  	{

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov