[Tarantool-patches] [PATCH] box: introduce "current" for sequence
Oleg Babin
olegrok at tarantool.org
Tue Mar 10 22:08:24 MSK 2020
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 at tarantool.org wrote:
>> From: Oleg Babin <babinoleg at 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.
More information about the Tarantool-patches
mailing list