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 CA4C42B5F8 for ; Wed, 3 Oct 2018 15:19:51 -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 rg6UIDCP78wT for ; Wed, 3 Oct 2018 15:19:51 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 133002AAC6 for ; Wed, 3 Oct 2018 15:19:50 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v5 1/1] sql: return all generated ids via IPROTO References: <23421254ee60958d4583af90100d77b222eb2aad.1538153530.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Wed, 3 Oct 2018 22:19:48 +0300 MIME-Version: 1.0 In-Reply-To: <23421254ee60958d4583af90100d77b222eb2aad.1538153530.git.imeevma@gmail.com> 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: tarantool-patches@freelists.org, imeevma@tarantool.org Hi! Thanks for the patch! I see that Kostja slightly reviewed it and you did not answer him, so I inlined here some of his comments to agree/disagree with them. See 8 comments below. On 28/09/2018 19:53, 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. > > 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 | 24 ++++++++++++++++ > src/box/execute.h | 1 + > src/box/lua/net_box.c | 21 ++++++++++++-- > src/box/sequence.c | 29 ++++++++++++++++++++ > src/box/sql/vdbe.c | 2 +- > src/box/sql/vdbe.h | 3 +- > src/box/sql/vdbeInt.h | 3 ++ > src/box/sql/vdbeaux.c | 15 ++++++++-- > src/box/txn.h | 13 +++++++++ > test/sql/iproto.result | 71 ++++++++++++++++++++++++++++++++++++++++++++++++ > test/sql/iproto.test.lua | 15 ++++++++++ > 11 files changed, 190 insertions(+), 7 deletions(-) > > diff --git a/src/box/execute.c b/src/box/execute.c > index 24459b4..d9fe33f 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/vdbeInt.h" 1. Kostja said: >> Ugh. You're including a header which ends with Int. Have you >> thought what this Int means? It's not Integer. Strictly speaking, I agree with him. Src/box/execute is not an SQL internal file. It even uses SQLite public API to fetch names and types. I guess, you should implement a getter for ids list like const struct stailq * vdbe_auto_ids_list(const struct Vdbe *v); > > const char *sql_type_strs[] = { > NULL, > @@ -639,14 +640,37 @@ err: > assert(port_tuple->size == 0); > if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0) > goto err; > + struct stailq *id_list = &((struct Vdbe *)stmt)->id_list; > + uint64_t id_count = 0; > int changes = sqlite3_changes(db); > int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) + > mp_sizeof_uint(changes); > + if (stailq_empty(id_list) == 0) { 2. Kostja said: >> What is the point of this == 0? Why are you comparing with an >> integer here, not boolean? I partially agree and I guess it is my fault that I am too harsh about code style. You used here == 0 because stailq_empty() returns an integer, but its name looks like a flag getter, so you can use it as a boolean here. > + struct id_entry *id_entry; > + stailq_foreach_entry(id_entry, id_list, link) { > + size += id_entry->id >= 0 ? > + mp_sizeof_uint(id_entry->id) : > + mp_sizeof_int(id_entry->id); > + id_count++; > + } > + size += mp_sizeof_uint(SQL_INFO_GENERATED_IDS) + > + mp_sizeof_array(id_count); > + }> char *buf = obuf_alloc(out, size); > if (buf == NULL) { > diag_set(OutOfMemory, size, "obuf_alloc", "buf"); > goto err; > } > + if (id_count > 0) { 3. Lets be consistent: either you use !stailq_empty() in both places, or id_count > 0. > + buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS); > + buf = mp_encode_array(buf, id_count); > + struct id_entry *id_entry; > + stailq_foreach_entry(id_entry, id_list, link) { > + buf = id_entry->id >= 0 ? > + mp_encode_uint(buf, id_entry->id) : > + mp_encode_int(buf, id_entry->id); > + } > + } > buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT); > buf = mp_encode_uint(buf, changes); > } > diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c > index a928a4c..5d2ac78 100644 > --- a/src/box/lua/net_box.c > +++ b/src/box/lua/net_box.c > @@ -671,11 +671,28 @@ netbox_decode_sql_info(struct lua_State *L, const char **data) > /* Only SQL_INFO_ROW_COUNT is available. */ > assert(map_size == 1); 4. How does it work? map_size can be 2 now, and the comment is outdated - not only SQL_INFO_ROW_COUNT is available. > (void) map_size; > + lua_newtable(L); > uint32_t key = mp_decode_uint(data); > + /* > + * If SQL_INFO_GENERATED_IDS in data then it should be > + * just before SQL_INFO_ROW_COUNT. > + */ > + if (key == SQL_INFO_GENERATED_IDS) { > + uint64_t count = mp_decode_array(data); > + 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 value = mp_decode_uint(data); > + lua_pushinteger(L, value); > + lua_rawseti(L, -2, j + 1); > + } > + lua_pop(L, 1); > + key = mp_decode_uint(data); > + } > assert(key == SQL_INFO_ROW_COUNT); > - (void) key; > uint32_t row_count = mp_decode_uint(data); > - lua_createtable(L, 0, 1); > lua_pushinteger(L, row_count); > lua_setfield(L, -2, "rowcount"); > } > diff --git a/src/box/sequence.c b/src/box/sequence.c > index 35b7605..ed6b2da 100644 > --- a/src/box/sequence.c > +++ b/src/box/sequence.c > @@ -38,6 +38,7 @@ > #include > #include > > +#include "txn.h" > #include "diag.h" > #include "error.h" > #include "errcode.h" > @@ -178,6 +179,30 @@ sequence_update(struct sequence *seq, int64_t value) > return 0; > } > > +/** > + * Save generated id in VDBE. > + * > + * @param value ID to save in VDBE. > + * @retval 0 Success. > + * @retval -1 Error. > + */ > +static inline int > +add_new_id_in_vdbe(int64_t value) 5. Kostja said: >> subject-verb-object I agree. He means 'vdbe_add_new_id'. And in such a case it is evidently a part of Vdbe API, so put it into vdbe.h with this signature: int vdbe_add_new_auto_id(struct Vdbe *vdbe, int64_t id); > +{ > + struct txn *txn = in_txn(); > + if (txn == NULL || txn->psql_txn == NULL) > + return 0; > + assert(txn->psql_txn->vdbe != NULL); > + struct id_entry *id_entry = malloc(sizeof(*id_entry)); > + if (id_entry == NULL) { > + diag_set(OutOfMemory, sizeof(*id_entry), "malloc", "id_entry"); > + return -1; > + } > + id_entry->id = value; > + stailq_add_tail_entry(txn->psql_txn->id_list, id_entry, link); 6. If you have Vdbe, you do not need to store the list both in Vdbe and sql_txn. Store it in Vdbe only. 7. Kostja said: >> Why malloc? It is understandable why malloc - txn after commit invalidates the region memory. We neither can save ids on obuf directly since while getting next id a yield is possible and the obuf will be messed with another request. But guess I thought up a solution. It is a bit tricky, so maybe it should be accounted as a separate issue and this solved with malloc. I propose to split txn_commit() into commit and region reset. Txn_commit() goal is to write data to disk, not to destroy a region. It saves us when a transaction is started and committed by Vdbe. But what about autocommit transactions? I think, we should get rid of autocommit transactions for Vdbe. Vdbe always starts a transaction for DML and commits it, but without region reset - it is a part of vdbe finalize routine (which already exists actually). This split of txn_commit logic allows to store anything on a region during Vdbe work and not being afraid about a reset. > + return 0; > +} > + > diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h > index d5da5d5..9b94183 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 vdbe VDBE that is being prepared or executed. > * @retval NULL on OOM, new psql_txn struct on success. > **/ > struct sql_txn * > -sql_alloc_txn(); > +sql_alloc_txn(struct Vdbe *vdbe); 8. I think, alloc should only alloc. sql_txn_begin should put Vdbe into sql_txn.