[Tarantool-patches] [PATCH] box: introduce "current" for sequence

[Tarantool-patches] [PATCH] box: introduce "current" for sequence

[olegrok]

From: Oleg Babin <babinoleg@mail.ru>

This patch introduces "current" function for
sequences. It returns current value of
specified sequence or throws an error
if sequence is not initialized ("next" called
at least once).

This patch partitially reverts 3ff1f1e36e14381c0ebb5862943d4da281254767
Closes #4752

@TarantoolBot document
Title: sequence:current()

New function returns current value of sequence
of throws an error if used before sequence
initialization.

Example

```lua
sq = box.schema.sequence.create('test')
---
...
sq:current()
---
- error: Sequence is not initialized yet
...
sq:next()
---
- 1
...
sq:current()
---
- 1
...
sq:set(42)
---
...
sq:current()
---
- 42
...
```
---
Issue: https://github.com/tarantool/tarantool/issues/4752
Branch: https://github.com/tarantool/tarantool/tree/olegrok/4752-sequence-current
 src/box/box.cc             | 14 ++++++++
 src/box/box.h              | 12 +++++++
 src/box/errcode.h          |  1 +
 src/box/lua/schema.lua     |  4 +++
 src/box/lua/sequence.c     | 12 +++++++
 src/box/sequence.c         | 12 ++++---
 src/box/sequence.h         |  7 ++--
 src/box/sql/vdbe.c         |  5 ++-
 test/box/misc.result       |  1 +
 test/box/sequence.result   | 68 +++++++++++++++++++++++++++++++++++++-
 test/box/sequence.test.lua | 22 ++++++++++++
 11 files changed, 149 insertions(+), 9 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 09dd67ab4..a5052dba4 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1414,6 +1414,20 @@ box_sequence_next(uint32_t seq_id, int64_t *result)
 	*result = value;
 	return 0;
 }
+
+int
+box_sequence_current(uint32_t seq_id, int64_t *result)
+{
+	struct sequence *seq = sequence_cache_find(seq_id);
+	if (seq == NULL)
+		return -1;
+	if (access_check_sequence(seq) != 0)
+		return -1;
+	if (sequence_get_value(seq, result) != 0)
+		return -1;
+	return 0;
+}
+
 int
 box_sequence_set(uint32_t seq_id, int64_t value)
 {
diff --git a/src/box/box.h b/src/box/box.h
index f37a945eb..8fb723630 100644
--- a/src/box/box.h
+++ b/src/box/box.h
@@ -423,6 +423,18 @@ box_truncate(uint32_t space_id);
 API_EXPORT int
 box_sequence_next(uint32_t seq_id, int64_t *result);
 
+/**
+ * Returns current value of sequence.
+ *
+ * \param seq_id sequence identifier
+ * \param[out] result pointer to a variable where the current sequence
+ * value will be stored on success
+ * \retval -1 on error (check box_error_last())
+ * \retval 0 on success
+ */
+API_EXPORT int
+box_sequence_current(uint32_t seq_id, int64_t *result);
+
 /**
  * Set a sequence value.
  *
diff --git a/src/box/errcode.h b/src/box/errcode.h
index d7ec97e8c..444171778 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -264,6 +264,7 @@ struct errcode_record {
 	/*209 */_(ER_SESSION_SETTING_INVALID_VALUE,	"Session setting %s expected a value of type %s") \
 	/*210 */_(ER_SQL_PREPARE,		"Failed to prepare SQL statement: %s") \
 	/*211 */_(ER_WRONG_QUERY_ID,		"Prepared statement with id %u does not exist") \
+	/*212 */_(ER_SEQUENCE_NOT_STARTED,		"Sequence '%s' is not started") \
 
 /*
  * !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index f537c3cec..0cd747350 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1812,6 +1812,10 @@ sequence_mt.next = function(self)
     return internal.sequence.next(self.id)
 end
 
+sequence_mt.current = function(self)
+    return internal.sequence.current(self.id)
+end
+
 sequence_mt.set = function(self, value)
     return internal.sequence.set(self.id, value)
 end
diff --git a/src/box/lua/sequence.c b/src/box/lua/sequence.c
index bf0714c1a..ae4b5d3ce 100644
--- a/src/box/lua/sequence.c
+++ b/src/box/lua/sequence.c
@@ -49,6 +49,17 @@ lbox_sequence_next(struct lua_State *L)
 	return 1;
 }
 
+static int
+lbox_sequence_current(struct lua_State *L)
+{
+	uint32_t seq_id = luaL_checkinteger(L, 1);
+	int64_t result;
+	if (box_sequence_current(seq_id, &result) != 0)
+		luaT_error(L);
+	luaL_pushint64(L, result);
+	return 1;
+}
+
 static int
 lbox_sequence_set(struct lua_State *L)
 {
@@ -174,6 +185,7 @@ box_lua_sequence_init(struct lua_State *L)
 {
 	static const struct luaL_Reg sequence_internal_lib[] = {
 		{"next", lbox_sequence_next},
+		{"current", lbox_sequence_current},
 		{"set", lbox_sequence_set},
 		{"reset", lbox_sequence_reset},
 		{NULL, NULL}
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.
+ * Return 0 on success, -1 if sequence is not initialized.
  */
-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;
+		}
 		if (vdbe_add_new_autoinc_id(p, value) != 0)
 			goto abort_due_to_error;
 	}
