From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 37B6E469719 for ; Tue, 10 Mar 2020 02:40:41 +0300 (MSK) References: <20200306163416.95494-1-olegrok@tarantool.org> From: Vladislav Shpilevoy Message-ID: <4a346413-08ff-fac6-fe4f-b3ea3e5566a6@tarantool.org> Date: Tue, 10 Mar 2020 00:40:38 +0100 MIME-Version: 1.0 In-Reply-To: <20200306163416.95494-1-olegrok@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] box: introduce "current" for sequence List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: olegrok@tarantool.org, tarantool-patches@dev.tarantool.org, korablev@tarantool.org Cc: Oleg Babin Hi! Thanks for the patch! On 06/03/2020 17:34, olegrok@tarantool.org wrote: > From: Oleg Babin 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