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 81A452DBFB for ; Thu, 25 Oct 2018 17:31:13 -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 GYYH5QGGuc6d for ; Thu, 25 Oct 2018 17:31:13 -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 B591F2DBE6 for ; Thu, 25 Oct 2018 17:31:12 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v5 1/1] sql: return all generated ids via IPROTO References: <23421254ee60958d4583af90100d77b222eb2aad.1538153530.git.imeevma@gmail.com> <6bbe2963-008e-79af-30d5-39dbbb52ddb0@tarantool.org> From: Vladislav Shpilevoy Message-ID: <761e97be-bf65-4ab5-2ce8-7b9641fec913@tarantool.org> Date: Fri, 26 Oct 2018 00:31:09 +0300 MIME-Version: 1.0 In-Reply-To: <6bbe2963-008e-79af-30d5-39dbbb52ddb0@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Imeev Mergen , tarantool-patches@freelists.org Hi! Thanks for the fixes! I pushed more and one additional commit to remove one attribute from struct Vdbe. Please, look at both commits, squash fixes if you are ok with them and send v2 to Nikita (including my commit, yes). My review fixes: ================================================================== commit 45019a8dccb89a7f89b5e59e442c539aa0e6ef41 Author: Vladislav Shpilevoy Date: Fri Oct 26 00:11:27 2018 +0300 Review fixes diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c index 1a9b3f740..4d79fdefb 100644 --- a/src/box/lua/net_box.c +++ b/src/box/lua/net_box.c @@ -685,19 +685,16 @@ netbox_decode_sql_info(struct lua_State *L, const char **data) if (map_size == 2) { key = mp_decode_uint(data); assert(key == SQL_INFO_GENERATED_IDS); - (void)key; uint64_t count = mp_decode_array(data); - assert (count > 0); + 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 id; mp_read_int64(data, &id); - lua_pushinteger(L, id); + luaL_pushint64(L, id); lua_rawseti(L, -2, j + 1); } - lua_pop(L, 1); + lua_setfield(L, -2, "generated_ids"); } } diff --git a/src/box/sequence.c b/src/box/sequence.c index b1c68e97e..9a87a56eb 100644 --- a/src/box/sequence.c +++ b/src/box/sequence.c @@ -233,7 +233,7 @@ done: unreachable(); *result = value; if (txn_vdbe() != NULL && - vdbe_add_new_generated_id(txn_vdbe(), *result) != 0) + vdbe_add_new_generated_id(txn_vdbe(), value) != 0) return -1; return 0; overflow: diff --git a/src/box/sequence.h b/src/box/sequence.h index d38fb2f97..7180ead76 100644 --- a/src/box/sequence.h +++ b/src/box/sequence.h @@ -141,7 +141,8 @@ int access_check_sequence(struct sequence *seq); /** - * Add new generated id in VDBE. + * 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. diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 7a3748eae..2571492fc 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -3015,8 +3015,9 @@ case OP_TransactionBegin: { * If there is no active transaction, raise an error. */ case OP_TransactionCommit: { - if (box_txn()) { - if (txn_commit(in_txn()) != 0) { + struct txn *txn = in_txn(); + if (txn != NULL) { + if (txn_commit(txn) != 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 8f877b3a4..ed7fb2f49 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 v Vdbe with which associate this transaction. * @retval NULL on OOM, new psql_txn struct on success. **/ struct sql_txn * -sql_alloc_txn(); +sql_alloc_txn(struct Vdbe *v); /** * Prepare given VDBE to execution: initialize structs connected diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index 37df5e290..7a7d3dec0 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -367,8 +367,10 @@ struct Vdbe { struct sql_txn *psql_txn; /** The auto-commit flag. */ bool auto_commit; - - /** List of id generated in current VDBE. */ + /** + * List of ids generated in current VDBE. It is returned + * as metadata of SQL response. + */ struct stailq generated_ids; /* When allocating a new Vdbe object, all of the fields below should be diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 0023dc6cd..ae73f2541 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -57,7 +57,6 @@ sqlite3VdbeCreate(Parse * pParse) return 0; memset(p, 0, sizeof(Vdbe)); p->db = db; - /* Init list of generated ids. */ stailq_create(&p->generated_ids); if (db->pVdbe) { db->pVdbe->pPrev = p; @@ -77,7 +76,7 @@ sqlite3VdbeCreate(Parse * pParse) } struct sql_txn * -sql_alloc_txn() +sql_alloc_txn(struct Vdbe *v) { struct sql_txn *txn = region_alloc_object(&fiber()->gc, struct sql_txn); @@ -86,6 +85,8 @@ sql_alloc_txn() "struct sql_txn"); return NULL; } + txn->vdbe = v; + v->psql_txn = txn; txn->pSavepoint = NULL; txn->fk_deferred_count = 0; return txn; @@ -105,12 +106,12 @@ 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; - txn->psql_txn->vdbe = vdbe; + } else { + vdbe->psql_txn = txn->psql_txn; } - vdbe->psql_txn = txn->psql_txn; } else { vdbe->psql_txn = NULL; } @@ -2264,13 +2265,11 @@ 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; } - ptxn->psql_txn->vdbe = p; - p->psql_txn = ptxn->psql_txn; return 0; } diff --git a/src/box/txn.h b/src/box/txn.h index 2acba9f87..12e9d1013 100644 --- a/src/box/txn.h +++ b/src/box/txn.h @@ -121,9 +121,11 @@ struct sql_txn { struct Vdbe *vdbe; }; -/** An element of list of generated ids. */ -struct id_entry -{ +/** + * An element of list of autogenerated ids, being returned as SQL + * response metadata. + */ +struct id_entry { /** A link in a generated id list. */ struct stailq_entry link; /** Generated id. */ @@ -349,9 +351,9 @@ void txn_on_stop(struct trigger *trigger, void *event); /** - * Return VDBE that is currently executed. + * Return VDBE that is being currently executed. * - * @retval VDBE that is currently executed. + * @retval VDBE that is being currently executed. * @retval NULL Either txn or ptxn_sql or vdbe is NULL; */ static inline struct Vdbe * diff --git a/test/sql/iproto.result b/test/sql/iproto.result index 163fb6ed7..af4fc124d 100644 --- a/test/sql/iproto.result +++ b/test/sql/iproto.result @@ -637,13 +637,28 @@ cn:execute('select * from test') - [125, 1] - [126, 1] ... -cn:execute('create table test2 (id integer primary key, a integer)') +s = box.schema.create_space('test2', {engine = engine}) --- -- rowcount: 1 ... -cn:execute('drop table test2') +sq = box.schema.sequence.create('test2') +--- +... +pk = s:create_index('pk', {sequence = 'test2'}) +--- +... +function push_id() s:replace{box.NULL} s:replace{box.NULL} end +--- +... +_ = box.space.TEST:on_replace(push_id) --- -- rowcount: 1 +... +cn:execute('insert into test values (null, 1)') +--- +- generated_ids: + - 127 + - 1 + - 2 + rowcount: 1 ... cn:close() --- @@ -651,6 +666,12 @@ cn:close() box.sql.execute('drop table test') --- ... +s:drop() +--- +... +sq:drop() +--- +... box.schema.user.revoke('guest', 'read,write,execute', 'universe') --- ... diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua index 6517cdef2..a2caa57b1 100644 --- a/test/sql/iproto.test.lua +++ b/test/sql/iproto.test.lua @@ -204,6 +204,7 @@ box.sql.execute('drop table test') -- gh-2618 Return generated columns after INSERT in IPROTO. -- Return all ids generated in current INSERT statement. box.sql.execute('create table test (id integer primary key autoincrement, a integer)') + cn = remote.connect(box.cfg.listen) cn:execute('insert into test values (1, 1)') cn:execute('insert into test values (null, 2)') @@ -211,10 +212,18 @@ cn:execute('update test set a = 11 where id == 1') cn:execute('insert into test values (100, 1), (null, 1), (120, 1), (null, 1)') cn:execute('insert into test values (null, 1), (null, 1), (null, 1), (null, 1), (null, 1)') cn:execute('select * from test') -cn:execute('create table test2 (id integer primary key, a integer)') -cn:execute('drop table test2') + +s = box.schema.create_space('test2', {engine = engine}) +sq = box.schema.sequence.create('test2') +pk = s:create_index('pk', {sequence = 'test2'}) +function push_id() s:replace{box.NULL} s:replace{box.NULL} end +_ = box.space.TEST:on_replace(push_id) +cn:execute('insert into test values (null, 1)') + cn:close() box.sql.execute('drop table test') +s:drop() +sq:drop() box.schema.user.revoke('guest', 'read,write,execute', 'universe') space = nil