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

[Oleg Babin <olegrok@tarantool.org>]

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 <value1> Meaning.
>      @retval <value2> 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];
  };
  ]]