[tarantool-patches] Re: [PATCH v8 2/3] sql: return all generated ids via IPROTO

["n.pettik" <korablev@tarantool.org>]

> On 29 Oct 2018, at 20:33, 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.

I guess you should be more specific in describing such things.
I don’t ask you to dive into details, but explaining main idea of
implementation would definitely help reviewer to do his/her job.

> 
> Closes #2618
> —
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 24459b4..ebf3962 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/vdbe.h"
> 
> 
> 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,

I vote for _AUTOINCREMENT_IDS, see comments below.
Anyway, in JDBC it is called GENERATED_KEYS. So, we either
follow JDBC convention or use our naming. In the latter case,
AUTOINCREMENT_IDS sounds more solid.

> 	sql_info_key_MAX,
> };
> 
> /**
> + * Append a new sequence value to Vdbe. List of automatically
> + * generated identifiers is returned as metadata of SQL response.
> + *
> + * @param Vdbe VDBE to save id in.
> + * @param id ID to save in VDBE.
> + * @retval 0 Success.
> + * @retval -1 Error.

It looks terrible. Does this function really need comment at all?

> + */
> +int
> +vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id);
> 
> +
> +int
> +vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id)
> +{
> +	assert(vdbe != NULL);
> +	struct id_entry *id_entry = region_alloc(&fiber()->gc, sizeof(*id_entry));
> +	if (id_entry == NULL) {
> +		diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc", "id_entry”);

Out of 80.

> +		return -1;
> +	}
> +	id_entry->id = id;
> +	stailq_add_tail_entry(vdbe_generated_ids(vdbe), id_entry, link);
> +	return 0;
> +}
> +
> /*
>  * Execute as much of a VDBE program as we can.
>  * This is the core of sqlite3_step().
> @@ -2995,8 +3015,9 @@ case OP_TransactionBegin: {
>  * If there is no active transaction, raise an error.
>  */
> case OP_TransactionCommit: {
> -	if (box_txn()) {
> -		if (box_txn_commit() != 0) {
> +	struct txn *txn = in_txn();
> +	if (txn != NULL) {
> +		if (txn_commit(txn) != 0) {

From first sigh it doesn’t look like diff related to patch.
Add comment pointing out that txn_commit() doesn’t
invoke fiber_gc() anymore and VDBE takes care of it.

> /**
>  * Prepare given VDBE to execution: initialize structs connected
> @@ -197,6 +198,15 @@ sql_alloc_txn();
> int
> sql_vdbe_prepare(struct Vdbe *vdbe);
> 
> +/**
> + * Return pointer to list of generated ids.
> + *
> + * @param vdbe VDBE to get list of generated ids from.
> + * @retval List of generated ids.
> + */
> +struct stailq *
> +vdbe_generated_ids(struct Vdbe *vdbe);

As for me, “generated_ids” seems to be too general name…
I would call it “autoincrement_ids" or sort of.

> +
> int sqlite3VdbeAddOp0(Vdbe *, int);
> int sqlite3VdbeAddOp1(Vdbe *, int, int);
> int sqlite3VdbeAddOp2(Vdbe *, int, int, int);
> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index ce97f49..7a7d3de 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -367,6 +367,11 @@ struct Vdbe {
> 	struct sql_txn *psql_txn;
> 	/** The auto-commit flag. */
> 	bool auto_commit;
> +	/**
> +	 * List of ids generated in current VDBE. It is returned
> +	 * as metadata of SQL response.
> +	 */
> +	struct stailq generated_ids;

Again: “autoincrement_ids”. Term “generated_ids” can be confused
with entity ids. For example, during execution of <CREATE TABLE>
statement VDBE operates on space id, index id etc (IMHO).

> 
> 	/* 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 6e42d05..ae73f25 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -57,6 +57,7 @@ sqlite3VdbeCreate(Parse * pParse)
> 		return 0;
> 	memset(p, 0, sizeof(Vdbe));
> 	p->db = db;
> +	stailq_create(&p->generated_ids);
> 	if (db->pVdbe) {
> 		db->pVdbe->pPrev = p;
> 	}
> 
> @@ -2414,9 +2417,9 @@ sqlite3VdbeHalt(Vdbe * p)
> 					 * key constraints to hold up the transaction. This means a commit
> 					 * is required.
> 					 */
> -					rc = box_txn_commit() ==
> -						    0 ? SQLITE_OK :
> -						    SQL_TARANTOOL_ERROR;
> +					rc = (in_txn() == NULL ||
> +					      txn_commit(in_txn()) == 0) ?
> +					     SQLITE_OK : SQL_TARANTOOL_ERROR;
> 					closeCursorsAndFree(p);
> 				}
> 				if (rc == SQLITE_BUSY && !p->pDelFrame) {
> @@ -2780,6 +2783,8 @@ sqlite3VdbeDelete(Vdbe * p)
> 	}
> 	p->magic = VDBE_MAGIC_DEAD;
> 	p->db = 0;
> +	if (in_txn() == NULL)
> +		fiber_gc();

Describe this change (in code, obviously):
“VDBE is responsible for releasing region …"
and so on.

> 	sqlite3DbFree(db, p);
> }
> 
> diff --git a/src/box/txn.h b/src/box/txn.h
> index e74c14d..87b55da 100644
> --- a/src/box/txn.h
> +++ b/src/box/txn.h
> @@ -49,6 +49,7 @@ struct engine;
> struct space;
> struct tuple;
> struct xrow_header;
> +struct Vdbe;
> 
> enum {
> 	/**
> @@ -117,6 +118,19 @@ struct sql_txn {
> 	 * VDBE to the next in the same transaction.
> 	 */
> 	uint32_t fk_deferred_count;
> +	/** Current VDBE. */
> +	struct Vdbe *vdbe;
> +};
> +
> +/**
> + * An element of list of autogenerated ids, being returned as SQL
> + * response metadata.
> + */
> +struct id_entry {

“id_entry” is too common name, be more specific pls. 

> +	/** A link in a generated id list. */
> +	struct stailq_entry link;
> +	/** Generated id. */
> +	int64_t id;

These comments are too obvious, they only contaminate
source code. 

> };
> 
> /**
>  * FFI bindings: do not throw exceptions, do not accept extra
> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
> index af474bc..af4fc12 100644
> --- a/test/sql/iproto.result
> +++ b/test/sql/iproto.result
> @@ -580,6 +580,98 @@ cn:close()
> box.sql.execute('drop table test')
> ---
> ...
> +-- gh-2618 Return generated columns after INSERT in IPROTO.
> +-- Return all ids generated in current INSERT statement.

Add pls case where generated ids are less than 0.