Tarantool development patches archive
 help / color / mirror / Atom feed
* [tarantool-patches] [PATCH] box: add tuple:size function
@ 2018-09-27 17:55 AlexeyIvushkin
  2018-10-03 14:20 ` [tarantool-patches] " Vladislav Shpilevoy
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: AlexeyIvushkin @ 2018-09-27 17:55 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Morgan-iv

From: Morgan-iv <ivushkinalex@gmail.com>

When operating with tuples, we only have tuple:bsize function
to get size of tuple. tuple:bsize returns only size of MessagePack
part of struct tuple, without tuple_meta. New function tuple:size
returns size of all tuple, with MessagePack and tuple_meta

Closes #2256
---
https://github.com/tarantool/tarantool/issues/2256
https://github.com/tarantool/tarantool/tree/Morgan-iv/gh-2256
 src/box/lua/tuple.c     | 10 ++++++++++
 src/box/lua/tuple.lua   |  1 +
 test/box/tuple.result   | 12 ++++++++++++
 test/box/tuple.test.lua |  6 ++++++
 4 files changed, 29 insertions(+)

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 65660ce..1ae49d6 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -489,6 +489,15 @@ lbox_tuple_to_string(struct lua_State *L)
 	return 1;
 }
 
+static int
+lbox_tuple_size(struct lua_State *L)
+{
+	struct tuple *tuple = lua_checktuple(L, 1);
+	size_t size = tuple_size(tuple);
+	lua_pushinteger(L, size);
+	return 1;
+}
+
 void
 luaT_pushtuple(struct lua_State *L, box_tuple_t *tuple)
 {
@@ -506,6 +515,7 @@ static const struct luaL_Reg lbox_tuple_meta[] = {
 	{"__gc", lbox_tuple_gc},
 	{"tostring", lbox_tuple_to_string},
 	{"slice", lbox_tuple_slice},
+	{"size", lbox_tuple_size},
 	{"transform", lbox_tuple_transform},
 	{"tuple_to_map", lbox_tuple_to_map},
 	{"tuple_field_by_path", lbox_tuple_field_by_path},
diff --git a/src/box/lua/tuple.lua b/src/box/lua/tuple.lua
index 63ea73e..801ee3c 100644
--- a/src/box/lua/tuple.lua
+++ b/src/box/lua/tuple.lua
@@ -286,6 +286,7 @@ local methods = {
     ["update"]      = tuple_update;
     ["upsert"]      = tuple_upsert;
     ["bsize"]       = tuple_bsize;
+    ["size"]        = internal.tuple.size;
     ["tomap"]       = internal.tuple.tuple_to_map;
 }
 
diff --git a/test/box/tuple.result b/test/box/tuple.result
index e035cb9..418f5f8 100644
--- a/test/box/tuple.result
+++ b/test/box/tuple.result
@@ -186,6 +186,18 @@ t:bsize()
 ---
 - 5
 ...
+-- tuple:size()
+t = box.tuple.new('abc')
+---
+...
+t
+---
+- ['abc']
+...
+t:size()
+---
+- 15
+...
 --
 -- Test cases for #106 box.tuple.new fails on multiple items
 --
diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
index 9df978d..7f4e2b3 100644
--- a/test/box/tuple.test.lua
+++ b/test/box/tuple.test.lua
@@ -49,6 +49,12 @@ t = box.tuple.new('abc')
 t
 t:bsize()
 
+-- tuple:size()
+
+t = box.tuple.new('abc')
+t
+t:size()
+
 --
 -- Test cases for #106 box.tuple.new fails on multiple items
 --
-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-09-27 17:55 [tarantool-patches] [PATCH] box: add tuple:size function AlexeyIvushkin
@ 2018-10-03 14:20 ` Vladislav Shpilevoy
  2018-10-03 15:05   ` Vladislav Shpilevoy
  2018-10-05 10:23 ` [tarantool-patches] " Vladimir Davydov
  2018-10-16 18:21 ` Konstantin Osipov
  2 siblings, 1 reply; 18+ messages in thread
From: Vladislav Shpilevoy @ 2018-10-03 14:20 UTC (permalink / raw)
  To: tarantool-patches, AlexeyIvushkin

Hi! Thanks for the patch!

On 27/09/2018 20:55, AlexeyIvushkin wrote:
> From: Morgan-iv <ivushkinalex@gmail.com>
> 
> When operating with tuples, we only have tuple:bsize function
> to get size of tuple. tuple:bsize returns only size of MessagePack
> part of struct tuple, without tuple_meta. New function tuple:size
> returns size of all tuple, with MessagePack and tuple_meta
> 
> Closes #2256

Please, write here a documentation request to fix doc of
tuple:bsize (it returns Message Pack size only) and to
describe the new method tuple:size().

How to write documentation requests in a commit see
tarantool/docbot repository and commits like this:
https://github.com/tarantool/tarantool/commit/1663bdc4a86fa6fd43903d2d52dd5a743b87f33b

> ---
> https://github.com/tarantool/tarantool/issues/2256
> https://github.com/tarantool/tarantool/tree/Morgan-iv/gh-2256
>   src/box/lua/tuple.c     | 10 ++++++++++
>   src/box/lua/tuple.lua   |  1 +
>   test/box/tuple.result   | 12 ++++++++++++
>   test/box/tuple.test.lua |  6 ++++++
>   4 files changed, 29 insertions(+)
> 
> diff --git a/test/box/tuple.result b/test/box/tuple.result
> index e035cb9..418f5f8 100644
> --- a/test/box/tuple.result
> +++ b/test/box/tuple.result
> @@ -186,6 +186,18 @@ t:bsize()
>   ---
>   - 5
>   ...
> +-- tuple:size()

Please, put more details in the comment about
the issue. See other tests. The comment should
look like:

--
-- gh-<issue_number>: <description>.
--
... a test ...

> +t = box.tuple.new('abc')
> +---
> +...
> +t
> +---
> +- ['abc']
> +...
> +t:size()
> +---
> +- 15
> +...
>   --
>   -- Test cases for #106 box.tuple.new fails on multiple items
>   --

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-10-03 14:20 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-10-03 15:05   ` Vladislav Shpilevoy
  0 siblings, 0 replies; 18+ messages in thread