diff --git a/test/box/misc.result b/test/box/misc.result
index 5ac5e0f26..047591b60 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -636,6 +636,7 @@ t;
   209: box.error.SESSION_SETTING_INVALID_VALUE
   210: box.error.SQL_PREPARE
   211: box.error.WRONG_QUERY_ID
+  212: box.error.SEQUENCE_NOT_STARTED
 ...
 test_run:cmd("setopt delimiter ''");
 ---
diff --git a/test/box/sequence.result b/test/box/sequence.result
index 0a6cfee2c..2eee1ba99 100644
--- a/test/box/sequence.result
+++ b/test/box/sequence.result
@@ -188,10 +188,18 @@ sq.step, sq.min, sq.max, sq.start, sq.cycle
 - 1
 - false
 ...
+sq:current()  -- error
+---
+- error: Sequence 'test' is not started
+...
 sq:next() -- 1
 ---
 - 1
 ...
+sq:current()  -- 1
+---
+- 1
+...
 sq:next() -- 2
 ---
 - 2
@@ -199,6 +207,10 @@ sq:next() -- 2
 sq:set(100)
 ---
 ...
+sq:current()  -- 100
+---
+- 100
+...
 sq:next() -- 101
 ---
 - 101
@@ -210,6 +222,10 @@ sq:next() -- 102
 sq:reset()
 ---
 ...
+sq:current()  -- error
+---
+- error: Sequence 'test' is not started
+...
 sq:next() -- 1
 ---
 - 1
@@ -233,10 +249,18 @@ sq.step, sq.min, sq.max, sq.start, sq.cycle
 - -1
 - false
 ...
+sq:current()  -- error
+---
+- error: Sequence 'test' is not started
+...
 sq:next() -- -1
 ---
 - -1
 ...
+sq:current()  -- -1
+---
+- -1
+...
 sq:next() -- -2
 ---
 - -2
@@ -244,6 +268,10 @@ sq:next() -- -2
 sq:set(-100)
 ---
 ...
+sq:current()  -- -100
+---
+- -100
+...
 sq:next() -- -101
 ---
 - -101
@@ -255,6 +283,10 @@ sq:next() -- -102
 sq:reset()
 ---
 ...
+sq:current()  -- error
+---
+- error: Sequence 'test' is not started
+...
 sq:next() -- -1
 ---
 - -1
@@ -1428,7 +1460,7 @@ box.schema.user.info()
     - _priv
   - - session,usage
     - universe
-    - 
+    -
   - - alter
     - user
     - user
@@ -2289,3 +2321,37 @@ box.space._space_sequence:update({s.id}, {{'=', 2, t[2]}})
 s:drop()
 ---
 ...
+--
+-- gh-4752: introduce sequence:current() method which
+-- fetches current sequence value but doesn't modify
+-- sequence itself.
+--
+sq = box.schema.sequence.create('test')
+---
+...
+sq:current()
+---
+- error: Sequence 'test' is not started
+...
+sq:next()
+---
+- 1
+...
+sq:current()
+---
+- 1
+...
+sq:set(42)
+---
+...
+sq:current()
+---
+- 42
+...
+sq:current()
+---
+- 42
+...
+sq:drop()
+---
+...
diff --git a/test/box/sequence.test.lua b/test/box/sequence.test.lua
index 8e00571e5..2342178e3 100644
--- a/test/box/sequence.test.lua
+++ b/test/box/sequence.test.lua
@@ -58,12 +58,16 @@ box.sequence.test == nil
 -- Default ascending sequence.
 sq = box.schema.sequence.create('test')
 sq.step, sq.min, sq.max, sq.start, sq.cycle
