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

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

Hi! Thanks for the patch! See 10 comments below.

On 17/09/2018 19:23, 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 implements such functinality.

1. Typos: 'way to implements', 'functinality'.

> 
> 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        |  17 ++++-
>   src/box/execute.h        |   1 +
>   src/box/lua/net_box.c    |  16 ++++-
>   src/box/sequence.c       |  37 ++++++++++
>   src/box/sql/sqliteInt.h  |   4 ++
>   src/box/sql/vdbeaux.c    |  29 ++++++++
>   src/box/txn.c            |   5 ++
>   src/box/txn.h            |   6 ++
>   test/sql/errinj.result   |   3 +-
>   test/sql/iproto.result   | 171 ++++++++++++++++++++++++++++++++++++++---------
>   test/sql/iproto.test.lua |  15 +++++
>   11 files changed, 266 insertions(+), 38 deletions(-)
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 24459b4..8e4bc80 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -641,14 +641,27 @@ err:
>   			goto err;
>   		int changes = sqlite3_changes(db);
>   		int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
> -			   mp_sizeof_uint(changes);
> +			   mp_sizeof_uint(changes) +
> +			   mp_sizeof_uint(SQL_INFO_GENERATED_IDS) +
> +			   mp_sizeof_array(db->generated_ids_count);

2. When you use '<noun>_count', the noun should be in the singular.

> +		for (uint64_t i = 0; i < db->generated_ids_count; ++i) {
> +			size += db->generated_ids_array[i] >= 0 ?

3. Type can be omitted in a variable name (no <name>_array, but <name>s).

> +				mp_sizeof_uint(db->generated_ids_array[i]) :
> +				mp_sizeof_int(db->generated_ids_array[i]);
> +		}
>   		char *buf = obuf_alloc(out, size);
>   		if (buf == NULL) {
>   			diag_set(OutOfMemory, size, "obuf_alloc", "buf");
> -			goto err;

4. Why?

>   		}
>   		buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
>   		buf = mp_encode_uint(buf, changes);
> +		buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);

5. I do not think we should send empty array. Maybe it is better to
omit the key in such a case?

> +		buf = mp_encode_array(buf, db->generated_ids_count);
> +		for (uint64_t i = 0; i < db->generated_ids_count; ++i) {
> +			buf = db->generated_ids_array[i] < 0 ?
> +			      mp_encode_int(buf, db->generated_ids_array[i]) :
> +			      mp_encode_uint(buf, db->generated_ids_array[i]);
> +		}
>   	}
>   	iproto_reply_sql(out, &header_svp, response->sync, schema_version,
>   			 keys);
> diff --git a/src/box/sequence.c b/src/box/sequence.c
> index 35b7605..234509c 100644
> --- a/src/box/sequence.c
> +++ b/src/box/sequence.c
> @@ -178,6 +180,37 @@ sequence_update(struct sequence *seq, int64_t value)
>   	return 0;
>   }
>   
> +/*
> + * Save generated value into txn.
> + *
> + * \param value value to save.
> + * \retval 0 Success.
> + * \retval -1 Error.
> + */
> +static inline int
> +save_value(int64_t value)
> +{
> +	struct txn *txn = in_txn();
> +	if (txn == NULL)
> +		return 0;
> +	if (txn->generated_ids.size == txn->generated_ids.capacity) {

6. Why do you store generated ids in box txn instead of vdbe? As I know,
after commit/rollback txn object is freed so it does not matter where do
you store ids array - on the heap or on a region - txn.generated_ids
attribute will be invalid after commit/rollback. What is more, we do not
need ids from non-SQL requests - they are never used.

Last time we discussed that you should store result of sequence_next from
OP_NextAutoincValue. And at the same place you can save the result into
Vdbe generated ids array. This will work the same way as your current
implementation, but will not slow down non-SQL requests.

> +		txn->generated_ids.capacity = txn->generated_ids.capacity > 0 ?
> +					      txn->generated_ids.capacity * 2 :
> +					      SEQUENCE_GENERATED_IDS_MIN_LEN;
> +		uint64_t capacity = txn->generated_ids.capacity *
> +				    sizeof(*txn->generated_ids.array);
> +		txn->generated_ids.array = realloc(txn->generated_ids.array,
> +						   capacity);
> +		if (txn->generated_ids.array == NULL) {
> +			diag_set(OutOfMemory, txn->generated_ids.capacity,
> +				 "realloc", "txn->generated_ids.array");
> +			return -1;
> +		}
> +	}
> +	txn->generated_ids.array[txn->generated_ids.size++] = value;
> +	return 0;
> +}
> +
>   int
>   sequence_next(struct sequence *seq, int64_t *result)
>   {
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 1d32c9a..7d41ece 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -1525,6 +1525,10 @@ struct sqlite3 {
>   	u8 mTrace;		/* zero or more SQLITE_TRACE flags */
>   	u32 magic;		/* Magic number for detect library misuse */
>   	int nChange;		/* Value returned by sqlite3_changes() */
> +	/* Number of generated ids. */
> +	uint64_t generated_ids_count;
> +	/* Array of generated ids. */
> +	int64_t *generated_ids_array;

7. Do not store ids in multiple places, and moreover in sqlite3 that will be
removed in future. Use struct Vdbe. Ids array is per-request attribute.

>   	int nTotalChange;	/* Value returned by sqlite3_total_changes() */
>   	int aLimit[SQLITE_N_LIMIT];	/* Limits */
>   	int nMaxSorterMmap;	/* Maximum size of regions mapped by sorter */
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index 3b0c90c..7b5d6b9 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -2303,6 +2303,29 @@ sql_savepoint(Vdbe *p, const char *zName)
>   }
>   
>   /*
> + * Move array of generated ids into db.
> + *
> + * \param db db to save array to.
> + * \retval 0 Success.
> + * \retval -1 Error.

8. Use @, not \.

9. You should not have this function. Ids must be stored in Vdbe.

> + */
> +static inline int
> +move_generated_ids_in_db(sqlite3 *db)
> +{
> +	struct txn *txn = in_txn();
> +	if (txn == NULL)
> +		return 0;
> +	uint64_t size = txn->generated_ids.size *
> +			sizeof(*txn->generated_ids.array);
> +	db->generated_ids_count = txn->generated_ids.size;
> +	db->generated_ids_array = malloc(size);
> +	if (db->generated_ids_array == NULL)
> +		return -1;
> +	memcpy(db->generated_ids_array, txn->generated_ids.array, size);
> +	return 0;
> +}
> +
> +/*
>    * This routine is called the when a VDBE tries to halt.  If the VDBE
>    * has made changes and is in autocommit mode, then commit those
>    * changes.  If a rollback is needed, then do the rollback.
10. I still think that we should not realloc a huge monolith array of ids.
My proposal is to use a list of small arrays. When a last chunk in the list
becomes full, we allocate a new chunk, append it to the list and use it for
next ids. But you can discuss it with Kirill if you do not agree.