From: Vladislav Shpilevoy @ 2018-10-03 15:05 UTC (permalink / raw)
  To: tarantool-patches

Also, make this patch on 1.10, not on 2.0.

On 03/10/2018 17:20, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 27/09/2018 20:55, AlexeyIvushkin wrote:
>> From: Morgan-iv <ivushkinalex@gmail.com>
>>
>> When operating with tuples, we only have tuple:bsize function
>> to get size of tuple. tuple:bsize returns only size of MessagePack
>> part of struct tuple, without tuple_meta. New function tuple:size
>> returns size of all tuple, with MessagePack and tuple_meta
>>
>> Closes #2256
> 
> Please, write here a documentation request to fix doc of
> tuple:bsize (it returns Message Pack size only) and to
> describe the new method tuple:size().
> 
> How to write documentation requests in a commit see
> tarantool/docbot repository and commits like this:
> https://github.com/tarantool/tarantool/commit/1663bdc4a86fa6fd43903d2d52dd5a743b87f33b
> 
>> ---
>> https://github.com/tarantool/tarantool/issues/2256
>> https://github.com/tarantool/tarantool/tree/Morgan-iv/gh-2256
>>   src/box/lua/tuple.c     | 10 ++++++++++
>>   src/box/lua/tuple.lua   |  1 +
>>   test/box/tuple.result   | 12 ++++++++++++
>>   test/box/tuple.test.lua |  6 ++++++
>>   4 files changed, 29 insertions(+)
>>
>> diff --git a/test/box/tuple.result b/test/box/tuple.result
>> index e035cb9..418f5f8 100644
>> --- a/test/box/tuple.result
>> +++ b/test/box/tuple.result
>> @@ -186,6 +186,18 @@ t:bsize()
>>   ---
>>   - 5
>>   ...
>> +-- tuple:size()
> 
> Please, put more details in the comment about
> the issue. See other tests. The comment should
> look like:
> 
> -- 
> -- gh-<issue_number>: <description>.
> -- 
> ... a test ...
> 
>> +t = box.tuple.new('abc')
>> +---
>> +...
>> +t
>> +---
>> +- ['abc']
>> +...
>> +t:size()
>> +---
>> +- 15
>> +...
>>   --
>>   -- Test cases for #106 box.tuple.new fails on multiple items
>>   --
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [tarantool-patches] [PATCH] box: add tuple:size function
  2018-09-27 17:55 [tarantool-patches] [PATCH] box: add tuple:size function AlexeyIvushkin
  2018-10-03 14:20 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-10-05 10:23 ` Vladimir Davydov
  2018-10-06 13:58   ` [tarantool-patches] " Vladislav Shpilevoy
  2018-10-16 18:21 ` Konstantin Osipov
  2 siblings, 1 reply; 18+ messages in thread
From: Vladimir Davydov @ 2018-10-05 10:23 UTC (permalink / raw)
  To: AlexeyIvushkin; +Cc: tarantool-patches

On Thu, Sep 27, 2018 at 08:55:23PM +0300, AlexeyIvushkin wrote:
> diff --git a/src/box/lua/tuple.lua b/src/box/lua/tuple.lua
> index 63ea73e..801ee3c 100644
> --- a/src/box/lua/tuple.lua
> +++ b/src/box/lua/tuple.lua
> @@ -286,6 +286,7 @@ local methods = {
>      ["update"]      = tuple_update;
>      ["upsert"]      = tuple_upsert;
>      ["bsize"]       = tuple_bsize;
> +    ["size"]        = internal.tuple.size;
>      ["tomap"]       = internal.tuple.tuple_to_map;

Why did you decide to introduce a new function rather than fixing
tuple.bsize, as it was explicitly requested in the ticket?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-10-05 10:23 ` [tarantool-patches] " Vladimir Davydov
@ 2018-10-06 13:58   ` Vladislav Shpilevoy
  2018-10-08 10:16     ` Vladimir Davydov
  0 siblings, 1 reply; 18+ messages in thread