+sq:current()  -- error
 sq:next() -- 1
+sq:current()  -- 1
 sq:next() -- 2
 sq:set(100)
+sq:current()  -- 100
 sq:next() -- 101
 sq:next() -- 102
 sq:reset()
+sq:current()  -- error
 sq:next() -- 1
 sq:next() -- 2
 sq:drop()
@@ -71,12 +75,16 @@ sq:drop()
 -- Default descending sequence.
 sq = box.schema.sequence.create('test', {step = -1})
 sq.step, sq.min, sq.max, sq.start, sq.cycle
+sq:current()  -- error
 sq:next() -- -1
+sq:current()  -- -1
 sq:next() -- -2
 sq:set(-100)
+sq:current()  -- -100
 sq:next() -- -101
 sq:next() -- -102
 sq:reset()
+sq:current()  -- error
 sq:next() -- -1
 sq:next() -- -2
 sq:drop()
@@ -780,3 +788,17 @@ pk = s:create_index('pk', {sequence = true})
 t = box.space._space_sequence:get({s.id})
 box.space._space_sequence:update({s.id}, {{'=', 2, t[2]}})
 s:drop()
+
+--
+-- gh-4752: introduce sequence:current() method which
+-- fetches current sequence value but doesn't modify
+-- sequence itself.
+--
+sq = box.schema.sequence.create('test')
+sq:current()
+sq:next()
+sq:current()
+sq:set(42)
+sq:current()
+sq:current()
+sq:drop()
-- 
2.23.0

Re: [Tarantool-patches] [PATCH] box: introduce "current" for sequence

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 06/03/2020 17:34, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>

JFYI, commit message width limit is not 46 symbols.
It is 66 or 80 - something around that. I usually use
66. 46 is hard to follow.

See 6 comments below.

> This patch introduces "current" function for
> sequences. It returns current value of
> specified sequence or throws an error
> if sequence is not initialized ("next" called
> at least once).
> 
> This patch partitially reverts 3ff1f1e36e14381c0ebb5862943d4da281254767

1. partitially -> partially

2. Please, leave an empty line before 'Closes',
and put the dot in the end of sentence.

> Closes #4752
> 
> @TarantoolBot document
> Title: sequence:current()
> 
> New function returns current value of sequence
> of throws an error if used before sequence
> initialization.
> 
> Example
> 
> ```lua
> sq = box.schema.sequence.create('test')
> ---
> ...
> sq:current()
> ---
> - error: Sequence is not initialized yet
> ...

3. This looks strange. Why is not initialized? The sequence is
already created. How is it possible to create 'half' of a
sequence, so it is not fully initialized? There is no 'init'
method to call after 'create'.

This single error will make users handle it in their code, wrap
into pcall(), and so on, just for one error. At least, it could
return nil or something like that. Or return (start - step) value.
Like 'before first'. I would go for the second option. In that
case the function would never fail.

If there are real reasons to fail in case a sequence is not
started, then it should return (nil, err), according to our Lua
code style. To avoid pcall(). I don't really know if pcall() is
so expensive. But anyway.

Besides, the error message is not the same as I see in
the patch. In the patch it is "Sequence '%s' is not started".

> sq:next()
> ---
> - 1
> ...
> sq:current()
> ---
> - 1
> ...
> sq:set(42)
> ---
> ...
> sq:current()
> ---
> - 42
> ...
> ```
> ---
> Issue: https://github.com/tarantool/tarantool/issues/4752
> Branch: https://github.com/tarantool/tarantool/tree/olegrok/4752-sequence-current

@ChangeLog
- sequence:current() - a function to get sequence's current
  value, without changing it (gh-4752).

