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 300CD2B49A for ; Thu, 20 Sep 2018 12:36: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 PrP9QI-iDamN for ; Thu, 20 Sep 2018 12:36:28 -0400 (EDT) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 DF9862B46C for ; Thu, 20 Sep 2018 12:36:27 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v4 1/1] sql: return all generated ids via IPROTO References: <4041f49f7a62d12d9a6c8a9a452ed4cac267b1e3.1537204943.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Thu, 20 Sep 2018 18:36:22 +0200 MIME-Version: 1.0 In-Reply-To: <4041f49f7a62d12d9a6c8a9a452ed4cac267b1e3.1537204943.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: imeevma@tarantool.org, tarantool-patches@freelists.org Hi! Thanks for the patch! See 10 comments below. On 17/09/2018 19:23, 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 implements such functinality. 1. Typos: 'way to implements', 'functinality'. > > 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 | 17 ++++- > src/box/execute.h | 1 + > src/box/lua/net_box.c | 16 ++++- > src/box/sequence.c | 37 ++++++++++ > src/box/sql/sqliteInt.h | 4 ++ > src/box/sql/vdbeaux.c | 29 ++++++++ > src/box/txn.c | 5 ++ > src/box/txn.h | 6 ++ > test/sql/errinj.result | 3 +- > test/sql/iproto.result | 171 ++++++++++++++++++++++++++++++++++++++--------- > test/sql/iproto.test.lua | 15 +++++ > 11 files changed, 266 insertions(+), 38 deletions(-) > > diff --git a/src/box/execute.c b/src/box/execute.c > index 24459b4..8e4bc80 100644 > --- a/src/box/execute.c > +++ b/src/box/execute.c > @@ -641,14 +641,27 @@ err: > goto err; > int changes = sqlite3_changes(db); > int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) + > - mp_sizeof_uint(changes); > + mp_sizeof_uint(changes) + > + mp_sizeof_uint(SQL_INFO_GENERATED_IDS) + > + mp_sizeof_array(db->generated_ids_count); 2. When you use '_count', the noun should be in the singular. > + for (uint64_t i = 0; i < db->generated_ids_count; ++i) { > + size += db->generated_ids_array[i] >= 0 ? 3. Type can be omitted in a variable name (no _array, but s). > + mp_sizeof_uint(db->generated_ids_array[i]) : > + mp_sizeof_int(db->generated_ids_array[i]); > + } > char *buf = obuf_alloc(out, size); > if (buf == NULL) { > diag_set(OutOfMemory, size, "obuf_alloc", "buf"); > - goto err; 4. Why? > } > buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT); > buf = mp_encode_uint(buf, changes); > + buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS); 5. I do not think we should send empty array. Maybe it is better to omit the key in such a case? > + buf = mp_encode_array(buf, db->generated_ids_count); > + for (uint64_t i = 0; i < db->generated_ids_count; ++i) { > + buf = db->generated_ids_array[i] < 0 ? > + mp_encode_int(buf, db->generated_ids_array[i]) : > + mp_encode_uint(buf, db->generated_ids_array[i]); > + } > } > iproto_reply_sql(out, &header_svp, response->sync, schema_version, > keys); > diff --git a/src/box/sequence.c b/src/box/sequence.c > index 35b7605..234509c 100644 > --- a/src/box/sequence.c > +++ b/src/box/sequence.c > @@ -178,6 +180,37 @@ sequence_update(struct sequence *seq, int64_t value) > return 0; > } > > +/* > + * Save generated value into txn. > + * > + * \param value value to save. > + * \retval 0 Success. > + * \retval -1 Error. > + */ > +static inline int > +save_value(int64_t value) > +{ > + struct txn *txn = in_txn(); > + if (txn == NULL) > + return 0; > + if (txn->generated_ids.size == txn->generated_ids.capacity) { 6. Why do you store generated ids in box txn instead of vdbe? As I know, after commit/rollback txn object is freed so it does not matter where do you store ids array - on the heap or on a region - txn.generated_ids attribute will be invalid after commit/rollback. What is more, we do not need ids from non-SQL requests - they are never used. Last time we discussed that you should store result of sequence_next from OP_NextAutoincValue. And at the same place you can save the result into Vdbe generated ids array. This will work the same way as your current implementation, but will not slow down non-SQL requests. > + txn->generated_ids.capacity = txn->generated_ids.capacity > 0 ? > + txn->generated_ids.capacity * 2 : > + SEQUENCE_GENERATED_IDS_MIN_LEN; > + uint64_t capacity = txn->generated_ids.capacity * > + sizeof(*txn->generated_ids.array); > + txn->generated_ids.array = realloc(txn->generated_ids.array, > + capacity); > + if (txn->generated_ids.array == NULL) { > + diag_set(OutOfMemory, txn->generated_ids.capacity, > + "realloc", "txn->generated_ids.array"); > + return -1; > + } > + } > + txn->generated_ids.array[txn->generated_ids.size++] = value; > + return 0; > +} > + > int > sequence_next(struct sequence *seq, int64_t *result) > { > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 1d32c9a..7d41ece 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -1525,6 +1525,10 @@ struct sqlite3 { > u8 mTrace; /* zero or more SQLITE_TRACE flags */ > u32 magic; /* Magic number for detect library misuse */ > int nChange; /* Value returned by sqlite3_changes() */ > + /* Number of generated ids. */ > + uint64_t generated_ids_count; > + /* Array of generated ids. */ > + int64_t *generated_ids_array; 7. Do not store ids in multiple places, and moreover in sqlite3 that will be removed in future. Use struct Vdbe. Ids array is per-request attribute. > int nTotalChange; /* Value returned by sqlite3_total_changes() */ > int aLimit[SQLITE_N_LIMIT]; /* Limits */ > int nMaxSorterMmap; /* Maximum size of regions mapped by sorter */ > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c > index 3b0c90c..7b5d6b9 100644 > --- a/src/box/sql/vdbeaux.c > +++ b/src/box/sql/vdbeaux.c > @@ -2303,6 +2303,29 @@ sql_savepoint(Vdbe *p, const char *zName) > } > > /* > + * Move array of generated ids into db. > + * > + * \param db db to save array to. > + * \retval 0 Success. > + * \retval -1 Error. 8. Use @, not \. 9. You should not have this function. Ids must be stored in Vdbe. > + */ > +static inline int > +move_generated_ids_in_db(sqlite3 *db) > +{ > + struct txn *txn = in_txn(); > + if (txn == NULL) > + return 0; > + uint64_t size = txn->generated_ids.size * > + sizeof(*txn->generated_ids.array); > + db->generated_ids_count = txn->generated_ids.size; > + db->generated_ids_array = malloc(size); > + if (db->generated_ids_array == NULL) > + return -1; > + memcpy(db->generated_ids_array, txn->generated_ids.array, size); > + return 0; > +} > + > +/* > * This routine is called the when a VDBE tries to halt. If the VDBE > * has made changes and is in autocommit mode, then commit those > * changes. If a rollback is needed, then do the rollback. 10. I still think that we should not realloc a huge monolith array of ids. My proposal is to use a list of small arrays. When a last chunk in the list becomes full, we allocate a new chunk, append it to the list and use it for next ids. But you can discuss it with Kirill if you do not agree.