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

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

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.