>  src/box/box.cc             | 14 ++++++++
>  src/box/box.h              | 12 +++++++
>  src/box/errcode.h          |  1 +
>  src/box/lua/schema.lua     |  4 +++
>  src/box/lua/sequence.c     | 12 +++++++
>  src/box/sequence.c         | 12 ++++---
>  src/box/sequence.h         |  7 ++--
>  src/box/sql/vdbe.c         |  5 ++-
>  test/box/misc.result       |  1 +
>  test/box/sequence.result   | 68 +++++++++++++++++++++++++++++++++++++-
>  test/box/sequence.test.lua | 22 ++++++++++++
>  11 files changed, 149 insertions(+), 9 deletions(-)
> 
> diff --git a/src/box/box.h b/src/box/box.h
> index f37a945eb..8fb723630 100644
> --- a/src/box/box.h
> +++ b/src/box/box.h
> @@ -423,6 +423,18 @@ box_truncate(uint32_t space_id);
>  API_EXPORT int
>  box_sequence_next(uint32_t seq_id, int64_t *result);
>  
> +/**
> + * Returns current value of sequence.

4. I would add what means 'current' - a value which will
be returned from next next() call, or returned from last
next() call?

> + *
> + * \param seq_id sequence identifier
> + * \param[out] result pointer to a variable where the current sequence
> + * value will be stored on success
> + * \retval -1 on error (check box_error_last())
> + * \retval 0 on success
> + */
> +API_EXPORT int
> +box_sequence_current(uint32_t seq_id, int64_t *result);
> +
> diff --git a/src/box/lua/sequence.c b/src/box/lua/sequence.c
> index bf0714c1a..ae4b5d3ce 100644
> --- a/src/box/lua/sequence.c
> +++ b/src/box/lua/sequence.c
> @@ -49,6 +49,17 @@ lbox_sequence_next(struct lua_State *L)
>  	return 1;
>  }
>  
> +static int
> +lbox_sequence_current(struct lua_State *L)
> +{
> +	uint32_t seq_id = luaL_checkinteger(L, 1);
> +	int64_t result;
> +	if (box_sequence_current(seq_id, &result) != 0)
> +		luaT_error(L);
> +	luaL_pushint64(L, result);
> +	return 1;
> +}

5. Seems like box_sequence_current() does not yield. So it
can be called via FFI. Should be faster.

> +
>  static int
>  lbox_sequence_set(struct lua_State *L)
>  {> diff --git a/test/box/sequence.result b/test/box/sequence.result
> index 0a6cfee2c..2eee1ba99 100644
> --- a/test/box/sequence.result
> +++ b/test/box/sequence.result
> @@ -1428,7 +1460,7 @@ box.schema.user.info()
>      - _priv
>    - - session,usage
>      - universe
> -    - 
> +    -

6. What is this?

>    - - alter
>      - user
>      - user

Re: [Tarantool-patches] [PATCH] box: introduce "current" for sequence

[Oleg Babin]

Hi! Thanks for your review!

On 10/03/2020 02:40, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 06/03/2020 17:34, olegrok@tarantool.org wrote:
>> From: Oleg Babin <babinoleg@mail.ru>
> 
> JFYI, commit message width limit is not 46 symbols.
> It is 66 or 80 - something around that. I usually use
> 66. 46 is hard to follow.
> 

I've edited my commit message, I hope this time it should be better.

> See 6 comments below.
> 
>> This patch introduces "current" function for
>> sequences. It returns current value of
>> specified sequence or throws an error
>> if sequence is not initialized ("next" called
>> at least once).
>>
>> This patch partitially reverts 3ff1f1e36e14381c0ebb5862943d4da281254767
> 
> 1. partitially -> partially
> 

Fixed.

> 2. Please, leave an empty line before 'Closes',
> and put the dot in the end of sentence.
> 

Fixed.

>> Closes #4752
>>
>> @TarantoolBot document
>> Title: sequence:current()
>>
>> New function returns current value of sequence
>> of throws an error if used before sequence
>> initialization.
>>
>> Example
>>
>> ```lua
>> sq = box.schema.sequence.create('test')
>> ---
>> ...
>> sq:current()
>> ---
>> - error: Sequence is not initialized yet
>> ...
> 
> 3. This looks strange. Why is not initialized? The sequence is
> already created. How is it possible to create 'half' of a
> sequence, so it is not fully initialized? There is no 'init'
> method to call after 'create'.
> 
Yes, may be it's not a clear enough statement.
Consider the following description:
     box: allow to retrieve the last generated value of sequence

     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. As Tarantool has no sessions therefore "current"
     returns the last globally retrieved value of the sequence.


> This single error will make users handle it in their code, wrap
> into pcall(), and so on, just for one error. At least, it could
> return nil or something like that. Or return (start - step) value.
> Like 'before first'. I would go for the second option. In that
> case the function would never fail.

Firstly, as I see "lbox_sequence_next", "lbox_sequence_set" and another 
functions raise an error in case of possible problems. It will not be 
consistent if new function will return `nil, error` pair. Also the 
absence of last returned value is not a single reason of the possible error.
Secondly, I preserved the behaviour of the function that was initially 
introduced and then removed: 
https://github.com/tarantool/tarantool/commit/f797eec424ed9f0df86937568e34f6109af065be#diff-0e3513c6b61a1b1395ec235869120107R58
Thirdly, I thought about `start - step` variant but it's not appropriate 
because could significantly overcomplicate our code. The cases of 
underflow and overflow should be considered. It's quite unpredictable 
(when compared to an explicit error) if
user set min value to INT64_MIN and step 1 and get INT64_MAX from "current".

> If there are real reasons to fail in case a sequence is not
> started, then it should return (nil, err), according to our Lua
> code style. To avoid pcall(). I don't really know if pcall() is
> so expensive. But anyway.
> 
> Besides, the error message is not the same as I see in
> the patch. In the patch it is "Sequence '%s' is not started".
> 

Yes, my bad. I've forgot to update description since the first version 
of the patch. I've pushed the correct variant: 
https://github.com/tarantool/tarantool/tree/olegrok/4752-sequence-current

>> sq:next()
>> ---
>> - 1
>> ...
>> sq:current()
>> ---
>> - 1
>> ...
>> sq:set(42)
>> ---
>> ...
>> sq:current()
>> ---
>> - 42
>> ...
>> ```
>> ---
>> Issue: https://github.com/tarantool/tarantool/issues/4752
>> Branch: https://github.com/tarantool/tarantool/tree/olegrok/4752-sequence-current
> 
> @ChangeLog
> - sequence:current() - a function to get sequence's current
>    value, without changing it (gh-4752).
> 
>>   src/box/box.cc             | 14 ++++++++
>>   src/box/box.h              | 12 +++++++
>>   src/box/errcode.h          |  1 +
>>   src/box/lua/schema.lua     |  4 +++
>>   src/box/lua/sequence.c     | 12 +++++++
>>   src/box/sequence.c         | 12 ++++---
>>   src/box/sequence.h         |  7 ++--
>>   src/box/sql/vdbe.c         |  5 ++-
>>   test/box/misc.result       |  1 +
>>   test/box/sequence.result   | 68 +++++++++++++++++++++++++++++++++++++-
>>   test/box/sequence.test.lua | 22 ++++++++++++
>>   11 files changed, 149 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/box/box.h b/src/box/box.h
>> index f37a945eb..8fb723630 100644
>> --- a/src/box/box.h
>> +++ b/src/box/box.h
>> @@ -423,6 +423,18 @@ box_truncate(uint32_t space_id);
>>   API_EXPORT int
>>   box_sequence_next(uint32_t seq_id, int64_t *result);
>>   
>> +/**
>> + * Returns current value of sequence.
> 
> 4. I would add what means 'current' - a value which will
> be returned from next next() call, or returned from last
> next() call?
> 
Changed to "Get the last value returned by a sequence".

>> + *
>> + * \param seq_id sequence identifier
>> + * \param[out] result pointer to a variable where the current sequence
>> + * value will be stored on success
>> + * \retval -1 on error (check box_error_last())
>> + * \retval 0 on success
>> + */
>> +API_EXPORT int
>> +box_sequence_current(uint32_t seq_id, int64_t *result);
>> +
>> diff --git a/src/box/lua/sequence.c b/src/box/lua/sequence.c
>> index bf0714c1a..ae4b5d3ce 100644
>> --- a/src/box/lua/sequence.c
>> +++ b/src/box/lua/sequence.c
>> @@ -49,6 +49,17 @@ lbox_sequence_next(struct lua_State *L)
>>   	return 1;
>>   }
>>   
>> +static int
>> +lbox_sequence_current(struct lua_State *L)
>> +{
>> +	uint32_t seq_id = luaL_checkinteger(L, 1);
>> +	int64_t result;
>> +	if (box_sequence_current(seq_id, &result) != 0)
>> +		luaT_error(L);
>> +	luaL_pushint64(L, result);
>> +	return 1;
>> +}
> 
> 5. Seems like box_sequence_current() does not yield. So it
> can be called via FFI. Should be faster.
> 
The reasons why I have not done yet are similar to an error contract. 
All sequence functions use Lua C API, not FFI. It could be done in the 
separate patch. I could file an issue. Ok?
>> +
>>   static int
>>   lbox_sequence_set(struct lua_State *L)
>>   {> diff --git a/test/box/sequence.result b/test/box/sequence.result
>> index 0a6cfee2c..2eee1ba99 100644
>> --- a/test/box/sequence.result
>> +++ b/test/box/sequence.result
>> @@ -1428,7 +1460,7 @@ box.schema.user.info()
>>       - _priv
>>     - - session,usage
>>       - universe
>> -    -
>> +    -
> 
> 6. What is this?
> 
>>     - - alter
>>       - user
>>       - user
It was a trailing space that was removed by my IDE. I've reverted this 
change because it caused test fail.

Re: [Tarantool-patches] [PATCH] box: introduce "current" for sequence

[Vladislav Shpilevoy]

Thanks for the fixes!

>     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. As Tarantool has no sessions therefore "current"
>     returns the last globally retrieved value of the sequence.

Here you said, that Tarantool has no sessions, but it is not so. We
have sessions, and even some session local things such as settings,
storage.

>> This single error will make users handle it in their code, wrap
>> into pcall(), and so on, just for one error. At least, it could
>> return nil or something like that. Or return (start - step) value.
>> Like 'before first'. I would go for the second option. In that
>> case the function would never fail.
> 
> Firstly, as I see "lbox_sequence_next", "lbox_sequence_set" and another functions raise an error in case of possible problems. It will not be consistent if new function will return `nil, error` pair. Also the absence of last returned value is not a single reason of the possible error.

Yeah, agree. I was thinking they fail only at OOM (which is allowed
to throw). But seems they also do access check.

> Secondly, I preserved the behaviour of the function that was initially introduced and then removed: https://github.com/tarantool/tarantool/commit/f797eec424ed9f0df86937568e34f6109af065be#diff-0e3513c6b61a1b1395ec235869120107R58

Well, this is not a real reason. The fact that something was implemented
in one way long time ago does not mean we should do this again exactly
in the same way.

> Thirdly, I thought about `start - step` variant but it's not appropriate because could significantly overcomplicate our code. The cases of underflow and overflow should be considered. It's quite unpredictable (when compared to an explicit error) if
> user set min value to INT64_MIN and step 1 and get INT64_MAX from "current".

Yes, true. Lets keep it then.

>>> + *
>>> + * \param seq_id sequence identifier
>>> + * \param[out] result pointer to a variable where the current sequence
>>> + * value will be stored on success
>>> + * \retval -1 on error (check box_error_last())
>>> + * \retval 0 on success
>>> + */
>>> +API_EXPORT int
>>> +box_sequence_current(uint32_t seq_id, int64_t *result);
>>> +
>>> diff --git a/src/box/lua/sequence.c b/src/box/lua/sequence.c
>>> index bf0714c1a..ae4b5d3ce 100644
>>> --- a/src/box/lua/sequence.c
>>> +++ b/src/box/lua/sequence.c
>>> @@ -49,6 +49,17 @@ lbox_sequence_next(struct lua_State *L)
>>>       return 1;
>>>   }
>>>   +static int
>>> +lbox_sequence_current(struct lua_State *L)
>>> +{
>>> +    uint32_t seq_id = luaL_checkinteger(L, 1);
>>> +    int64_t result;
>>> +    if (box_sequence_current(seq_id, &result) != 0)
>>> +        luaT_error(L);
>>> +    luaL_pushint64(L, result);
>>> +    return 1;
>>> +}
>>
>> 5. Seems like box_sequence_current() does not yield. So it
>> can be called via FFI. Should be faster.
>>
> The reasons why I have not done yet are similar to an error contract. All sequence functions use Lua C API, not FFI. It could be done in the separate patch. I could file an issue. Ok?

