From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 1949D469719 for ; Mon, 16 Mar 2020 22:06:01 +0300 (MSK) References: <20200313140612.29775-1-olegrok@tarantool.org> From: Oleg Babin Message-ID: Date: Mon, 16 Mar 2020 22:05:58 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB 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: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org, korablev@tarantool.org Cc: Oleg Babin Hi! Thanks a lot for review! On 15/03/2020 19:13, Vladislav Shpilevoy wrote: > 1. I just realized, C function also should be documented, > since you added it to the public C API. I've add a section about C API changes to docbot request: | C API: | | ```C | int | box_sequence_current(uint32_t seq_id, int64_t *result); | ``` | | Where: | * seq_id - sequence identifier; | * result - pointer to a variable where the current sequence | value will be stored on success. | | Returns 0 on success and -1 otherwise. In case of an error user | could get it via `box_error_last()`. >> 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]; > }; > ]] > ==================== I've apply your changes to my branch. Thanks! >> +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. > >> */ I looked how it was done for "sequence_next" and have changed in the same manner: | /** | * Get last element of given sequence. | * | * @param seq sequence to get value from. | * On success, return 0 and assign the current value of the | * sequence to @result, otherwise return -1 and set diag. | */ >> 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; >> } Fixed. I've already applied this changes to my branch: https://github.com/tarantool/tarantool/tree/olegrok/4752-sequence-current Diff: ============== diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua index 55efb9896..57deeee89 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,9 +2308,9 @@ 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) end local function chpasswd(uid, new_password) diff --git a/src/box/sequence.h b/src/box/sequence.h index 8c442872a..400bdc6f9 100644 --- a/src/box/sequence.h +++ b/src/box/sequence.h @@ -171,8 +171,8 @@ sequence_data_iterator_create(void); * Get last element of given sequence. * * @param seq sequence to get value from. - * @result value of sequence. - * Return 0 on success, -1 if sequence is not initialized. + * On success, return 0 and assign the current value of the + * sequence to @result, otherwise return -1 and set diag. */ int sequence_get_value(struct sequence *seq, int64_t *result); diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 0de07ca1d..e8a029a8a 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4329,9 +4329,8 @@ case OP_IdxInsert: { if (pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) { assert(space->sequence != NULL); int64_t value; - if (sequence_get_value(space->sequence, &value) != 0) { + if (sequence_get_value(space->sequence, &value) != 0) goto abort_due_to_error; - } if (vdbe_add_new_autoinc_id(p, value) != 0) goto abort_due_to_error; } 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]; }; ]]