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

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

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