FFI has nothing to do with contracts. It is about performance.
We don't have a rule, that a whole subsystem should be either
completely in FFI, or completely in Lua C. Lots of things are
implemented in FFI + Lua C + Lua. For example, fiber module -
it uses all the 3 ways simultaneously. box_select() is FFI for
memtx, is Lua C for vinyl, and so on.

On the contrary, we have a rule to make things via FFI when it
is possible (there was a discussion about that recently, don't
know whether it was formalized anywhere).

Also I don't see any reason to make it FFI in a separate patch.
Why not in this one? What is a purpose of introducing a function
and re-implementing it right in a next patch and even create an
issue for that?

The most reasonable split I see here is to introduce the C
function in one patch, and FFI in Lua in a second patch. In
scope of one patchset.

Re: [Tarantool-patches] [PATCH] box: introduce "current" for sequence

[Oleg Babin]

Thanks for comments!

On 11/03/2020 01:49, Vladislav Shpilevoy wrote:
> Here you said, that Tarantool has no sessions, but it is not so. We
> have sessions, and even some session local things such as settings,
> storage.
> 

It was a cite from 
https://github.com/tarantool/tarantool/commit/3ff1f1e36e14381c0ebb5862943d4da281254767:
| In contrast to PostgreSQL, this method doesn't make sense in 
  | Tarantool, because we don't have sessions.

