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 CDC562C791 for ; Sat, 29 Sep 2018 00:36:23 -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 wc9oDt1Aar8V for ; Sat, 29 Sep 2018 00:36:23 -0400 (EDT) Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (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 54AFD2C772 for ; Sat, 29 Sep 2018 00:36:23 -0400 (EDT) Date: Sat, 29 Sep 2018 07:36:17 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH v5 1/1] sql: return all generated ids via IPROTO Message-ID: <20180929043617.GD32712@chai> References: <23421254ee60958d4583af90100d77b222eb2aad.1538153530.git.imeevma@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <23421254ee60958d4583af90100d77b222eb2aad.1538153530.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: v.shpilevoy@tarantool.org * imeevma@tarantool.org [18/09/28 19:53]: > 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" Ugh. You're including a header which ends with Int. Have you thought what this Int means? It's not Integer. > > 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; The name is very general. autoinc_id_list would be more specific. Why not simply call it generated_ids here and everywhere? This is what these values are called in the standard. > int changes = sqlite3_changes(db); > int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) + > mp_sizeof_uint(changes); > + if (stailq_empty(id_list) == 0) { What is the point of this == 0? Why are you comparing with an integer here, not boolean? > + 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); How can you get a negative 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) { > + 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/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, > sql_info_key_MAX, > }; > > 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); > (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) subject-verb-object > +{ > + 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)); Why malloc? > + 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); > + return 0; > +} > + > int > sequence_next(struct sequence *seq, int64_t *result) > { > @@ -194,6 +219,8 @@ sequence_next(struct sequence *seq, int64_t *result) > new_data) == light_sequence_end) > return -1; > *result = def->start; > + if(add_new_id_in_vdbe(*result) != 0) > + return -1; > return 0; > } > old_data = light_sequence_get(&sequence_data_index, pos); > @@ -228,6 +255,8 @@ done: > new_data, &old_data) == light_sequence_end) > unreachable(); > *result = value; > + if(add_new_id_in_vdbe(*result) != 0) > + return -1; You continue to miss a space after if. > return 0; > overflow: > if (!def->cycle) { > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 00ffb0c..cbec9a2 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -3040,7 +3040,7 @@ case OP_TransactionRollback: { > */ > case OP_TTransaction: { > if (!box_txn()) { > - if (box_txn_begin() != 0) { > + if (sql_txn_begin(p) != 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 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); > > /** > * Prepare given VDBE to execution: initialize structs connected > diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h > index ce97f49..ca1794c 100644 > --- a/src/box/sql/vdbeInt.h > +++ b/src/box/sql/vdbeInt.h > @@ -368,6 +368,9 @@ struct Vdbe { > /** The auto-commit flag. */ > bool auto_commit; > > + /** List of id generated in current VDBE. */ > + struct stailq id_list; > + > /* 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 54edf1b..98ddef7 100644 > --- a/src/box/sql/vdbeaux.c > +++ b/src/box/sql/vdbeaux.c > @@ -57,6 +57,8 @@ sqlite3VdbeCreate(Parse * pParse) > return 0; > memset(p, 0, sizeof(Vdbe)); > p->db = db; > + /* Init list of generated ids. */ > + stailq_create(&p->id_list); > if (db->pVdbe) { > db->pVdbe->pPrev = p; > } > @@ -75,7 +77,7 @@ sqlite3VdbeCreate(Parse * pParse) > } > > struct sql_txn * > -sql_alloc_txn() > +sql_alloc_txn(struct Vdbe *vdbe) > { > struct sql_txn *txn = region_alloc_object(&fiber()->gc, > struct sql_txn); > @@ -86,6 +88,8 @@ sql_alloc_txn() > } > txn->pSavepoint = NULL; > txn->fk_deferred_count = 0; > + txn->vdbe = vdbe; > + txn->id_list = &vdbe->id_list; Why do you need both? > return txn; > } > > @@ -103,7 +107,7 @@ 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; > } > @@ -2261,7 +2265,7 @@ 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; > @@ -2746,6 +2750,11 @@ sqlite3VdbeClearObject(sqlite3 * db, Vdbe * p) > vdbeFreeOpArray(db, p->aOp, p->nOp); > sqlite3DbFree(db, p->aColName); > sqlite3DbFree(db, p->zSql); > + while (stailq_empty(&p->id_list) == 0) { > + struct id_entry *id_entry = > + stailq_shift_entry(&p->id_list, struct id_entry, link); > + free(id_entry); > + } > #ifdef SQLITE_ENABLE_STMT_SCANSTATUS > { -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov