Re: custom function for converting human readable sizes to bytes

On 2/16/16, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 16 February 2016 at 05:01, Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
>> 2016-02-15 10:16 GMT+01:00 Dean Rasheed <dean.a.rasheed@gmail.com>:
>>> Is there any reason not to make
>>> pg_size_bytes() return numeric?
>>>
>> This is a question. I have not a strong opinion about it. There are no
>> any
>> technical objection - the result will be +/- same. But you will enforce
>> Numeric into outer expression evaluation.
>>
>> The result will not be used together with function pg_size_pretty, but
>> much
>> more with functions pg_relation_size, pg_relation_size, .. and these
>> functions doesn't return Numeric. These functions returns bigint. Bigint
>> is
>> much more natural type for this purpose.
>>
>> Is there any use case for Numeric?
>>
>
> [Shrug] I don't really have a strong opinion about it either, but it
> seemed that maybe the function might have some wider uses beyond just
> comparing object sizes, and since it's already computing the numeric
> size, it might as well just return it.

I agree with Pavel, it leads to a comparison in numeric, which is
overhead. int8 can always be casted to numeric on-demand, but not vice
versa. The main reasons to return int8 instead of numeric are
performance and inability to imagine use case where so big numbers
could take place.

> Looking at the rest of the patch, I think there are other things that
> need tidying up -- the unit parsing code for one. This is going to
> some effort to reuse the memory_unit_conversion_table from guc.c, but
> the result is that it has added quite a bit of new code and now the
> responsibility for parsing different units is handled by different
> functions in different files, which IMO is quite messy. Worse, the
> error message is wrong and misleading:
>
> select pg_size_bytes('10 bytes'); -- OK
> select pg_size_bytes('10 gallons');
> ERROR:  invalid size: "10 gallons"
> DETAIL:  Invalid size unit "gallons"
> HINT:  Valid units for this parameter are "kB", "MB", "GB", and "TB".
>
> which fails to mention that "bytes" is also a valid unit.

:-D

Yes, it is fail. I missed it...

> Fixing that in parse_memory_unit() would be messy because it assumes a
> base unit of kB, so it would require a negative multiplier, and
> pg_size_bytes() would have to be taught to divide by the magnitude of
> negative multipliers in the same way that guc.c does.

I guess the best way is to simply make "bytes" a valid size unit even
in GUC by adding it to the memory_unit_conversion_table with
reflecting it in memory_units_hint and removing an extra checking from
pg_size_bytes.

> ISTM that it would be far less code, and much simpler and more
> readable to just parse the supported units directly in
> pg_size_bytes(), rather than trying to share code with guc.c, when the
> supported units are actually different and may well diverge further in
> the future.
>
> I'll try to hack something up, and see what it looks like.
>
> Regards,
> Dean

-- 
Best regards,
Vitaly Burovoy