Re: custom function for converting human readable sizes to bytes

2015-12-01 11:02 GMT+01:00 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.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.stehule@gmail.com> wrote in <CAFj8pRCd6We3TQMR0VbCN98wF0P3O3H6NaU4BTaosWcj443Qjw@mail.gmail.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