It was said before _session_storage was implemented but box.session has 
been already existed. I will try to drop this misleading statement and 
add: `In contrast "current" returns the last globally retrieved value of 
the sequence`. Ok?

> FFI has nothing to do with contracts. It is about performance.
> We don't have a rule, that a whole subsystem should be either
> completely in FFI, or completely in Lua C. Lots of things are
> implemented in FFI + Lua C + Lua. For example, fiber module -
> it uses all the 3 ways simultaneously. box_select() is FFI for
> memtx, is Lua C for vinyl, and so on.
> 
> On the contrary, we have a rule to make things via FFI when it
> is possible (there was a discussion about that recently, don't
> know whether it was formalized anywhere).
> 
> Also I don't see any reason to make it FFI in a separate patch.
> Why not in this one? What is a purpose of introducing a function
> and re-implementing it right in a next patch and even create an
> issue for that?
> 
> The most reasonable split I see here is to introduce the C
> function in one patch, and FFI in Lua in a second patch. In
> scope of one patchset.

As we can use FFI for all functions that doesn't yield, we can rewrite 
e.g. "next" with FFI as well. It will be a bit more uniformly. And the 
idea is to do it in separate patch. But ok, I'll change "current" from 
Lua C API to FFI and send new patch.

--
Oleg Babin

Re: [Tarantool-patches] [PATCH] box: introduce "current" for sequence

[Vladislav Shpilevoy]

On 12/03/2020 16:42, Oleg Babin wrote:
> Thanks for comments!
> 
> On 11/03/2020 01:49, Vladislav Shpilevoy wrote:
>> Here you said, that Tarantool has no sessions, but it is not so. We
>> have sessions, and even some session local things such as settings,
>> storage.
>>
> 
> It was a cite from https://github.com/tarantool/tarantool/commit/3ff1f1e36e14381c0ebb5862943d4da281254767:
> | In contrast to PostgreSQL, this method doesn't make sense in  | Tarantool, because we don't have sessions.
> 
> It was said before _session_storage was implemented but box.session has been already existed. I will try to drop this misleading statement and add: `In contrast "current" returns the last globally retrieved value of the sequence`. Ok?

I don't know why was it said back then that we don't have
sessions. Maybe Vova meant we don't have exactly the same sessions
as PostgreSQL, with full isolation and so on.

>> FFI has nothing to do with contracts. It is about performance.
>> We don't have a rule, that a whole subsystem should be either
>> completely in FFI, or completely in Lua C. Lots of things are
>> implemented in FFI + Lua C + Lua. For example, fiber module -
>> it uses all the 3 ways simultaneously. box_select() is FFI for
>> memtx, is Lua C for vinyl, and so on.
>>
>> On the contrary, we have a rule to make things via FFI when it
>> is possible (there was a discussion about that recently, don't
>> know whether it was formalized anywhere).
>>
>> Also I don't see any reason to make it FFI in a separate patch.
>> Why not in this one? What is a purpose of introducing a function
>> and re-implementing it right in a next patch and even create an
>> issue for that?
>>
>> The most reasonable split I see here is to introduce the C
>> function in one patch, and FFI in Lua in a second patch. In
>> scope of one patchset.
> 
> As we can use FFI for all functions that doesn't yield, we can rewrite e.g. "next" with FFI as well. It will be a bit more uniformly. And the idea is to do it in separate patch. But ok, I'll change "current" from Lua C API to FFI and send new patch.

It is not possible. Because sequence_next() can be used only via
box_sequence_next() by the public API. And box_sequence_next() can
yield. Box_sequence_set/reset/next() - all of them can yield.

> -- 
> Oleg Babin