Re: custom function for converting human readable sizes to bytes

2015-12-01 11:02 GMT+01:00 Kyotaro HORIGUCHI <
horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>:

> Hello, The meaning of "be orange" (I couldn't find it) interests
> me but putting it aside..
>
> I have some random comments.
>
> At Mon, 30 Nov 2015 18:30:18 +0100, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote in <
> CAFj8pRCd6We3TQMR0VbCN98wF0P3O3H6NaU4BTaosWcj443Qjw(at)mail(dot)gmail(dot)com>
> > Hi
> >
> >
> > > 2. using independent implementation - there is some redundant code,
> but we
> > > can support duble insted int, and we can support some additional units.
> > >
> >
> > new patch is based on own implementation - use numeric/bigint
> calculations
> >
> > + regress tests and doc
>
> 1. What do you think about binary byte prefixes? (kiB, MiB...)
>

I don't know this mechanics - can you write it? It can be good idea/

>
> I couldn't read what Robert wrote upthread but I also would like
> to have 2-base byte unit prifixes. (Sorry I couldn't put it
> aside..)
>
>
> 2. Doesn't it allow units in lower case?
>

The parser is consistent with a behave used in configure file processing.
We can change it, but then there will be divergence between this function
and GUC parser.

>
> I think '1gb' and so should be acceptable as an input.
>
>
> 3. Why are you allow positive sign prefixing digits? I suppose
> that positive sign also shouldn't be allowed if it rejects
> prifixing negative sign.
>

I have not strong opinion about it. '-' is exactly wrong, but '+' can be
acceptable. But it can be changed.

>
> 4. Useless includes
>
> dbsize.c is modified to include guc.h but it is useless.
>

I'll fix it.

>
> 5. Error message
>
> + if (ndigits == 0)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("string is empty")));
>
> If pg_size_bytes allows prefixing positive sign or spaces,
> ndigits == 0 here doesn't mean that the whole string is empty.
>
>
I'll fix it.

>
> 6. Symmetry with pg_size_pretty
>
> pg_size_pretty doesn't have units larger than TB. Just adding PB
> to it breaks backward compatibility, but leaving as it is breaks
> symmetry with this function. Some possible solution for this
> that I can guess for now are..
>

I prefer a warning about possible compatibility issue in pg_size_pretty and
support PB directly there.

>
> - Leave it as is.
>
> - New GUC to allow PB for pg_size_pretty().
>
> - Expanded function such like pg_size_pretty2 (oops..)
>
> - New polymorphic (sibling?) function with additional
> parameters to instruct how they work. (The following
> involving 2-base representations)
>
> pg_size_pretty(n, <2base>, <allow_PB or such..>);
> pg_size_bytes(n, <2base>, <allow_PB or such..>);
>
> 7. Docs
>
> + Converts a size in human-readable format with size units
> + to bytes
>
> The descriptions for the functions around use 'into' instead of
> 'to' for the preposition.
>
>
> 8. Regression
>
> The regression in the patch looks good enough as is (except that
> it forgets the unit 'B' or prifixing spaces or sings), but they
> are necessarily have individual tests. The following SQL
> statement will do them at once.
>
> SELECT s, pg_size_bytes(s) FROM (VALUES ('1'), ('1B')...) as t(s);
>
>
I'll fix it.

Thank you for your ideas

Regards

Pavel

> regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
>
>