[tarantool-patches] Re: [PATCH v8 2/3] sql: return all generated ids via IPROTO
n.pettik
korablev at tarantool.org
Tue Oct 30 17:30:26 MSK 2018
> On 29 Oct 2018, at 20:33, imeevma at 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.
More information about the Tarantool-patches
mailing list