From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B5FE92EB63 for ; Tue, 30 Oct 2018 10:30:28 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 2K_-Za75lI2W for ; Tue, 30 Oct 2018 10:30:28 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6DA242EB5F for ; Tue, 30 Oct 2018 10:30:28 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH v8 2/3] sql: return all generated ids via IPROTO From: "n.pettik" In-Reply-To: <53b98187eba28a890db153e97d9c0a2b3759d1dd.1540832830.git.imeevma@gmail.com> Date: Tue, 30 Oct 2018 17:30:26 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <6167E418-CCCC-4596-9E6C-F04C02045456@tarantool.org> References: <53b98187eba28a890db153e97d9c0a2b3759d1dd.1540832830.git.imeevma@gmail.com> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy , Imeev Mergen > On 29 Oct 2018, at 20:33, imeevma@tarantool.org wrote: >=20 > 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=E2=80=99t ask you to dive into details, but explaining main idea = of implementation would definitely help reviewer to do his/her job. >=20 > Closes #2618 > =E2=80=94 > 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" >=20 >=20 > 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 =3D 0, > + SQL_INFO_GENERATED_IDS =3D 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, > }; >=20 > /** > + * 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); >=20 > + > +int > +vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id) > +{ > + assert(vdbe !=3D NULL); > + struct id_entry *id_entry =3D region_alloc(&fiber()->gc, = sizeof(*id_entry)); > + if (id_entry =3D=3D NULL) { > + diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc", = "id_entry=E2=80=9D); Out of 80. > + return -1; > + } > + id_entry->id =3D 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() !=3D 0) { > + struct txn *txn =3D in_txn(); > + if (txn !=3D NULL) { > + if (txn_commit(txn) !=3D 0) { =46rom first sigh it doesn=E2=80=99t look like diff related to patch. Add comment pointing out that txn_commit() doesn=E2=80=99t 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); >=20 > +/** > + * 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, =E2=80=9Cgenerated_ids=E2=80=9D seems to be too general = name=E2=80=A6 I would call it =E2=80=9Cautoincrement_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: =E2=80=9Cautoincrement_ids=E2=80=9D. Term =E2=80=9Cgenerated_ids=E2= =80=9D can be confused with entity ids. For example, during execution of statement VDBE operates on space id, index id etc (IMHO). >=20 > /* 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 =3D db; > + stailq_create(&p->generated_ids); > if (db->pVdbe) { > db->pVdbe->pPrev =3D p; > } >=20 > @@ -2414,9 +2417,9 @@ sqlite3VdbeHalt(Vdbe * p) > * key constraints to hold up = the transaction. This means a commit > * is required. > */ > - rc =3D box_txn_commit() =3D=3D > - 0 ? SQLITE_OK : > - SQL_TARANTOOL_ERROR; > + rc =3D (in_txn() =3D=3D NULL || > + txn_commit(in_txn()) =3D=3D = 0) ? > + SQLITE_OK : = SQL_TARANTOOL_ERROR; > closeCursorsAndFree(p); > } > if (rc =3D=3D SQLITE_BUSY && = !p->pDelFrame) { > @@ -2780,6 +2783,8 @@ sqlite3VdbeDelete(Vdbe * p) > } > p->magic =3D VDBE_MAGIC_DEAD; > p->db =3D 0; > + if (in_txn() =3D=3D NULL) > + fiber_gc(); Describe this change (in code, obviously): =E2=80=9CVDBE is responsible for releasing region =E2=80=A6" and so on. > sqlite3DbFree(db, p); > } >=20 > 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; >=20 > 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 { =E2=80=9Cid_entry=E2=80=9D is too common name, be more specific pls.=20 > + /** 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.=20 > }; >=20 > /** > * 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.