Re: [Tarantool-patches] [PATCH v3] box: allow to retrieve the last generated value of sequence

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

See 5 comments below.

On 13/03/2020 15:06, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@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;
>  	}