[Tarantool-patches] [PATCH v3] box: allow to retrieve the last generated value of sequence
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun Mar 15 19:13:30 MSK 2020
Hi! Thanks for the patch!
See 5 comments below.
On 13/03/2020 15:06, olegrok at tarantool.org wrote:
> From: Oleg Babin <babinoleg at mail.ru>
>
> 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 <value1> Meaning.
@retval <value2> 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;
> }
More information about the Tarantool-patches
mailing list