From: Vladislav Shpilevoy @ 2018-10-06 13:58 UTC (permalink / raw)
  To: tarantool-patches, Vladimir Davydov, AlexeyIvushkin



On 05/10/2018 13:23, Vladimir Davydov wrote:
> On Thu, Sep 27, 2018 at 08:55:23PM +0300, AlexeyIvushkin wrote:
>> diff --git a/src/box/lua/tuple.lua b/src/box/lua/tuple.lua
>> index 63ea73e..801ee3c 100644
>> --- a/src/box/lua/tuple.lua
>> +++ b/src/box/lua/tuple.lua
>> @@ -286,6 +286,7 @@ local methods = {
>>       ["update"]      = tuple_update;
>>       ["upsert"]      = tuple_upsert;
>>       ["bsize"]       = tuple_bsize;
>> +    ["size"]        = internal.tuple.size;
>>       ["tomap"]       = internal.tuple.tuple_to_map;
> 
> Why did you decide to introduce a new function rather than fixing
> tuple.bsize, as it was explicitly requested in the ticket?

It breaks compatibility. Now bsize returns only Message Pack part
of tuple despite what the documentation says.

> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-10-06 13:58   ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-10-08 10:16     ` Vladimir Davydov
  2018-10-10 10:25       ` Vladislav Shpilevoy
  0 siblings, 1 reply; 18+ messages in thread
From: Vladimir Davydov @ 2018-10-08 10:16 UTC (permalink / raw)
  To: Vladislav Shpilevoy, Konstantin Osipov; +Cc: tarantool-patches, AlexeyIvushkin

On Sat, Oct 06, 2018 at 04:58:46PM +0300, Vladislav Shpilevoy wrote:
> 
> 
> On 05/10/2018 13:23, Vladimir Davydov wrote:
> > On Thu, Sep 27, 2018 at 08:55:23PM +0300, AlexeyIvushkin wrote:
> > > diff --git a/src/box/lua/tuple.lua b/src/box/lua/tuple.lua
> > > index 63ea73e..801ee3c 100644
> > > --- a/src/box/lua/tuple.lua
> > > +++ b/src/box/lua/tuple.lua
> > > @@ -286,6 +286,7 @@ local methods = {
> > >       ["update"]      = tuple_update;
> > >       ["upsert"]      = tuple_upsert;
> > >       ["bsize"]       = tuple_bsize;
> > > +    ["size"]        = internal.tuple.size;
> > >       ["tomap"]       = internal.tuple.tuple_to_map;
> > 
> > Why did you decide to introduce a new function rather than fixing
> > tuple.bsize, as it was explicitly requested in the ticket?
> 
> It breaks compatibility. Now bsize returns only Message Pack part
> of tuple despite what the documentation says.

Frankly, I don't think that introducing a new method just to keep the
old behavior of bsize intact is a good idea, because we probably won't
stop at that. The next thing we have to add will probably be space.size,
which would be defined as a sum of tuple.size of constituent tuples and
that wouldn't be as trivial to implement as tuple.size...

Anyway, IMO having two methods for getting the size of binary data
stored in a tuple is confusing (which one should I use as a user?).
Up to Kostja.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-10-08 10:16     ` Vladimir Davydov
@ 2018-10-10 10:25       ` Vladislav Shpilevoy
  0 siblings, 0 replies; 18+ messages in thread
From: Vladislav Shpilevoy @ 2018-10-10 10:25 UTC (permalink / raw)
  To: tarantool-patches, Konstantin Osipov; +Cc: Vladimir Davydov, AlexeyIvushkin

Kostja, please, do not ignore.

On 08/10/2018 13:16, Vladimir Davydov wrote:
> On Sat, Oct 06, 2018 at 04:58:46PM +0300, Vladislav Shpilevoy wrote:
>>
>>
>> On 05/10/2018 13:23, Vladimir Davydov wrote:
>>> On Thu, Sep 27, 2018 at 08:55:23PM +0300, AlexeyIvushkin wrote:
>>>> diff --git a/src/box/lua/tuple.lua b/src/box/lua/tuple.lua
>>>> index 63ea73e..801ee3c 100644
>>>> --- a/src/box/lua/tuple.lua
>>>> +++ b/src/box/lua/tuple.lua
>>>> @@ -286,6 +286,7 @@ local methods = {
>>>>        ["update"]      = tuple_update;
>>>>        ["upsert"]      = tuple_upsert;
>>>>        ["bsize"]       = tuple_bsize;
>>>> +    ["size"]        = internal.tuple.size;
>>>>        ["tomap"]       = internal.tuple.tuple_to_map;
>>>
>>> Why did you decide to introduce a new function rather than fixing
>>> tuple.bsize, as it was explicitly requested in the ticket?
>>
>> It breaks compatibility. Now bsize returns only Message Pack part
>> of tuple despite what the documentation says.
> 
> Frankly, I don't think that introducing a new method just to keep the
> old behavior of bsize intact is a good idea, because we probably won't
> stop at that. The next thing we have to add will probably be space.size,
> which would be defined as a sum of tuple.size of constituent tuples and
> that wouldn't be as trivial to implement as tuple.size...
> 
> Anyway, IMO having two methods for getting the size of binary data
> stored in a tuple is confusing (which one should I use as a user?).
> Up to Kostja.
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-09-27 17:55 [tarantool-patches] [PATCH] box: add tuple:size function AlexeyIvushkin
  2018-10-03 14:20 ` [tarantool-patches] " Vladislav Shpilevoy
  2018-10-05 10:23 ` [tarantool-patches] " Vladimir Davydov
@ 2018-10-16 18:21 ` Konstantin Osipov
  2018-10-17  7:28   ` Alexander Turenko
  2 siblings, 1 reply; 18+ messages in thread
From: Konstantin Osipov @ 2018-10-16 18:21 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Morgan-iv

* AlexeyIvushkin <ivushkinalex@gmail.com> [18/09/27 20:57]:
> From: Morgan-iv <ivushkinalex@gmail.com>
> 
> When operating with tuples, we only have tuple:bsize function
> to get size of tuple. tuple:bsize returns only size of MessagePack
> part of struct tuple, without tuple_meta. New function tuple:size
> returns size of all tuple, with MessagePack and tuple_meta
> 
> Closes #2256

The bug should be fixed as described by Roman.

tuple:bsize() behaviour should be changed to be more accurate,
i.e. return entire binary footprint of a tuple.

No need to introduce a new function.

I don't think it's a "true" incompatible change, the purpose of
bsize() has always been to inform the user about the binary
footprint, so we're simply fixing a bug in the implementation.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-10-16 18:21 ` Konstantin Osipov
@ 2018-10-17  7:28   ` Alexander Turenko
  2018-10-17 15:29     ` Konstantin Osipov
  0 siblings, 1 reply; 18+ messages in thread
From: Alexander Turenko @ 2018-10-17  7:28 UTC (permalink / raw)
  To: Konstantin Osipov; +Cc: tarantool-patches, Morgan-iv

On Tue, Oct 16, 2018 at 09:21:44PM +0300, Konstantin Osipov wrote:
> * AlexeyIvushkin <ivushkinalex@gmail.com> [18/09/27 20:57]:
> > From: Morgan-iv <ivushkinalex@gmail.com>
> > 
> > When operating with tuples, we only have tuple:bsize function
> > to get size of tuple. tuple:bsize returns only size of MessagePack
> > part of struct tuple, without tuple_meta. New function tuple:size
> > returns size of all tuple, with MessagePack and tuple_meta
> > 
> > Closes #2256
> 
> The bug should be fixed as described by Roman.
> 
> tuple:bsize() behaviour should be changed to be more accurate,
> i.e. return entire binary footprint of a tuple.
> 
> No need to introduce a new function.
> 
> I don't think it's a "true" incompatible change, the purpose of
> bsize() has always been to inform the user about the binary
> footprint, so we're simply fixing a bug in the implementation.
> 

Are tuple.bsize and box_tuple_bsize() subjects to change or it is only
about the Lua part?

> -- 
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> http://tarantool.io - www.twitter.com/kostja_osipov
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-10-17  7:28   ` Alexander Turenko
@ 2018-10-17 15:29     ` Konstantin Osipov
  2018-10-17 15:50       ` Alexander Turenko
  2018-10-17 18:06       ` Vladislav Shpilevoy
  0 siblings, 2 replies; 18+ messages in thread
From: Konstantin Osipov @ 2018-10-17 15:29 UTC (permalink / raw)
  To: Alexander Turenko; +Cc: tarantool-patches, Morgan-iv

* Alexander Turenko <alexander.turenko@tarantool.org> [18/10/17 10:45]:
> Are tuple.bsize and box_tuple_bsize() subjects to change or it is only
> about the Lua part?

tuple.bsize is used internally, so I don't think you should change
it. But it's better to rename it to msgpack_size or something like
that to avoid ambiguity.

box_tuple_bsize() should be ok to change.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-10-17 15:29     ` Konstantin Osipov
@ 2018-10-17 15:50       ` Alexander Turenko
  2018-10-18 18:11         ` Konstantin Osipov
  2018-10-17 18:06       ` Vladislav Shpilevoy
  1 sibling, 1 reply; 18+ messages in thread
From: Alexander Turenko @ 2018-10-17 15:50 UTC (permalink / raw)
  To: Konstantin Osipov; +Cc: tarantool-patches, Morgan-iv

On Wed, Oct 17, 2018 at 06:29:49PM +0300, Konstantin Osipov wrote:
> * Alexander Turenko <alexander.turenko@tarantool.org> [18/10/17 10:45]:
> > Are tuple.bsize and box_tuple_bsize() subjects to change or it is only
> > about the Lua part?
> 
> tuple.bsize is used internally, so I don't think you should change
> it. But it's better to rename it to msgpack_size or something like
> that to avoid ambiguity.
> 
> box_tuple_bsize() should be ok to change.
> 

So should I use tuple.bsize in my current WIP patch (merger) and don't
use box_tuple_bsize() and things will not become broken in the future?

BTW, I wonder why such simple public accessor function as
box_tuple_bsize() is not defined in the header file to allow inlining?

WBR, Alexander Turenko.

> 
> -- 
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> http://tarantool.io - www.twitter.com/kostja_osipov

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-10-17 15:29     ` Konstantin Osipov
  2018-10-17 15:50       ` Alexander Turenko
@ 2018-10-17 18:06       ` Vladislav Shpilevoy
  2018-10-17 18:10         ` Vladislav Shpilevoy
  2018-10-17 18:14         ` Konstantin Osipov
  1 sibling, 2 replies; 18+ messages in thread
From: Vladislav Shpilevoy @ 2018-10-17 18:06 UTC (permalink / raw)
  To: tarantool-patches, Konstantin Osipov, Alexander Turenko; +Cc: Morgan-iv



On 17/10/2018 18:29, Konstantin Osipov wrote:
> * Alexander Turenko <alexander.turenko@tarantool.org> [18/10/17 10:45]:
>> Are tuple.bsize and box_tuple_bsize() subjects to change or it is only
>> about the Lua part?
> 
> tuple.bsize is used internally, so I don't think you should change
> it. But it's better to rename it to msgpack_size or something like
> that to avoid ambiguity.
> 
> box_tuple_bsize() should be ok to change.
> 

box_tuple_bsize() is already documented in module.h as
returning only tuple data.

Also, some people can use it right now to allocate a
buffer of a correct size before calling box_tuple_to_buf.
I understand, that box_tuple_to_buf(tuple, NULL, 0) returns
bsize as well, but some people could miss it, or just use
box_tuple_bsize because it looks better when you write like
this:

     size = box_tuple_bsize(tuple)
     buf = alloc(size)
     box_tuple_to_buf(tuple, buf, size)

than like this:

     size = box_tuple_to_buf(tuple, NULL, 0) // <- difference
     buf = alloc(size)
     box_tuple_to_buf(tuple, buf, size)

Even if we close eyes on the fact, that a user of
the first way will allocate more data than needed,
imagine, that then he does something like this:

     send(sockfd, buf, size)

Now, he send some garbage uninitialized data of 14
bytes at the end of buf.

I understand, that you likely not believe me that
the first way looks more intuitive, but look at
our own code: box_tuple_bsize() is used now in

*box/lua/tuple.c in tuple_to_mpstream();
* box/lua/tuple.lua via FFI;

to allocate a buffer before copying tuple data
directly or via tuple_to_buf. We can fix our code,
but can not fix user's one.

So here we can either

* fix documentation of the site without breaking anything
* or break behavior of public API destroying an ability to
   learn tuple data size in a natural way;
* add new method and again fix the site documentation.

I described it just for the record that you have chosen
the second, most destructive way.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-10-17 18:06       ` Vladislav Shpilevoy
@ 2018-10-17 18:10         ` Vladislav Shpilevoy
  2018-10-17 18:14         ` Konstantin Osipov
  1 sibling, 0 replies; 18+ messages in thread
From: Vladislav Shpilevoy @ 2018-10-17 18:10 UTC (permalink / raw)
  To: tarantool-patches

In addition, a function, returning some internal size
that can not be applied to any user code object, is
useless except statistics.

So box_tuple_bsize will be useless.

On 17/10/2018 21:06, Vladislav Shpilevoy wrote:
> 
> 
> On 17/10/2018 18:29, Konstantin Osipov wrote:
>> * Alexander Turenko <alexander.turenko@tarantool.org> [18/10/17 10:45]:
>>> Are tuple.bsize and box_tuple_bsize() subjects to change or it is only
>>> about the Lua part?
>>
>> tuple.bsize is used internally, so I don't think you should change
>> it. But it's better to rename it to msgpack_size or something like
>> that to avoid ambiguity.
>>
>> box_tuple_bsize() should be ok to change.
>>
> 
> box_tuple_bsize() is already documented in module.h as
> returning only tuple data.
> 
> Also, some people can use it right now to allocate a
> buffer of a correct size before calling box_tuple_to_buf.
> I understand, that box_tuple_to_buf(tuple, NULL, 0) returns
> bsize as well, but some people could miss it, or just use
> box_tuple_bsize because it looks better when you write like
> this:
> 
>      size = box_tuple_bsize(tuple)
>      buf = alloc(size)
>      box_tuple_to_buf(tuple, buf, size)
> 
> than like this:
> 
>      size = box_tuple_to_buf(tuple, NULL, 0) // <- difference
>      buf = alloc(size)
>      box_tuple_to_buf(tuple, buf, size)
> 
> Even if we close eyes on the fact, that a user of
> the first way will allocate more data than needed,
> imagine, that then he does something like this:
> 
>      send(sockfd, buf, size)
> 
> Now, he send some garbage uninitialized data of 14
> bytes at the end of buf.
> 
> I understand, that you likely not believe me that
> the first way looks more intuitive, but look at
> our own code: box_tuple_bsize() is used now in
> 
> *box/lua/tuple.c in tuple_to_mpstream();
> * box/lua/tuple.lua via FFI;
> 
> to allocate a buffer before copying tuple data
> directly or via tuple_to_buf. We can fix our code,
> but can not fix user's one.
> 
> So here we can either
> 
> * fix documentation of the site without breaking anything
> * or break behavior of public API destroying an ability to
>    learn tuple data size in a natural way;
> * add new method and again fix the site documentation.
> 
> I described it just for the record that you have chosen
> the second, most destructive way.
> 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-10-17 18:06       ` Vladislav Shpilevoy
  2018-10-17 18:10         ` Vladislav Shpilevoy
@ 2018-10-17 18:14         ` Konstantin Osipov
  2018-10-17 18:20           ` Alexander Turenko
  2018-10-17 20:36           ` Vladislav Shpilevoy
  1 sibling, 2 replies; 18+ messages in thread
From: Konstantin Osipov @ 2018-10-17 18:14 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko, Morgan-iv

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/10/17 21:12]:

> Also, some people can use it right now to allocate a
> buffer of a correct size before calling box_tuple_to_buf.
> I understand, that box_tuple_to_buf(tuple, NULL, 0) returns
> bsize as well, but some people could miss it, or just use
> box_tuple_bsize because it looks better when you write like
> this:
> 
>     size = box_tuple_bsize(tuple)
>     buf = alloc(size)
>     box_tuple_to_buf(tuple, buf, size)
> 
> than like this:
> 
>     size = box_tuple_to_buf(tuple, NULL, 0) // <- difference
>     buf = alloc(size)
>     box_tuple_to_buf(tuple, buf, size)
> 
> Even if we close eyes on the fact, that a user of
> the first way will allocate more data than needed,
> imagine, that then he does something like this:
> 
>     send(sockfd, buf, size)
> 
> Now, he send some garbage uninitialized data of 14
> bytes at the end of buf.

Is there any known use?

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-10-17 18:14         ` Konstantin Osipov
@ 2018-10-17 18:20           ` Alexander Turenko
  2018-10-17 20:36           ` Vladislav Shpilevoy
  1 sibling, 0 replies; 18+ messages in thread
From: Alexander Turenko @ 2018-10-17 18:20 UTC (permalink / raw)
  To: Konstantin Osipov; +Cc: Vladislav Shpilevoy, tarantool-patches, Morgan-iv

On Wed, Oct 17, 2018 at 09:14:47PM +0300, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/10/17 21:12]:
> 
> > Also, some people can use it right now to allocate a
> > buffer of a correct size before calling box_tuple_to_buf.
> > I understand, that box_tuple_to_buf(tuple, NULL, 0) returns
> > bsize as well, but some people could miss it, or just use
> > box_tuple_bsize because it looks better when you write like
> > this:
> > 
> >     size = box_tuple_bsize(tuple)
> >     buf = alloc(size)
> >     box_tuple_to_buf(tuple, buf, size)
> > 
> > than like this:
> > 
> >     size = box_tuple_to_buf(tuple, NULL, 0) // <- difference
> >     buf = alloc(size)
> >     box_tuple_to_buf(tuple, buf, size)
> > 
> > Even if we close eyes on the fact, that a user of
> > the first way will allocate more data than needed,
> > imagine, that then he does something like this:
> > 
> >     send(sockfd, buf, size)
> > 
> > Now, he send some garbage uninitialized data of 14
> > bytes at the end of buf.
> 
> Is there any known use?
> 

https://github.com/search?p=1&q=box_tuple_bsize&type=Code

One seems to be the real (ok, more or less real) usage:
https://github.com/rtsisyk/tarantool-rust

> -- 
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> http://tarantool.io - www.twitter.com/kostja_osipov
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-10-17 18:14         ` Konstantin Osipov
  2018-10-17 18:20           ` Alexander Turenko
@ 2018-10-17 20:36           ` Vladislav Shpilevoy
  1 sibling, 0 replies; 18+ messages in thread
From: Vladislav Shpilevoy @ 2018-10-17 20:36 UTC (permalink / raw)
  To: tarantool-patches, Konstantin Osipov; +Cc: Alexander Turenko, Morgan-iv



On 17/10/2018 21:14, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/10/17 21:12]:
> 
>> Also, some people can use it right now to allocate a
>> buffer of a correct size before calling box_tuple_to_buf.
>> I understand, that box_tuple_to_buf(tuple, NULL, 0) returns
>> bsize as well, but some people could miss it, or just use
>> box_tuple_bsize because it looks better when you write like
>> this:
>>
>>      size = box_tuple_bsize(tuple)
>>      buf = alloc(size)
>>      box_tuple_to_buf(tuple, buf, size)
>>
>> than like this:
>>
>>      size = box_tuple_to_buf(tuple, NULL, 0) // <- difference
>>      buf = alloc(size)
>>      box_tuple_to_buf(tuple, buf, size)
>>
>> Even if we close eyes on the fact, that a user of
>> the first way will allocate more data than needed,
>> imagine, that then he does something like this:
>>
>>      send(sockfd, buf, size)
>>
>> Now, he send some garbage uninitialized data of 14
>> bytes at the end of buf.
> 
> Is there any known use?
> 

It is a strange question, especially from you. Open
source public API's nature is to be assumed as being
used right now in a real project. When a product is
free and open source, you can not be sure how many
users do you have exactly and who uses which API, so
you HAVE to assume that each API function is used
somewhere in all possible ways.

At first, it can be used in some GitHub project as
Alexander underlined, in some connectors. At second,
not all projects are hosted on GH. Out API can be
used in any imaginable proprietary project.

As I said, I knew you will not believe me, but if you
asked about why somebody needs 'send' in their Tarantool
module, I would answer this:

* it can be a customer's proxy - he could create own
   TCP server in front of Tarantool to send/receive
   tuples with very simplified protocol, easier than
   IProto, or to just convert to protocol of another DB;

* it is not necessary 'send'. It can be 'write' to a
   disk. For example, Tarantool is an extra light and
   fast in-memory cache for some non-critical data, that
   is persisted by user's facilities, or in another DB,
   having disk storage;

* it can be Mons-like weird way to get tuple data
   without iterating over it via box_tuple_iterator nor
   copying into buf:

       const char *tuple_whole_data = box_tuple_field(tuple, 0);
       size_t tuple_msgpack_size = box_tuple_size(tuple);
       /*
        * then use whole range
        * [tuple_whole_data, tuple_whole_data + tuple_msgpack_size]
        */

   I know that it is illegal way, but it works now, and allows to
   do not copy data.
   

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-10-17 15:50       ` Alexander Turenko
@ 2018-10-18 18:11         ` Konstantin Osipov
  2018-10-18 18:15           ` Alexander Turenko
  0 siblings, 1 reply; 18+ messages in thread
From: Konstantin Osipov @ 2018-10-18 18:11 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Morgan-iv

* Alexander Turenko <alexander.turenko@tarantool.org> [18/10/17 18:53]:
> On Wed, Oct 17, 2018 at 06:29:49PM +0300, Konstantin Osipov wrote:
> > * Alexander Turenko <alexander.turenko@tarantool.org> [18/10/17 10:45]:
> > > Are tuple.bsize and box_tuple_bsize() subjects to change or it is only
> > > about the Lua part?
> > 
> > tuple.bsize is used internally, so I don't think you should change
> > it. But it's better to rename it to msgpack_size or something like
> > that to avoid ambiguity.
> > 
> > box_tuple_bsize() should be ok to change.
> > 
> 
> So should I use tuple.bsize in my current WIP patch (merger) and don't
> use box_tuple_bsize() and things will not become broken in the future?

No, I suggested to rename tuple.bsize to tuple.msgpack-size.
Direct access to struct tuple members from modules should be
prohibited.

> BTW, I wonder why such simple public accessor function as
> box_tuple_bsize() is not defined in the header file to allow inlining?

Since it's part of the plugin api I guess.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tarantool-patches] Re: [PATCH] box: add tuple:size function
  2018-10-18 18:11         ` Konstantin Osipov
@ 2018-10-18 18:15           ` Alexander Turenko
  0 siblings, 0 replies; 18+ messages in thread
From: Alexander Turenko @ 2018-10-18 18:15 UTC (permalink / raw)
  To: Konstantin Osipov; +Cc: tarantool-patches, Morgan-iv

On Thu, Oct 18, 2018 at 09:11:31PM +0300, Konstantin Osipov wrote:
> * Alexander Turenko <alexander.turenko@tarantool.org> [18/10/17 18:53]:
> > On Wed, Oct 17, 2018 at 06:29:49PM +0300, Konstantin Osipov wrote:
> > > * Alexander Turenko <alexander.turenko@tarantool.org> [18/10/17 10:45]:
> > > > Are tuple.bsize and box_tuple_bsize() subjects to change or it is only
> > > > about the Lua part?
> > > 
> > > tuple.bsize is used internally, so I don't think you should change
> > > it. But it's better to rename it to msgpack_size or something like
> > > that to avoid ambiguity.
> > > 
> > > box_tuple_bsize() should be ok to change.
> > > 
> > 
> > So should I use tuple.bsize in my current WIP patch (merger) and don't
> > use box_tuple_bsize() and things will not become broken in the future?
> 
> No, I suggested to rename tuple.bsize to tuple.msgpack-size.
> Direct access to struct tuple members from modules should be
> prohibited.
> 

It was about the build-in module.

> > BTW, I wonder why such simple public accessor function as
> > box_tuple_bsize() is not defined in the header file to allow inlining?
> 
> Since it's part of the plugin api I guess.
> 

Thanks, got it.

> -- 
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> http://tarantool.io - www.twitter.com/kostja_osipov
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2018-10-18 18:15 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-27 17:55 [tarantool-patches] [PATCH] box: add tuple:size function AlexeyIvushkin
2018-10-03 14:20 ` [tarantool-patches] " Vladislav Shpilevoy
2018-10-03 15:05   ` Vladislav Shpilevoy
2018-10-05 10:23 ` [tarantool-patches] " Vladimir Davydov
2018-10-06 13:58   ` [tarantool-patches] " Vladislav Shpilevoy
2018-10-08 10:16     ` Vladimir Davydov
2018-10-10 10:25       ` Vladislav Shpilevoy
2018-10-16 18:21 ` Konstantin Osipov
2018-10-17  7:28   ` Alexander Turenko
2018-10-17 15:29     ` Konstantin Osipov
2018-10-17 15:50       ` Alexander Turenko
2018-10-18 18:11         ` Konstantin Osipov
2018-10-18 18:15           ` Alexander Turenko
2018-10-17 18:06       ` Vladislav Shpilevoy
2018-10-17 18:10         ` Vladislav Shpilevoy
2018-10-17 18:14         ` Konstantin Osipov
2018-10-17 18:20           ` Alexander Turenko
2018-10-17 20:36           ` Vladislav Shpilevoy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox