From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2BC71469719 for ; Sun, 15 Mar 2020 19:13:34 +0300 (MSK) References: <20200313140612.29775-1-olegrok@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Sun, 15 Mar 2020 17:13:30 +0100 MIME-Version: 1.0 In-Reply-To: <20200313140612.29775-1-olegrok@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v3] box: allow to retrieve the last generated value of sequence List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: olegrok@tarantool.org, tarantool-patches@dev.tarantool.org, korablev@tarantool.org Cc: Oleg Babin Hi! Thanks for the patch! See 5 comments below. On 13/03/2020 15:06, olegrok@tarantool.org wrote: > From: Oleg Babin > > This patch introduces "current" function for sequences. > It returns the last retrieved value of specified sequence or > throws an error if no value has been generated yet. > > This patch partially reverts 3ff1f1e36e14381c0ebb5862943d4da281254767 > (box: remove sequence_get) here similar function "get" was removed > to avoid possible misleading with "currval" function of PosgreSQL > that returns the last obtained value of the sequence in the scope > of current session. In contrast "current" returns the last globally > retrieved value of the sequence. > > Closes #4752 > > @TarantoolBot document > Title: sequence:current() > > This patch introduces "current" function for sequences. > It returns the last retrieved value of specified sequence or > throws an error if no value has been generated yet ("next" > has not been called yet or right after "reset" is called). 1. I just realized, C function also should be documented, since you added it to the public C API. > diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua > index aba439ffb..55efb9896 100644 > --- a/src/box/lua/schema.lua > +++ b/src/box/lua/schema.lua > @@ -1812,6 +1816,15 @@ sequence_mt.next = function(self) > return internal.sequence.next(self.id) > end > > +local sequence_value = ffi.new('int64_t[1]') 2. Specifically for this there is buffer.reg1/reg2. A pre-allocated set of scalar values and pointers at them. ==================== diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua index 55efb9896..dafa749a9 100644 --- a/src/box/lua/schema.lua +++ b/src/box/lua/schema.lua @@ -7,7 +7,7 @@ local fun = require('fun') local log = require('log') local fio = require('fio') local json = require('json') -local static_alloc = require('buffer').static_alloc +local buffer = require('buffer') local session = box.session local internal = require('box.internal') local function setmap(table) @@ -1816,13 +1816,13 @@ sequence_mt.next = function(self) return internal.sequence.next(self.id) end -local sequence_value = ffi.new('int64_t[1]') sequence_mt.current = function(self) - local rc = builtin.box_sequence_current(self.id, sequence_value) + local ai64 = buffer.reg1.ai64 + local rc = builtin.box_sequence_current(self.id, ai64) if rc < 0 then box.error(box.error.last()) end - return sequence_value[0] + return ai64[0] end sequence_mt.set = function(self, value) @@ -2308,7 +2308,7 @@ box.schema.user = {} box.schema.user.password = function(password) local BUF_SIZE = 128 - local buf = static_alloc('char', BUF_SIZE) + local buf = buffer.static_alloc('char', BUF_SIZE) builtin.password_prepare(password, #password, buf, BUF_SIZE) return ffi.string(buf) end diff --git a/src/lua/buffer.lua b/src/lua/buffer.lua index ef0118c7c..9aac82b39 100644 --- a/src/lua/buffer.lua +++ b/src/lua/buffer.lua @@ -63,6 +63,7 @@ union c_register { uint32_t u32; uint64_t u64; int64_t i64; + int64_t ai64[1]; }; ]] ==================== > +sequence_mt.current = function(self) > + local rc = builtin.box_sequence_current(self.id, sequence_value) > + if rc < 0 then > + box.error(box.error.last()) > + end > + return sequence_value[0] > +end > + > sequence_mt.set = function(self, value) > return internal.sequence.set(self.id, value) > end > diff --git a/src/box/sequence.c b/src/box/sequence.c > index 5ebfa2747..4afbc2666 100644 > --- a/src/box/sequence.c > +++ b/src/box/sequence.c > @@ -367,14 +367,18 @@ sequence_data_iterator_create(void) > return &iter->base; > } > > -int64_t > -sequence_get_value(struct sequence *seq) > +int > +sequence_get_value(struct sequence *seq, int64_t *result) > { > uint32_t key = seq->def->id; > uint32_t hash = sequence_hash(key); > uint32_t pos = light_sequence_find_key(&sequence_data_index, hash, key); > - assert(pos != light_sequence_end); > + if (pos == light_sequence_end) { > + diag_set(ClientError, ER_SEQUENCE_NOT_STARTED, seq->def->name); > + return -1; > + } > struct sequence_data data = light_sequence_get(&sequence_data_index, > pos); > - return data.value; > + *result = data.value; > + return 0; > } > diff --git a/src/box/sequence.h b/src/box/sequence.h > index a164da9af..8c442872a 100644 > --- a/src/box/sequence.h > +++ b/src/box/sequence.h > @@ -171,10 +171,11 @@ sequence_data_iterator_create(void); > * Get last element of given sequence. > * > * @param seq sequence to get value from. > - * @retval last element of sequence. > + * @result value of sequence. 3. According to doxygen doc, @result is the same as @return. So this comment says "return value of sequence". I think you wanted to refer to 'result' parameter. For that doxygen provides command @param. > + * Return 0 on success, -1 if sequence is not initialized. 4. Doxygen way of documenting return values is either @retval Meaning. @retval Meaning. Or @return/returns Meaning. > */ > -int64_t > -sequence_get_value(struct sequence *seq); > +int > +sequence_get_value(struct sequence *seq, int64_t *result); > > #if defined(__cplusplus) > } /* extern "C" */ > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 912a10153..0de07ca1d 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4328,7 +4328,10 @@ case OP_IdxInsert: { > p->nChange++; > if (pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) { > assert(space->sequence != NULL); > - int64_t value = sequence_get_value(space->sequence); > + int64_t value; > + if (sequence_get_value(space->sequence, &value) != 0) { > + goto abort_due_to_error; 5. We usually omit {} for one-line if-body. > + } > if (vdbe_add_new_autoinc_id(p, value) != 0) > goto abort_due_to_error; > }